diff --git a/CHANGELOG.md b/CHANGELOG.md index 24f82e1cd3..0ece16c97f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,14 @@ ## [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) + - Disable Updating of User Emails [#917](https://github.com/portagenetwork/roadmap/pull/917) - Apply `translation:sync` to `yaml` Files and Remove Unused `locale/` + `locales/` Files [#937](https://github.com/portagenetwork/roadmap/pull/937) diff --git a/app/controllers/concerns/email_confirmation_handler.rb b/app/controllers/concerns/email_confirmation_handler.rb new file mode 100644 index 0000000000..fd7a79fc46 --- /dev/null +++ b/app/controllers/concerns/email_confirmation_handler.rb @@ -0,0 +1,31 @@ +# 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 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 + 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/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 04e15f1de2..76af94c882 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,6 +15,9 @@ def create existing_user = User.find_by(email: params[:user][:email]) unless existing_user.nil? + # 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? args = { diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 3640b5de1f..08ce9e4cce 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -3,9 +3,10 @@ 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 - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + # This is for the OpenidConnect CILogon + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def openid_connect # First or create auth = request.env['omniauth.auth'] @@ -23,23 +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) - 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 - 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 + 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, @@ -58,7 +43,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')) @@ -140,5 +125,32 @@ def handle_omniauth(scheme) def failure redirect_to root_path end + + 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? + # See app/controllers/concerns/email_confirmation_handler.rb + return if confirmation_instructions_missing_and_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 + 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/app/models/user.rb b/app/models/user.rb index 8d3f59ca47..5be35953c6 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] ## @@ -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.downcase) - return unless user.new_record? + return user unless user.new_record? user.update!( firstname: provider_data.info&.first_name.presence || _('First name'), @@ -196,7 +196,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.utc` and bypass Devise's email confirmation step + confirmed_at: Time.now.utc ) user end diff --git a/config/environments/test.rb b/config/environments/test.rb index 0810479296..f6bb16786c 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/config/locales/translation.en-CA.yml b/config/locales/translation.en-CA.yml index 644d72056e..3c7709f6da 100644 --- a/config/locales/translation.en-CA.yml +++ b/config/locales/translation.en-CA.yml @@ -229,11 +229,12 @@ 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 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: - subject: Confirm your DMPonline account + subject: Confirm your DMP Assistant account reset_password_instructions: subject: Reset password instructions unlock_instructions: diff --git a/config/locales/translation.fr-CA.yml b/config/locales/translation.fr-CA.yml index 5077ef1694..cde86b1524 100644 --- a/config/locales/translation.fr-CA.yml +++ b/config/locales/translation.fr-CA.yml @@ -241,12 +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. + 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: diff --git a/lib/tasks/dmp_assistant_upgrade.rake b/lib/tasks/dmp_assistant_upgrade.rake new file mode 100644 index 0000000000..2447754cb2 --- /dev/null +++ b/lib/tasks/dmp_assistant_upgrade.rake @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +# 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 + p '------------------------------------------------------------------------' + p 'Executing upgrade tasks for DMP Assistant 4.1.1+portage-4.2.3' + p 'Beginning task: Handle email confirmations for existing users' + p '------------------------------------------------------------------------' + handle_email_confirmations_for_existing_users + p 'Task completed: Handle email confirmations for existing users' + 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 + + private + + 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 '------------------------------------------------------------------------' + 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.utc` for superusers)' + p '------------------------------------------------------------------------' + confirm_superusers + end + + # 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 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" + end + + # Sets `confirmed_at` to `Time.now.utc` for all superusers + def confirm_superusers + confirmed_at = Time.now.utc + 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 + + # 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_and_update_identifiers(identifier_scheme, old_prefix) + 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 + + 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 diff --git a/spec/factories/users.rb b/spec/factories/users.rb index c91c756604..ee461564bb 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.utc } trait :org_admin do after(:create) do |user, _evaluator| @@ -81,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/features/registrations_spec.rb b/spec/features/registrations_spec.rb index 50ef4d60ab..cfb40eea8c 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/email_confirmation_spec.rb b/spec/integration/email_confirmation_spec.rb new file mode 100644 index 0000000000..f1b1384660 --- /dev/null +++ b/spec/integration/email_confirmation_spec.rb @@ -0,0 +1,86 @@ +# 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 + 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) + + # Actions + sign_in(user) + user.reload + + # 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" + 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 + + # Actions + click_link 'Sign in with institutional or social ID' + + # Expectations + expect_request_new_confirmation_link_message + 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) + # 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 need to confirm your account before continuing. ' \ + '(Click to request a new confirmation email)' + expect(page.text).to have_text(msg) + end +end diff --git a/spec/integration/openid_connect_sso_spec.rb b/spec/integration/openid_connect_sso_spec.rb index abf0d01b68..317b040ae0 100644 --- a/spec/integration/openid_connect_sso_spec.rb +++ b/spec/integration/openid_connect_sso_spec.rb @@ -35,16 +35,19 @@ 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) + 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' - 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) + + # 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 it 'links account from external credentails', :js do