diff --git a/app/controllers/admin/oidc_settings_controller.rb b/app/controllers/admin/oidc_settings_controller.rb index f668442bc0..69ce9dbfc9 100644 --- a/app/controllers/admin/oidc_settings_controller.rb +++ b/app/controllers/admin/oidc_settings_controller.rb @@ -2,6 +2,13 @@ module Admin class OidcSettingsController < Spree::Admin::BaseController - def index; end + def index + @account = spree_current_user.oidc_account + end + + def destroy + spree_current_user.oidc_account&.destroy + redirect_to admin_oidc_settings_path + end end end diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index b3db1607ef..c483d6f4b7 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -2,7 +2,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController def openid_connect - spree_current_user.link_from_omniauth(request.env["omniauth.auth"]) + OidcAccount.link(spree_current_user, request.env["omniauth.auth"]) redirect_to admin_oidc_settings_path end diff --git a/app/models/oidc_account.rb b/app/models/oidc_account.rb new file mode 100644 index 0000000000..b755849753 --- /dev/null +++ b/app/models/oidc_account.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class OidcAccount < ApplicationRecord + belongs_to :user, class_name: "Spree::User" + + # When a user authenticates via token, the `uid` should be mapped to only one + # OFN user and therefore it needs to be unique. + validates :uid, presence: true, uniqueness: true + + def self.link(user, auth) + attributes = { + user_id: user.id, + provider: auth.provider, + uid: auth.uid, + token: auth.dig(:credentials, :token), + refresh_token: auth.dig(:credentials, :refresh_token), + } + # This skips validations but we have database constraints in place. + # We may replace this at some point. + upsert_all([attributes], unique_by: [:user_id]) # rubocop:disable Rails/SkipsModelValidations + end +end diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb index f4d776bc02..127b9c2766 100644 --- a/app/models/spree/ability.rb +++ b/app/models/spree/ability.rb @@ -177,7 +177,7 @@ module Spree can [:admin, :create], :manager_invitation - can [:admin, :index], :oidc_setting + can [:admin, :index, :destroy], :oidc_setting can [:admin, :create], Voucher end diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index fce0828b4c..56c0607262 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -42,6 +42,7 @@ module Spree has_many :credit_cards, dependent: :destroy has_many :report_rendering_options, class_name: "::ReportRenderingOptions", dependent: :destroy has_many :webhook_endpoints, dependent: :destroy + has_one :oidc_account, dependent: :destroy accepts_nested_attributes_for :enterprise_roles, allow_destroy: true accepts_nested_attributes_for :webhook_endpoints @@ -51,11 +52,6 @@ module Spree validates :email, 'valid_email_2/email': { mx: true }, if: :email_changed? validate :limit_owned_enterprises - validates :uid, uniqueness: true, if: lambda { uid.present? } - - # Same validation as in the openid_connect gem. - # This validator is totally outdated but we indirectly depend on it. - validates :uid, email: true, if: lambda { uid.present? } class DestroyWithOrdersError < StandardError; end @@ -63,10 +59,6 @@ module Spree User.admin.count > 0 end - def link_from_omniauth(auth) - update!(provider: auth.provider, uid: auth.uid) - end - # Whether a user has a role or not. def has_spree_role?(role_in_question) spree_roles.where(name: role_in_question.to_s).any? diff --git a/app/views/admin/oidc_settings/index.html.haml b/app/views/admin/oidc_settings/index.html.haml index eed72c78d2..fac04aaec7 100644 --- a/app/views/admin/oidc_settings/index.html.haml +++ b/app/views/admin/oidc_settings/index.html.haml @@ -7,14 +7,26 @@ %h2= t(".connect") %br - - if spree_current_user.provider == 'openid_connect' && spree_current_user.uid.present? - = t(".already_connected") - = spree_current_user.uid + - if @account + = t(".connected", uid: @account.uid) %br %br = t(".view_account") = link_to t(".les_communs_link"), "#{ Devise.omniauth_configs[:openid_connect].options[:issuer] }/account" + %br + %br + = button_to t(".disconnect"), admin_oidc_setting_path(@account), method: :delete + + - if @account.refresh_token.blank? + %br + %br + %p= t(".note_expiry") + %br + %br + = button_to t(".refresh"), + Spree::Core::Engine.routes.url_helpers.spree_user_openid_connect_omniauth_authorize_path(auth_type: "login"), + data: { method: :post, "ujs-navigate": "false" } - else = t(".link_your_account") diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index ab762473db..5c3c0ca585 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -145,7 +145,13 @@ end if ENV["OPENID_APP_ID"].present? && ENV["OPENID_APP_SECRET"].present? Devise.setup do |config| - protocol = Rails.env.development? ? "http://" : "https://" + site = if Rails.env.development? + # The lescommuns server accepts localhost:3000 as valid. + # So you can test in development. + "http://localhost:3000" + else + "https://#{ENV["SITE_URL"]}" + end config.omniauth :openid_connect, { name: :openid_connect, issuer: "https://login.lescommuns.org/auth/realms/data-food-consortium", @@ -158,7 +164,7 @@ if ENV["OPENID_APP_ID"].present? && ENV["OPENID_APP_SECRET"].present? client_options: { identifier: ENV["OPENID_APP_ID"], secret: ENV["OPENID_APP_SECRET"], - redirect_uri: "#{protocol}#{ENV["SITE_URL"]}/user/spree_user/auth/openid_connect/callback", + redirect_uri: "#{site}/user/spree_user/auth/openid_connect/callback", jwks_uri: 'https://login.lescommuns.org/auth/realms/data-food-consortium/protocol/openid-connect/certs' } } diff --git a/config/locales/en.yml b/config/locales/en.yml index e615792f8c..8d88ce2cd7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1712,10 +1712,15 @@ en: index: title: "OIDC Settings" connect: "Connect Your Account" - already_connected: "Your account is already linked to this DFC authorization account:" + disconnect: "Disconnect" + connected: "Your account is linked to %{uid}." les_communs_link: "Les Communs Open ID server" link_your_account: "You need first to link your account with the authorization provider used by DFC (Les Communs Open ID Connect)." link_account_button: "Link your Les Communs OIDC Account" + note_expiry: | + Tokens to access connected apps have expired. Please refresh your + account connection to keep all integrations working. + refresh: "Refresh authorisation" view_account: "To view your account, see:" subscriptions: index: diff --git a/config/routes/admin.rb b/config/routes/admin.rb index b8fef0838c..20dace8c6a 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -113,7 +113,7 @@ Openfoodnetwork::Application.routes.draw do put :unpause, on: :member end - resources :oidc_settings, only: :index + resources :oidc_settings, only: [:index, :destroy] resources :subscription_line_items, only: [], format: :json do post :build, on: :collection diff --git a/db/migrate/20240213042618_create_oidc_accounts.rb b/db/migrate/20240213042618_create_oidc_accounts.rb new file mode 100644 index 0000000000..7a77e8b716 --- /dev/null +++ b/db/migrate/20240213042618_create_oidc_accounts.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class CreateOidcAccounts < ActiveRecord::Migration[7.0] + def change + create_table :oidc_accounts do |t| + # We may allow multiple OIDC accounts per user in the future but for now + # we assume only one and therefore make this unique. + t.belongs_to :user, null: false, foreign_key: { to_table: :spree_users }, + index: { unique: true } + t.string :provider + t.string :uid, null: false, index: { unique: true } + t.string :token + t.string :refresh_token + + t.timestamps + end + end +end diff --git a/db/migrate/20240213044159_copy_oidc_data_to_oidc_accounts.rb b/db/migrate/20240213044159_copy_oidc_data_to_oidc_accounts.rb new file mode 100644 index 0000000000..72b1d88d14 --- /dev/null +++ b/db/migrate/20240213044159_copy_oidc_data_to_oidc_accounts.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CopyOidcDataToOidcAccounts < ActiveRecord::Migration[7.0] + def up + execute <<~SQL.squish + INSERT INTO oidc_accounts (user_id, provider, uid, created_at, updated_at) + SELECT id, provider, uid, updated_at, updated_at + FROM spree_users WHERE provider IS NOT NULL + SQL + end + + def down + execute "DELETE FROM oidc_accounts" + end +end diff --git a/db/schema.rb b/db/schema.rb index 312cf4e23e..0ab2fb2f2e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_01_15_022359) do +ActiveRecord::Schema[7.0].define(version: 2024_02_13_044159) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "plpgsql" @@ -308,6 +308,18 @@ ActiveRecord::Schema[7.0].define(version: 2024_01_15_022359) do t.index ["order_id"], name: "index_invoices_on_order_id" end + create_table "oidc_accounts", force: :cascade do |t| + t.bigint "user_id", null: false + t.string "provider" + t.string "uid", null: false + t.string "token" + t.string "refresh_token" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["uid"], name: "index_oidc_accounts_on_uid", unique: true + t.index ["user_id"], name: "index_oidc_accounts_on_user_id", unique: true + end + create_table "order_cycle_schedules", id: :serial, force: :cascade do |t| t.integer "order_cycle_id", null: false t.integer "schedule_id", null: false @@ -1146,6 +1158,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_01_15_022359) do add_foreign_key "inventory_items", "enterprises" add_foreign_key "inventory_items", "spree_variants", column: "variant_id" add_foreign_key "invoices", "spree_orders", column: "order_id" + add_foreign_key "oidc_accounts", "spree_users", column: "user_id" add_foreign_key "order_cycle_schedules", "order_cycles", name: "oc_schedules_order_cycle_id_fk" add_foreign_key "order_cycle_schedules", "schedules", name: "oc_schedules_schedule_id_fk" add_foreign_key "order_cycles", "enterprises", column: "coordinator_id", name: "order_cycles_coordinator_id_fk" diff --git a/engines/dfc_provider/app/services/authorization_control.rb b/engines/dfc_provider/app/services/authorization_control.rb index e2643b43fd..254c6cee67 100644 --- a/engines/dfc_provider/app/services/authorization_control.rb +++ b/engines/dfc_provider/app/services/authorization_control.rb @@ -57,6 +57,6 @@ class AuthorizationControl def find_ofn_user(payload) return if payload["email"].blank? - Spree::User.find_by(uid: payload["email"]) + OidcAccount.find_by(uid: payload["email"])&.user end end diff --git a/engines/dfc_provider/spec/services/authorization_control_spec.rb b/engines/dfc_provider/spec/services/authorization_control_spec.rb index ed1f4cfe97..97cd646df3 100644 --- a/engines/dfc_provider/spec/services/authorization_control_spec.rb +++ b/engines/dfc_provider/spec/services/authorization_control_spec.rb @@ -16,9 +16,8 @@ describe AuthorizationControl do end it "ignores blank email" do - create(:user, uid: nil) - create(:user, uid: "") - token = allow_token_for(email: nil) + OidcAccount.where(user:).update_all(uid: "") + token = allow_token_for(email: "") expect(auth(oidc_token: token).user).to eq nil end diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb index e52d6c878d..80f2ecf432 100644 --- a/spec/factories/user_factory.rb +++ b/spec/factories/user_factory.rb @@ -30,14 +30,18 @@ FactoryBot.define do end end + factory :enterprise_user do + enterprises { [build(:enterprise)] } + end + factory :admin_user do spree_roles { [Spree::Role.find_or_create_by!(name: 'admin')] } end factory :oidc_user do - after(:create) do |user| - user.update uid: user.email - end + oidc_account { + OidcAccount.new(provider: "openid_connect", uid: email) + } end end end diff --git a/spec/fixtures/files/omniauth.auth.json b/spec/fixtures/files/omniauth.auth.json new file mode 100644 index 0000000000..d89f690f04 --- /dev/null +++ b/spec/fixtures/files/omniauth.auth.json @@ -0,0 +1,48 @@ +{ + "provider": "openid_connect", + "uid": "ofn@example.com", + "info": { + "name": "OFN Developer", + "email": "ofn@example.com", + "email_verified": false, + "nickname": "ofn@example.com", + "first_name": "OFN", + "last_name": "Developer", + "gender": null, + "image": null, + "phone": null, + "urls": { + "website": null + } + }, + "credentials": { + "id_token": "ey...id_token...zg", + "token": "ey...token...9g", + "refresh_token": "ey...refresh_token...bk", + "expires_in": 1800, + "scope": "openid profile email" + }, + "extra": { + "raw_info": { + "sub": "97da8027-a7a9-44c8-9cfd-ad639cec8630", + "email_verified": false, + "name": "OFN Developer", + "preferred_username": "ofn@example.com", + "given_name": "OFN", + "family_name": "Developer", + "email": "ofn@example.com", + "exp": 1707458565, + "iat": 1707456765, + "auth_time": 1707456763, + "jti": "00643994-5914-4699-96b0-2b4a308fca65", + "iss": "https://login.lescommuns.org/auth/realms/data-food-consortium", + "aud": "coopcircuits", + "typ": "ID", + "azp": "coopcircuits", + "nonce": "215831991b35c70d43fb2102ee78be55", + "session_state": "8b5725a1-e83a-4f78-a54b-36c5a2983dd4", + "at_hash": "RT8oVVJdFDiaytyDxHJLyQ", + "sid": "8b5725a1-e83a-4f78-a54b-36c5a2983dd4" + } + } +} diff --git a/spec/migrations/20240213044159_copy_oidc_data_to_oidc_accounts_spec.rb b/spec/migrations/20240213044159_copy_oidc_data_to_oidc_accounts_spec.rb new file mode 100644 index 0000000000..d2bbd81694 --- /dev/null +++ b/spec/migrations/20240213044159_copy_oidc_data_to_oidc_accounts_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_relative '../../db/migrate/20240213044159_copy_oidc_data_to_oidc_accounts' + +describe CopyOidcDataToOidcAccounts do + describe "up" do + let!(:user) { create(:user) } + let!(:oidc_user) { + create(:user, provider: "openid_connect", uid: "ofn@example.net") + } + + it "copies data" do + expect { subject.up }.to change { + OidcAccount.count + }.from(0).to(1) + + account = OidcAccount.first + + expect(account.user).to eq oidc_user + expect(account.provider).to eq oidc_user.provider + expect(account.uid).to eq oidc_user.uid + expect(account.token).to eq nil + end + end + + describe "down" do + it "removes data" do + user = create(:user) + OidcAccount.create!(user:, provider: "oidc", uid: "ofn@exmpl.net") + + expect { subject.down }.to change { OidcAccount.count }.from(1).to(0) + end + end +end diff --git a/spec/models/oidc_account_spec.rb b/spec/models/oidc_account_spec.rb new file mode 100644 index 0000000000..7d0ef6939e --- /dev/null +++ b/spec/models/oidc_account_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe OidcAccount, type: :model do + describe "associations and validations" do + subject { + OidcAccount.new( + user: build(:user), + provider: "openid_connect", + uid: "user@example.net" + ) + } + + it { is_expected.to belong_to :user } + it { is_expected.to validate_uniqueness_of :uid } + end + + describe ".link" do + let(:user) { create(:user, email: "user@example.com") } + let(:auth) { + OmniAuth::AuthHash.new( + JSON.parse(file_fixture("omniauth.auth.json").read) + ) + } + + it "creates or updates an account record" do + expect { OidcAccount.link(user, auth) } + .to change { OidcAccount.count }.by(1) + + account = OidcAccount.last + expect(account.user).to eq user + expect(account.provider).to eq "openid_connect" + expect(account.token).to match /^ey/ + expect(account.refresh_token).to match /^ey/ + + auth.uid = "user@example.org" + + expect { + OidcAccount.link(user, auth) + account.reload + } + .to change { OidcAccount.count }.by(0) + .and change { account.uid } + .from("ofn@example.com").to("user@example.org") + end + end +end diff --git a/spec/models/spree/user_spec.rb b/spec/models/spree/user_spec.rb index cc1284d050..8fc144b275 100644 --- a/spec/models/spree/user_spec.rb +++ b/spec/models/spree/user_spec.rb @@ -268,18 +268,4 @@ describe Spree::User do expect(user.disabled).to be_falsey end end - - describe "#link_from_omniauth" do - let!(:user) { create(:user, email: "user@email.com") } - let(:auth) { double(:auth, provider: "openid_connect", uid: "user@email.com") } - - it "creates a user without errors" do - user.link_from_omniauth(auth) - - expect(user.errors.present?).to be false - expect(user.confirmed?).to be true - expect(user.provider).to eq "openid_connect" - expect(user.uid).to eq "user@email.com" - end - end end diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index b96e79e7b0..0aad02a08c 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -2,55 +2,49 @@ require 'spec_helper' -describe OmniauthCallbacksController, type: :request do +# Devise calls OmniauthCallbacksController for OpenID Connect callbacks. +describe '/user/spree_user/auth/openid_connect/callback', type: :request do include AuthenticationHelper - OmniAuth.config.test_mode = true + let(:user) { create(:user) } + let(:params) { { code: 'code123' } } - subject do + before do + OmniAuth.config.test_mode = true login_as user - post '/user/spree_user/auth/openid_connect/callback', params: { code: 'code123' } - - request.env['devise.mapping'] = Devise.mappings[:spree_user] - request.env['omniauth.auth'] = omniauth_response end - let(:user) { create(:user) } + def request! + post(self.class.top_level_description, params:) + end context 'when the omniauth setup is returning with an authorization' do - let!(:omniauth_response) do + # The auth hash data has been observed by connecting to the Keycloak server + # https://login.lescommuns.org/. + before do OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new( - 'provider' => 'openid_connect', - 'uid' => 'john@doe.com', - 'info' => { - 'email' => 'john@doe.com', - 'first_name' => 'John', - 'last_name' => 'Doe' - } + JSON.parse(file_fixture("omniauth.auth.json").read) ) end it 'is successful' do - subject + expect { request! }.to change { OidcAccount.count }.by(1) - expect(user.provider).to eq "openid_connect" - expect(user.uid).to eq "john@doe.com" - expect(request.cookies[:omniauth_connect]).to be_nil + account = OidcAccount.last + expect(account.provider).to eq "openid_connect" + expect(account.uid).to eq "ofn@example.com" expect(response.status).to eq(302) end end context 'when the omniauth openid_connect is mocked with an error' do - let!(:omniauth_response) do + before do OmniAuth.config.mock_auth[:openid_connect] = :invalid_credentials end it 'fails with bad auth data' do - subject + expect { request! }.to_not change { OidcAccount.count } - expect(user.provider).to be_nil - expect(user.uid).to be_nil - expect(request.cookies[:omniauth_connect]).to be_nil expect(response.status).to eq(302) end end diff --git a/spec/system/admin/oidc_settings_spec.rb b/spec/system/admin/oidc_settings_spec.rb new file mode 100644 index 0000000000..160a9ca189 --- /dev/null +++ b/spec/system/admin/oidc_settings_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'system_helper' + +describe "OIDC Settings" do + it "requires login" do + visit admin_oidc_settings_path + expect(page).to have_button "Login" + expect(page).to_not have_button "Link your Les Communs OIDC Account" + end + + describe "with valid login" do + let(:user) { create(:enterprise_user) } + + before do + OmniAuth.config.test_mode = true + login_as user + end + + it "allows you to connect to an account and disconnect again" do + visit admin_oidc_settings_path + click_button "Link your Les Communs OIDC Account" + expect(page).to have_content "Your account is linked" + + click_button "Disconnect" + expect(page).to have_button "Link your Les Communs OIDC Account" + end + + it "allows you to refresh authorisation tokens" do + OidcAccount.create!(user:, provider: "openid_connect", uid: "a@b.com") + OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new( + JSON.parse(file_fixture("omniauth.auth.json").read) + ) + + visit admin_oidc_settings_path + + expect(page).to have_content "Tokens to access connected apps have expired" + click_button "Refresh authorisation" + expect(page).to_not have_content "Tokens to access connected apps have expired" + end + end +end