Allow existing users to log-in with SSO

[hosamaly]

Previously, a user could only log in with one method. They could use credentials or an SSO provider, but they couldn't use SSO once they were registered with credentials. And once they were registered using one SSO provider, they couldn't log in with any others.

This PR introduces the ability to associate multiple SSO providers with a user, and allows the use of SSO for previously-registered users.

[hosamaly]

As we're writing to two tables, I wrapped it in a transaction to ensure data integrity.

[hosamaly]

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

	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)
	end

[hosamaly]

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.

[mcrutch]

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.

[hosamaly]

That's a valid concern. I can think of two solutions to it:

• Make this behaviour configurable from settings.yaml, defaulting to the previous behaviour if unspecified.
• Require the user to sign in with a known identity (or credentials) before associating their accounts.

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?

[mcrutch]

The first feels valid, justified. I am planning to spend a lot of time on NBG merge requests this Friday btw

[hosamaly]

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:

[mcrutch]

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.