-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow existing users to log-in with SSO #891
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,8 +9,10 @@ def self.find_with_omniauth(auth) | |
def self.create_with_omniauth(auth) | ||
Rails.logger.debug('Creating user from') | ||
Rails.logger.debug(auth) | ||
user = User.create_with_omniauth(auth.info, auth.provider) | ||
Rails.logger.debug(user.inspect) | ||
create(uid: auth['uid'], provider: auth['provider'], user_id: user.id) | ||
transaction do | ||
user = User.find_or_create_with_omniauth(auth.info, auth.provider) | ||
Rails.logger.debug(user.inspect) | ||
create!(uid: auth['uid'], provider: auth['provider'], user_id: user.id) | ||
end | ||
Comment on lines
+12
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we're writing to two tables, I wrapped it in a transaction to ensure data integrity. |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -573,16 +573,14 @@ def tag_recommendations(allow_run=true) | |
.sort_by {|_tag, count| -count + rand} | ||
end | ||
|
||
def self.create_with_omniauth(info, _provider) | ||
user = { | ||
email: info['email'], | ||
password: Devise.friendly_token[0, 20], | ||
confirmed_at: Time.now.utc.to_datetime.to_s, | ||
confirmation_token: nil, | ||
first_name: info.first_name, | ||
last_name: info.last_name | ||
} | ||
create!(user) | ||
def self.find_or_create_with_omniauth(info, _provider) | ||
find_or_create_by!(email: info.email) do |user| | ||
Comment on lines
-585
to
+577
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously, there was an assumption that an unknown OAuth identity must be related to an unknown user. This change allows existing users to associate their accounts with new OAuth identities as long as the email address matches. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was an intentional decision on this at one point in that inconsistencies of email validation of external services could lead to someone signing up with the same email at a different service and using that for login as well. Conversely, if that is a concern you have you might have evaluated that when you decided which oauth providers to enable. Want to think about this a bit moer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a valid concern. I can think of two solutions to it:
I think that the second option is best, but I don't think I have the time to implement it, so I'm in favour of the first one. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first feels valid, justified. I am planning to spend a lot of time on NBG merge requests this Friday btw There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made an attempt to make it configurable. I haven't tested it yet, but it runs along these lines. What do you think? diff --git i/app/controllers/callbacks_controller.rb w/app/controllers/callbacks_controller.rb
index 2920fe8e..caf4cb5b 100644
--- i/app/controllers/callbacks_controller.rb
+++ w/app/controllers/callbacks_controller.rb
@@ -10,10 +10,10 @@ class CallbacksController < Devise::OmniauthCallbacksController
Rails.logger.debug('Registering')
# If no identity was found, create a brand new one here
begin
- @identity = Identity.create_with_omniauth(auth)
+ @identity = register_new_identity(auth)
rescue StandardError => e
Rails.logger.error(e.message)
flash[:error] = "#{e.message.to_s}"
redirect_to root_url
return
end
@@ -47,4 +48,27 @@ class CallbacksController < Devise::OmniauthCallbacksController
#finish_signup_path(@user)
end
end
+
+ private
+
+ def register_new_identity(auth)
+ case sso_config.new_identities
+ when 'accept_all'
+ Identity.create_with_omniauth!(auth)
+ when 'accept_new_users_only'
+ raise ArgumentError, <<~MSG if User.omniauth_credentials_exist?(auth)
+ This user is registered using another sign-in method.
+ Please log in using the same method that was used to register.
+ MSG
+
+ Identity.create_with_omniauth!(auth)
+ else
+ Rails.logger.error("Unsupported SSO config #{sso_config.new_identities}!")
+ raise ArgumentError, 'SSO is misconfigured. Please contact the system administrator.'
+ end
+ end
+
+ def sso_config
+ GalleryConfig.registration.sso
+ end
end
diff --git i/app/models/identity.rb w/app/models/identity.rb
index 2bfafd3c..831c169e 100644
--- i/app/models/identity.rb
+++ w/app/models/identity.rb
@@ -6,11 +6,11 @@ class Identity < ApplicationRecord
find_by(uid: auth['uid'], provider: auth['provider'])
end
- def self.create_with_omniauth(auth)
+ def self.create_with_omniauth!(auth)
Rails.logger.debug('Creating user from')
Rails.logger.debug(auth)
transaction do
- user = User.find_or_create_with_omniauth(auth.info, auth.provider)
+ user = User.find_or_create_with_omniauth!(auth)
Rails.logger.debug(user.inspect)
create!(uid: auth['uid'], provider: auth['provider'], user_id: user.id)
end
diff --git i/app/models/user.rb w/app/models/user.rb
index df6e1e09..c6147fdd 100755
--- i/app/models/user.rb
+++ w/app/models/user.rb
@@ -570,7 +570,12 @@ class User < ApplicationRecord
.sort_by {|_tag, count| -count + rand}
end
+ def self.omniauth_credentials_exist?(auth)
+ where(email: auth.info.email).exists?
+ end
+
- def self.find_or_create_with_omniauth(info, _provider)
+ def self.find_or_create_with_omniauth!(auth)
+ info = auth.info
find_or_create_by!(email: info.email) do |user|
user.password = Devise.friendly_token[0, 20]
user.confirmed_at = Time.now.utc.to_datetime.to_s
diff --git i/config/settings.yml w/config/settings.yml
index 6465c146..2d19b818 100755
--- i/config/settings.yml
+++ w/config/settings.yml
@@ -185,6 +185,13 @@ notebooks:
registration:
require_admin_approval: false
allowed_domains: []
+ sso:
+ # What to do when a user signs in with an unknown SSO identity
+ # Options:
+ # * accept_all: register new users, and associate new identities with existing users
+ # * accept_new_users_only: register new users, but reject new identities for existing users
+ # * require_login: (not implemented!) require the user to log in with a known identity to associate the new one
+ new_identities: accept_new_users_only
# Kernel language display
languages: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems sane, unfortunately my oauth test environment I have right now is totally screwed up so even with this pushed, I will have a hard time testing it at the moment. |
||
user.password = Devise.friendly_token[0, 20] | ||
user.confirmed_at = Time.now.utc.to_datetime.to_s | ||
user.confirmation_token = nil | ||
user.first_name = info.first_name | ||
user.last_name = info.last_name | ||
end | ||
end | ||
|
||
def active_for_authentication? | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
require 'test_helper' | ||
|
||
class IdentityTest < ActiveSupport::TestCase | ||
def user_info | ||
@user_info ||= OmniAuth::AuthHash.new( | ||
name: 'Mr X', | ||
first_name: 'Mr', | ||
last_name: 'X', | ||
email: 'mr.x@example.com', | ||
nickname: 'mr.x@example.com' | ||
) | ||
end | ||
|
||
def omniauth_hash | ||
@omniauth_hash ||= OmniAuth::AuthHash.new( | ||
provider: %w[azure_activedirectory_v2 facebook github gitlab google_oauth2 twitter].sample, | ||
uid: 'e3b8c0b1-68de-454e-8cdb-d7e2d8e9390a', | ||
info: user_info | ||
) | ||
end | ||
|
||
def ensure_user_does_not_exist | ||
User.destroy_by(email: user_info.email) # if it exists | ||
end | ||
|
||
def create_user | ||
User.create!( | ||
password: 'Password #123456', | ||
**user_info.slice(:email, :first_name, :last_name) | ||
) | ||
end | ||
|
||
setup do | ||
ensure_user_does_not_exist | ||
end | ||
|
||
test 'it creates new users' do | ||
identity = assert_difference -> {User.count + Identity.count}, 2 do | ||
Identity.create_with_omniauth(omniauth_hash) | ||
end | ||
|
||
assert_equal omniauth_hash.uid, identity.uid | ||
|
||
user = identity.user | ||
assert_equal user_info.email, user.email | ||
assert_equal user_info.first_name, user.first_name | ||
assert_equal user_info.last_name, user.last_name | ||
end | ||
|
||
test 'it associates existing users with new SSO identities' do | ||
user = create_user | ||
identity = assert_difference -> {Identity.count} do | ||
Identity.create_with_omniauth(omniauth_hash) | ||
end | ||
|
||
assert_equal user.id, identity.user_id | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User.create_with_omniauth
uses the bang version (create!
) to raise any errors, so I thought it made sense that this method did the same to unify error handling logic in the caller.nbgallery/app/models/user.rb
Lines 576 to 586 in cd6116d