diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb
index d0112254e2..0507db299c 100644
--- a/app/controllers/auth/omniauth_callbacks_controller.rb
+++ b/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,
diff --git a/app/models/user.rb b/app/models/user.rb
index 463fcc757f..2eebbfda38 100644
--- a/app/models/user.rb
+++ b/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
diff --git a/app/views/auth/redirect_to_known_provider.html.erb b/app/views/auth/redirect_to_known_provider.html.erb
index c35b907f20..aa442569b7 100644
--- a/app/views/auth/redirect_to_known_provider.html.erb
+++ b/app/views/auth/redirect_to_known_provider.html.erb
@@ -16,6 +16,13 @@
<% end %>
<%= t ".contact_support_html", form: link_to(t(".contact_form"), contact_path)%>
diff --git a/config/locales/views/auth/en.yml b/config/locales/views/auth/en.yml
index 5b13a5de71..06ccc8d3d5 100644
--- a/config/locales/views/auth/en.yml
+++ b/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:
diff --git a/config/locales/views/auth/nl.yml b/config/locales/views/auth/nl.yml
index 31ed872a82..daa0aacd09 100644
--- a/config/locales/views/auth/nl.yml
+++ b/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:
diff --git a/db/migrate/20240911085152_remove_email_uniqueness.rb b/db/migrate/20240911085152_remove_email_uniqueness.rb
new file mode 100644
index 0000000000..9d26df3fdb
--- /dev/null
+++ b/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
diff --git a/db/schema.rb b/db/schema.rb
index 3eacf5b77b..1afaa8229c 100644
--- a/db/schema.rb
+++ b/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
diff --git a/test/controllers/auth/omniauth_callbacks_controller_test.rb b/test/controllers/auth/omniauth_callbacks_controller_test.rb
index 5caa8ff223..c38e06180c 100644
--- a/test/controllers/auth/omniauth_callbacks_controller_test.rb
+++ b/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
diff --git a/test/testhelpers/constants.rb b/test/testhelpers/constants.rb
index e4ee28e83a..0da19eace5 100644
--- a/test/testhelpers/constants.rb
+++ b/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