From 725a13fe8ae7370eb5354b0964765dcb10d0405d Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 25 Sep 2024 12:59:09 -0600 Subject: [PATCH 01/24] 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 b67e036d2..3e6cff4c5 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 openid_connect] ## From c4eecda845a4a3d4f28d4417e68263be8e46d0a3 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 26 Sep 2024 14:07:46 -0600 Subject: [PATCH 02/24] Replace .yml value from DMPOnline to DMP Assistant The corresponding value for `devise.mailer.confirmation_instructions.subject` in `config/locales/translation.fr-CA.yml` specifies "Assistant PGD", but maybe it should be verified as well. Note: I don't think this is the proper way to update the locales files. Rather, I believe only `config/locales/en.yml` should be directly modified. However, https://github.com/portagenetwork/roadmap/issues/920 is currently preventing that. --- config/locales/translation.en-CA.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/locales/translation.en-CA.yml b/config/locales/translation.en-CA.yml index 1189b742f..643a3e768 100644 --- a/config/locales/translation.en-CA.yml +++ b/config/locales/translation.en-CA.yml @@ -232,7 +232,7 @@ en-CA: invited: You have a pending invitation, accept it to finish creating your account. mailer: confirmation_instructions: - subject: Confirm your DMPonline account + subject: Confirm your DMP Assistant account reset_password_instructions: subject: Reset password instructions unlock_instructions: From 6c41bd7d2bb44a4e632330380691fe82c0fc020d Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 26 Sep 2024 14:40:34 -0600 Subject: [PATCH 03/24] TEMP: customise devise.failure.unconfirmed values This commit customises Devise's default value for the key `devise.failure.unconfirmed`. The value adds a link to the email confirmation page. Note: The values for this key in both the `...en-CA.yml` and `...fr-CA.yml` files were provided solely by me and need to be approved/finalised by The Alliance --- config/locales/translation.en-CA.yml | 4 +++- config/locales/translation.fr-CA.yml | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/config/locales/translation.en-CA.yml b/config/locales/translation.en-CA.yml index 643a3e768..57424cfc3 100644 --- a/config/locales/translation.en-CA.yml +++ b/config/locales/translation.en-CA.yml @@ -228,7 +228,9 @@ en-CA: 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 9656bc6a4..d42bb88bc 100644 --- a/config/locales/translation.fr-CA.yml +++ b/config/locales/translation.fr-CA.yml @@ -242,7 +242,9 @@ fr-CA: not_found_in_database: Courriel 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 compte pour continuer. + unconfirmed: | + Vous devez confirmer votre compte pour continuer. + (Cliquez pour demander un nouvel e-mail de confirmation) invited: Vous avez une invitation en attente. Acceptez-la pour terminer la création de votre compte. mailer: From 6087aeed1340de9957610214135abab8fc2502ec Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 2 Oct 2024 14:33:57 -0600 Subject: [PATCH 04/24] Improve UX flow for existing unconfirmed users This commit seeks to improve the UX flow for existing users with unconfirmed emails. By default, when using Devise's `:confirmable` module, confirmation instructions are sent to the user at the time of account creation. This will be perfect for newly created users, but poses some challenges for existing users. A "first category" of these existing users would be those who were created while `:confirmable` was absent from the codebase. These users are unconfirmed and a confirmation token was never generated for them. A "second category" of these existing users are those that are unconfirmed but do have an existing confirmation token. Since `:confirmable` has been absent from the codebase for years, the most recent `confirmation_sent_at` value is quite old (`Mon, 22 Feb 2021 15:18:24.000000000 UTC +00:00`). The code changes in `app/controllers/sessions_controller.rb` and `app/models/user.rb` improves the UX flow for users in the "first category". Rather than requiring the user to manually request new confirmation instructions, these changes auto-send the email when these users first attempt to sign in. `lib/tasks/dmp_assistant_upgrade.rake` enables this improved UX flow for the aforementioned "second category" of users. `def handle_unconfirmed_users_with_outstanding_invitations` queries for these unconfirmed users and then updates both `confirmation_token` and `confirmation_sent_at` to null. As a result, these users now meet the criteria for the "first category" and can also make use of the improved UX flow. --- app/controllers/sessions_controller.rb | 13 ++++++++ app/models/user.rb | 8 +++++ lib/tasks/dmp_assistant_upgrade.rake | 46 ++++++++++++++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 lib/tasks/dmp_assistant_upgrade.rake diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 04e15f1de..80fa031fd 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.confirmation_instructions_handled? + handle_confirmation_instructions(existing_user) + return + end + # Until ORCID login is supported unless session['devise.shibboleth_data'].nil? args = { @@ -45,3 +50,11 @@ def destroy set_locale end end + +private + +def handle_confirmation_instructions(user) + user.send_confirmation_instructions + flash[:notice] = _('A confirmation email has been sent to you. Please check your inbox to confirm your account.') + redirect_to root_path +end diff --git a/app/models/user.rb b/app/models/user.rb index 3e6cff4c5..513574f72 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -407,6 +407,14 @@ def deliver_invitation(options = {}) ) end + # If this method returns false, then one of the following two scenarios must be true: + # 1) The user was created while the app wasn't using Devise :confirmable (a confirmation_token was never generated) + # 2) An outdated confirmation_token existed, but was set to nil via `rake dmp_assistant_upgrade:v4_2_3` + # (see `def handle_unconfirmed_users_with_outstanding_invitations`` in `lib/tasks/dmp_assistant_upgrade.rake`) + def confirmation_instructions_handled? + confirmed? || confirmation_token.present? + end + # Case insensitive search over User model # # field - The name of the field being queried diff --git a/lib/tasks/dmp_assistant_upgrade.rake b/lib/tasks/dmp_assistant_upgrade.rake new file mode 100644 index 000000000..373446b6e --- /dev/null +++ b/lib/tasks/dmp_assistant_upgrade.rake @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +# File for DMP Assistant upgrade tasks, beginning with release 4.1.1+portage-4.2.3 + +namespace :dmp_assistant_upgrade do + desc 'Upgrade to DMP Assistant 4.1.1+portage-4.2.3' + task v4_2_3: :environment do + p 'Executing upgrade tasks for DMP Assistant 4.1.1+portage-4.2.3' + p 'Beginning task 1: Handle email confirmation statuses of existing users' + p '------------------------------------------------------------------------' + handle_email_confirmation_statuses + end + + private + + def handle_email_confirmation_statuses + p 'Beginning task 1a: Handle email confirmation statuses for users with outstanding invitations' + p '------------------------------------------------------------------------' + handle_unconfirmed_users_with_outstanding_invitations + p 'Task 1 completed successfully' + end + + # Fetches users where 'confirmed_at IS NULL AND confirmation_token IS NOT NULL' + # Sets confirmation_token = NULL AND confirmation_sent_at = NULL for the aforementioned users + # `confirmation_sent_at` dates corresponding to these particular users are quite old (the most recent is 2021-02-22 15:18:24) + # With respect to confirming their email addresses, these db changes will improve the UX flow for these users + # (For more insight regarding this improved UX flow, + # refer to def handle_missing_confirmation_instructions(user) in app/controllers/sessions_controller.rb) + def handle_unconfirmed_users_with_outstanding_invitations + p 'Querying for users with unconfirmed emails AND outstanding/outdated confirmation invitations' + expected_count = 6413 + p "Expecting to find #{expected_count} users" + p '------------------------------------------------------------------------' + unconfirmed_users = User.where(confirmed_at: nil).where.not(confirmation_token: nil) + count = unconfirmed_users.count + abort "#{count} users found. Aborting the upgrade task." if count != expected_count + + p "#{count} users found" + p '------------------------------------------------------------------------' + p 'Setting confirmation_token = NULL AND confirmation_sent_at = NULL for all of these unconfirmed users' + p '------------------------------------------------------------------------' + unconfirmed_users.update_all(confirmation_token: nil, confirmation_sent_at: nil) + p '------------------------------------------------------------------------' + p 'Task 1a completed successfully' + end +end From 465d6712a069294b43748ee3b857204bb256652a Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 2 Oct 2024 14:55:44 -0600 Subject: [PATCH 05/24] Handle confirmation for SSO created users An "SSO created user" is a user whose account was created via the `Sign in with institutional or social ID` button. This occurs when the chosen SSO email is neither already linked to an existing account, nor does there exist a user account with the same email as the chosen SSO supplied email. In order to sign in via SSO, the user must provide the password/credentials for the SSO account that they have chosen. This could be considered a form of email confirmation. And because the user's account email will be set to match this SSO email, it follows that email confirmation via Devise's `:confirmable` module is redundant and can be bypassed. Taking the above reasoning into account, `app/models/user.rb` sets `confirmed_at: Time.now` for any future SSO created accounts. `lib/tasks/dmp_assistant_upgrade.rake` sets `lib/tasks/dmp_assistant_upgrade.rake` for any "existing SSO created accounts". Specifically, to be an "existing SSO created account", BOTH of the following conditions must be met: 1. An Identifier corresponding to the "openid_connect" `IdentifierScheme` exists for the user 2. For the identifier and corresponding user found in 1., the values of `identifier.created_at` and `user.created_at` are within 1 second of each other. --- app/models/user.rb | 5 ++++- lib/tasks/dmp_assistant_upgrade.rake | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 513574f72..06236756a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -197,7 +197,10 @@ def self.create_from_provider_data(provider_data) # We don't know which organization to setup so we will use other org: Org.find_by(is_other: true), accept_terms: true, - password: Devise.friendly_token[0, 20] + password: Devise.friendly_token[0, 20], + # provider_data.info.email comes from CILogon sign-in, which requires email confirmation + # It follows that we can set `confirmed_at: Time.now` and bypass Devise's email confirmation step + confirmed_at: Time.now ) user end diff --git a/lib/tasks/dmp_assistant_upgrade.rake b/lib/tasks/dmp_assistant_upgrade.rake index 373446b6e..2b59774d2 100644 --- a/lib/tasks/dmp_assistant_upgrade.rake +++ b/lib/tasks/dmp_assistant_upgrade.rake @@ -17,6 +17,9 @@ namespace :dmp_assistant_upgrade do p 'Beginning task 1a: Handle email confirmation statuses for users with outstanding invitations' p '------------------------------------------------------------------------' handle_unconfirmed_users_with_outstanding_invitations + p 'Beginning task 1b: Handle email confirmation statuses of user accounts created via SSO' + p '------------------------------------------------------------------------' + handle_unconfirmed_users_created_via_sso p 'Task 1 completed successfully' end @@ -43,4 +46,26 @@ namespace :dmp_assistant_upgrade do p '------------------------------------------------------------------------' p 'Task 1a completed successfully' end + + # Fetches users "created via SSO" and updates confirmed_at = Time.now for those users + # We define a user as "created via SSO" if BOTH of the following conditions are true: + # 1. An Identifier corresponding to the "openid_connect" IdentifierScheme exists for the user + # 2. For the identifier and corresponding user found in 1., + # identifier.created_at AND user.created_at are within 1 second of each other + def handle_unconfirmed_users_created_via_sso + openid_connect_scheme = IdentifierScheme.find_by(name: 'openid_connect') + p 'Querying for all users created via SSO' + p '------------------------------------------------------------------------' + sql_where = 'ABS(EXTRACT(EPOCH FROM users.created_at) - EXTRACT(EPOCH FROM identifiers.created_at)) <= 1' + users_created_via_sso = User.joins(:identifiers) + .where(identifiers: { identifier_scheme_id: openid_connect_scheme.id }) + .where(sql_where) + .distinct + p "Found #{users_created_via_sso.count} users created via SSO" + confirmed_at = Time.now + p "Updating these users as confirmed with 'confirmed_at = #{confirmed_at}'" + p '------------------------------------------------------------------------' + users_created_via_sso.update_all(confirmed_at: confirmed_at) + p 'Task 1b completed successfully' + end end From 965c3365b00ca17ac1d4d68dfb54a0c3075b96d8 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 3 Oct 2024 10:02:25 -0600 Subject: [PATCH 06/24] Re-add SSO linking capability for signed-out users This commit re-allows existing users to link via SSO while signed out. This functionality was previously removed via commit e112202fa. The logic works as follows: 1) A user clicks the "Sign in with institutional or social ID" button 2) Within CILogon, the user chooses an email to sign in with. For this particular functionality to be used, the following conditions must both be true for the chosen email: a) It is not currently linked to any user accounts b) It matches an existing user.email in the database 3) If the matching user.email is confirmed, then the SSO email is linked to the user account and the user is signed-in. Otherwise, the user remains signed out and a flash notice is rendered indicating that the user.email requires confirmation. --- .../users/omniauth_callbacks_controller.rb | 19 ++++++------------- app/models/user.rb | 4 ++-- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 3640b5de1..91a5e7b0b 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -24,20 +24,13 @@ def openid_connect if current_user.nil? # if user is not signed in (They clicked the SSO sign in button) if user.nil? # If an entry does not exist in the identifiers table for the chosen SSO account - user = User.create_from_provider_data(auth) - if user.nil? # if a user was NOT created (a match was found for User.find_by(email: auth.info.email) - # Do not link SSO credentials for the signed out, existing user - flash[:alert] = _('That email appears to be associated with an existing account.
' \ - 'Sign into your existing account, and you can link that ' \ - "account with SSO from the 'Edit Profile' page.") - redirect_to root_path - return + user = User.find_or_create_from_provider_data(auth) + if user.confirmed? # Only create the Identifier entry if user.email is confirmed + user.identifiers << Identifier.create(identifier_scheme: identifier_scheme, + value: auth.uid, + attrs: auth, + identifiable: user) end - # A new user was created, link the SSO credentials (we can do this for a newly created user) - user.identifiers << Identifier.create(identifier_scheme: identifier_scheme, - value: auth.uid, - attrs: auth, - identifiable: user) end sign_in_and_redirect user, event: :authentication elsif user.nil? diff --git a/app/models/user.rb b/app/models/user.rb index 06236756a..6ee1eb6df 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -185,10 +185,10 @@ def self.from_omniauth(auth) ## # Handle user creation from provider # rubocop:disable Metrics/AbcSize - def self.create_from_provider_data(provider_data) + def self.find_or_create_from_provider_data(provider_data) user = User.find_or_initialize_by(email: provider_data.info.email) - return unless user.new_record? + return user unless user.new_record? user.update!( firstname: provider_data.info&.first_name.present? ? provider_data.info.first_name : _('First name'), From 1caf1f7e60d84ce2d7c8e512a0cb7ee214a89b2b Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 3 Oct 2024 14:30:05 -0600 Subject: [PATCH 07/24] Refactor / Make Rubocop happy --- app/controllers/sessions_controller.rb | 4 +-- .../users/omniauth_callbacks_controller.rb | 32 ++++++++++++------- lib/tasks/dmp_assistant_upgrade.rake | 6 ++-- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 80fa031fd..f14b3d9e4 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -8,7 +8,7 @@ def new # Capture the user's shibboleth id if they're coming in from an IDP # --------------------------------------------------------------------- - # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def create existing_user = User.find_by(email: params[:user][:email]) unless existing_user.nil? @@ -41,7 +41,7 @@ def create end end end - # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength def destroy super diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 91a5e7b0b..103cc1497 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -5,7 +5,7 @@ module Users class OmniauthCallbacksController < Devise::OmniauthCallbacksController # This is for the OpenidConnect CILogon - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def openid_connect # First or create auth = request.env['omniauth.auth'] @@ -23,15 +23,8 @@ def openid_connect identifier_scheme = IdentifierScheme.find_by(name: auth.provider) if current_user.nil? # if user is not signed in (They clicked the SSO sign in button) - if user.nil? # If an entry does not exist in the identifiers table for the chosen SSO account - user = User.find_or_create_from_provider_data(auth) - if user.confirmed? # Only create the Identifier entry if user.email is confirmed - user.identifiers << Identifier.create(identifier_scheme: identifier_scheme, - value: auth.uid, - attrs: auth, - identifiable: user) - end - end + # user.nil? is true if the chosen CILogon email is not currently linked to an existing user account + user = handle_new_sso_email_for_signed_out_user(auth, identifier_scheme) if user.nil? sign_in_and_redirect user, event: :authentication elsif user.nil? # we need to link @@ -51,7 +44,7 @@ def openid_connect redirect_to edit_user_registration_path end end - # rubocop:enable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength def orcid handle_omniauth(IdentifierScheme.for_authentication.find_by(name: 'orcid')) @@ -133,5 +126,22 @@ def handle_omniauth(scheme) def failure redirect_to root_path end + + private + + # This method is executed when a user performs the following two steps: + # 1) clicks "Sign in with institutional or social ID" + # 2) Within CILogon, selects an email that is not currently linked to an existing user account + def handle_new_sso_email_for_signed_out_user(auth, identifier_scheme) + # Find or create the user with user.email == email selected via SSO + user = User.find_or_create_from_provider_data(auth) + if user.confirmed? # Only link the SSO email if user.email is confirmed + user.identifiers << Identifier.create(identifier_scheme: identifier_scheme, + value: auth.uid, + attrs: auth, + identifiable: user) + end + user + end end end diff --git a/lib/tasks/dmp_assistant_upgrade.rake b/lib/tasks/dmp_assistant_upgrade.rake index 2b59774d2..f291e8e7c 100644 --- a/lib/tasks/dmp_assistant_upgrade.rake +++ b/lib/tasks/dmp_assistant_upgrade.rake @@ -2,6 +2,7 @@ # File for DMP Assistant upgrade tasks, beginning with release 4.1.1+portage-4.2.3 +# rubocop:disable Naming/VariableNumber namespace :dmp_assistant_upgrade do desc 'Upgrade to DMP Assistant 4.1.1+portage-4.2.3' task v4_2_3: :environment do @@ -9,6 +10,7 @@ namespace :dmp_assistant_upgrade do p 'Beginning task 1: Handle email confirmation statuses of existing users' p '------------------------------------------------------------------------' handle_email_confirmation_statuses + p 'All tasks completed successfully' end private @@ -25,7 +27,7 @@ namespace :dmp_assistant_upgrade do # Fetches users where 'confirmed_at IS NULL AND confirmation_token IS NOT NULL' # Sets confirmation_token = NULL AND confirmation_sent_at = NULL for the aforementioned users - # `confirmation_sent_at` dates corresponding to these particular users are quite old (the most recent is 2021-02-22 15:18:24) + # `confirmation_sent_at` dates for these users are quite old (the most recent is 2021-02-22 15:18:24) # With respect to confirming their email addresses, these db changes will improve the UX flow for these users # (For more insight regarding this improved UX flow, # refer to def handle_missing_confirmation_instructions(user) in app/controllers/sessions_controller.rb) @@ -43,7 +45,6 @@ namespace :dmp_assistant_upgrade do p 'Setting confirmation_token = NULL AND confirmation_sent_at = NULL for all of these unconfirmed users' p '------------------------------------------------------------------------' unconfirmed_users.update_all(confirmation_token: nil, confirmation_sent_at: nil) - p '------------------------------------------------------------------------' p 'Task 1a completed successfully' end @@ -69,3 +70,4 @@ namespace :dmp_assistant_upgrade do p 'Task 1b completed successfully' end end +# rubocop:enable Naming/VariableNumber From bf54e2faaf0ba4dc9e1b362a06618d6321c712f0 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Oct 2024 09:28:57 -0600 Subject: [PATCH 08/24] 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.now }` 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 `spec/integration/openid_connect_sso_spec.rb` - Removing `it 'does not create SSO link when user is signed out and SSO email is an existing account email'` test. Commit 965c3365b ("Re-add SSO linking capability for signed-out users") updated the SSO behaviour in a manner that this test is no longer relevant. --- config/environments/test.rb | 1 + spec/factories/users.rb | 1 + spec/features/registrations_spec.rb | 6 +++--- spec/integration/openid_connect_sso_spec.rb | 12 ------------ 4 files changed, 5 insertions(+), 15 deletions(-) diff --git a/config/environments/test.rb b/config/environments/test.rb index 081047929..f6bb16786 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 c91c75660..6114b60e8 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.now } trait :org_admin do after(:create) do |user, _evaluator| diff --git a/spec/features/registrations_spec.rb b/spec/features/registrations_spec.rb index 50ef4d60a..cfb40eea8 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('A message with a confirmation link has been sent to your email address.') + expect(User.count).to eq(1) end scenario 'User attempts to create a new acccount with invalid atts', :js do diff --git a/spec/integration/openid_connect_sso_spec.rb b/spec/integration/openid_connect_sso_spec.rb index 70fb27a67..063dc0e1d 100644 --- a/spec/integration/openid_connect_sso_spec.rb +++ b/spec/integration/openid_connect_sso_spec.rb @@ -39,18 +39,6 @@ expect(page).to have_content('John Doe') end - it 'does not create SSO link when user is signed out and SSO email is an existing account email' do - # Hardcode user email to what we are mocking via OmniAuth.config.mock_auth[:openid_connect] - user = create(:user, :org_admin, org: @org, email: 'user@organization.ca', firstname: 'DMP Name', - surname: 'DMP Lastname') - expect(user.identifiers.count).to eql(0) - visit root_path - click_link 'Sign in with institutional or social ID' - error_message = 'That email appears to be associated with an existing account' - expect(page).to have_content(error_message) - expect(user.identifiers.count).to eql(0) - end - xit 'links account from external credentails' do # Create existing user user = create(:user, :org_admin, org: @org, email: 'user@organization.ca', firstname: 'DMP Name', From c68f71367551e0aebb29b651694e2c078d2d76bb Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Oct 2024 11:57:33 -0600 Subject: [PATCH 09/24] Reset `:confirmable` cols for all non-superusers - These changes replace the changes made to `lib/tasks/dmp_assistant_upgrade.rake` in commits 6087aeed1340de9957610214135abab8fc2502ec and 465d6712a069294b43748ee3b857204bb256652a. We are now setting `confirmed_at`, `confirmation_token`, and `confirmation_sent_at` to nil for all users. (the rake task subsequently sets `confirmed_at = Time.now` for all superusers). --- app/models/user.rb | 2 +- lib/tasks/dmp_assistant_upgrade.rake | 82 +++++++++++----------------- 2 files changed, 34 insertions(+), 50 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 6ee1eb6df..b1b7eef13 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -413,7 +413,7 @@ def deliver_invitation(options = {}) # If this method returns false, then one of the following two scenarios must be true: # 1) The user was created while the app wasn't using Devise :confirmable (a confirmation_token was never generated) # 2) An outdated confirmation_token existed, but was set to nil via `rake dmp_assistant_upgrade:v4_2_3` - # (see `def handle_unconfirmed_users_with_outstanding_invitations`` in `lib/tasks/dmp_assistant_upgrade.rake`) + # (see `def handle_email_confirmations_for_existing_users`` in `lib/tasks/dmp_assistant_upgrade.rake`) def confirmation_instructions_handled? confirmed? || confirmation_token.present? end diff --git a/lib/tasks/dmp_assistant_upgrade.rake b/lib/tasks/dmp_assistant_upgrade.rake index f291e8e7c..22d6650f8 100644 --- a/lib/tasks/dmp_assistant_upgrade.rake +++ b/lib/tasks/dmp_assistant_upgrade.rake @@ -6,68 +6,52 @@ namespace :dmp_assistant_upgrade do desc 'Upgrade to DMP Assistant 4.1.1+portage-4.2.3' task v4_2_3: :environment do + p '------------------------------------------------------------------------' p 'Executing upgrade tasks for DMP Assistant 4.1.1+portage-4.2.3' - p 'Beginning task 1: Handle email confirmation statuses of existing users' + p 'Beginning task: Handle email confirmations for existing users' p '------------------------------------------------------------------------' - handle_email_confirmation_statuses + handle_email_confirmations_for_existing_users + p 'Task completed: Handle email confirmations for existing users' p 'All tasks completed successfully' end + # rubocop:enable Naming/VariableNumber private - def handle_email_confirmation_statuses - p 'Beginning task 1a: Handle email confirmation statuses for users with outstanding invitations' + def handle_email_confirmations_for_existing_users + 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 '------------------------------------------------------------------------' - handle_unconfirmed_users_with_outstanding_invitations - p 'Beginning task 1b: Handle email confirmation statuses of user accounts created via SSO' + set_confirmable_cols_to_nil_for_all_users p '------------------------------------------------------------------------' - handle_unconfirmed_users_created_via_sso - p 'Task 1 completed successfully' - end - - # Fetches users where 'confirmed_at IS NULL AND confirmation_token IS NOT NULL' - # Sets confirmation_token = NULL AND confirmation_sent_at = NULL for the aforementioned users - # `confirmation_sent_at` dates for these users are quite old (the most recent is 2021-02-22 15:18:24) - # With respect to confirming their email addresses, these db changes will improve the UX flow for these users - # (For more insight regarding this improved UX flow, - # refer to def handle_missing_confirmation_instructions(user) in app/controllers/sessions_controller.rb) - def handle_unconfirmed_users_with_outstanding_invitations - p 'Querying for users with unconfirmed emails AND outstanding/outdated confirmation invitations' - expected_count = 6413 - p "Expecting to find #{expected_count} users" + p 'Updating superusers so that they are not required to confirm their email addresses' + p '(i.e. Setting `confirmed_at = Time.now`` for superusers)' p '------------------------------------------------------------------------' - unconfirmed_users = User.where(confirmed_at: nil).where.not(confirmation_token: nil) - count = unconfirmed_users.count - abort "#{count} users found. Aborting the upgrade task." if count != expected_count + confirm_superusers + end - p "#{count} users found" - p '------------------------------------------------------------------------' - p 'Setting confirmation_token = NULL AND confirmation_sent_at = NULL for all of these unconfirmed users' - p '------------------------------------------------------------------------' - unconfirmed_users.update_all(confirmation_token: nil, confirmation_sent_at: nil) - p 'Task 1a completed successfully' + # Setting `confirmed_at` to nil will require users to confirm their email addresses when using :confirmable + # Setting `confirmation_token` to nil will improve the email confirmation-related UX flow for existing users + # (For more information regarding this improved UX flow, refer to the following: + # `def handle_confirmation_instructions(user)` in `app/controllers/sessions_controller.rb`) + 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 - # Fetches users "created via SSO" and updates confirmed_at = Time.now for those users - # We define a user as "created via SSO" if BOTH of the following conditions are true: - # 1. An Identifier corresponding to the "openid_connect" IdentifierScheme exists for the user - # 2. For the identifier and corresponding user found in 1., - # identifier.created_at AND user.created_at are within 1 second of each other - def handle_unconfirmed_users_created_via_sso - openid_connect_scheme = IdentifierScheme.find_by(name: 'openid_connect') - p 'Querying for all users created via SSO' - p '------------------------------------------------------------------------' - sql_where = 'ABS(EXTRACT(EPOCH FROM users.created_at) - EXTRACT(EPOCH FROM identifiers.created_at)) <= 1' - users_created_via_sso = User.joins(:identifiers) - .where(identifiers: { identifier_scheme_id: openid_connect_scheme.id }) - .where(sql_where) - .distinct - p "Found #{users_created_via_sso.count} users created via SSO" + # Sets `confirmed_at` to `Time.now` for all superusers + def confirm_superusers confirmed_at = Time.now - p "Updating these users as confirmed with 'confirmed_at = #{confirmed_at}'" - p '------------------------------------------------------------------------' - users_created_via_sso.update_all(confirmed_at: confirmed_at) - p 'Task 1b completed successfully' + 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 -# rubocop:enable Naming/VariableNumber From fb883ec7a42d4d67b62f13f7fce7f177b8204784 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Oct 2024 16:40:53 -0600 Subject: [PATCH 10/24] Reuse `signed_up_but_unconfirmed` flash message This commit changes the flash message we are using for our custom UX flow. Now, rather than a custom message, we are reusing `devise.registrations.signed_up_but_unconfirmed`. --- app/controllers/sessions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index f14b3d9e4..5e1dd6ddd 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -55,6 +55,6 @@ def destroy def handle_confirmation_instructions(user) user.send_confirmation_instructions - flash[:notice] = _('A confirmation email has been sent to you. Please check your inbox to confirm your account.') + flash[:notice] = I18n.t('devise.registrations.signed_up_but_unconfirmed') redirect_to root_path end From a2f607b13e8e60909d03f220e05d9c9e916c4cb5 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 15 Oct 2024 10:35:54 -0600 Subject: [PATCH 11/24] Enable custom confirmation UX flow for SSO sign in This commit takes the idea in commit 6087aeed1340de9957610214135abab8fc2502ec and applies it to users signing in via SSO. Via the addition of `app/controllers/concerns/email_confirmation_handler.rb`, some refactoring has been applied here as well. --- app/controllers/concerns/email_confirmation_handler.rb | 9 +++++++++ app/controllers/sessions_controller.rb | 10 ++-------- app/controllers/users/omniauth_callbacks_controller.rb | 7 ++++++- 3 files changed, 17 insertions(+), 9 deletions(-) create mode 100644 app/controllers/concerns/email_confirmation_handler.rb diff --git a/app/controllers/concerns/email_confirmation_handler.rb b/app/controllers/concerns/email_confirmation_handler.rb new file mode 100644 index 000000000..615d91d85 --- /dev/null +++ b/app/controllers/concerns/email_confirmation_handler.rb @@ -0,0 +1,9 @@ +module EmailConfirmationHandler + extend ActiveSupport::Concern + + def handle_confirmation_instructions(user) + user.send_confirmation_instructions + flash[:notice] = I18n.t('devise.registrations.signed_up_but_unconfirmed') + redirect_to root_path + end +end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 5e1dd6ddd..063b00b3a 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 @@ -50,11 +52,3 @@ def destroy set_locale end end - -private - -def handle_confirmation_instructions(user) - user.send_confirmation_instructions - 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 103cc1497..9972bdce3 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -3,8 +3,9 @@ module Users # Controller that handles callbacks from OmniAuth integrations (e.g. Shibboleth and ORCID) class OmniauthCallbacksController < Devise::OmniauthCallbacksController - # This is for the OpenidConnect CILogon + include EmailConfirmationHandler + # This is for the OpenidConnect CILogon # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def openid_connect # First or create @@ -25,6 +26,10 @@ def openid_connect if current_user.nil? # if user is not signed in (They clicked the SSO sign in button) # user.nil? is true if the chosen CILogon email is not currently linked to an existing user account user = handle_new_sso_email_for_signed_out_user(auth, identifier_scheme) if user.nil? + unless user.confirmation_instructions_handled? + handle_confirmation_instructions(user) + return + end sign_in_and_redirect user, event: :authentication elsif user.nil? # we need to link From 414e376c881b052b4c19224caa4499872f3256fe Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 15 Oct 2024 11:16:59 -0600 Subject: [PATCH 12/24] Refactor: Move logic to `EmailConfirmationHandler` --- .../concerns/email_confirmation_handler.rb | 22 ++++++++++++++++--- app/controllers/sessions_controller.rb | 9 +++----- .../users/omniauth_callbacks_controller.rb | 6 ++--- app/models/user.rb | 6 +---- 4 files changed, 25 insertions(+), 18 deletions(-) diff --git a/app/controllers/concerns/email_confirmation_handler.rb b/app/controllers/concerns/email_confirmation_handler.rb index 615d91d85..1191387f6 100644 --- a/app/controllers/concerns/email_confirmation_handler.rb +++ b/app/controllers/concerns/email_confirmation_handler.rb @@ -1,9 +1,25 @@ +# EmailConfirmationHandler + +# 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 have not always used Devise's :confirmable module (:confirmable generates a confirmation_token at the time of User creation) +# - We have set `confirmed_at` and `confirmation_token` to nil via Rake tasks +# This concern is meant to improve the confirmation process for those users module EmailConfirmationHandler extend ActiveSupport::Concern - def handle_confirmation_instructions(user) + # confirmation instructions are "missing" if the user is both unconfirmed AND has no outstanding confirmation_token + def missing_confirmation_instructions_handled?(user) + return false if user.confirmed_or_has_confirmation_token? + + handle_missing_confirmation_instructions(user) + true + end + + private + + def handle_missing_confirmation_instructions(user) user.send_confirmation_instructions - flash[:notice] = I18n.t('devise.registrations.signed_up_but_unconfirmed') - redirect_to root_path + redirect_to root_path, notice: I18n.t('devise.registrations.signed_up_but_unconfirmed') end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 063b00b3a..df9a8b7ce 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -10,15 +10,12 @@ def new # Capture the user's shibboleth id if they're coming in from an IDP # --------------------------------------------------------------------- - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + # rubocop:disable Metrics/AbcSize def create existing_user = User.find_by(email: params[:user][:email]) unless existing_user.nil? - unless existing_user.confirmation_instructions_handled? - handle_confirmation_instructions(existing_user) - return - end + return if missing_confirmation_instructions_handled?(existing_user) # Until ORCID login is supported unless session['devise.shibboleth_data'].nil? @@ -43,7 +40,7 @@ def create end end end - # rubocop:enable Metrics/AbcSize, Metrics/MethodLength + # rubocop:enable Metrics/AbcSize def destroy super diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 9972bdce3..2bca4fc3d 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -26,10 +26,8 @@ def openid_connect if current_user.nil? # if user is not signed in (They clicked the SSO sign in button) # user.nil? is true if the chosen CILogon email is not currently linked to an existing user account user = handle_new_sso_email_for_signed_out_user(auth, identifier_scheme) if user.nil? - unless user.confirmation_instructions_handled? - handle_confirmation_instructions(user) - return - end + return if missing_confirmation_instructions_handled?(user) + sign_in_and_redirect user, event: :authentication elsif user.nil? # we need to link diff --git a/app/models/user.rb b/app/models/user.rb index b1b7eef13..89aa3dcfd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -410,11 +410,7 @@ def deliver_invitation(options = {}) ) end - # If this method returns false, then one of the following two scenarios must be true: - # 1) The user was created while the app wasn't using Devise :confirmable (a confirmation_token was never generated) - # 2) An outdated confirmation_token existed, but was set to nil via `rake dmp_assistant_upgrade:v4_2_3` - # (see `def handle_email_confirmations_for_existing_users`` in `lib/tasks/dmp_assistant_upgrade.rake`) - def confirmation_instructions_handled? + def confirmed_or_has_confirmation_token? confirmed? || confirmation_token.present? end From a25a66af5d1fcb7457093ddf58fcc33b8df424ff Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 15 Oct 2024 20:11:53 -0600 Subject: [PATCH 13/24] Refactor / Make rubocop happy Some refactoring was performed in `def openid_connect` to avoid adding `Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity` to `rubocop:disable`. --- .../concerns/email_confirmation_handler.rb | 4 +++- .../users/omniauth_callbacks_controller.rb | 14 +++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/app/controllers/concerns/email_confirmation_handler.rb b/app/controllers/concerns/email_confirmation_handler.rb index 1191387f6..90bd48938 100644 --- a/app/controllers/concerns/email_confirmation_handler.rb +++ b/app/controllers/concerns/email_confirmation_handler.rb @@ -1,8 +1,10 @@ +# frozen_string_literal: true + # EmailConfirmationHandler # 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 have not always used Devise's :confirmable module (:confirmable generates a confirmation_token at the time of User creation) +# - 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 # This concern is meant to improve the confirmation process for those users module EmailConfirmationHandler diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 2bca4fc3d..9e1e3f219 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -24,11 +24,7 @@ def openid_connect identifier_scheme = IdentifierScheme.find_by(name: auth.provider) if current_user.nil? # if user is not signed in (They clicked the SSO sign in button) - # user.nil? is true if the chosen CILogon email is not currently linked to an existing user account - user = handle_new_sso_email_for_signed_out_user(auth, identifier_scheme) if user.nil? - return if missing_confirmation_instructions_handled?(user) - - sign_in_and_redirect user, event: :authentication + handle_openid_connect_for_signed_out_user(user, auth, identifier_scheme) elsif user.nil? # we need to link current_user.identifiers << Identifier.create(identifier_scheme: identifier_scheme, @@ -132,6 +128,14 @@ def failure private + def handle_openid_connect_for_signed_out_user(user, auth, identifier_scheme) + # user.nil? is true if the chosen CILogon email is not currently linked to an existing user account + user = handle_new_sso_email_for_signed_out_user(auth, identifier_scheme) if user.nil? + return if missing_confirmation_instructions_handled?(user) + + sign_in_and_redirect user, event: :authentication + end + # This method is executed when a user performs the following two steps: # 1) clicks "Sign in with institutional or social ID" # 2) Within CILogon, selects an email that is not currently linked to an existing user account From a77e5fef49d98c34cfdbb4ace7d1924fa86cb0f9 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 17 Oct 2024 09:52:04 -0600 Subject: [PATCH 14/24] Add test for SSO linking of signed-out users --- spec/integration/openid_connect_sso_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/integration/openid_connect_sso_spec.rb b/spec/integration/openid_connect_sso_spec.rb index 063dc0e1d..544aae31d 100644 --- a/spec/integration/openid_connect_sso_spec.rb +++ b/spec/integration/openid_connect_sso_spec.rb @@ -39,6 +39,21 @@ expect(page).to have_content('John Doe') end + scenario 'A user attempts to sign in via the "Sign in with institutional + or social ID" button with an email that is not currently linked + to any account. The chosen SSO email matches an existing user account email.', :js do + user = create(:user, :org_admin, org: @org, email: 'user@organization.ca', + firstname: 'DMP Name', surname: 'DMP Lastname') + visit root_path + click_link 'Sign in with institutional or social ID' + + # The user is signed in + expect(current_path).to eql(plans_path) + # The SSO email is linked to the user + expect(user.identifiers.count).to eql(1) + expect(Identifier.last.identifiable).to eql(user) + end + xit 'links account from external credentails' do # Create existing user user = create(:user, :org_admin, org: @org, email: 'user@organization.ca', firstname: 'DMP Name', From 90f65ba14badd419d6be08c24b531c8c3c0b6ccf Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 9 Oct 2024 16:37:10 -0600 Subject: [PATCH 15/24] Add tests for custom email confirmation UX flow - These tests are meant to confirm that the desired behaviour is encountered via our custom email confirmation UX flow. - For more info, See comments for `app/controllers/concerns/email_confirmation_handler.rb` --- spec/factories/users.rb | 9 ++++ spec/integration/email_confirmation_spec.rb | 50 +++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 spec/integration/email_confirmation_spec.rb diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 6114b60e8..1eb5bbcad 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -82,5 +82,14 @@ end end end + + trait :unconfirmable 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/integration/email_confirmation_spec.rb b/spec/integration/email_confirmation_spec.rb new file mode 100644 index 000000000..eb6ae44fa --- /dev/null +++ b/spec/integration/email_confirmation_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# For testing the custom email confirmation UX flow +# (See `app/controllers/concerns/email_confirmation_handler.rb`) +# Here, we define "unconfirmable" as a user that is both unconfirmed and has no outstanding confirmation token +RSpec.describe 'Email Confirmation', type: :feature do + scenario 'A user attempts to sign in via the "Sign In" button. However, they are unconfirmable.', :js do + # Setup + user = create(:user, :unconfirmable) + + # Actions + sign_in(user) + user.reload + + # Expectations + expectations_for_unconfirmable_user_after_sign_in_attempt(user) + end + + scenario 'A user attempts to sign in via the "Sign in with institutional or social ID" + button with an email that is not currently linked to any account. The chosen + SSO email matches an existing user account email. However, they are unconfirmable.', :js do + # Setup + Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] + Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect] + user = create(:user, :unconfirmable, email: OmniAuth.config.mock_auth[:openid_connect][:info][:email]) + + # Actions + visit root_path + click_link 'Sign in with institutional or social ID' + user.reload + + # Expectations + expectations_for_unconfirmable_user_after_sign_in_attempt(user) + # An Identifier entry was not created + expect(Identifier.count).to be_zero + end + + private + + def expectations_for_unconfirmable_user_after_sign_in_attempt(user) + # The user remains unconfirmed + expect(user.confirmed?).to be(false) + # A confirmation_token now exists + expect(user.confirmation_token).to be_present + # The user is not signed in + expect(current_path).to eq(root_path) + end +end From d0749fbeed3553fd1c78c16edc4ffa93923e5d50 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 17 Oct 2024 16:49:39 -0600 Subject: [PATCH 16/24] Test custom I18n strings / override default_locale In order to properly test the values of our customised `I18n.t` values (specifically, `I18n.t('devise.failure.unconfirmed')` and `I18n.t('devise.registrations.signed_up_but_unconfirmed')`), we need to set `I18n.default_locale` to match our app's default locale. NOTE: These changes only set `I18n.default_locale = :'en-CA'` for the duration of the email confirmation tests. However, it may be best to apply this update globally throughout the tests. --- spec/integration/email_confirmation_spec.rb | 36 +++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/spec/integration/email_confirmation_spec.rb b/spec/integration/email_confirmation_spec.rb index eb6ae44fa..18507efb9 100644 --- a/spec/integration/email_confirmation_spec.rb +++ b/spec/integration/email_confirmation_spec.rb @@ -6,6 +6,15 @@ # (See `app/controllers/concerns/email_confirmation_handler.rb`) # Here, we define "unconfirmable" as a user that is both unconfirmed and has no outstanding confirmation token RSpec.describe 'Email Confirmation', type: :feature do + before(:each) do + @default_locale = I18n.default_locale + I18n.default_locale = :'en-CA' + end + + after(:each) do + I18n.default_locale = @default_locale + end + scenario 'A user attempts to sign in via the "Sign In" button. However, they are unconfirmable.', :js do # Setup user = create(:user, :unconfirmable) @@ -16,6 +25,12 @@ # Expectations expectations_for_unconfirmable_user_after_sign_in_attempt(user) + + # Actions + sign_in(user) + + # Expectations + expect_request_new_confirmation_link_message end scenario 'A user attempts to sign in via the "Sign in with institutional or social ID" @@ -35,6 +50,12 @@ expectations_for_unconfirmable_user_after_sign_in_attempt(user) # An Identifier entry was not created expect(Identifier.count).to be_zero + + # Actions + click_link 'Sign in with institutional or social ID' + + # Expectations + expect_request_new_confirmation_link_message end private @@ -46,5 +67,20 @@ def expectations_for_unconfirmable_user_after_sign_in_attempt(user) expect(user.confirmation_token).to be_present # The user is not signed in expect(current_path).to eq(root_path) + # The correct flash message was rendered + expect_confirmation_link_has_been_sent_message + end + + def expect_confirmation_link_has_been_sent_message + msg = 'A message with a confirmation link has been sent to your email address. ' \ + 'Please open the link to activate your account. ' \ + 'If you do not receive the confirmation email, please check your spam filter.' + expect(page.text).to have_text(msg) + end + + def expect_request_new_confirmation_link_message + msg = 'You have to confirm your account before continuing. ' \ + '(Click to request a new confirmation email)' + expect(page.text).to have_text(msg) end end From 89f32d02782430600b52a55aeadc727b300fcf45 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 17 Oct 2024 17:32:08 -0600 Subject: [PATCH 17/24] Refactor EmailConfirmationHandler - Rename `def missing_confirmation_instructions_handled?(user)` to `def confirmation_instructions_missing_and_handled?(user)` and add comments to where it is called for easy reference - Move `def confirmed_or_has_confirmation_token?` from User model to `EmailConfirmationHandler`. This check is only accessed by the concern, and it seems easiest to make it a private method and encapsulate the logic there. --- app/controllers/concerns/email_confirmation_handler.rb | 10 +++++++--- app/controllers/sessions_controller.rb | 3 ++- app/controllers/users/omniauth_callbacks_controller.rb | 6 ++++-- app/models/user.rb | 4 ---- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/app/controllers/concerns/email_confirmation_handler.rb b/app/controllers/concerns/email_confirmation_handler.rb index 90bd48938..fd7a79fc4 100644 --- a/app/controllers/concerns/email_confirmation_handler.rb +++ b/app/controllers/concerns/email_confirmation_handler.rb @@ -10,9 +10,9 @@ module EmailConfirmationHandler extend ActiveSupport::Concern - # confirmation instructions are "missing" if the user is both unconfirmed AND has no outstanding confirmation_token - def missing_confirmation_instructions_handled?(user) - return false if user.confirmed_or_has_confirmation_token? + 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 @@ -20,6 +20,10 @@ def missing_confirmation_instructions_handled?(user) 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') diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index df9a8b7ce..76af94c88 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -15,7 +15,8 @@ def create existing_user = User.find_by(email: params[:user][:email]) unless existing_user.nil? - return if missing_confirmation_instructions_handled?(existing_user) + # See app/controllers/concerns/email_confirmation_handler.rb + return if confirmation_instructions_missing_and_handled?(existing_user) # Until ORCID login is supported unless session['devise.shibboleth_data'].nil? diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 9e1e3f219..08ce9e4cc 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -131,7 +131,8 @@ def failure def handle_openid_connect_for_signed_out_user(user, auth, identifier_scheme) # user.nil? is true if the chosen CILogon email is not currently linked to an existing user account user = handle_new_sso_email_for_signed_out_user(auth, identifier_scheme) if user.nil? - return if missing_confirmation_instructions_handled?(user) + # See app/controllers/concerns/email_confirmation_handler.rb + return if confirmation_instructions_missing_and_handled?(user) sign_in_and_redirect user, event: :authentication end @@ -142,7 +143,8 @@ def handle_openid_connect_for_signed_out_user(user, auth, identifier_scheme) def handle_new_sso_email_for_signed_out_user(auth, identifier_scheme) # Find or create the user with user.email == email selected via SSO user = User.find_or_create_from_provider_data(auth) - if user.confirmed? # Only link the SSO email if user.email is confirmed + if user.confirmed? + # Only link the SSO email if user.email is confirmed user.identifiers << Identifier.create(identifier_scheme: identifier_scheme, value: auth.uid, attrs: auth, diff --git a/app/models/user.rb b/app/models/user.rb index 89aa3dcfd..dfe51abaf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -410,10 +410,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 da9fc60fd828c83d06b59d8fdd9ef9a8f40996d6 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 22 Oct 2024 15:47:33 -0600 Subject: [PATCH 18/24] Update `Time.now` to `Time.now.utc` --- app/models/user.rb | 4 ++-- lib/tasks/dmp_assistant_upgrade.rake | 6 +++--- spec/factories/users.rb | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index dfe51abaf..63f62ab95 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -199,8 +199,8 @@ def self.find_or_create_from_provider_data(provider_data) accept_terms: true, password: Devise.friendly_token[0, 20], # provider_data.info.email comes from CILogon sign-in, which requires email confirmation - # It follows that we can set `confirmed_at: Time.now` and bypass Devise's email confirmation step - confirmed_at: Time.now + # It follows that we can set `confirmed_at: Time.now.utc` and bypass Devise's email confirmation step + confirmed_at: Time.now.utc ) user end diff --git a/lib/tasks/dmp_assistant_upgrade.rake b/lib/tasks/dmp_assistant_upgrade.rake index 22d6650f8..67420c44f 100644 --- a/lib/tasks/dmp_assistant_upgrade.rake +++ b/lib/tasks/dmp_assistant_upgrade.rake @@ -25,7 +25,7 @@ namespace :dmp_assistant_upgrade do 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.now`` for superusers)' + p '(i.e. Setting `confirmed_at = Time.now.utc` for superusers)' p '------------------------------------------------------------------------' confirm_superusers end @@ -39,9 +39,9 @@ namespace :dmp_assistant_upgrade do p ":confirmable columns updated to nil for #{count} users" end - # Sets `confirmed_at` to `Time.now` for all superusers + # Sets `confirmed_at` to `Time.now.utc` for all superusers def confirm_superusers - confirmed_at = Time.now + confirmed_at = Time.now.utc count = User.joins(:perms).where(perms: { id: super_admin_perm_ids }) .distinct .update_all(confirmed_at: confirmed_at) diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 1eb5bbcad..ee461564b 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -60,7 +60,7 @@ email { Faker::Internet.unique.email } password { 'password' } accept_terms { true } - confirmed_at { Time.now } + confirmed_at { Time.now.utc } trait :org_admin do after(:create) do |user, _evaluator| From c1b241106d9be778dab725bc90edfd91280c04f3 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Thu, 24 Oct 2024 10:47:14 -0600 Subject: [PATCH 19/24] Update CHANGELOG.md (and fix comment) --- CHANGELOG.md | 2 ++ lib/tasks/dmp_assistant_upgrade.rake | 3 +-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d7dc524f..c480b67cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Changed + - Email Confirmation Changes [#923](https://github.com/portagenetwork/roadmap/pull/923) + - Disable Updating of User Emails [#917](https://github.com/portagenetwork/roadmap/pull/917) ### Fixed diff --git a/lib/tasks/dmp_assistant_upgrade.rake b/lib/tasks/dmp_assistant_upgrade.rake index 67420c44f..150364010 100644 --- a/lib/tasks/dmp_assistant_upgrade.rake +++ b/lib/tasks/dmp_assistant_upgrade.rake @@ -32,8 +32,7 @@ namespace :dmp_assistant_upgrade do # Setting `confirmed_at` to nil will require users to confirm their email addresses when using :confirmable # Setting `confirmation_token` to nil will improve the email confirmation-related UX flow for existing users - # (For more information regarding this improved UX flow, refer to the following: - # `def handle_confirmation_instructions(user)` in `app/controllers/sessions_controller.rb`) + # For more info regarding this improved UX flow, see app/controllers/concerns/email_confirmation_handler.rb 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" From 7ae2c3bc4b2d5485838432be973ac0cee7584b7f Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 1 Nov 2024 16:32:31 -0600 Subject: [PATCH 20/24] Add rake task for openid_connect / CILogon cleanup Intended to resolve this issue: https://github.com/portagenetwork/roadmap/issues/912 The following PR was used as a reference: https://github.com/portagenetwork/roadmap/pull/915 Co-Authored-By: Omar Rodriguez Arenas <1537909+lagoan@users.noreply.github.com> --- lib/tasks/dmp_assistant_upgrade.rake | 43 +++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/lib/tasks/dmp_assistant_upgrade.rake b/lib/tasks/dmp_assistant_upgrade.rake index 150364010..0231f1e4b 100644 --- a/lib/tasks/dmp_assistant_upgrade.rake +++ b/lib/tasks/dmp_assistant_upgrade.rake @@ -12,7 +12,12 @@ namespace :dmp_assistant_upgrade do p '------------------------------------------------------------------------' handle_email_confirmations_for_existing_users p 'Task completed: Handle email confirmations for existing users' - p 'All tasks completed successfully' + p 'Beginning task: Update `openid_connect` IdentifierScheme and its identifers' + p '------------------------------------------------------------------------' + if openid_scheme_and_its_identifiers_updated? + p 'Task completed: Update `openid_connect` IdentifierScheme and its identifers' + p 'All tasks completed successfully' + end end # rubocop:enable Naming/VariableNumber @@ -53,4 +58,40 @@ namespace :dmp_assistant_upgrade do def super_admin_perm_ids [Perm.add_orgs.id, Perm.grant_api.id, Perm.change_affiliation.id] end + + # Returns a boolean indicating whether the task was executed successfully + def openid_scheme_and_its_identifiers_updated? + identifier_scheme = IdentifierScheme.includes(:identifiers) + .find_by!(name: 'openid_connect') + old_prefix = identifier_scheme.identifier_prefix + p "Updating identifier_prefix to '' for openid_connect IdentifierScheme" + p '------------------------------------------------------------------------' + begin + ensure_correct_identifier_prefix(old_prefix) + rescue StandardError => e + p "Error updating IdentifierScheme: #{e.message}" + return false + end + + # Update identifier_prefix to '' for openid_connect + identifier_scheme.update!(identifier_prefix: '') + p "identifier_prefix updated from #{old_prefix} to '' for #{identifier_scheme.name} IdentifierScheme" + p "Updating prefixed value for identifiers with multiple occurences of 'cilogon'" + p '------------------------------------------------------------------------' + # Find all identifiers having multiple occurences of 'cilogon' in their value + identifiers = identifier_scheme.identifiers.where('value ~* ?', "#{old_prefix}.+cilogon.+") + count = identifiers.count + p "(Found #{count} such identifiers)" + # identifier_scheme.identifier_prefix was used to prefix identifier.value + # Update the affected identifier values from `old_prefix` to '' + identifiers.each { |identifier| identifier.update!(value: identifier.value.delete_prefix(old_prefix)) } + p "Updated prefixed value from #{old_prefix} to '' for #{count} identifiers" + true + end + + def ensure_correct_identifier_prefix(prefix) + expected_prefix = 'http://cilogon.org/serverE/users/' + error_msg = "Unexpected identifier_prefix! Expected #{expected_prefix} but got #{prefix}." + raise error_msg unless prefix == expected_prefix + end end From 7f158a7acac49f8c5ec2e10aacf30ad82082154d Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 4 Nov 2024 14:27:08 -0700 Subject: [PATCH 21/24] Make Rubocop happy --- lib/tasks/dmp_assistant_upgrade.rake | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/tasks/dmp_assistant_upgrade.rake b/lib/tasks/dmp_assistant_upgrade.rake index 0231f1e4b..2447754cb 100644 --- a/lib/tasks/dmp_assistant_upgrade.rake +++ b/lib/tasks/dmp_assistant_upgrade.rake @@ -78,14 +78,7 @@ namespace :dmp_assistant_upgrade do p "identifier_prefix updated from #{old_prefix} to '' for #{identifier_scheme.name} IdentifierScheme" p "Updating prefixed value for identifiers with multiple occurences of 'cilogon'" p '------------------------------------------------------------------------' - # Find all identifiers having multiple occurences of 'cilogon' in their value - identifiers = identifier_scheme.identifiers.where('value ~* ?', "#{old_prefix}.+cilogon.+") - count = identifiers.count - p "(Found #{count} such identifiers)" - # identifier_scheme.identifier_prefix was used to prefix identifier.value - # Update the affected identifier values from `old_prefix` to '' - identifiers.each { |identifier| identifier.update!(value: identifier.value.delete_prefix(old_prefix)) } - p "Updated prefixed value from #{old_prefix} to '' for #{count} identifiers" + find_and_update_identifiers(identifier_scheme, old_prefix) true end @@ -94,4 +87,16 @@ namespace :dmp_assistant_upgrade do error_msg = "Unexpected identifier_prefix! Expected #{expected_prefix} but got #{prefix}." raise error_msg unless prefix == expected_prefix end + + def find_and_update_identifiers(identifier_scheme, old_prefix) + # `identifier_scheme.identifiers` == openid_connect-related identifiers + # Get identifiers where `.value` has both the old_prefix and multiple occurences of 'cilogon' + identifiers = identifier_scheme.identifiers.where('value ~* ?', "#{old_prefix}.+cilogon.+") + count = identifiers.count + p "(Found #{count} such identifiers)" + # identifier_scheme.identifier_prefix was initially used to prefix identifier.value + # Update by removing old_prefix from identifier.value + identifiers.each { |identifier| identifier.update!(value: identifier.value.delete_prefix(old_prefix)) } + p "Updated prefixed value from #{old_prefix} to '' for #{count} identifiers" + end end From 09e731e4ebf1c8d426ad9981871531c8d351d26d Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 4 Nov 2024 15:44:18 -0700 Subject: [PATCH 22/24] Add finalised email confirmation translations - Updated via `bundle exec rake translation:sync` with prod key. - These changes finalise some of the initial changes we made for testing in commit 6c41bd7d2bb44a4e632330380691fe82c0fc020d --- config/locales/translation.en-CA.yml | 5 ++--- config/locales/translation.fr-CA.yml | 7 +++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/config/locales/translation.en-CA.yml b/config/locales/translation.en-CA.yml index d143551c8..3c7709f6d 100644 --- a/config/locales/translation.en-CA.yml +++ b/config/locales/translation.en-CA.yml @@ -229,9 +229,8 @@ en-CA: 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. - (Click to request a new confirmation email) + unconfirmed: You need 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 9fdd263ae..cde86b152 100644 --- a/config/locales/translation.fr-CA.yml +++ b/config/locales/translation.fr-CA.yml @@ -241,14 +241,13 @@ fr-CA: not_found_in_database: Courriel 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 compte pour continuer. - (Cliquez pour demander un nouvel e-mail de confirmation) + 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: confirmation_instructions: - subject: Valider votre compte Assistant PGD + subject: Validez votre compte Assistant PGD reset_password_instructions: subject: Directives pour réinitialiser le mot de passe unlock_instructions: From c6134ae81a88a57d9e5a56e99b8e38eb63983dd6 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 6 Nov 2024 09:15:52 -0700 Subject: [PATCH 23/24] Update test to match updated translation - The corresponding translation was commited via the following commit: 09e731e4ebf1c8d426ad9981871531c8d351d26d --- spec/integration/email_confirmation_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/integration/email_confirmation_spec.rb b/spec/integration/email_confirmation_spec.rb index 18507efb9..f1b138466 100644 --- a/spec/integration/email_confirmation_spec.rb +++ b/spec/integration/email_confirmation_spec.rb @@ -79,7 +79,7 @@ def expect_confirmation_link_has_been_sent_message end def expect_request_new_confirmation_link_message - msg = 'You have to confirm your account before continuing. ' \ + msg = 'You need to confirm your account before continuing. ' \ '(Click to request a new confirmation email)' expect(page.text).to have_text(msg) end From 0a2b126c4a9901570129d6685c5897d3a4f14319 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 6 Nov 2024 11:38:40 -0700 Subject: [PATCH 24/24] Update CHANGELOG.md --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c480b67cd..be4139559 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + + - Add rake task for openid_connect / CILogon cleanup [#944](https://github.com/portagenetwork/roadmap/pull/944) + ### Changed - Email Confirmation Changes [#923](https://github.com/portagenetwork/roadmap/pull/923)