From 9b935be4d666780c9dfabb8464a5f81a20e90db0 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 3 Feb 2025 13:33:42 +1100 Subject: [PATCH 1/3] Catch error and provide message --- app/controllers/omniauth_callbacks_controller.rb | 3 +++ config/locales/en.yml | 1 + 2 files changed, 4 insertions(+) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index c483d6f4b7..d55c5600e2 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -4,6 +4,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController def openid_connect OidcAccount.link(spree_current_user, request.env["omniauth.auth"]) + redirect_to admin_oidc_settings_path + rescue ActiveRecord::RecordNotUnique + flash[:error] = t("devise.oidc.record_not_unique", uid: request.env["omniauth.auth"].uid) redirect_to admin_oidc_settings_path end diff --git a/config/locales/en.yml b/config/locales/en.yml index 946f9c24d7..6e5d5f6f3d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -321,6 +321,7 @@ en: send_instructions: "You will receive an email with instructions about how to confirm your account in a few minutes." oidc: failure: "Could not sign in: %{error}" + record_not_unique: "%{uid} is already associated with another account" home_page_alert_html: "Home page alert HTML" hub_signup_case_studies_html: "Hub signup case studies HTML" hub_signup_detail_html: "Hub signup detail HTML" From f6f1a005cbe0b65299538e04507329a5a63d92eb Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 12 Feb 2025 16:07:44 +1100 Subject: [PATCH 2/3] Add spec Oh, and a transaction block. Because the controller after hooks tried to update the DB which resulted in PG::InFailedSqlTransaction: ERROR: current transaction is aborted, commands ignored until end of transaction block Even for a small rescue statement, it's worth adding a spec. You never know what might not be working! --- app/controllers/omniauth_callbacks_controller.rb | 4 +++- .../omniauth_callbacks_controller_spec.rb | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index d55c5600e2..31f9a29ac3 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -2,7 +2,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController def openid_connect - OidcAccount.link(spree_current_user, request.env["omniauth.auth"]) + ActiveRecord::Base.transaction do + OidcAccount.link(spree_current_user, request.env["omniauth.auth"]) + end redirect_to admin_oidc_settings_path rescue ActiveRecord::RecordNotUnique diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index aef5ef69d6..9e8d9979f8 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -35,6 +35,20 @@ RSpec.describe '/user/spree_user/auth/openid_connect/callback', type: :request d expect(account.uid).to eq "ofn@example.com" expect(response.status).to eq(302) end + + context 'when OIDC account already linked with a different user' do + before do + other_user = create(:user, email: "ofn@elsewhere.com") + OidcAccount.create! user_id: other_user.id, uid: "ofn@example.com" + end + + it 'fails with error message' do + expect { request! }.not_to change { OidcAccount.count } + + expect(response.status).to eq(302) + expect(flash[:error]).to match "ofn@example.com is already associated with another account" + end + end end context 'when the omniauth openid_connect is mocked with an error' do @@ -46,6 +60,7 @@ RSpec.describe '/user/spree_user/auth/openid_connect/callback', type: :request d expect { request! }.not_to change { OidcAccount.count } expect(response.status).to eq(302) + expect(flash[:error]).to match "Could not sign in" end end end From 13f5009563fbc2ef3fa6c0b112d418a00e635df8 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 13 Feb 2025 09:19:03 +1100 Subject: [PATCH 3/3] Refactor Co-authored-by: Maikel --- spec/requests/omniauth_callbacks_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 9e8d9979f8..6c02540b33 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -38,8 +38,8 @@ RSpec.describe '/user/spree_user/auth/openid_connect/callback', type: :request d context 'when OIDC account already linked with a different user' do before do - other_user = create(:user, email: "ofn@elsewhere.com") - OidcAccount.create! user_id: other_user.id, uid: "ofn@example.com" + create(:user, email: "ofn@elsewhere.com") + .create_oidc_account!(uid: "ofn@example.com") end it 'fails with error message' do