Email Confirmation Changes

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)

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

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 = {

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.<br>' \
-                              '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

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,18 +185,21 @@ 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'),
       surname: provider_data.info&.last_name.presence || _('Last name'),
       # 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

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

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. <a href="/users/confirmation/new"
+        class="a-orange">(Click to request a new confirmation email)</a>
       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:

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. <a href="/users/confirmation/new"
+        class="a-orange">(cliquez pour demander un nouveau courriel de confirmation)</a>
       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:

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

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

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