Merge branch 'release/6.0.9'

app/controllers/auth/omniauth_callbacks_controller.rb

@@ -56,7 +56,30 @@ def privacy_prompt
   end
 
   def accept_privacy_policy
-    sign_in_new_user_from_session!
+    identity = create_new_user_and_identity!
+
+    sign_in!(identity)
+  end
+
+  # Confirm duplicate email before new user creation
+
+  def confirm_new_user
+    @institution = provider.institution
+    @users = User.where(email: auth_email)
+    @email = auth_email
+    store_hash_in_session!
+    render 'auth/confirm_new_user'
+  end
+
+  def accept_confirm_new_user
+    # Redirect to privacy prompt before we create a new private user
+    return redirect_to_privacy_prompt if provider&.institution.nil?
+
+    # Institutional users don't need to accept the privacy policy
+    # Thus we can immediately create a new user
+    identity = create_new_user_and_identity!
+
+    sign_in!(identity)
   end
 
   private
@@ -99,38 +122,42 @@ def try_login!
       # If we found an existing user, which already has an identity for this provider
       # This will require a manual intervention by the development team, notify the user and the team
       return redirect_duplicate_email_for_provider! if user&.providers&.exists?(id: provider.id)
-      # If we found an existing user with the same username or email
+      # If we found an existing user with the same username or email within the same institution
       # We will ask the user to verify if this was the user they wanted to sign in to
       # if yes => redirect to a previously used provider for this user
       # if no => contact dodona for a manual intervention
       return redirect_to_known_provider!(user) if user.present?
 
+      # Try to find if the email address is already in use in an other institution
+      # If so, ask the user to confirm to which account they want to sign in
+      users_with_same_email = auth_email.present? && User.where(email: auth_email)
+      return redirect_to_confirm_new_user! if users_with_same_email.present?
+
       # No existing user was found
       # Redirect to privacy prompt before we create a new private user
       return redirect_to_privacy_prompt if provider&.institution.nil?
 
       # Institutional users don't need to accept the privacy policy
       # Thus we can immediately create a new user
-      user = User.new institution: provider&.institution
-      # Create a new identity for the newly created user
-      identity = user.identities.build identifier: auth_uid, provider: provider
+      identity = create_new_user_and_identity!
     end
 
+    sign_in!(identity)
+  end
+
+  # ==> Utilities.
+
+  def sign_in!(identity)
     # Validation.
     raise 'Identity should not be nil here' if identity.nil?
 
     # Get the user independent of it being newly created or an existing user
     user = identity.user
+
     # Update the user information from the authentication response.
     user.update_from_provider(auth_hash, provider)
     return redirect_with_errors!(user) if user.errors.any?
 
-    sign_in!(user)
-  end
-
-  # ==> Utilities.
-
-  def sign_in!(user)
     # If the session contains credentials for another identity, add this identity to the signed in user
     create_identity_from_session!(user)
 
@@ -143,38 +170,14 @@ def sign_in!(user)
   end
 
   def redirect_to_privacy_prompt
-    session[:new_user_identifier] = auth_hash.uid
-    session[:new_user_username] = auth_hash.info.username || auth_hash.uid
-    session[:new_user_email] = auth_hash.info.email
-    session[:new_user_first_name] = auth_hash.info.first_name
-    session[:new_user_last_name] = auth_hash.info.last_name
-    session[:new_user_provider_id] = provider&.id
-    session[:new_user_auth_target] = auth_target
-
+    store_hash_in_session!
     redirect_to privacy_prompt_path
   end
 
-  def sign_in_new_user_from_session!
-    identifier = session.delete(:new_user_identifier)
-    username = session.delete(:new_user_username)
-    email = session.delete(:new_user_email)
-    first_name = session.delete(:new_user_first_name)
-    last_name = session.delete(:new_user_last_name)
-    provider_id = session.delete(:new_user_provider_id)
-
-    provider = Provider.find(provider_id)
-
-    # Create a new user
-    user = User.new institution: provider&.institution, email: email, first_name: first_name, last_name: last_name,
-                    username: username
-
+  def create_new_user_and_identity!
+    user = User.new institution: provider&.institution
     # Create a new identity for the newly created user
-    user.identities.build identifier: identifier, provider: provider
-    user.save
-
-    return redirect_with_errors!(user) if user.errors.any?
-
-    sign_in!(user)
+    user.identities.build identifier: auth_uid, provider: provider
   end
 
   def create_identity_from_session!(user)
@@ -220,7 +223,7 @@ def find_identity_by_uid
       identity = Identity.find_by(identifier: auth_email.split('@').first, provider: provider, identifier_based_on_email: true)
 
       # Try to find user by preferred username
-      identity = Identity.find_by(identifier: auth_hash.extra.raw_info.preferred_username.split('@').first, provider: provider, identifier_based_on_email: true) if identity.nil? && auth_hash&.extra&.raw_info&.preferred_username.present?
+      identity = Identity.find_by(identifier: auth_hash.extra.preferred_username.split('@').first, provider: provider, identifier_based_on_email: true) if identity.nil? && auth_hash&.extra&.preferred_username.present?
 
       # Try to find user by name
       identity = Identity.joins(:user).find_by(user: { first_name: auth_hash.info.first_name, last_name: auth_hash.info.last_name }, provider: provider, identifier_based_on_email: true) if identity.nil?
@@ -298,6 +301,12 @@ def store_identity_in_session!
     session[:auth_original_uid] = auth_uid unless provider.redirect?
   end
 
+  def store_hash_in_session!
+    # Filter raw info and credentials from hash to limit cookie size
+    hash = auth_hash.except('extra', 'credentials').merge({ 'extra' => auth_hash.extra&.except('raw_info') })
+    session[:new_user_auth_hash] = hash.to_json
+  end
+
   def redirect_to_provider!(target_provider)
     store_identity_in_session!
 
@@ -357,6 +366,11 @@ def redirect_to_known_provider!(user)
     render 'auth/redirect_to_known_provider'
   end
 
+  def redirect_to_confirm_new_user!
+    store_hash_in_session!
+    redirect_to confirm_new_user_path
+  end
+
   def redirect_duplicate_email_for_provider!
     ApplicationMailer.with(
       authinfo: auth_hash,
@@ -385,11 +399,17 @@ def redirect_to_target!(user)
   # ==> Shorthands.
 
   def target_path(user)
-    auth_target || session.delete(:new_user_auth_target) || after_sign_in_path_for(user)
+    auth_target || after_sign_in_path_for(user)
   end
 
   def auth_hash
-    request.env['omniauth.auth']
+    return request.env['omniauth.auth'] if request.env['omniauth.auth'].present?
+
+    # if auth hash was present in session, we can use that
+    # we do want to remove it from the session so it does not stay there indefinitely
+    @new_user_auth_hash = JSON.parse(session.delete(:new_user_auth_hash), object_class: OmniAuth::AuthHash) if session[:new_user_auth_hash].present?
+
+    @new_user_auth_hash
   end
 
   def auth_email
@@ -430,7 +450,7 @@ def oauth_provider_id
 
   def provider
     # Extract the provider from the authentication hash.
-    return auth_hash.extra.provider if [Provider::Lti.sym, Provider::Saml.sym].include?(auth_provider_type)
+    return Provider.find(auth_hash.extra.provider_id) if [Provider::Lti.sym, Provider::Saml.sym].include?(auth_provider_type)
 
     # Fallback to an oauth provider
     oauth_provider

app/models/user.rb

@@ -111,7 +111,7 @@ class User < ApplicationRecord
   devise :omniauthable, omniauth_providers: %i[google_oauth2 lti office365 oidc saml smartschool surf]
 
   validates :username, uniqueness: { case_sensitive: false, allow_blank: true, scope: :institution }
-  validates :email, uniqueness: { case_sensitive: false, allow_blank: true }
+  validates :email, uniqueness: { case_sensitive: false, allow_blank: true, scope: :institution }
   validate :max_one_institution
   validate :provider_allows_blank_email
 

app/views/auth/confirm_new_user.html.erb

@@ -0,0 +1,37 @@
+<div class="row">
+  <div class="col-md-10">
+    <div class="row">
+      <h1><%= t ".title" %></h1>
+      <% if @users.length > 1 %>
+        <%= markdown t(".message_plural_p1", email: @email) %>
+      <% else %>
+        <%= markdown t(".message_single_p1", email: @email) %>
+      <% end %>
+      <% if @institution.present? %>
+        <%= markdown t(".message_institutional_p2", institution: @institution.name) %>
+      <% else %>
+        <%= markdown t(".message_personal_p2") %>
+      <% end %>
+      <div>
+        <%= link_to t(".create_new_account"), confirm_new_user_path, method: :post, class: "btn btn-primary" %>
+      </div>
+    </div>
+    <% @users.each do |user| %>
+      <div class="row pt-4">
+        <h3>
+          <% if user.institutional? %>
+            <%= markdown t(".sign_in_to_institutional", institution: user.institution.name, name: user.full_name) %>
+          <% else %>
+            <%= markdown t(".sign_in_to_personal", name: user.full_name) %>
+          <% end %>
+        </h3>
+        <% user.providers.where(mode: %i[prefer secondary]).each do |provider| %>
+          <%= render partial: 'auth/provider_button', locals: { provider: provider } %>
+        <% end %>
+      </div>
+    <% end %>
+    <div class="row pt-4">
+      <p><%= t ".contact_support_html", form: link_to(t(".contact_form"), contact_path)%></p>
+    </div>
+  </div>
+</div>

config/initializers/00_version.rb

@@ -3,7 +3,7 @@ class Application
     module Version
       MAJOR = 6
       MINOR = 0
-      PATCH = 8
+      PATCH = 9
 
       STRING = [MAJOR, MINOR, PATCH].compact.join('.')
     end

config/initializers/office365.rb

@@ -44,7 +44,8 @@ def authorize_params
 
       extra do
         {
-            'raw_info' => raw_info
+            'raw_info' => raw_info,
+            'preferred_username' => raw_info['preferred_username']
         }
       end
 

config/locales/views/auth/en.yml

@@ -29,3 +29,15 @@ en:
       text_html: "In order to use Dodona, you need to accept our privacy statement. On the <a href=\"%{your_data}\" target=\"_blank\">your data</a> page we explain in clear and understandable language what data we collect and how we use it. Our <a href=\"%{privacy_statement}\" target=\"_blank\">privacy statement</a> contains the legally binding version."
       accept_button: Accept the privacy statement
       decline_button: Decline
+    confirm_new_user:
+      title: Email already in use
+      message_single_p1: "A Dodona user with the same email address (**%{email}**) already exist within a different institution. This is probably you. If you want to sign in with the existing account, click on the sign in button below and sign in again."
+      message_plural_p1: "Multiple Dodona users with the same email address (**%{email}**) already exist within different institutions. These are probably you. If you want to sign in with one of these existing accounts, click on the relevant sign in button below and sign in again."
+      message_personal_p2: "If you want to create a new personal account, click on \"Create new account\"."
+      message_institutional_p2: "If you want to create a new account for the institution **%{institution}**, click on \"Create new account\"."
+      sign_in_to_institutional: "Sign in to **%{name}** (%{institution})"
+      sign_in_to_personal: "Sign in to **%{name}** (personal account)"
+      create_new_account: "Create new account"
+      "contact_support_html": "If this isn't you or if you keep having issues, fill in the %{form} and we will assist you as soon as possible."
+      contact_form: "contact form"
+

config/locales/views/auth/nl.yml

@@ -29,3 +29,14 @@ nl:
       text_html: "Om Dodona te gebruiken moet je onze privacyverklaring accepteren. Op de pagina <a href=\"%{your_data}\" target=\"_blank\">jouw data</a> leggen we in mensentaal uit welke data we verzamelen en hoe we die gebruiken. De juridisch bindende versie kan je in onze <a href=\"%{privacy_statement}\" target=\"_blank\">privacyverklaring</a> vinden."
       accept_button: Privacyverklaring accepteren
       decline_button: Weigeren
+    confirm_new_user:
+      title: Email al in gebruik
+      message_single_p1: "Er bestaat al een Dodonagebruiker met hetzelfde emailadres (**%{email}**) in een andere onderwijsinstelling. Waarschijnlijk ben jij dit. Wil je aanmelden met deze bestaande account, klik dan onderaan op de aanmeldknop."
+      message_plural_p1: "Er bestaan al meerdere Dodonagebruikers met hetzelfde emailadres (**%{email}**) in andere onderwijsinstellingen. Waarschijnlijk ben jij dit. Wil je aanmelden met een van deze bestaande accounts, klik dan onderaan op de correcte aanmeldknop."
+      message_personal_p2: "Als je een nieuwe persoonlijke account wilt aanmaken, klik dan op \"Maak een nieuwe account\"."
+      message_institutional_p2: "Als je een nieuwe account wilt aanmaken voor de onderwijsinstelling **%{institution}**, klik dan op \"Maak een nieuwe account\"."
+      sign_in_to_institutional: "Meld aan als **%{name}** (%{institution})"
+      sign_in_to_personal: "Meld aan als **%{name}** (%{persoonlijke account})"
+      create_new_account: "Maak een nieuwe account"
+      "contact_support_html": "Ben jij dit niet of blijf je problemen hebben, vul dan het %{form} in en we helpen je zo snel mogelijk verder."
+      contact_form: "contactformulier"

config/routes.rb

@@ -20,6 +20,8 @@
         delete '/sign_out' => 'authentication#destroy', as: 'sign_out'
         get '/privacy_prompt' => 'omniauth_callbacks#privacy_prompt'
         post '/privacy_prompt' => 'omniauth_callbacks#accept_privacy_policy'
+        get '/confirm_new_user' => 'omniauth_callbacks#confirm_new_user'
+        post '/confirm_new_user' => 'omniauth_callbacks#accept_confirm_new_user'
       end
 
       get '/users/saml/metadata' => 'saml#metadata'

db/migrate/20220914075029_update_index_to_users_to_email.rb

@@ -0,0 +1,6 @@
+class UpdateIndexToUsersToEmail < ActiveRecord::Migration[7.0]
+  def change
+    remove_index :users, :email if index_exists?(:users, :email)
+    add_index :users, [:email, :institution_id], unique: true
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2022_08_30_124315) do
+ActiveRecord::Schema[7.0].define(version: 2022_09_14_075029) do
   create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
     t.string "name", null: false
     t.string "record_type", null: false
@@ -496,7 +496,7 @@
     t.string "search", limit: 4096
     t.datetime "seen_at"
     t.datetime "sign_in_at"
-    t.index ["email"], name: "index_users_on_email", unique: true
+    t.index ["email", "institution_id"], name: "index_users_on_email_and_institution_id", unique: true
     t.index ["institution_id"], name: "index_users_on_institution_id"
     t.index ["token"], name: "index_users_on_token"
     t.index ["username", "institution_id"], name: "index_users_on_username_and_institution_id", unique: true

lib/LTI/auth/strategy.rb

@@ -50,7 +50,7 @@ def id_token_callback_phase
                 email: raw_info[:email]
             },
             extra: {
-                provider: provider,
+                provider_id: provider&.id,
                 redirect_params: {
                     id_token: jwt_token,
                     provider_id: provider&.id

lib/SAML/strategy.rb

@@ -55,7 +55,7 @@ def callback_phase
         info['username'] || @name_id
       end
 
-      extra { {provider: @provider} }
+      extra { {provider_id: @provider&.id} }
 
       def on_callback_path?
         # Intercept requests sent to /users/saml/auth and forward those to the

test/controllers/auth/omniauth_callbacks_controller_test.rb

@@ -22,7 +22,7 @@ def omniauth_mock_identity(identity, params = {})
     }.deep_merge(params)
 
     # LTI and SAML include the provider.
-    auth_hash = auth_hash.deep_merge({ extra: { provider: identity.provider } }) if [Provider::Lti, Provider::Saml].include?(identity.provider.class)
+    auth_hash = auth_hash.deep_merge({ extra: { provider_id: identity.provider.id } }) if [Provider::Lti, Provider::Saml].include?(identity.provider.class)
 
     OmniAuth.config.mock_auth[:default] = OmniAuth::AuthHash.new(auth_hash)
   end
@@ -243,7 +243,7 @@ def omniauth_path(provider)
       # Setup.
       provider = create provider_name
       user = create :user, institution: provider.institution
-      other_user = create :user
+      other_user = create :user, institution: provider.institution
       identity = create :identity, provider: provider, user: user
 
       omniauth_mock_identity identity,
@@ -301,7 +301,7 @@ def omniauth_path(provider)
     end
   end
 
-  test 'login with other institution should not work' do
+  test 'login with other institution should ask confirmation' do
     AUTH_PROVIDERS.each do |provider_name|
       first_provider = create provider_name
       first_user = create :user, institution: first_provider.institution
@@ -317,9 +317,21 @@ def omniauth_path(provider)
         follow_redirect!
       end
 
-      assert_redirected_to root_path
+      # Before confirm no one is signed in
+      assert_redirected_to confirm_new_user_path
       assert_nil @controller.current_user
       assert_not_equal first_user.reload.username, second_user.username
+
+      assert_difference 'User.count', 1 do
+        post confirm_new_user_path
+        follow_redirect!
+      end
+
+      # After confirm a new user is created
+      assert_equal @controller.current_user.username, second_user.username
+      assert_not_equal first_user.reload.username, second_user.username
+
+      sign_out @controller.current_user
     end
   end