Email Confirmation Changes #923
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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, #920 is currently preventing that.
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
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.
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.
This commit re-allows existing users to link via SSO while signed out. This functionality was previously removed via commit e112202. 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.
`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 965c336 ("Re-add SSO linking capability for signed-out users") updated the SSO behaviour in a manner that this test is no longer relevant.
- These changes replace the changes made to `lib/tasks/dmp_assistant_upgrade.rake` in commits 6087aee and 465d671. 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).
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`.
lagoan
requested changes
Oct 9, 2024
config/locales/translation.en-CA.yml
Outdated
Comment on lines
232
to
233
| You have to confirm your account before continuing. | ||
| <a href="/users/confirmation/new" class="a-orange">(Click to request a new confirmation email)</a> |
There was a problem hiding this comment.
Right now we are sending the email verification message sent when the user tries to log in and is not verified. What do you think about letting the user know about this sent email in this message instead of asking the user to request the email confirmation again ?
There was a problem hiding this comment.
Good question. This flash message is actually only rendered when the user already has an outstanding confirmation. An additional confirmation email will not be auto-sent in this case and they will have to manually request a new one.
If a user is unconfirmed and has no outstanding confirmation token, then a different message will render after they attempt to sign in (specifically, devise.registrations.signed_up_but_unconfirmed).
3.1.4 :001 > I18n.t('devise.registrations.signed_up_but_unconfirmed')
=> "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."
# app/models/user.rb
def confirmation_instructions_handled?
confirmed? || confirmation_token.present?
end# app/controllers/sessions_controller.rb
unless existing_user.confirmation_instructions_handled?
handle_confirmation_instructions(existing_user)
return
end
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
endThis commit takes the idea in commit 6087aee 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.
Some refactoring was performed in `def openid_connect` to avoid adding `Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity` to `rubocop:disable`.
- 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`
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.
- 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.
lagoan
requested changes
Oct 22, 2024
aaronskiba
force-pushed
the
aaron/add-email-confirmation
branch
from
bd69f88
to
c1b2411
Compare
Intended to resolve this issue: #912 The following PR was used as a reference: #915 Co-Authored-By: Omar Rodriguez Arenas <[email protected]>
Add rake task for `openid_connect` / CILogon cleanup
This was referenced Nov 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes *
Changes proposed in this PR:
:confirmablemodule