Skip to content
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

Remove validation on uniqueness of email adresses and usernames #5786

Merged
merged 9 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 5 additions & 49 deletions app/controllers/auth/omniauth_callbacks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -222,40 +219,7 @@ def find_identity_by_uid
# In case of provider without uids, don't return any identity (As it won't be matching a unique user)
return nil if auth_uid.nil?

identity = Identity.find_by(identifier: auth_uid, provider: provider)

return identity unless identity.nil?

if provider.class.sym == :office365 && auth_email.present?
# This code supports a migration of the office365 oauth api from v1 to v2
# Try to find the user by the legacy identifier
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.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?
return nil if identity.nil?

# Update the identifier to the new uid
identity.update(identifier: auth_uid, identifier_based_on_email: false)
elsif provider.class.sym == :smartschool && auth_username.present?
# This code supports a migration of smartschool usernames to userID as identifier
# Try to find user by username
identity = Identity.find_by(identifier: auth_username, provider: provider, identifier_based_on_username: true)

# Try to find user by email
identity = Identity.joins(:user).find_by(user: { email: auth_email }, provider: provider, identifier_based_on_username: true) if identity.nil?

# 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_username: true) if identity.nil?
return nil if identity.nil?

# Update the identifier to the new uid
identity.update(identifier: auth_uid, identifier_based_on_username: false)
end
identity
Identity.find_by(identifier: auth_uid, provider: provider)
end

def find_identity_by_user(user)
Expand Down Expand Up @@ -373,6 +337,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
Expand All @@ -386,17 +353,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,
Expand Down
2 changes: 0 additions & 2 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
7 changes: 7 additions & 0 deletions app/views/auth/redirect_to_known_provider.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/auth/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
3 changes: 3 additions & 0 deletions config/locales/views/auth/nl.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
6 changes: 6 additions & 0 deletions db/migrate/20240911085152_remove_email_uniqueness.rb
Original file line number Diff line number Diff line change
@@ -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
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 1 addition & 3 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
Loading
Loading