Merge pull request #5786 from dodona-edu/fix/mail

Remove validation on uniqueness of email adresses and usernames

app/controllers/auth/omniauth_callbacks_controller.rb

@@ -128,13 +128,10 @@ def try_login!
       # If no identity exist, we want to check if it is a new user or an existing user using a new provider
       # Try to find an existing user
       user = find_user_in_institution
-      # 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 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
+      # if no => create a new user or contact support
       return redirect_to_known_provider!(user) if user.present?
 
       # Try to find if the email address is already in use in an other institution
@@ -373,6 +370,9 @@ def redirect_with_flash!(message)
   end
 
   def redirect_to_known_provider!(user)
+    # information required if the user wants to create a new account
+    store_hash_in_session!
+    # information required if the user wants to link the new sign in method to an existing account
     store_identity_in_session!
     session[:auth_original_user_id] = user.id
     @provider = provider
@@ -386,17 +386,6 @@ def redirect_to_confirm_new_user!
     redirect_to confirm_new_user_path
   end
 
-  def redirect_duplicate_email_for_provider!
-    ApplicationMailer.with(
-      authinfo: auth_hash,
-      errors: I18n.t('devise.omniauth_callbacks.duplicate_email_for_provider', email_address: auth_email, provider: provider.class.sym.to_s)
-    ).user_unable_to_log_in.deliver_later
-
-    set_flash_message :alert, :duplicate_email_for_provider, email_address: auth_email, provider: provider.class.sym.to_s
-    flash[:options] = [{ url: contact_path, message: I18n.t('pages.contact.prompt') }]
-    redirect_to root_path
-  end
-
   def flash_wrong_provider(tried_provider, user_provider)
     set_flash_message :alert, :wrong_provider,
                       tried_email_address: auth_email,

app/models/user.rb

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

app/views/auth/redirect_to_known_provider.html.erb

@@ -16,6 +16,13 @@
       <% end %>
     </div>
     <div class="row">
+      <h2><%= t ".new_account" %></h2>
+      <p><%= t ".new_account_help"%></p>
+      <div>
+        <%= link_to t(".create_new_account"), confirm_new_user_path, method: :post, class: "btn btn-filled" %>
+      </div>
+    </div>
+    <div class="row mt-4">
       <p><%= t ".contact_support_html", form: link_to(t(".contact_form"), contact_path)%></p>
     </div>
   </div>

config/locales/views/auth/en.yml

@@ -22,6 +22,9 @@ en:
       message_p1_personal: "A user with the same username (**%{username}**) or email address (**%{email}**) already exists on Dodona. This is probably you, but you have used a different sign in method on previous occasions."
       message_p2: "Before you can access your data using **%{provider}**, we have to link your existing account. To proceed, click on one of the sign in methods below and sign in again."
       providers_title: "Link accounts"
+      new_account: "Create new account"
+      new_account_help: "If the existing account is not yours, or if you want to create a new account, click on the button below."
+      create_new_account: "Create new account"
       "contact_support_html": "If this isn't you or you keep having issues? Fill in the %{form} and we will assist you as soon as possible."
       contact_form: "contact form"
     privacy_prompt:

config/locales/views/auth/nl.yml

@@ -22,6 +22,9 @@ nl:
       message_p1_personal: "Er bestaat al een gebruiker met dezelfde gebruikersnaam (**%{username}**) of hetzelfde e-mailadres (**%{email}**) op Dodona. Waarschijnlijk ben jij dit, maar gebruikte je vorige keer een andere loginmethode."
       message_p2: "Om ook via **%{provider}** aan je gegevens te kunnen, moeten we even je bestaande account koppelen. Klik daarvoor op de aanmeldknop hieronder en log nogmaals in."
       providers_title: "Accounts koppelen"
+      new_account: "Nieuwe account aanmaken"
+      new_account_help: "Als het bestaande account niet van jou is, of als je een nieuwe account wilt aanmaken, klik dan op de knop hieronder."
+      create_new_account: "Maak een nieuwe account"
       "contact_support_html": "Ben jij dit niet of blijf je problemen hebben? Vul het %{form} in en we helpen je zo snel mogelijk verder."
       contact_form: "contactformulier"
     privacy_prompt:

db/migrate/20240911085152_remove_email_uniqueness.rb

@@ -0,0 +1,6 @@
+class RemoveEmailUniqueness < ActiveRecord::Migration[7.2]
+  def change
+    remove_index :users, name: "index_users_on_email_and_institution_id"
+    remove_index :users, name: "index_users_on_username_and_institution_id"
+  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.1].define(version: 2024_06_26_093130) do
+ActiveRecord::Schema[7.2].define(version: 2024_09_11_085152) 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
@@ -529,10 +529,8 @@
     t.datetime "sign_in_at", precision: nil
     t.integer "open_questions_count", default: 0, null: false
     t.integer "theme", default: 0, null: false
-    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
     t.index ["username"], name: "index_users_on_username"
   end
 

test/controllers/auth/omniauth_callbacks_controller_test.rb

@@ -252,16 +252,15 @@ def omniauth_path(provider)
   end
 
   test 'update attributes should be checked for validity' do
-    OAUTH_PROVIDERS.each do |provider_name|
+    EMAIL_REQUIRED_PROVIDERS.each do |provider_name|
       # Setup.
       provider = create provider_name
       user = create :user, institution: provider.institution
-      other_user = create :user, institution: provider.institution
       identity = create :identity, provider: provider, user: user
 
       omniauth_mock_identity identity,
                              info: {
-                               email: other_user.email
+                               email: nil
                              }
 
       assert_emails 1 do
@@ -275,7 +274,7 @@ def omniauth_path(provider)
       assert_nil @controller.current_user
       user.reload
 
-      assert_not_equal other_user.email, user.email
+      assert_not_nil user.email
 
       # Cleanup.
       sign_out user
@@ -499,9 +498,8 @@ def omniauth_path(provider)
     post omniauth_url(provider)
     follow_redirect!
 
-    # Assert successful authentication.
-    assert_redirected_to root_path
-    assert_not_equal @controller.current_user, user
+    # assert not signed in because of double account (first has to confirm new account)
+    assert_nil @controller.current_user
     identity.reload
 
     assert_equal('NEW-UID', identity.identifier)
@@ -608,8 +606,8 @@ def omniauth_path(provider)
     post omniauth_url(provider)
     follow_redirect!
 
-    # Assert successful authentication.
-    assert_redirected_to root_path
+    # assert not signed in because of double account (first has to confirm new account)
+    assert_nil @controller.current_user
     assert_not_equal @controller.current_user, user
     identity.reload
 
@@ -827,4 +825,47 @@ def omniauth_path(provider)
     # Done.
     sign_out user
   end
+
+  test 'can create new user with same email and username as existing user' do
+    institution = create :institution
+    user = create :user, institution: institution, identities: []
+    first_provider = create :provider, institution: institution, mode: :prefer, identities: []
+    second_provider = create :provider, institution: institution, mode: :secondary, identities: []
+
+    # Link user to first provider.
+    create :identity, provider: first_provider, user: user
+
+    # Build, but don't save the identity for the second provider.
+    # This allows us to log in with the second provider for the 'first' time.
+    second_identity = build :identity, provider: second_provider, user: user
+    omniauth_mock_identity second_identity
+
+    # Simulate the user logging in with the second provider.
+    # It should not create a user.
+    assert_difference 'User.count', 0 do
+      post omniauth_url(second_provider)
+      follow_redirect!
+    end
+
+    # It should render the page where the user can choose.
+    assert_response :success
+
+    # It is actually the page we expect.
+    assert_select 'h1', t('auth.redirect_to_known_provider.title')
+    # There should be a button to create a new user
+    assert_select 'a', t('auth.redirect_to_known_provider.create_new_account')
+
+    # Confirming the new user should create a new user
+    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, user.username
+    assert_equal @controller.current_user.email, user.email
+    assert_not_equal @controller.current_user.id, user.id
+
+    sign_out @controller.current_user
+  end
 end

test/testhelpers/constants.rb

@@ -1,4 +1,5 @@
 module Constants
   OAUTH_PROVIDERS = %i[gsuite_provider office365_provider smartschool_provider].freeze
   AUTH_PROVIDERS = %i[lti_provider saml_provider].concat(OAUTH_PROVIDERS).freeze
+  EMAIL_REQUIRED_PROVIDERS = AUTH_PROVIDERS - %i[lti_provider smartschool_provider].freeze
 end