diff --git a/CHANGELOG.md b/CHANGELOG.md index c06babae56..fe1f3f3280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ - Fix failing eslint workflow / upgrade `actions/checkout` & `actions/setup-node` to v3 [#3503](https://github.com/DMPRoadmap/roadmap/pull/3503) - Fix rendering of `confirm_merge` partial [#3515](https://github.com/DMPRoadmap/roadmap/pull/3515) - Remove Auto-Generated TinyMCE Skins and Add `public/tinymce/skins/` to `.gitignore` [#3466](https://github.com/DMPRoadmap/roadmap/pull/3466) +- Re-implement email confirmation [#3507](https://github.com/DMPRoadmap/roadmap/pull/3507) +- Update `spec/support/faker.rb` to use `I18n.default_locale` [#3507](https://github.com/DMPRoadmap/roadmap/pull/3507) ## v5.0.0 diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 04e15f1de2..4a91e68b46 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -2,6 +2,8 @@ # Controller that handles user login and logout class SessionsController < Devise::SessionsController + include EmailConfirmationHandler + def new redirect_to(root_path) end @@ -11,18 +13,13 @@ def new # rubocop:disable Metrics/AbcSize def create existing_user = User.find_by(email: params[:user][:email]) - unless existing_user.nil? + if existing_user.present? + + # (see app/models/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 = { - identifier_scheme: IdentifierScheme.find_by(name: 'shibboleth'), - value: session['devise.shibboleth_data']['uid'], - identifiable: existing_user, - attrs: session['devise.shibboleth_data'] - } - @ui = Identifier.new(args) - end + @ui = create_shibboleth_identifier(existing_user) unless session['devise.shibboleth_data'].nil? session[:locale] = existing_user.locale unless existing_user.locale.nil? # Method defined at controllers/application_controller.rb set_locale @@ -30,9 +27,8 @@ def create super do if !@ui.nil? && @ui.save - # rubocop:disable Layout/LineLength - flash[:notice] = _('Your account has been successfully linked to your institutional credentials. You will now be able to sign in with them.') - # rubocop:enable Layout/LineLength + flash[:notice] = _('Your account has been successfully linked to your institutional credentials. ' \ + 'You will now be able to sign in with them.') end end end @@ -45,3 +41,15 @@ def destroy set_locale end end + +private + +def create_shibboleth_identifier(user) + args = { + identifier_scheme: IdentifierScheme.find_by(name: 'shibboleth'), + value: session['devise.shibboleth_data']['uid'], + identifiable: user, + attrs: session['devise.shibboleth_data'] + } + Identifier.new(args) +end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 01ec5491e7..4c0dcf9ebf 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -3,6 +3,7 @@ module Users # Controller that handles callbacks from OmniAuth integrations (e.g. Shibboleth and ORCID) class OmniauthCallbacksController < Devise::OmniauthCallbacksController + include EmailConfirmationHandler ## # Dynamically build a handler for each omniauth provider # ------------------------------------------------------------- @@ -21,8 +22,6 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # # scheme - The IdentifierScheme for the provider # - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength - # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def handle_omniauth(scheme) user = if request.env['omniauth.auth'].nil? User.from_omniauth(request.env) @@ -32,55 +31,68 @@ def handle_omniauth(scheme) # If the user isn't logged in if current_user.nil? - # If the uid didn't have a match in the system send them to register - if user.nil? - session["devise.#{scheme.name.downcase}_data"] = request.env['omniauth.auth'] - redirect_to new_user_registration_url - - # Otherwise sign them in - elsif scheme.name == 'shibboleth' - # Until ORCID becomes supported as a login method - set_flash_message(:notice, :success, kind: scheme.description) if is_navigational_format? - sign_in_and_redirect user, event: :authentication - else - flash[:notice] = _('Successfully signed in') - redirect_to new_user_registration_url - end - + handle_omniauth_for_signed_out_user(user, scheme) # The user is already logged in and just registering the uid with us else - # If the user could not be found by that uid then attach it to their record - if user.nil? - if Identifier.create(identifier_scheme: scheme, - value: request.env['omniauth.auth'].uid, - attrs: request.env['omniauth.auth'], - identifiable: current_user) - flash[:notice] = - format(_('Your account has been successfully linked to %{scheme}.'), - scheme: scheme.description) + handle_omniauth_for_signed_in_user(user, scheme) + end + end + + def failure + redirect_to root_path + end - else - flash[:alert] = format(_('Unable to link your account to %{scheme}.'), - scheme: scheme.description) - end + private - elsif user.id != current_user.id - # If a user was found but does NOT match the current user then the identifier has - # already been attached to another account (likely the user has 2 accounts) - # rubocop:disable Layout/LineLength - flash[:alert] = _("The current #{scheme.description} iD has been already linked to a user with email #{identifier.user.email}") - # rubocop:enable Layout/LineLength + # rubocop:disable Metrics/AbcSize + def handle_omniauth_for_signed_in_user(user, scheme) + # If the user could not be found by that uid then attach it to their record + if user.nil? + if Identifier.create(identifier_scheme: scheme, + value: request.env['omniauth.auth'].uid, + attrs: request.env['omniauth.auth'], + identifiable: current_user) + flash[:notice] = format(_('Your account has been successfully linked to %{scheme}.'), + scheme: scheme.description) + + else + flash[:alert] = format(_('Unable to link your account to %{scheme}.'), + scheme: scheme.description) end - # Redirect to the User Profile page - redirect_to edit_user_registration_path + elsif user.id != current_user.id + # If a user was found but does NOT match the current user then the identifier has + # already been attached to another account (likely the user has 2 accounts) + flash[:alert] = _("The current #{scheme.description} iD has been already linked " \ + "to a user with email #{identifier.user.email}") end + + # Redirect to the User Profile page + redirect_to edit_user_registration_path end - # rubocop:enable Metrics/AbcSize, Metrics/MethodLength - # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + # rubocop:enable Metrics/AbcSize - def failure - redirect_to root_path + # rubocop:disable Metrics/AbcSize + def handle_omniauth_for_signed_out_user(user, scheme) + # If the uid didn't have a match in the system send them to register + if user.nil? + session["devise.#{scheme.name.downcase}_data"] = request.env['omniauth.auth'] + redirect_to new_user_registration_url + + # Otherwise sign them in + elsif scheme.name == 'shibboleth' + # Until ORCID becomes supported as a login method + + # (see app/models/concerns/email_confirmation_handler.rb) + return if confirmation_instructions_missing_and_handled?(user) + + set_flash_message(:notice, :success, kind: scheme.description) if is_navigational_format? + sign_in_and_redirect user, event: :authentication + else + flash[:notice] = _('Successfully signed in') + redirect_to new_user_registration_url + end end + # rubocop:enable Metrics/AbcSize end end diff --git a/app/models/concerns/email_confirmation_handler.rb b/app/models/concerns/email_confirmation_handler.rb new file mode 100644 index 0000000000..674db79408 --- /dev/null +++ b/app/models/concerns/email_confirmation_handler.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +# Some users in our db are both unconfirmed AND have no outstanding confirmation_token +# This is true for those users due to the following: +# - We haven't always used Devise's :confirmable module (it generates a confirmation_token when a user is created) +# - We have set `confirmed_at` and `confirmation_token` to nil via Rake tasks (lib/tasks/email_confirmation.rake) +# This concern is meant to streamline the confirmation process for those users +module EmailConfirmationHandler + extend ActiveSupport::Concern + + def confirmation_instructions_missing_and_handled?(user) + # A user's "confirmation instructions are missing" if they're both unconfirmed and have no confirmation_token + return false if user_confirmed_or_has_confirmation_token?(user) + + handle_missing_confirmation_instructions(user) + true + end + + private + + def user_confirmed_or_has_confirmation_token?(user) + user.confirmed? || user.confirmation_token.present? + end + + def handle_missing_confirmation_instructions(user) + user.send_confirmation_instructions + redirect_to root_path, notice: I18n.t('devise.registrations.signed_up_but_unconfirmed') + end +end diff --git a/app/models/user.rb b/app/models/user.rb index e3ecccf039..c056ac083c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -65,7 +65,7 @@ class User < ApplicationRecord # :token_authenticatable, :confirmable, # :lockable, :timeoutable and :omniauthable devise :invitable, :database_authenticatable, :registerable, :recoverable, - :rememberable, :trackable, :validatable, :omniauthable, + :rememberable, :trackable, :validatable, :omniauthable, :confirmable, omniauth_providers: %i[shibboleth orcid] # default user language to the default language diff --git a/config/environments/test.rb b/config/environments/test.rb index 122ad051b6..9195462b99 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -45,6 +45,7 @@ # The :test delivery method accumulates sent emails in the # ActionMailer::Base.deliveries array. config.action_mailer.delivery_method = :test + config.action_mailer.default_options = { from: 'noreply@example.org' } # Print deprecation notices to the stderr. config.active_support.deprecation = :stderr diff --git a/config/locales/translation.en-CA.yml b/config/locales/translation.en-CA.yml index 9e5ed6e7c8..784fc06e39 100644 --- a/config/locales/translation.en-CA.yml +++ b/config/locales/translation.en-CA.yml @@ -230,7 +230,8 @@ en-CA: not_found_in_database: Invalid %{authentication_keys} or password. timeout: Your session expired. Please sign in again to continue. unauthenticated: You need to sign in or sign up before continuing. - unconfirmed: You have to confirm your email address before continuing. + unconfirmed: You need to confirm your account before continuing. (Click to request a new confirmation email) invited: mailer: confirmation_instructions: diff --git a/config/locales/translation.en-GB.yml b/config/locales/translation.en-GB.yml index 91014ac51c..711ed6a306 100644 --- a/config/locales/translation.en-GB.yml +++ b/config/locales/translation.en-GB.yml @@ -227,7 +227,8 @@ en-GB: not_found_in_database: Invalid email or password. timeout: Your session expired, please sign in again to continue. unauthenticated: You need to sign in or sign up before continuing. - unconfirmed: You have to confirm your account before continuing. + unconfirmed: You have to confirm your account before continuing. (Click to request a new confirmation email) invited: You have a pending invitation, accept it to finish creating your account. mailer: confirmation_instructions: diff --git a/config/locales/translation.fr-CA.yml b/config/locales/translation.fr-CA.yml index 8f23004765..82067551fe 100644 --- a/config/locales/translation.fr-CA.yml +++ b/config/locales/translation.fr-CA.yml @@ -242,7 +242,8 @@ fr-CA: not_found_in_database: "%{authentication_keys} ou mot de passe incorrect." timeout: Votre session est expirée. Veuillez vous reconnecter pour continuer. unauthenticated: Vous devez vous connecter ou vous inscrire pour continuer. - unconfirmed: Vous devez confirmer votre adresse courriel pour continuer. + unconfirmed: Vous devez confirmer votre compte avant de continuer. (cliquez pour demander un nouveau courriel de confirmation) invited: Vous avez une invitation en attente. Acceptez-la pour terminer la création de votre compte. mailer: diff --git a/lib/tasks/email_confirmation.rake b/lib/tasks/email_confirmation.rake new file mode 100644 index 0000000000..eca4692734 --- /dev/null +++ b/lib/tasks/email_confirmation.rake @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +namespace :email_confirmation do + desc 'Reset confirmation status for all users, excluding superusers' + task clear_all: :environment do + p '------------------------------------------------------------------------' + p 'Beginning task: Unconfirming all users except superusers' + p '------------------------------------------------------------------------' + unconfirm_all_users_except_superusers + p 'Task completed: Unconfirmed all users except superusers' + end + + private + + def unconfirm_all_users_except_superusers + p 'Updating :confirmable columns to nil for all users' + p '(i.e. Setting confirmed_at, confirmation_token, and confirmation_sent_at to nil for all users)' + p '------------------------------------------------------------------------' + set_confirmable_cols_to_nil_for_all_users + p '------------------------------------------------------------------------' + p 'Updating superusers so that they are not required to confirm their email addresses' + p '(i.e. Setting `confirmed_at = Time.current` for superusers)' + p '------------------------------------------------------------------------' + confirm_superusers + end + + def set_confirmable_cols_to_nil_for_all_users + count = User.update_all(confirmed_at: nil, confirmation_token: nil, confirmation_sent_at: nil) + p ":confirmable columns updated to nil for #{count} users" + end + + # Sets `confirmed_at` to `Time.current` for all superusers + def confirm_superusers + confirmed_at = Time.current + count = User.joins(:perms).where(perms: { id: super_admin_perm_ids }) + .distinct + .update_all(confirmed_at: confirmed_at) + p "Updated confirmed_at = #{confirmed_at} for #{count} superuser(s)" + end + + # Returns an array of all perm ids that are considered super admin perms + # (Based off of `def can_super_admin?` in `app/models/user.rb` + # i.e. `can_add_orgs? || can_grant_api_to_orgs? || can_change_org?` ) + def super_admin_perm_ids + [Perm.add_orgs.id, Perm.grant_api.id, Perm.change_affiliation.id] + end +end diff --git a/spec/factories/identifier_schemes.rb b/spec/factories/identifier_schemes.rb index 24fd508511..067cfb04f3 100644 --- a/spec/factories/identifier_schemes.rb +++ b/spec/factories/identifier_schemes.rb @@ -27,6 +27,16 @@ context_count { 1 } end + # Add a trait for Shibboleth + trait :shibboleth do + name { 'shibboleth' } + context { 11 } + description { 'Institutional Sign In (Shibboleth)' } + logo_url { nil } + identifier_prefix { nil } + active { true } + end + after(:create) do |identifier_scheme, evaluator| (0..evaluator.context_count - 1).each do |idx| identifier_scheme.update("#{identifier_scheme.all_context[idx]}": true) diff --git a/spec/factories/users.rb b/spec/factories/users.rb index c91c756604..b87e86d835 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -60,6 +60,7 @@ email { Faker::Internet.unique.email } password { 'password' } accept_terms { true } + confirmed_at { Time.current } trait :org_admin do after(:create) do |user, _evaluator| @@ -81,5 +82,14 @@ end end end + + trait :unconfirmed_and_no_confirmation_token do + confirmed_at { nil } + # When using :confirmable, a confirmation_token is generated at the time of user creation + # Setting it to nil allows us to duplicate the attributes of some existing users + after(:create) do |user| + user.update(confirmation_token: nil) + end + end end end diff --git a/spec/features/email_confirmation_spec.rb b/spec/features/email_confirmation_spec.rb new file mode 100644 index 0000000000..ec1e9de1fc --- /dev/null +++ b/spec/features/email_confirmation_spec.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require 'rails_helper' +SSO_SIGN_IN_BUTTON_TEXT = 'Sign in with your institutional credentials' + +# For testing the custom email confirmation UX flow for unconfirmed users with no outstanding confirmation_token +RSpec.describe 'Email Confirmation', type: :feature do + include OmniAuthHelper + + scenario 'A user attempts to sign in via the "Sign In" button. + However, they are unconfirmed and have no confirmation_token.', :js do + user = create(:user, :unconfirmed_and_no_confirmation_token) + + # Sign-in attempt #1 (user is unconfirmed and has no confirmation_token) + sign_in(user) + expectations_for_unconfirmed_user_with_no_confirmation_token_after_sign_in_attempt(user) + + # Sign-in attempt #2 (user still unconfirmed but has a confirmation_token) + sign_in(user) + expectations_for_unconfirmed_user_with_confirmation_token_after_sign_in_attempt + + # Sign-in attempt #3 (user is now confirmed) + user.confirm + sign_in(user) + # The user is signed in + expect(page).to have_current_path(plans_path) + end + + describe 'Initial setup for shibboleth sign-in' do + before do + # Set up the user and identifier scheme + @user = create(:user, :unconfirmed_and_no_confirmation_token) + scheme = create(:identifier_scheme, :shibboleth) + # Mock OmniAuth authentication hash for Shibboleth via OmniAuthHelper + OmniAuth.config.mock_auth[:shibboleth] = mock_auth_hash(@user, scheme) + # Explicitly define a Users::OmniauthCallbacksController action for the scheme + define_omniauth_callback_for(scheme) + + # Create the identifier for the user + Identifier.create(identifier_scheme: scheme, + value: OmniAuth.config.mock_auth[:shibboleth].uid, + attrs: OmniAuth.config.mock_auth[:shibboleth], + identifiable: @user) + end + + scenario 'A user attempts to sign in via the "Sign in with your institutional credentials" + button. The email is linked to a user account, however the account is + unconfirmed and has no confirmation token.', :js do + visit root_path + # Sign-in attempt #1 (user is unconfirmed and has no confirmation_token) + click_link SSO_SIGN_IN_BUTTON_TEXT + expectations_for_unconfirmed_user_with_no_confirmation_token_after_sign_in_attempt(@user) + + visit root_path + # Sign-in attempt #2 (user still unconfirmed but has a confirmation_token) + click_link SSO_SIGN_IN_BUTTON_TEXT + expectations_for_unconfirmed_user_with_confirmation_token_after_sign_in_attempt + + # Sign-in attempt #3 (user is now confirmed) + @user.confirm + click_link SSO_SIGN_IN_BUTTON_TEXT + # The user is signed in + expect(page).to have_current_path(plans_path) + end + end + + scenario 'A user has a confirmation token but is unconfirmed. + There sign in attempt fails, and a custom flash message + is rendered that can be used to navigate to the confirmation page.', :js do + # Create a user with a confirmation token but make them unconfirmed + user = create(:user, confirmed_at: nil) + # Attempt to sign in the unconfirmed user + sign_in(user) + expect(page).to have_current_path(root_path) + # A flash warning is displayed informing the user that they have to confirm their email + within('#notification-area') do + # Click the link embedded in the flash message + find('a.a-orange').click + end + # The user is redirected to the confirmation page + expect(current_path).to eq(new_user_confirmation_path) + end + + private + + # rubocop:disable Metrics/AbcSize + def expectations_for_unconfirmed_user_with_no_confirmation_token_after_sign_in_attempt(user) + # The user is not signed in + expect(page).to have_current_path(root_path) + # A flash notice is displayed informing the user that a confirmation email has been sent + expect(page).to have_selector('#notification-area', + text: I18n.t('devise.registrations.signed_up_but_unconfirmed')) + # reload the user to check confirmation values + user.reload + # The user remains unconfirmed + expect(user.confirmed?).to be(false) + # A confirmation_token now exists + expect(user.confirmation_token).to be_present + end + # rubocop:enable Metrics/AbcSize + + def expectations_for_unconfirmed_user_with_confirmation_token_after_sign_in_attempt + # The user is not signed in + expect(current_path).to eq(root_path) + within('#notification-area') do + # Get the HTML content of the flash message + html_content = find(:xpath, '.').native.attribute('innerHTML') + # A flash warning is displayed informing the user that they have to confirm their email + expect(html_content).to include(I18n.t('devise.failure.unconfirmed')) + end + end +end diff --git a/spec/features/registrations_spec.rb b/spec/features/registrations_spec.rb index 50ef4d60ab..4f7840d21b 100644 --- a/spec/features/registrations_spec.rb +++ b/spec/features/registrations_spec.rb @@ -26,9 +26,9 @@ click_button 'Create account' # Expectations - expect(current_path).to eql(plans_path) - expect(page).to have_text(user_attributes[:firstname]) - expect(page).to have_text(user_attributes[:surname]) + expect(current_path).to eql(root_path) + expect(page).to have_text(I18n.t('devise.registrations.signed_up_but_unconfirmed')) + expect(User.count).to eq(1) end scenario 'User attempts to create a new acccount with invalid atts', :js do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 98c265b5cf..bc5549e832 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -79,4 +79,9 @@ config.include Devise::Test::ControllerHelpers, type: :controller config.include Devise::Test::ControllerHelpers, type: :view config.include Pundit::Matchers, type: :policy + + # Enable test mode for OmniAuth + OmniAuth.config.test_mode = true + # Set the full host to nil to avoid URL generation issues in tests + OmniAuth.config.full_host = nil end diff --git a/spec/support/faker.rb b/spec/support/faker.rb index afa86a907d..def6431e97 100644 --- a/spec/support/faker.rb +++ b/spec/support/faker.rb @@ -2,14 +2,10 @@ require 'faker' -# Keep this as :en. Faker doesn't have :en-GB -LOCALE = 'en' - RSpec.configure do |config| config.before(:each) do - I18n.locale = LOCALE - Faker::Config.locale = LOCALE - I18n.default_locale = LOCALE + I18n.locale = I18n.default_locale + Faker::Config.locale = I18n.default_locale end config.after(:each) do diff --git a/spec/support/helpers/omniauth_helper.rb b/spec/support/helpers/omniauth_helper.rb new file mode 100644 index 0000000000..8ccd997ccc --- /dev/null +++ b/spec/support/helpers/omniauth_helper.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +# Helper module for mocking OmniAuth authentication in tests +module OmniAuthHelper + # This method sets and returns an OmniAuth::AuthHash + # that simulates the authentication data returned by + # an OmniAuth provider (e.g. Shibboleth). + def mock_auth_hash(user, scheme) + # Ensure Devise correctly maps the :user model for OmniAuth in tests. + Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] + Rails.application.env_config['omniauth.auth'] = + OmniAuth::AuthHash.new({ + provider: scheme.name, + uid: '12345', + info: { + email: user.email, + first_name: user.firstname, + last_name: user.surname + } + }) + Rails.application.env_config['omniauth.auth'] + end + + # Manually define an action in Users::OmniauthCallbacksController. + # These actions are dynamically defined in the controller (based on IdentifierScheme entries). + # Because required db entries may not yet exist when the controller is loaded in the test environment, + # an action can be explicitly defined here for the test to work. + def define_omniauth_callback_for(scheme) + # Only define the action if it passes the .for_authentication validation + return unless IdentifierScheme.for_authentication.exists?(id: scheme.id) + + Users::OmniauthCallbacksController.define_method(scheme.name.downcase) do + handle_omniauth(scheme) + end + end +end