From 895a4c86363f134023563f836463c238debaa21d Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 25 Sep 2024 12:59:09 -0600 Subject: [PATCH 01/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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) From d163ecc9671d547e3449d9542c1621692324758a Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 7 Feb 2025 15:17:28 -0700 Subject: [PATCH 18/26] Add logic allowing superusers to confirm emails This change addresses an issue where some users reported not receiving the confirmation instructions email. Rather than having to manually update the users email confirmation status within the Rails console, this change allows users to confirm/unconfirm user emails within the app. --- .../super_admin/users_controller.rb | 19 ++++++++++++++++++- .../users/_email_confirmation_status.html.erb | 7 +++++++ app/views/super_admin/users/edit.html.erb | 2 ++ 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 app/views/super_admin/users/_email_confirmation_status.html.erb diff --git a/app/controllers/super_admin/users_controller.rb b/app/controllers/super_admin/users_controller.rb index c4b96141b7..36a95d5d5f 100644 --- a/app/controllers/super_admin/users_controller.rb +++ b/app/controllers/super_admin/users_controller.rb @@ -38,6 +38,7 @@ def update # Remove the extraneous Org Selector hidden fields attrs = remove_org_selection_params(params_in: attrs) + attrs = handle_confirmed_at_param(attrs) if @user.update(attrs) # If its a new Org create it @@ -125,7 +126,8 @@ def user_params :org_id, :org_name, :org_crosswalk, :department_id, :language_id, - :other_organisation) + :other_organisation, + :confirmed_at) end def merge_accounts @@ -136,5 +138,20 @@ def merge_accounts flash.now[:alert] = failure_message(@user, _('merge')) end end + + def handle_confirmed_at_param(attrs) + # if an unconfirmed email is now being confirmed + if !@user.confirmed? && attrs[:confirmed_at] == '1' + attrs[:confirmed_at] = Time.current + # elsif a confirmed email is now being unconfirmed + elsif @user.confirmed? && attrs[:confirmed_at] == '0' + attrs[:confirmed_at] = nil + else + # else delete the param + # (keeps value nil for unconfirmed user and maintains previous Time value for confirmed user) + attrs.delete(:confirmed_at) + end + attrs + end end end diff --git a/app/views/super_admin/users/_email_confirmation_status.html.erb b/app/views/super_admin/users/_email_confirmation_status.html.erb new file mode 100644 index 0000000000..21670c2e91 --- /dev/null +++ b/app/views/super_admin/users/_email_confirmation_status.html.erb @@ -0,0 +1,7 @@ +
+ <%= f.label(:confirmed_at, _('Email Confirmation Status'), class: 'control-label') %> +
+ <%= f.check_box(:confirmed_at, checked: @user.confirmed? ) %> + <%= @user.confirmed? ? _("Confirmed.") : _("Unconfirmed.") %> + <%= _("(Use checkbox to change status.)") %> +
diff --git a/app/views/super_admin/users/edit.html.erb b/app/views/super_admin/users/edit.html.erb index d4dc76902b..e58da75449 100644 --- a/app/views/super_admin/users/edit.html.erb +++ b/app/views/super_admin/users/edit.html.erb @@ -57,6 +57,8 @@ <% end %> + <%= render 'email_confirmation_status', f: f %> +
<%= f.button(_('Save'), class: 'btn btn-secondary', type: "submit", id: "personal_details_registration_form_submit") %> From 8dd4180919f29c753ba76e10ebb558f112a06408 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 10 Feb 2025 09:50:04 -0700 Subject: [PATCH 19/26] Add users_controller_spec + confirmation tests --- .../super_admin/users_controller_spec.rb | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 spec/controllers/super_admin/users_controller_spec.rb diff --git a/spec/controllers/super_admin/users_controller_spec.rb b/spec/controllers/super_admin/users_controller_spec.rb new file mode 100644 index 0000000000..85bd2eb372 --- /dev/null +++ b/spec/controllers/super_admin/users_controller_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe SuperAdmin::UsersController, type: :controller do + let(:super_admin) { create(:user, :super_admin) } + let(:user) { create(:user, confirmed_at: nil) } + + before do + sign_in super_admin + end + + describe 'PUT #update' do + context 'when confirming an unconfirmed user' do + it 'sets confirmed_at to the current time' do + put :update, params: { id: user.id, user: { confirmed_at: '1' } } + user.reload + expect(user.confirmed_at).to be_a(Time) + end + end + + context 'when unconfirming a confirmed user' do + before do + user.update(confirmed_at: Time.current) + end + + it 'sets confirmed_at to nil' do + put :update, params: { id: user.id, user: { confirmed_at: '0' } } + user.reload + expect(user.confirmed_at).to be_nil + end + end + + context 'when update will not affect confirmation status' do + it 'does not update confirmed_at value for an already confirmed user' do + # (usec: 0) removes mircoseconds to better enable comparison + user.update(confirmed_at: Time.current.change(usec: 0)) + original_confirmed_at = user.confirmed_at + patch :update, params: { id: user.id, user: { firstname: 'NewName', confirmed_at: '1' } } + user.reload + expect(user.confirmed_at).to eq(original_confirmed_at) + end + end + end +end From b9231819ddd078d23181adf3970e769127eb2617 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 10 Feb 2025 10:44:28 -0700 Subject: [PATCH 20/26] Fix email confirmation checkbox's alignment --- .../super_admin/users/_email_confirmation_status.html.erb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/views/super_admin/users/_email_confirmation_status.html.erb b/app/views/super_admin/users/_email_confirmation_status.html.erb index 21670c2e91..fe7b09f705 100644 --- a/app/views/super_admin/users/_email_confirmation_status.html.erb +++ b/app/views/super_admin/users/_email_confirmation_status.html.erb @@ -1,7 +1,9 @@
<%= f.label(:confirmed_at, _('Email Confirmation Status'), class: 'control-label') %>
- <%= f.check_box(:confirmed_at, checked: @user.confirmed? ) %> + <%= f.check_box(:confirmed_at, { checked: @user.confirmed?, + style: 'vertical-align: middle; + margin-top: -2px;' }) %> <%= @user.confirmed? ? _("Confirmed.") : _("Unconfirmed.") %> <%= _("(Use checkbox to change status.)") %>
From 6a695385c07e9ae08ebbed8129e0784b3cc54d45 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 10 Feb 2025 12:15:29 -0700 Subject: [PATCH 21/26] Prevent unconfirming of super admin emails --- .../super_admin/users/_email_confirmation_status.html.erb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/views/super_admin/users/_email_confirmation_status.html.erb b/app/views/super_admin/users/_email_confirmation_status.html.erb index fe7b09f705..7528238d91 100644 --- a/app/views/super_admin/users/_email_confirmation_status.html.erb +++ b/app/views/super_admin/users/_email_confirmation_status.html.erb @@ -1,9 +1,11 @@
<%= f.label(:confirmed_at, _('Email Confirmation Status'), class: 'control-label') %>
- <%= f.check_box(:confirmed_at, { checked: @user.confirmed?, + <% is_checkbox_disabled = @user.can_super_admin? && @user.confirmed_at.present? %> + <%= f.check_box(:confirmed_at, { checked: @user.confirmed?, + disabled: is_checkbox_disabled, style: 'vertical-align: middle; margin-top: -2px;' }) %> <%= @user.confirmed? ? _("Confirmed.") : _("Unconfirmed.") %> - <%= _("(Use checkbox to change status.)") %> + <%= content_tag(:small, _("(Use checkbox to change status.)")) unless is_checkbox_disabled %>
From fff120457dc6a860c17d950fea2e85505d02ac83 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 10 Feb 2025 12:56:42 -0700 Subject: [PATCH 22/26] Prevent superadmin un-confirmation if UI bypassed `spec/controllers/super_admin/users_controller_spec.rb`: - Added test to prevent un-confirming of any super admin email within app `app/controllers/super_admin/users_controller.rb`: - Added !@user.can_super_admin? check to enable the aforementioned test to pass - Also, updated a `patch` statement to `put` --- app/controllers/super_admin/users_controller.rb | 4 ++-- spec/controllers/super_admin/users_controller_spec.rb | 10 +++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/controllers/super_admin/users_controller.rb b/app/controllers/super_admin/users_controller.rb index 36a95d5d5f..d701e82019 100644 --- a/app/controllers/super_admin/users_controller.rb +++ b/app/controllers/super_admin/users_controller.rb @@ -143,8 +143,8 @@ def handle_confirmed_at_param(attrs) # if an unconfirmed email is now being confirmed if !@user.confirmed? && attrs[:confirmed_at] == '1' attrs[:confirmed_at] = Time.current - # elsif a confirmed email is now being unconfirmed - elsif @user.confirmed? && attrs[:confirmed_at] == '0' + # elsif a confirmed email is now being unconfirmed and the user is not a super admin + elsif @user.confirmed? && attrs[:confirmed_at] == '0' && !@user.can_super_admin? attrs[:confirmed_at] = nil else # else delete the param diff --git a/spec/controllers/super_admin/users_controller_spec.rb b/spec/controllers/super_admin/users_controller_spec.rb index 85bd2eb372..5d3232eb76 100644 --- a/spec/controllers/super_admin/users_controller_spec.rb +++ b/spec/controllers/super_admin/users_controller_spec.rb @@ -36,10 +36,18 @@ # (usec: 0) removes mircoseconds to better enable comparison user.update(confirmed_at: Time.current.change(usec: 0)) original_confirmed_at = user.confirmed_at - patch :update, params: { id: user.id, user: { firstname: 'NewName', confirmed_at: '1' } } + put :update, params: { id: user.id, user: { firstname: 'NewName', confirmed_at: '1' } } user.reload expect(user.confirmed_at).to eq(original_confirmed_at) end end + + context 'when attempting to set a super_admin to unconfirmed' do + it 'does not update confirmed_at value to nil' do + put :update, params: { id: super_admin.id, user: { confirmed_at: '0' } } + super_admin.reload + expect(super_admin.confirmed_at).not_to be_nil + end + end end end From 6a4071700601525fce27dfe882e403531aa09a15 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 10 Feb 2025 13:10:19 -0700 Subject: [PATCH 23/26] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe1f3f3280..c62924decc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Changelog +- Add Functionality Allowing Super Admins to Confirm/Unconfirm User Emails [#1009](https://github.com/portagenetwork/roadmap/pull/1009) - Updated seeds.rb file for identifier_schemes to include context value and removed logo_url and idenitifier_prefix for Shibboleth (as it was causing issues with SSO). [#3525](https://github.com/DMPRoadmap/roadmap/pull/3525) - Adjustments to style of select tags and plan download layout [#3509](https://github.com/DMPRoadmap/roadmap/pull/3509) - Fix failing eslint workflow / upgrade `actions/checkout` & `actions/setup-node` to v3 [#3503](https://github.com/DMPRoadmap/roadmap/pull/3503) From b00f75b97fcea9673f7a682c80988b32421cfa01 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 27 May 2025 17:59:04 -0600 Subject: [PATCH 24/26] Add comment to `def handle_confirmed_at_param` --- app/controllers/super_admin/users_controller.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/controllers/super_admin/users_controller.rb b/app/controllers/super_admin/users_controller.rb index d701e82019..3b98ce978c 100644 --- a/app/controllers/super_admin/users_controller.rb +++ b/app/controllers/super_admin/users_controller.rb @@ -140,6 +140,11 @@ def merge_accounts end def handle_confirmed_at_param(attrs) + # NOTE: The :confirmed_at param is controlled by a check_box in the form + # `app/views/super_admin/users/_email_confirmation_status.html.erb`. + # When the checkbox is checked, Rails submits the string '1' (indicating "confirmed"). + # When unchecked, it submits the string '0' (indicating "unconfirmed"). + # if an unconfirmed email is now being confirmed if !@user.confirmed? && attrs[:confirmed_at] == '1' attrs[:confirmed_at] = Time.current From 8b4cdd35798e27d7010f0ebc9ab93493bb3c53d2 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 28 May 2025 11:57:15 -0600 Subject: [PATCH 25/26] Clarify email confirmation checkbox instructions Improves UX and reduces confusion when updating a user's email confirmation status. - Moved confirmation status inline with the "Email status:" heading - Updated checkbox helper text to reflect current @user.confirmed? state: - Shows "(Check to confirm email)" if unconfirmed - Shows "(Uncheck to unconfirm email)" if confirmed --- .../super_admin/users/_email_confirmation_status.html.erb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/views/super_admin/users/_email_confirmation_status.html.erb b/app/views/super_admin/users/_email_confirmation_status.html.erb index 7528238d91..2641962229 100644 --- a/app/views/super_admin/users/_email_confirmation_status.html.erb +++ b/app/views/super_admin/users/_email_confirmation_status.html.erb @@ -1,11 +1,12 @@
- <%= f.label(:confirmed_at, _('Email Confirmation Status'), class: 'control-label') %> + <%= f.label(:confirmed_at, _('Email status: '), class: 'control-label') %> + <%= @user.confirmed? ? _("Confirmed.") : _("Unconfirmed.") %>
<% is_checkbox_disabled = @user.can_super_admin? && @user.confirmed_at.present? %> <%= f.check_box(:confirmed_at, { checked: @user.confirmed?, disabled: is_checkbox_disabled, style: 'vertical-align: middle; margin-top: -2px;' }) %> - <%= @user.confirmed? ? _("Confirmed.") : _("Unconfirmed.") %> - <%= content_tag(:small, _("(Use checkbox to change status.)")) unless is_checkbox_disabled %> + <% checkbox_helper_text = @user.confirmed? ? _('(Uncheck to unconfirm email)') : _('(Check to confirm email)') %> + <%= content_tag(:small, checkbox_helper_text) unless is_checkbox_disabled %>
From fbc38ec537575c119d3ea8d8bebdecc8394fd05f Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 4 Jul 2025 09:56:13 -0600 Subject: [PATCH 26/26] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c62924decc..022ca96f02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -- Add Functionality Allowing Super Admins to Confirm/Unconfirm User Emails [#1009](https://github.com/portagenetwork/roadmap/pull/1009) +- Add Functionality Allowing Super Admins to Confirm/Unconfirm User Emails [#3535](https://github.com/DMPRoadmap/roadmap/pull/3535) - Updated seeds.rb file for identifier_schemes to include context value and removed logo_url and idenitifier_prefix for Shibboleth (as it was causing issues with SSO). [#3525](https://github.com/DMPRoadmap/roadmap/pull/3525) - Adjustments to style of select tags and plan download layout [#3509](https://github.com/DMPRoadmap/roadmap/pull/3509) - Fix failing eslint workflow / upgrade `actions/checkout` & `actions/setup-node` to v3 [#3503](https://github.com/DMPRoadmap/roadmap/pull/3503)