From 895a4c86363f134023563f836463c238debaa21d Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 25 Sep 2024 12:59:09 -0600 Subject: [PATCH 01/17] Add `:confirmable` to included devise modules --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index e3ecccf039..c056ac083c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -65,7 +65,7 @@ class User < ApplicationRecord # :token_authenticatable, :confirmable, # :lockable, :timeoutable and :omniauthable devise :invitable, :database_authenticatable, :registerable, :recoverable, - :rememberable, :trackable, :validatable, :omniauthable, + :rememberable, :trackable, :validatable, :omniauthable, :confirmable, omniauth_providers: %i[shibboleth orcid] # default user language to the default language From 90e644596cb05e0040895d9df94aaf4f759883dd Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 16 Apr 2025 09:45:44 -0600 Subject: [PATCH 02/17] Streamline email confirmation for existing users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By default, Devise's `:confirmable` module generates a `confirmation_token` and sends confirmation instructions when a new user is created. This commit enhances that behaviour to streamline the email confirmation process for **existing** users. A new rake task (`lib/tasks/email_confirmation.rake#clear_all`) resets the following confirmation-related fields—`confirmed_at`, `confirmation_token`, and `confirmation_sent_at`—to `nil` for all non-superusers. After this reset, these users will be unable to sign in until they confirm their email. To avoid requiring manual re-sending of confirmation instructions, we introduce a new check: `User#confirmed_or_has_confirmation_token?`, which returns `false` if a user is unconfirmed *and* has no outstanding confirmation_token. In the `SessionsController#create` method, if a signing-in user fails the `confirmed_or_has_confirmation_token?` check, we invoke `handle_missing_confirmation_instructions(user)`. This generates a new confirmation_token and sends email instructions. On subsequent sign-in attempts, the check will return `true`, preventing redundant emails. This approach ensures that email confirmations are triggered automatically and only once per affected user, minimising friction while preserving security. --- app/controllers/sessions_controller.rb | 15 ++++++++ app/models/user.rb | 4 +++ lib/tasks/email_confirmation.rake | 47 ++++++++++++++++++++++++++ 3 files changed, 66 insertions(+) create mode 100644 lib/tasks/email_confirmation.rake diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 04e15f1de2..15d2ac2390 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -13,6 +13,11 @@ def create existing_user = User.find_by(email: params[:user][:email]) unless existing_user.nil? + unless existing_user.confirmed_or_has_confirmation_token? + handle_missing_confirmation_instructions(existing_user) + return + end + # Until ORCID login is supported unless session['devise.shibboleth_data'].nil? args = { @@ -45,3 +50,13 @@ def destroy set_locale end end + +private + +def handle_missing_confirmation_instructions(user) + # Generate a confirmation_token and email confirmation instructions to the user + user.send_confirmation_instructions + # Notify the user they are unconfirmed but confirmation instructions have been sent + flash[:notice] = I18n.t('devise.registrations.signed_up_but_unconfirmed') + redirect_to root_path +end diff --git a/app/models/user.rb b/app/models/user.rb index c056ac083c..4b3894a242 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -382,6 +382,10 @@ def deliver_invitation(options = {}) ) end + def confirmed_or_has_confirmation_token? + confirmed? || confirmation_token.present? + end + # Case insensitive search over User model # # field - The name of the field being queried diff --git a/lib/tasks/email_confirmation.rake b/lib/tasks/email_confirmation.rake new file mode 100644 index 0000000000..eca4692734 --- /dev/null +++ b/lib/tasks/email_confirmation.rake @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +namespace :email_confirmation do + desc 'Reset confirmation status for all users, excluding superusers' + task clear_all: :environment do + p '------------------------------------------------------------------------' + p 'Beginning task: Unconfirming all users except superusers' + p '------------------------------------------------------------------------' + unconfirm_all_users_except_superusers + p 'Task completed: Unconfirmed all users except superusers' + end + + private + + def unconfirm_all_users_except_superusers + p 'Updating :confirmable columns to nil for all users' + p '(i.e. Setting confirmed_at, confirmation_token, and confirmation_sent_at to nil for all users)' + p '------------------------------------------------------------------------' + set_confirmable_cols_to_nil_for_all_users + p '------------------------------------------------------------------------' + p 'Updating superusers so that they are not required to confirm their email addresses' + p '(i.e. Setting `confirmed_at = Time.current` for superusers)' + p '------------------------------------------------------------------------' + confirm_superusers + end + + def set_confirmable_cols_to_nil_for_all_users + count = User.update_all(confirmed_at: nil, confirmation_token: nil, confirmation_sent_at: nil) + p ":confirmable columns updated to nil for #{count} users" + end + + # Sets `confirmed_at` to `Time.current` for all superusers + def confirm_superusers + confirmed_at = Time.current + count = User.joins(:perms).where(perms: { id: super_admin_perm_ids }) + .distinct + .update_all(confirmed_at: confirmed_at) + p "Updated confirmed_at = #{confirmed_at} for #{count} superuser(s)" + end + + # Returns an array of all perm ids that are considered super admin perms + # (Based off of `def can_super_admin?` in `app/models/user.rb` + # i.e. `can_add_orgs? || can_grant_api_to_orgs? || can_change_org?` ) + def super_admin_perm_ids + [Perm.add_orgs.id, Perm.grant_api.id, Perm.change_affiliation.id] + end +end From dcbc5249804617a7711fe09581fbc91cb0572ccf Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 16 Apr 2025 15:38:21 -0600 Subject: [PATCH 03/17] Refactor SessionsController#create Moved the creation of the Shibboleth identifier to a helper method --- app/controllers/sessions_controller.rb | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 15d2ac2390..f30c930b1b 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -19,15 +19,7 @@ def create end # Until ORCID login is supported - unless session['devise.shibboleth_data'].nil? - args = { - identifier_scheme: IdentifierScheme.find_by(name: 'shibboleth'), - value: session['devise.shibboleth_data']['uid'], - identifiable: existing_user, - attrs: session['devise.shibboleth_data'] - } - @ui = Identifier.new(args) - end + @ui = create_shibboleth_identifier(existing_user) unless session['devise.shibboleth_data'].nil? session[:locale] = existing_user.locale unless existing_user.locale.nil? # Method defined at controllers/application_controller.rb set_locale @@ -53,6 +45,16 @@ def destroy private +def create_shibboleth_identifier(user) + args = { + identifier_scheme: IdentifierScheme.find_by(name: 'shibboleth'), + value: session['devise.shibboleth_data']['uid'], + identifiable: user, + attrs: session['devise.shibboleth_data'] + } + Identifier.new(args) +end + def handle_missing_confirmation_instructions(user) # Generate a confirmation_token and email confirmation instructions to the user user.send_confirmation_instructions From 758746630edd877f4c13132a862fdd299a3914d0 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 16 Apr 2025 15:42:09 -0600 Subject: [PATCH 04/17] Refactor SessionsController#create - Replaced double-negative `unless existing_user.nil?` with `if existing_user.present?` - Used string concatenation with backslash to allow removal of disabled rubocop offence. --- app/controllers/sessions_controller.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index f30c930b1b..db7cd0cbb4 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -11,7 +11,7 @@ def new # rubocop:disable Metrics/AbcSize def create existing_user = User.find_by(email: params[:user][:email]) - unless existing_user.nil? + if existing_user.present? unless existing_user.confirmed_or_has_confirmation_token? handle_missing_confirmation_instructions(existing_user) @@ -27,9 +27,8 @@ def create super do if !@ui.nil? && @ui.save - # rubocop:disable Layout/LineLength - flash[:notice] = _('Your account has been successfully linked to your institutional credentials. You will now be able to sign in with them.') - # rubocop:enable Layout/LineLength + flash[:notice] = _('Your account has been successfully linked to your institutional credentials. ' \ + 'You will now be able to sign in with them.') end end end From 638e17963b864c4f5833ff0d025c70140441312c Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Oct 2024 09:28:57 -0600 Subject: [PATCH 05/17] Adapt existing tests to `:confirmable` re-addition `config/environments/test.rb` - Add "SMTP From address" to mock Devise's sending of confirmation instructions email at time of account creation `spec/factories/users.rb` - Set `confirmed_at { Time.current }` in user factory `spec/features/registrations_spec.rb` - Change expected path to `root_path`. This is because the user should not be able to access `plans_path` until after they confirm their email - Add check to verify that the confirmation-related flash message was sent --- config/environments/test.rb | 1 + spec/factories/users.rb | 1 + spec/features/registrations_spec.rb | 6 +++--- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/config/environments/test.rb b/config/environments/test.rb index 122ad051b6..9195462b99 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -45,6 +45,7 @@ # The :test delivery method accumulates sent emails in the # ActionMailer::Base.deliveries array. config.action_mailer.delivery_method = :test + config.action_mailer.default_options = { from: 'noreply@example.org' } # Print deprecation notices to the stderr. config.active_support.deprecation = :stderr diff --git a/spec/factories/users.rb b/spec/factories/users.rb index c91c756604..9eb743a38b 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -60,6 +60,7 @@ email { Faker::Internet.unique.email } password { 'password' } accept_terms { true } + confirmed_at { Time.current } trait :org_admin do after(:create) do |user, _evaluator| diff --git a/spec/features/registrations_spec.rb b/spec/features/registrations_spec.rb index 50ef4d60ab..4f7840d21b 100644 --- a/spec/features/registrations_spec.rb +++ b/spec/features/registrations_spec.rb @@ -26,9 +26,9 @@ click_button 'Create account' # Expectations - expect(current_path).to eql(plans_path) - expect(page).to have_text(user_attributes[:firstname]) - expect(page).to have_text(user_attributes[:surname]) + expect(current_path).to eql(root_path) + expect(page).to have_text(I18n.t('devise.registrations.signed_up_but_unconfirmed')) + expect(User.count).to eq(1) end scenario 'User attempts to create a new acccount with invalid atts', :js do From 1ceeec05753d44a4cc78a96fdf85dd257e4f850b Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 26 Sep 2024 14:40:34 -0600 Subject: [PATCH 06/17] Add email confirmation .yml translations - Customises Devise's default value for the key `devise.failure.unconfirmed`. The value adds a link to the email confirmation page. - Updated via `bundle exec rake translation:sync` --- config/locales/translation.en-CA.yml | 3 ++- config/locales/translation.en-GB.yml | 3 ++- config/locales/translation.fr-CA.yml | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/config/locales/translation.en-CA.yml b/config/locales/translation.en-CA.yml index 9e5ed6e7c8..784fc06e39 100644 --- a/config/locales/translation.en-CA.yml +++ b/config/locales/translation.en-CA.yml @@ -230,7 +230,8 @@ en-CA: not_found_in_database: Invalid %{authentication_keys} or password. timeout: Your session expired. Please sign in again to continue. unauthenticated: You need to sign in or sign up before continuing. - unconfirmed: You have to confirm your email address before continuing. + unconfirmed: You need to confirm your account before continuing. (Click to request a new confirmation email) invited: mailer: confirmation_instructions: diff --git a/config/locales/translation.en-GB.yml b/config/locales/translation.en-GB.yml index 91014ac51c..711ed6a306 100644 --- a/config/locales/translation.en-GB.yml +++ b/config/locales/translation.en-GB.yml @@ -227,7 +227,8 @@ en-GB: not_found_in_database: Invalid email or password. timeout: Your session expired, please sign in again to continue. unauthenticated: You need to sign in or sign up before continuing. - unconfirmed: You have to confirm your account before continuing. + unconfirmed: You have to confirm your account before continuing. (Click to request a new confirmation email) invited: You have a pending invitation, accept it to finish creating your account. mailer: confirmation_instructions: diff --git a/config/locales/translation.fr-CA.yml b/config/locales/translation.fr-CA.yml index 8f23004765..82067551fe 100644 --- a/config/locales/translation.fr-CA.yml +++ b/config/locales/translation.fr-CA.yml @@ -242,7 +242,8 @@ fr-CA: not_found_in_database: "%{authentication_keys} ou mot de passe incorrect." timeout: Votre session est expirée. Veuillez vous reconnecter pour continuer. unauthenticated: Vous devez vous connecter ou vous inscrire pour continuer. - unconfirmed: Vous devez confirmer votre adresse courriel pour continuer. + unconfirmed: Vous devez confirmer votre compte avant de continuer. (cliquez pour demander un nouveau courriel de confirmation) invited: Vous avez une invitation en attente. Acceptez-la pour terminer la création de votre compte. mailer: From 6f0924e9ffa5ab1a3ee37e409fa6236765a1c80d Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 17 Apr 2025 10:01:09 -0600 Subject: [PATCH 07/17] Streamline email confirmation for SSO sign-in Commit 6af91b61af754510773611430d60f16b2bcec26f streamlined the email confirmation process for unconfirmed users with no confirmation_token. Specifically, it targeted users attempting to sign in via the app's standard sign-in form. This change applies the same streamlining to users attempting to sign in via SSO/Shibboleth. Specifically, when a user attempts to sign in this way, the `.confirmed_or_has_confirmation_token?` is performed on them. If it returns false, then the confirmation_token is generated for the user and they are auto-sent a confirmation instructions email. (Refer to commit 6af91b61af754510773611430d60f16b2bcec26f for more regarding this streamlined process.) --- .../users/omniauth_callbacks_controller.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 01ec5491e7..bf40fe62ee 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -40,6 +40,10 @@ def handle_omniauth(scheme) # Otherwise sign them in elsif scheme.name == 'shibboleth' # Until ORCID becomes supported as a login method + unless existing_user.confirmed_or_has_confirmation_token? + handle_missing_confirmation_instructions(existing_user) + return + end set_flash_message(:notice, :success, kind: scheme.description) if is_navigational_format? sign_in_and_redirect user, event: :authentication else @@ -83,4 +87,14 @@ def failure redirect_to root_path end end + + private + + def handle_missing_confirmation_instructions(user) + # Generate a confirmation_token and email confirmation instructions to the user + user.send_confirmation_instructions + # Notify the user they are unconfirmed but confirmation instructions have been sent + flash[:notice] = I18n.t('devise.registrations.signed_up_but_unconfirmed') + redirect_to root_path + end end From e1cdee9fc571c1eaa092d21f81a1ce51424c60e1 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 17 Apr 2025 10:32:10 -0600 Subject: [PATCH 08/17] Create EmailConfirmationHandler concern & refactor This change addresses the duplicate code shared between `app/controllers/sessions_controller.rb` and `app/controllers/users/omniauth_callbacks_controller.rb`. It does so by creating `app/models/concerns/email_confirmation_handler.rb` which allows us to better adhere to DRY principles. - Additionally, the `user.confirmed? || user.confirmation_token.present?` check has been moved from the User model to the concern. It made sense to have this check as a User method. However, the method itself is simply an or check of two other User methods, and only the `EmailConfirmationHandler` module is currently needing it. --- app/controllers/sessions_controller.rb | 16 +++------- .../users/omniauth_callbacks_controller.rb | 19 ++++-------- .../concerns/email_confirmation_handler.rb | 29 +++++++++++++++++++ app/models/user.rb | 4 --- 4 files changed, 38 insertions(+), 30 deletions(-) create mode 100644 app/models/concerns/email_confirmation_handler.rb diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index db7cd0cbb4..4a91e68b46 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -2,6 +2,8 @@ # Controller that handles user login and logout class SessionsController < Devise::SessionsController + include EmailConfirmationHandler + def new redirect_to(root_path) end @@ -13,10 +15,8 @@ def create existing_user = User.find_by(email: params[:user][:email]) if existing_user.present? - unless existing_user.confirmed_or_has_confirmation_token? - handle_missing_confirmation_instructions(existing_user) - return - end + # (see app/models/concerns/email_confirmation_handler.rb) + return if confirmation_instructions_missing_and_handled?(existing_user) # Until ORCID login is supported @ui = create_shibboleth_identifier(existing_user) unless session['devise.shibboleth_data'].nil? @@ -53,11 +53,3 @@ def create_shibboleth_identifier(user) } Identifier.new(args) end - -def handle_missing_confirmation_instructions(user) - # Generate a confirmation_token and email confirmation instructions to the user - user.send_confirmation_instructions - # Notify the user they are unconfirmed but confirmation instructions have been sent - flash[:notice] = I18n.t('devise.registrations.signed_up_but_unconfirmed') - redirect_to root_path -end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index bf40fe62ee..cbfdf3bdb7 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -3,6 +3,7 @@ module Users # Controller that handles callbacks from OmniAuth integrations (e.g. Shibboleth and ORCID) class OmniauthCallbacksController < Devise::OmniauthCallbacksController + include EmailConfirmationHandler ## # Dynamically build a handler for each omniauth provider # ------------------------------------------------------------- @@ -40,10 +41,10 @@ def handle_omniauth(scheme) # Otherwise sign them in elsif scheme.name == 'shibboleth' # Until ORCID becomes supported as a login method - unless existing_user.confirmed_or_has_confirmation_token? - handle_missing_confirmation_instructions(existing_user) - return - end + + # (see app/models/concerns/email_confirmation_handler.rb) + return if confirmation_instructions_missing_and_handled?(user) + set_flash_message(:notice, :success, kind: scheme.description) if is_navigational_format? sign_in_and_redirect user, event: :authentication else @@ -87,14 +88,4 @@ def failure redirect_to root_path end end - - private - - def handle_missing_confirmation_instructions(user) - # Generate a confirmation_token and email confirmation instructions to the user - user.send_confirmation_instructions - # Notify the user they are unconfirmed but confirmation instructions have been sent - flash[:notice] = I18n.t('devise.registrations.signed_up_but_unconfirmed') - redirect_to root_path - end end diff --git a/app/models/concerns/email_confirmation_handler.rb b/app/models/concerns/email_confirmation_handler.rb new file mode 100644 index 0000000000..674db79408 --- /dev/null +++ b/app/models/concerns/email_confirmation_handler.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +# Some users in our db are both unconfirmed AND have no outstanding confirmation_token +# This is true for those users due to the following: +# - We haven't always used Devise's :confirmable module (it generates a confirmation_token when a user is created) +# - We have set `confirmed_at` and `confirmation_token` to nil via Rake tasks (lib/tasks/email_confirmation.rake) +# This concern is meant to streamline the confirmation process for those users +module EmailConfirmationHandler + extend ActiveSupport::Concern + + def confirmation_instructions_missing_and_handled?(user) + # A user's "confirmation instructions are missing" if they're both unconfirmed and have no confirmation_token + return false if user_confirmed_or_has_confirmation_token?(user) + + handle_missing_confirmation_instructions(user) + true + end + + private + + def user_confirmed_or_has_confirmation_token?(user) + user.confirmed? || user.confirmation_token.present? + end + + def handle_missing_confirmation_instructions(user) + user.send_confirmation_instructions + redirect_to root_path, notice: I18n.t('devise.registrations.signed_up_but_unconfirmed') + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 4b3894a242..c056ac083c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -382,10 +382,6 @@ def deliver_invitation(options = {}) ) end - def confirmed_or_has_confirmation_token? - confirmed? || confirmation_token.present? - end - # Case insensitive search over User model # # field - The name of the field being queried From cdfe8fd3ac394d2748c42ba9b1f4e416c9138ca0 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 17 Apr 2025 11:04:37 -0600 Subject: [PATCH 09/17] Refactor `def handle_omniauth` This change maintains the functionality of `def handle_omniauth(scheme)` while making the code more maintainable and addressing its disabled rubocop offences. The private methods `def handle_omniauth_for_signed_in_user(user, scheme)` and `def handle_omniauth_for_signed_out_user(user, scheme)` have been created to handle the omniauth logic for signed in vs signed out users respectively. This change is simply a refactor and maintains the pre-existing code logic. --- .../users/omniauth_callbacks_controller.rb | 95 ++++++++++--------- 1 file changed, 50 insertions(+), 45 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index cbfdf3bdb7..cc4610205d 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -22,8 +22,6 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # # scheme - The IdentifierScheme for the provider # - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength - # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def handle_omniauth(scheme) user = if request.env['omniauth.auth'].nil? User.from_omniauth(request.env) @@ -33,59 +31,66 @@ def handle_omniauth(scheme) # If the user isn't logged in if current_user.nil? - # If the uid didn't have a match in the system send them to register - if user.nil? - session["devise.#{scheme.name.downcase}_data"] = request.env['omniauth.auth'] - redirect_to new_user_registration_url + handle_omniauth_for_signed_out_user(user, scheme) + # The user is already logged in and just registering the uid with us + else + handle_omniauth_for_signed_in_user(user, scheme) + end + end + + def failure + redirect_to root_path + end - # Otherwise sign them in - elsif scheme.name == 'shibboleth' - # Until ORCID becomes supported as a login method + private - # (see app/models/concerns/email_confirmation_handler.rb) - return if confirmation_instructions_missing_and_handled?(user) + def handle_omniauth_for_signed_in_user(user, scheme) + # If the user could not be found by that uid then attach it to their record + if user.nil? + if Identifier.create(identifier_scheme: scheme, + value: request.env['omniauth.auth'].uid, + attrs: request.env['omniauth.auth'], + identifiable: current_user) + flash[:notice] = + format(_('Your account has been successfully linked to %{scheme}.'), + scheme: scheme.description) - set_flash_message(:notice, :success, kind: scheme.description) if is_navigational_format? - sign_in_and_redirect user, event: :authentication else - flash[:notice] = _('Successfully signed in') - redirect_to new_user_registration_url + flash[:alert] = format(_('Unable to link your account to %{scheme}.'), + scheme: scheme.description) end - # The user is already logged in and just registering the uid with us - else - # If the user could not be found by that uid then attach it to their record - if user.nil? - if Identifier.create(identifier_scheme: scheme, - value: request.env['omniauth.auth'].uid, - attrs: request.env['omniauth.auth'], - identifiable: current_user) - flash[:notice] = - format(_('Your account has been successfully linked to %{scheme}.'), - scheme: scheme.description) + elsif user.id != current_user.id + # If a user was found but does NOT match the current user then the identifier has + # already been attached to another account (likely the user has 2 accounts) + # rubocop:disable Layout/LineLength + flash[:alert] = _("The current #{scheme.description} iD has been already linked to a user with email #{identifier.user.email}") + # rubocop:enable Layout/LineLength + end - else - flash[:alert] = format(_('Unable to link your account to %{scheme}.'), - scheme: scheme.description) - end + # Redirect to the User Profile page + redirect_to edit_user_registration_path + end - elsif user.id != current_user.id - # If a user was found but does NOT match the current user then the identifier has - # already been attached to another account (likely the user has 2 accounts) - # rubocop:disable Layout/LineLength - flash[:alert] = _("The current #{scheme.description} iD has been already linked to a user with email #{identifier.user.email}") - # rubocop:enable Layout/LineLength - end + def handle_omniauth_for_signed_out_user(user, scheme) + # If the uid didn't have a match in the system send them to register + if user.nil? + session["devise.#{scheme.name.downcase}_data"] = request.env['omniauth.auth'] + redirect_to new_user_registration_url - # Redirect to the User Profile page - redirect_to edit_user_registration_path - end - end - # rubocop:enable Metrics/AbcSize, Metrics/MethodLength - # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + # Otherwise sign them in + elsif scheme.name == 'shibboleth' + # Until ORCID becomes supported as a login method - def failure - redirect_to root_path + # (see app/models/concerns/email_confirmation_handler.rb) + return if confirmation_instructions_missing_and_handled?(user) + + set_flash_message(:notice, :success, kind: scheme.description) if is_navigational_format? + sign_in_and_redirect user, event: :authentication + else + flash[:notice] = _('Successfully signed in') + redirect_to new_user_registration_url + end end end end From ffdfd56ac8a566ee6885c7ce768bfca99aab3e64 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 17 Apr 2025 15:43:46 -0600 Subject: [PATCH 10/17] Small formatting cleanup for omniauth controller --- app/controllers/users/omniauth_callbacks_controller.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index cc4610205d..9214befe5e 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -51,9 +51,8 @@ def handle_omniauth_for_signed_in_user(user, scheme) value: request.env['omniauth.auth'].uid, attrs: request.env['omniauth.auth'], identifiable: current_user) - flash[:notice] = - format(_('Your account has been successfully linked to %{scheme}.'), - scheme: scheme.description) + flash[:notice] = format(_('Your account has been successfully linked to %{scheme}.'), + scheme: scheme.description) else flash[:alert] = format(_('Unable to link your account to %{scheme}.'), @@ -63,9 +62,8 @@ def handle_omniauth_for_signed_in_user(user, scheme) elsif user.id != current_user.id # If a user was found but does NOT match the current user then the identifier has # already been attached to another account (likely the user has 2 accounts) - # rubocop:disable Layout/LineLength - flash[:alert] = _("The current #{scheme.description} iD has been already linked to a user with email #{identifier.user.email}") - # rubocop:enable Layout/LineLength + flash[:alert] = _("The current #{scheme.description} iD has been already linked " \ + "to a user with email #{identifier.user.email}") end # Redirect to the User Profile page From f9341726640c445d573e34ee350040a35dab9ec8 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 17 Apr 2025 15:46:04 -0600 Subject: [PATCH 11/17] Disable rubocop offences Commit fdb54e03bb71ffaad17bea562e0ca8d87b081059 resolved the rubocop offences within `def handle_omniauth(scheme)`. However, the created helper methods are now triggering rubocop offences as well. Although, I'm hesitant to simply disable these offences, I also don't want to go overboard with refactoring this file. --- app/controllers/users/omniauth_callbacks_controller.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 9214befe5e..4c0dcf9ebf 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -44,6 +44,7 @@ def failure private + # rubocop:disable Metrics/AbcSize def handle_omniauth_for_signed_in_user(user, scheme) # If the user could not be found by that uid then attach it to their record if user.nil? @@ -69,7 +70,9 @@ def handle_omniauth_for_signed_in_user(user, scheme) # Redirect to the User Profile page redirect_to edit_user_registration_path end + # rubocop:enable Metrics/AbcSize + # rubocop:disable Metrics/AbcSize def handle_omniauth_for_signed_out_user(user, scheme) # If the uid didn't have a match in the system send them to register if user.nil? @@ -90,5 +93,6 @@ def handle_omniauth_for_signed_out_user(user, scheme) redirect_to new_user_registration_url end end + # rubocop:enable Metrics/AbcSize end end From c3075f1a9477e8cb457f87ae3baa3aba405af4ef Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 22 Apr 2025 12:00:05 -0600 Subject: [PATCH 12/17] Enable OmniAuth testing in RSpec tests - Created `spec/support/helpers/omniauth_helper.rb` for simulating authentication with providers like Shibboleth - Added OmniAuth test config settings in `spec/rails_helper.rb`. --- spec/rails_helper.rb | 5 +++++ spec/support/helpers/omniauth_helper.rb | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 spec/support/helpers/omniauth_helper.rb diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 98c265b5cf..bc5549e832 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -79,4 +79,9 @@ config.include Devise::Test::ControllerHelpers, type: :controller config.include Devise::Test::ControllerHelpers, type: :view config.include Pundit::Matchers, type: :policy + + # Enable test mode for OmniAuth + OmniAuth.config.test_mode = true + # Set the full host to nil to avoid URL generation issues in tests + OmniAuth.config.full_host = nil end diff --git a/spec/support/helpers/omniauth_helper.rb b/spec/support/helpers/omniauth_helper.rb new file mode 100644 index 0000000000..ea6b5b2bd1 --- /dev/null +++ b/spec/support/helpers/omniauth_helper.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# Helper module for mocking OmniAuth authentication in tests +module OmniAuthHelper + # This method sets and returns an OmniAuth::AuthHash + # that simulates the authentication data returned by + # an OmniAuth provider (e.g. Shibboleth). + def mock_auth_hash(user, scheme) + Rails.application.env_config['omniauth.auth'] = + OmniAuth::AuthHash.new({ + provider: scheme.name, + uid: '12345', + info: { + email: user.email, + first_name: user.firstname, + last_name: user.surname + } + }) + Rails.application.env_config['omniauth.auth'] + end +end From 47233c7b84733afb0b726588fc622af03fd05055 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 22 Apr 2025 12:00:53 -0600 Subject: [PATCH 13/17] Add tests for custom email confirmation UX flow - Tests the custom flow for both system and SSO sign in - Tests the custom flash messages including the embedded link to the confirmation email request page --- spec/factories/identifier_schemes.rb | 10 +++ spec/factories/users.rb | 9 ++ spec/features/email_confirmation_spec.rb | 103 +++++++++++++++++++++++ 3 files changed, 122 insertions(+) create mode 100644 spec/features/email_confirmation_spec.rb diff --git a/spec/factories/identifier_schemes.rb b/spec/factories/identifier_schemes.rb index 24fd508511..067cfb04f3 100644 --- a/spec/factories/identifier_schemes.rb +++ b/spec/factories/identifier_schemes.rb @@ -27,6 +27,16 @@ context_count { 1 } end + # Add a trait for Shibboleth + trait :shibboleth do + name { 'shibboleth' } + context { 11 } + description { 'Institutional Sign In (Shibboleth)' } + logo_url { nil } + identifier_prefix { nil } + active { true } + end + after(:create) do |identifier_scheme, evaluator| (0..evaluator.context_count - 1).each do |idx| identifier_scheme.update("#{identifier_scheme.all_context[idx]}": true) diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 9eb743a38b..b87e86d835 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -82,5 +82,14 @@ end end end + + trait :unconfirmed_and_no_confirmation_token do + confirmed_at { nil } + # When using :confirmable, a confirmation_token is generated at the time of user creation + # Setting it to nil allows us to duplicate the attributes of some existing users + after(:create) do |user| + user.update(confirmation_token: nil) + end + end end end diff --git a/spec/features/email_confirmation_spec.rb b/spec/features/email_confirmation_spec.rb new file mode 100644 index 0000000000..93f744a620 --- /dev/null +++ b/spec/features/email_confirmation_spec.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +require 'rails_helper' +SSO_SIGN_IN_BUTTON_TEXT = 'Sign in with your institutional credentials' + +# For testing the custom email confirmation UX flow for unconfirmed users with no outstanding confirmation_token +RSpec.describe 'Email Confirmation', type: :feature do + include OmniAuthHelper + + scenario 'A user attempts to sign in via the "Sign In" button. + However, they are unconfirmed and have no confirmation_token.', :js do + user = create(:user, :unconfirmed_and_no_confirmation_token) + + # Sign-in attempt #1 (user is unconfirmed and has no confirmation_token) + sign_in(user) + expectations_for_unconfirmed_user_with_no_confirmation_token_after_sign_in_attempt(user) + + # Sign-in attempt #2 (user still unconfirmed but has a confirmation_token) + sign_in(user) + expectations_for_unconfirmed_user_with_confirmation_token_after_sign_in_attempt + + # Sign-in attempt #3 (user is now confirmed) + user.confirm + sign_in(user) + # The user is signed in + expect(page).to have_current_path(plans_path) + end + + scenario 'A user attempts to sign in via the "Sign in with your institutional credentials" + button. The email is linked to a user account, however the account is + unconfirmed and has no confirmation token.', :js do + user = create(:user, :unconfirmed_and_no_confirmation_token) + scheme = create(:identifier_scheme, :shibboleth) + # Mock the OmniAuth authentication hash for Shibboleth via OmniAuthHelper + OmniAuth.config.mock_auth[:shibboleth] = mock_auth_hash(user, scheme) + # Create a Shibboleth-related Identifier for the user + Identifier.create(identifier_scheme: scheme, + value: OmniAuth.config.mock_auth[:shibboleth].uid, + attrs: OmniAuth.config.mock_auth[:shibboleth], + identifiable: user) + + visit root_path + # Sign-in attempt #1 (user is unconfirmed and has no confirmation_token) + click_link SSO_SIGN_IN_BUTTON_TEXT + expectations_for_unconfirmed_user_with_no_confirmation_token_after_sign_in_attempt(user) + + visit root_path + # Sign-in attempt #2 (user still unconfirmed but has a confirmation_token) + click_link SSO_SIGN_IN_BUTTON_TEXT + expectations_for_unconfirmed_user_with_confirmation_token_after_sign_in_attempt + + # Sign-in attempt #3 (user is now confirmed) + user.confirm + click_link SSO_SIGN_IN_BUTTON_TEXT + # The user is signed in + expect(page).to have_current_path(plans_path) + end + + scenario 'A user is unconfirmed but has no confirmation token. + There sign in attempt fails, and a custom flash message + is rendered that can be used to navigate to the confirmation page.', :js do + user = create(:user, confirmed_at: nil) + # Attempt to sign in the unconfirmed user + sign_in(user) + expect(page).to have_current_path(root_path) + # A flash warning is displayed informing the user that they have to confirm their email + within('#notification-area') do + # Click the link embedded in the flash message + find('a.a-orange').click + end + # The user is redirected to the confirmation page + expect(current_path).to eq(new_user_confirmation_path) + end + + private + + # rubocop:disable Metrics/AbcSize + def expectations_for_unconfirmed_user_with_no_confirmation_token_after_sign_in_attempt(user) + # The user is not signed in + expect(page).to have_current_path(root_path) + # A flash notice is displayed informing the user that a confirmation email has been sent + expect(page).to have_selector('#notification-area', + text: I18n.t('devise.registrations.signed_up_but_unconfirmed')) + # reload the user to check confirmation values + user.reload + # The user remains unconfirmed + expect(user.confirmed?).to be(false) + # A confirmation_token now exists + expect(user.confirmation_token).to be_present + end + # rubocop:enable Metrics/AbcSize + + def expectations_for_unconfirmed_user_with_confirmation_token_after_sign_in_attempt + # The user is not signed in + expect(current_path).to eq(root_path) + within('#notification-area') do + # Get the HTML content of the flash message + html_content = find(:xpath, '.').native.attribute('innerHTML') + # A flash warning is displayed informing the user that they have to confirm their email + expect(html_content).to include(I18n.t('devise.failure.unconfirmed')) + end + end +end From ad899ae7567067a1d86a32070a7407349c2d8850 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 22 Apr 2025 16:48:49 -0600 Subject: [PATCH 14/17] Fix failing shibboleth test This change attempts to resolve the following error: ``` 1) Email Confirmation A user attempts to sign in via the "Sign in with your institutional credentials" button. The email is linked to a user account, however the account is unconfirmed and has no confirmation token. Failure/Error: raise ActionNotFound.new("The action '#{action}' could not be found for #{self.class.name}", self, action) AbstractController::ActionNotFound: The action 'shibboleth' could not be found for Users::OmniauthCallbacksController ``` The Users::OmniauthCallbacksController#shibboleth action is defined dynamically within the controller. Everything is working locally with commit bcc8d1f0ef493819c3f9fa001989e36636f50269, but the test is failing when executed as a GitHub Action. This change explicitly defines the shibboleth-related Users::OmniauthCallbacksController action --- spec/features/email_confirmation_spec.rb | 58 ++++++++++++++---------- spec/support/helpers/omniauth_helper.rb | 15 ++++++ 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/spec/features/email_confirmation_spec.rb b/spec/features/email_confirmation_spec.rb index 93f744a620..292bbe6f01 100644 --- a/spec/features/email_confirmation_spec.rb +++ b/spec/features/email_confirmation_spec.rb @@ -26,34 +26,42 @@ expect(page).to have_current_path(plans_path) end - scenario 'A user attempts to sign in via the "Sign in with your institutional credentials" - button. The email is linked to a user account, however the account is - unconfirmed and has no confirmation token.', :js do - user = create(:user, :unconfirmed_and_no_confirmation_token) - scheme = create(:identifier_scheme, :shibboleth) - # Mock the OmniAuth authentication hash for Shibboleth via OmniAuthHelper - OmniAuth.config.mock_auth[:shibboleth] = mock_auth_hash(user, scheme) - # Create a Shibboleth-related Identifier for the user - Identifier.create(identifier_scheme: scheme, - value: OmniAuth.config.mock_auth[:shibboleth].uid, - attrs: OmniAuth.config.mock_auth[:shibboleth], - identifiable: user) + describe 'Initial setup for shibboleth sign-in' do + before do + # Set up the user and identifier scheme + @user = create(:user, :unconfirmed_and_no_confirmation_token) + scheme = create(:identifier_scheme, :shibboleth) + # Mock OmniAuth authentication hash for Shibboleth via OmniAuthHelper + OmniAuth.config.mock_auth[:shibboleth] = mock_auth_hash(@user, scheme) + # Explicitly define a Users::OmniauthCallbacksController action for the scheme + define_omniauth_callback_for(scheme) - visit root_path - # Sign-in attempt #1 (user is unconfirmed and has no confirmation_token) - click_link SSO_SIGN_IN_BUTTON_TEXT - expectations_for_unconfirmed_user_with_no_confirmation_token_after_sign_in_attempt(user) + # Create the identifier for the user + Identifier.create(identifier_scheme: scheme, + value: OmniAuth.config.mock_auth[:shibboleth].uid, + attrs: OmniAuth.config.mock_auth[:shibboleth], + identifiable: @user) + end - visit root_path - # Sign-in attempt #2 (user still unconfirmed but has a confirmation_token) - click_link SSO_SIGN_IN_BUTTON_TEXT - expectations_for_unconfirmed_user_with_confirmation_token_after_sign_in_attempt + scenario 'A user attempts to sign in via the "Sign in with your institutional credentials" + button. The email is linked to a user account, however the account is + unconfirmed and has no confirmation token.', :js do + visit root_path + # Sign-in attempt #1 (user is unconfirmed and has no confirmation_token) + click_link SSO_SIGN_IN_BUTTON_TEXT + expectations_for_unconfirmed_user_with_no_confirmation_token_after_sign_in_attempt(@user) - # Sign-in attempt #3 (user is now confirmed) - user.confirm - click_link SSO_SIGN_IN_BUTTON_TEXT - # The user is signed in - expect(page).to have_current_path(plans_path) + visit root_path + # Sign-in attempt #2 (user still unconfirmed but has a confirmation_token) + click_link SSO_SIGN_IN_BUTTON_TEXT + expectations_for_unconfirmed_user_with_confirmation_token_after_sign_in_attempt + + # Sign-in attempt #3 (user is now confirmed) + @user.confirm + click_link SSO_SIGN_IN_BUTTON_TEXT + # The user is signed in + expect(page).to have_current_path(plans_path) + end end scenario 'A user is unconfirmed but has no confirmation token. diff --git a/spec/support/helpers/omniauth_helper.rb b/spec/support/helpers/omniauth_helper.rb index ea6b5b2bd1..8ccd997ccc 100644 --- a/spec/support/helpers/omniauth_helper.rb +++ b/spec/support/helpers/omniauth_helper.rb @@ -6,6 +6,8 @@ module OmniAuthHelper # that simulates the authentication data returned by # an OmniAuth provider (e.g. Shibboleth). def mock_auth_hash(user, scheme) + # Ensure Devise correctly maps the :user model for OmniAuth in tests. + Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] Rails.application.env_config['omniauth.auth'] = OmniAuth::AuthHash.new({ provider: scheme.name, @@ -18,4 +20,17 @@ def mock_auth_hash(user, scheme) }) Rails.application.env_config['omniauth.auth'] end + + # Manually define an action in Users::OmniauthCallbacksController. + # These actions are dynamically defined in the controller (based on IdentifierScheme entries). + # Because required db entries may not yet exist when the controller is loaded in the test environment, + # an action can be explicitly defined here for the test to work. + def define_omniauth_callback_for(scheme) + # Only define the action if it passes the .for_authentication validation + return unless IdentifierScheme.for_authentication.exists?(id: scheme.id) + + Users::OmniauthCallbacksController.define_method(scheme.name.downcase) do + handle_omniauth(scheme) + end + end end From b13e4ff4e0078150e1fd4b03126e9f43151f3308 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 15 Apr 2025 14:36:03 -0600 Subject: [PATCH 15/17] Use `I18n.default_locale` in faker config The previous configuration had a hardcoded 'en' locale with a comment stating "Keep this as :en. Faker doesn't have :en-GB". However, testing confirms that Faker now works properly with :"en-GB". This change should allow for more accurate testing by using the application's specified locale. --- spec/support/faker.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/spec/support/faker.rb b/spec/support/faker.rb index afa86a907d..def6431e97 100644 --- a/spec/support/faker.rb +++ b/spec/support/faker.rb @@ -2,14 +2,10 @@ require 'faker' -# Keep this as :en. Faker doesn't have :en-GB -LOCALE = 'en' - RSpec.configure do |config| config.before(:each) do - I18n.locale = LOCALE - Faker::Config.locale = LOCALE - I18n.default_locale = LOCALE + I18n.locale = I18n.default_locale + Faker::Config.locale = I18n.default_locale end config.after(:each) do From b7d16c6d5b55a9cb462336da2e8ffbb45d1946a6 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 24 Apr 2025 09:51:14 -0600 Subject: [PATCH 16/17] Update CHANGELOG.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c06babae56..fe1f3f3280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ - Fix failing eslint workflow / upgrade `actions/checkout` & `actions/setup-node` to v3 [#3503](https://github.com/DMPRoadmap/roadmap/pull/3503) - Fix rendering of `confirm_merge` partial [#3515](https://github.com/DMPRoadmap/roadmap/pull/3515) - Remove Auto-Generated TinyMCE Skins and Add `public/tinymce/skins/` to `.gitignore` [#3466](https://github.com/DMPRoadmap/roadmap/pull/3466) +- Re-implement email confirmation [#3507](https://github.com/DMPRoadmap/roadmap/pull/3507) +- Update `spec/support/faker.rb` to use `I18n.default_locale` [#3507](https://github.com/DMPRoadmap/roadmap/pull/3507) ## v5.0.0 From ab61ae0d0f3e9d542185ede29c979a5e0f78f5ed Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 5 Jun 2025 09:18:25 -0600 Subject: [PATCH 17/17] Fix comments in Shibboleth test - The previous comments mistakenly stated that the user had no confirmation token. --- spec/features/email_confirmation_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/features/email_confirmation_spec.rb b/spec/features/email_confirmation_spec.rb index 292bbe6f01..ec1e9de1fc 100644 --- a/spec/features/email_confirmation_spec.rb +++ b/spec/features/email_confirmation_spec.rb @@ -64,9 +64,10 @@ end end - scenario 'A user is unconfirmed but has no confirmation token. + scenario 'A user has a confirmation token but is unconfirmed. There sign in attempt fails, and a custom flash message is rendered that can be used to navigate to the confirmation page.', :js do + # Create a user with a confirmation token but make them unconfirmed user = create(:user, confirmed_at: nil) # Attempt to sign in the unconfirmed user sign_in(user)