From 53b73ed6a27d2d0efe8424729b3f5a90e6fc3f42 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 14 Feb 2024 15:16:07 +0100 Subject: [PATCH] Merge pull request from GHSA-vm39-j3vx-pch3 * Prevent different identities from a same SSO provider from accessing a same account * Lock auth provider changes behind `ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH=true` * Rename methods to avoid confusion between OAuth and OmniAuth --- .../auth/omniauth_callbacks_controller.rb | 2 +- app/models/concerns/user/omniauthable.rb | 52 ++++++++++++++----- app/models/identity.rb | 2 +- spec/models/identity_spec.rb | 6 +-- spec/requests/omniauth_callbacks_spec.rb | 2 +- 5 files changed, 44 insertions(+), 20 deletions(-) diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb index 707b50ef9e..9b83de945b 100644 --- a/app/controllers/auth/omniauth_callbacks_controller.rb +++ b/app/controllers/auth/omniauth_callbacks_controller.rb @@ -7,7 +7,7 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController def self.provides_callback_for(provider) define_method provider do @provider = provider - @user = User.find_for_oauth(request.env['omniauth.auth'], current_user) + @user = User.find_for_omniauth(request.env['omniauth.auth'], current_user) if @user.persisted? record_login_activity diff --git a/app/models/concerns/user/omniauthable.rb b/app/models/concerns/user/omniauthable.rb index 113bfda230..396a0598f8 100644 --- a/app/models/concerns/user/omniauthable.rb +++ b/app/models/concerns/user/omniauthable.rb @@ -19,17 +19,18 @@ module User::Omniauthable end class_methods do - def find_for_oauth(auth, signed_in_resource = nil) + def find_for_omniauth(auth, signed_in_resource = nil) # EOLE-SSO Patch auth.uid = (auth.uid[0][:uid] || auth.uid[0][:user]) if auth.uid.is_a? Hashie::Array - identity = Identity.find_for_oauth(auth) + identity = Identity.find_for_omniauth(auth) # If a signed_in_resource is provided it always overrides the existing user # to prevent the identity being locked with accidentally created accounts. # Note that this may leave zombie accounts (with no associated identity) which # can be cleaned up at a later date. user = signed_in_resource || identity.user - user ||= create_for_oauth(auth) + user ||= reattach_for_auth(auth) + user ||= create_for_auth(auth) if identity.user.nil? identity.user = user @@ -39,19 +40,35 @@ module User::Omniauthable user end - def create_for_oauth(auth) - # Check if the user exists with provided email. If no email was provided, + private + + def reattach_for_auth(auth) + # If allowed, check if a user exists with the provided email address, + # and return it if they does not have an associated identity with the + # current authentication provider. + + # This can be used to provide a choice of alternative auth providers + # or provide smooth gradual transition between multiple auth providers, + # but this is discouraged because any insecure provider will put *all* + # local users at risk, regardless of which provider they registered with. + + return unless ENV['ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH'] == 'true' + + email, email_is_verified = email_from_auth(auth) + return unless email_is_verified + + user = User.find_by(email: email) + return if user.nil? || Identity.exists?(provider: auth.provider, user_id: user.id) + + user + end + + def create_for_auth(auth) + # Create a user for the given auth params. If no email was provided, # we assign a temporary email and ask the user to verify it on # the next step via Auth::SetupController.show - strategy = Devise.omniauth_configs[auth.provider.to_sym].strategy - assume_verified = strategy&.security&.assume_email_is_verified - email_is_verified = auth.info.verified || auth.info.verified_email || auth.info.email_verified || assume_verified - email = auth.info.verified_email || auth.info.email - - user = User.find_by(email: email) if email_is_verified - - return user unless user.nil? + email, email_is_verified = email_from_auth(auth) user = User.new(user_params_from_auth(email, auth)) @@ -66,7 +83,14 @@ module User::Omniauthable user end - private + def email_from_auth(auth) + strategy = Devise.omniauth_configs[auth.provider.to_sym].strategy + assume_verified = strategy&.security&.assume_email_is_verified + email_is_verified = auth.info.verified || auth.info.verified_email || auth.info.email_verified || assume_verified + email = auth.info.verified_email || auth.info.email + + [email, email_is_verified] + end def user_params_from_auth(email, auth) { diff --git a/app/models/identity.rb b/app/models/identity.rb index c95a68a6f6..77821b78fa 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -17,7 +17,7 @@ class Identity < ApplicationRecord validates :uid, presence: true, uniqueness: { scope: :provider } validates :provider, presence: true - def self.find_for_oauth(auth) + def self.find_for_omniauth(auth) find_or_create_by(uid: auth.uid, provider: auth.provider) end end diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index 7022454443..d5a2ffbc86 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -3,19 +3,19 @@ require 'rails_helper' RSpec.describe Identity do - describe '.find_for_oauth' do + describe '.find_for_omniauth' do let(:auth) { Fabricate(:identity, user: Fabricate(:user)) } it 'calls .find_or_create_by' do allow(described_class).to receive(:find_or_create_by) - described_class.find_for_oauth(auth) + described_class.find_for_omniauth(auth) expect(described_class).to have_received(:find_or_create_by).with(uid: auth.uid, provider: auth.provider) end it 'returns an instance of Identity' do - expect(described_class.find_for_oauth(auth)).to be_instance_of described_class + expect(described_class.find_for_omniauth(auth)).to be_instance_of described_class end end end diff --git a/spec/requests/omniauth_callbacks_spec.rb b/spec/requests/omniauth_callbacks_spec.rb index 0d37c41140..b478ca1ce6 100644 --- a/spec/requests/omniauth_callbacks_spec.rb +++ b/spec/requests/omniauth_callbacks_spec.rb @@ -96,7 +96,7 @@ describe 'OmniAuth callbacks' do context 'when a user cannot be built' do before do - allow(User).to receive(:find_for_oauth).and_return(User.new) + allow(User).to receive(:find_for_omniauth).and_return(User.new) end it 'redirects to the new user signup page' do