From 9e5af6bb58241134a8ff313e40768b1b067e5715 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 14 Feb 2024 22:49:45 +0100 Subject: [PATCH] Fix user creation failure handling in OAuth paths (#29207) Co-authored-by: Matt Jankowski --- .../auth/omniauth_callbacks_controller.rb | 3 + config/locales/devise.en.yml | 1 + spec/requests/omniauth_callbacks_spec.rb | 143 ++++++++++++++++++ spec/support/omniauth_mocks.rb | 7 + 4 files changed, 154 insertions(+) create mode 100644 spec/requests/omniauth_callbacks_spec.rb create mode 100644 spec/support/omniauth_mocks.rb diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb index 3c4984b3f3..3968537ad3 100644 --- a/app/controllers/auth/omniauth_callbacks_controller.rb +++ b/app/controllers/auth/omniauth_callbacks_controller.rb @@ -24,6 +24,9 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController session["devise.#{provider}_data"] = request.env['omniauth.auth'] redirect_to new_user_registration_url end + rescue ActiveRecord::RecordInvalid + flash[:alert] = I18n.t('devise.failure.omniauth_user_creation_failure') if is_navigational_format? + redirect_to new_user_session_url end end diff --git a/config/locales/devise.en.yml b/config/locales/devise.en.yml index 458fa6d759..d47b38321b 100644 --- a/config/locales/devise.en.yml +++ b/config/locales/devise.en.yml @@ -12,6 +12,7 @@ en: last_attempt: You have one more attempt before your account is locked. locked: Your account is locked. not_found_in_database: Invalid %{authentication_keys} or password. + omniauth_user_creation_failure: Error creating an account for this identity. pending: Your account is still under review. timeout: Your session expired. Please sign in again to continue. unauthenticated: You need to sign in or sign up before continuing. diff --git a/spec/requests/omniauth_callbacks_spec.rb b/spec/requests/omniauth_callbacks_spec.rb new file mode 100644 index 0000000000..095535e485 --- /dev/null +++ b/spec/requests/omniauth_callbacks_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe 'OmniAuth callbacks' do + shared_examples 'omniauth provider callbacks' do |provider| + subject { post send :"user_#{provider}_omniauth_callback_path" } + + context 'with full information in response' do + before do + mock_omniauth(provider, { + provider: provider.to_s, + uid: '123', + info: { + verified: 'true', + email: 'user@host.example', + }, + }) + end + + context 'without a matching user' do + it 'creates a user and an identity and redirects to root path' do + expect { subject } + .to change(User, :count) + .by(1) + .and change(Identity, :count) + .by(1) + .and change(LoginActivity, :count) + .by(1) + + expect(User.last.email).to eq('user@host.example') + expect(Identity.find_by(user: User.last).uid).to eq('123') + expect(response).to redirect_to(root_path) + end + end + + context 'with a matching user and no matching identity' do + before do + Fabricate(:user, email: 'user@host.example') + end + + context 'when ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH is set to true' do + around do |example| + ClimateControl.modify ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH: 'true' do + example.run + end + end + + it 'matches the existing user, creates an identity, and redirects to root path' do + expect { subject } + .to not_change(User, :count) + .and change(Identity, :count) + .by(1) + .and change(LoginActivity, :count) + .by(1) + + expect(Identity.find_by(user: User.last).uid).to eq('123') + expect(response).to redirect_to(root_path) + end + end + + context 'when ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH is not set to true' do + it 'does not match the existing user or create an identity, and redirects to login page' do + expect { subject } + .to not_change(User, :count) + .and not_change(Identity, :count) + .and not_change(LoginActivity, :count) + + expect(response).to redirect_to(new_user_session_url) + end + end + end + + context 'with a matching user and a matching identity' do + before do + user = Fabricate(:user, email: 'user@host.example') + Fabricate(:identity, user: user, uid: '123', provider: provider) + end + + it 'matches the existing records and redirects to root path' do + expect { subject } + .to not_change(User, :count) + .and not_change(Identity, :count) + .and change(LoginActivity, :count) + .by(1) + + expect(response).to redirect_to(root_path) + end + end + end + + context 'with a response missing email address' do + before do + mock_omniauth(provider, { + provider: provider.to_s, + uid: '123', + info: { + verified: 'true', + }, + }) + end + + it 'redirects to the auth setup page' do + expect { subject } + .to change(User, :count) + .by(1) + .and change(Identity, :count) + .by(1) + .and change(LoginActivity, :count) + .by(1) + + expect(response).to redirect_to(auth_setup_path(missing_email: '1')) + end + end + + context 'when a user cannot be built' do + before do + allow(User).to receive(:find_for_omniauth).and_return(User.new) + end + + it 'redirects to the new user signup page' do + expect { subject } + .to not_change(User, :count) + .and not_change(Identity, :count) + .and not_change(LoginActivity, :count) + + expect(response).to redirect_to(new_user_registration_url) + end + end + end + + describe '#openid_connect', if: ENV['OIDC_ENABLED'] == 'true' && ENV['OIDC_SCOPE'].present? do + include_examples 'omniauth provider callbacks', :openid_connect + end + + describe '#cas', if: ENV['CAS_ENABLED'] == 'true' do + include_examples 'omniauth provider callbacks', :cas + end + + describe '#saml', if: ENV['SAML_ENABLED'] == 'true' do + include_examples 'omniauth provider callbacks', :saml + end +end diff --git a/spec/support/omniauth_mocks.rb b/spec/support/omniauth_mocks.rb new file mode 100644 index 0000000000..9883adec7a --- /dev/null +++ b/spec/support/omniauth_mocks.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +OmniAuth.config.test_mode = true + +def mock_omniauth(provider, data) + OmniAuth.config.mock_auth[provider] = OmniAuth::AuthHash.new(data) +end