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
This commit is contained in:
Claire 2024-02-14 15:16:07 +01:00 committed by GitHub
parent 0b0c7af2c1
commit f1700523f1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 44 additions and 20 deletions

View file

@ -6,7 +6,7 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController
def self.provides_callback_for(provider) def self.provides_callback_for(provider)
define_method provider do define_method provider do
@provider = provider @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? if @user.persisted?
record_login_activity record_login_activity

View file

@ -19,17 +19,18 @@ module Omniauthable
end end
class_methods do class_methods do
def find_for_oauth(auth, signed_in_resource = nil) def find_for_omniauth(auth, signed_in_resource = nil)
# EOLE-SSO Patch # EOLE-SSO Patch
auth.uid = (auth.uid[0][:uid] || auth.uid[0][:user]) if auth.uid.is_a? Hashie::Array 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 # If a signed_in_resource is provided it always overrides the existing user
# to prevent the identity being locked with accidentally created accounts. # to prevent the identity being locked with accidentally created accounts.
# Note that this may leave zombie accounts (with no associated identity) which # Note that this may leave zombie accounts (with no associated identity) which
# can be cleaned up at a later date. # can be cleaned up at a later date.
user = signed_in_resource || identity.user 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? if identity.user.nil?
identity.user = user identity.user = user
@ -39,19 +40,35 @@ module Omniauthable
user user
end end
def create_for_oauth(auth) private
# Check if the user exists with provided email. If no email was provided,
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 # we assign a temporary email and ask the user to verify it on
# the next step via Auth::SetupController.show # the next step via Auth::SetupController.show
strategy = Devise.omniauth_configs[auth.provider.to_sym].strategy email, email_is_verified = email_from_auth(auth)
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?
user = User.new(user_params_from_auth(email, auth)) user = User.new(user_params_from_auth(email, auth))
@ -66,7 +83,14 @@ module Omniauthable
user user
end 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) def user_params_from_auth(email, auth)
{ {

View file

@ -17,7 +17,7 @@ class Identity < ApplicationRecord
validates :uid, presence: true, uniqueness: { scope: :provider } validates :uid, presence: true, uniqueness: { scope: :provider }
validates :provider, presence: true 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) find_or_create_by(uid: auth.uid, provider: auth.provider)
end end
end end

View file

@ -3,16 +3,16 @@
require 'rails_helper' require 'rails_helper'
RSpec.describe Identity do RSpec.describe Identity do
describe '.find_for_oauth' do describe '.find_for_omniauth' do
let(:auth) { Fabricate(:identity, user: Fabricate(:user)) } let(:auth) { Fabricate(:identity, user: Fabricate(:user)) }
it 'calls .find_or_create_by' do it 'calls .find_or_create_by' do
expect(described_class).to receive(:find_or_create_by).with(uid: auth.uid, provider: auth.provider) expect(described_class).to receive(:find_or_create_by).with(uid: auth.uid, provider: auth.provider)
described_class.find_for_oauth(auth) described_class.find_for_omniauth(auth)
end end
it 'returns an instance of Identity' do 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 end
end end

View file

@ -96,7 +96,7 @@ describe 'OmniAuth callbacks' do
context 'when a user cannot be built' do context 'when a user cannot be built' do
before 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 end
it 'redirects to the new user signup page' do it 'redirects to the new user signup page' do