Re-Implement Email Confirmation #3507
Merged
Conversation
This file contains hidden or 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
aaronskiba
commented
Apr 14, 2025
aaronskiba
commented
Apr 14, 2025
aaronskiba
commented
Apr 14, 2025
aaronskiba
force-pushed
the
upstream/aaron/add-email-confirmation
branch
2 times, most recently
from
cf70685 to
bcc8d1f
Compare
aaronskiba
force-pushed
the
upstream/aaron/add-email-confirmation
branch
4 times, most recently
from
85d724e to
7d27fd6
Compare
Contributor
|
Just need to check code with Shibb login. Should be doing that on Thursday 5th June. CHANGELOG.md needs updating. |
By default, Devise's `:confirmable` module generates a `confirmation_token` and sends confirmation instructions when a new user is created. This commit enhances that behaviour to streamline the email confirmation process for **existing** users. A new rake task (`lib/tasks/email_confirmation.rake#clear_all`) resets the following confirmation-related fields—`confirmed_at`, `confirmation_token`, and `confirmation_sent_at`—to `nil` for all non-superusers. After this reset, these users will be unable to sign in until they confirm their email. To avoid requiring manual re-sending of confirmation instructions, we introduce a new check: `User#confirmed_or_has_confirmation_token?`, which returns `false` if a user is unconfirmed *and* has no outstanding confirmation_token. In the `SessionsController#create` method, if a signing-in user fails the `confirmed_or_has_confirmation_token?` check, we invoke `handle_missing_confirmation_instructions(user)`. This generates a new confirmation_token and sends email instructions. On subsequent sign-in attempts, the check will return `true`, preventing redundant emails. This approach ensures that email confirmations are triggered automatically and only once per affected user, minimising friction while preserving security.
Moved the creation of the Shibboleth identifier to a helper method
- Replaced double-negative `unless existing_user.nil?` with `if existing_user.present?` - Used string concatenation with backslash to allow removal of disabled rubocop offence.
`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.current }` 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
- Customises Devise's default value for the key `devise.failure.unconfirmed`. The value adds a link to the email confirmation page. - Updated via `bundle exec rake translation:sync`
Commit 6af91b6 streamlined the email confirmation process for unconfirmed users with no confirmation_token. Specifically, it targeted users attempting to sign in via the app's standard sign-in form. This change applies the same streamlining to users attempting to sign in via SSO/Shibboleth. Specifically, when a user attempts to sign in this way, the `.confirmed_or_has_confirmation_token?` is performed on them. If it returns false, then the confirmation_token is generated for the user and they are auto-sent a confirmation instructions email. (Refer to commit 6af91b6 for more regarding this streamlined process.)
This change addresses the duplicate code shared between `app/controllers/sessions_controller.rb` and `app/controllers/users/omniauth_callbacks_controller.rb`. It does so by creating `app/models/concerns/email_confirmation_handler.rb` which allows us to better adhere to DRY principles. - Additionally, the `user.confirmed? || user.confirmation_token.present?` check has been moved from the User model to the concern. It made sense to have this check as a User method. However, the method itself is simply an or check of two other User methods, and only the `EmailConfirmationHandler` module is currently needing it.
This change maintains the functionality of `def handle_omniauth(scheme)` while making the code more maintainable and addressing its disabled rubocop offences. The private methods `def handle_omniauth_for_signed_in_user(user, scheme)` and `def handle_omniauth_for_signed_out_user(user, scheme)` have been created to handle the omniauth logic for signed in vs signed out users respectively. This change is simply a refactor and maintains the pre-existing code logic.
Commit fdb54e03bb71ffaad17bea562e0ca8d87b081059 resolved the rubocop offences within `def handle_omniauth(scheme)`. However, the created helper methods are now triggering rubocop offences as well. Although, I'm hesitant to simply disable these offences, I also don't want to go overboard with refactoring this file.
- Created `spec/support/helpers/omniauth_helper.rb` for simulating authentication with providers like Shibboleth - Added OmniAuth test config settings in `spec/rails_helper.rb`.
- Tests the custom flow for both system and SSO sign in - Tests the custom flash messages including the embedded link to the confirmation email request page
This change attempts to resolve the following error:
```
1) Email Confirmation 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.
Failure/Error: raise ActionNotFound.new("The action '#{action}' could not be found for #{self.class.name}", self, action)
AbstractController::ActionNotFound:
The action 'shibboleth' could not be found for Users::OmniauthCallbacksController
```
The Users::OmniauthCallbacksController#shibboleth action is defined dynamically within the controller. Everything is working locally with commit bcc8d1f, but the test is failing when executed as a GitHub Action.
This change explicitly defines the shibboleth-related Users::OmniauthCallbacksController action
The previous configuration had a hardcoded 'en' locale with a comment stating "Keep this as :en. Faker doesn't have :en-GB". However, testing confirms that Faker now works properly with :"en-GB". This change should allow for more accurate testing by using the application's specified locale.
aaronskiba
force-pushed
the
upstream/aaron/add-email-confirmation
branch
from
e3be5dc to
cf22918
Compare
aaronskiba
force-pushed
the
upstream/aaron/add-email-confirmation
branch
from
cf22918 to
b7d16c6
Compare
johnpinto1
approved these changes
Jun 5, 2025
Contributor
johnpinto1
left a comment
johnpinto1
left a comment
There was a problem hiding this comment.
I am unable to check Shibb. But if @aaronskiba has it working, then I suggest merging PR.
- The previous comments mistakenly stated that the user had no confirmation token.
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.
Changes proposed in this PR:
Added
:confirmablemodule to User model, which implements email confirmation via Devise.Added the rake task,
email_confirmation:clear_all(seelib/tasks/email_confirmation.rake)confirmed_at,confirmation_token, andconfirmation_sent_at) to nil for all users. It then proceeds to confirm all superusers within the app.Streamline the email confirmation process for existing users
:confirmablemodule generates aconfirmation_tokenand auto-sends confirmation instructions when a new user is created.:confirmablenow, existing users can't receive these autosent instructions. However, this PR implements autosent confirmation instructions in the following manner:return if confirmation_instructions_missing_and_handled?(user)line of code is executed. This method belongs to theEmailConfirmationHandlerconcern, and works as follows:i) returns
falseif the user is either already confirmed or has an outstanding confirmation_tokenii) Else (the user is unconfirmed AND has no outstanding confirmation_token). Generate the confirmation token and auto-send the confirmation instructions email. (Note: on subsequent sign-in attempts, attempts, the method will return
false, preventing redundant emails.)Customise
devise.failure.unconfirmedvalue in variousconfig/locales/*.ymlfiles. The customised value includes an embedded link to/users/confirmation/new. The following is a screenshot of the customised value for:"en-CA":Updated existing tests
confirmed_at { Time.current }to User factoryconfig.action_mailer.default_options = { from: 'noreply@example.org' }to enable email confirmation testsAdd new tests (spec/features/email_confirmation_spec.rb)
spec/support/helpers/omniauth_helper.rband additions tospec/rails_helper.rb)/users/confirmation/newembedded in the customiseddevise.failure.unconfirmedflash message.Refactoring
SessionsController#createandUsers::OmniauthCallbacksController#handle_omniauthUpdate
spec/support/faker.rbAssignments to UseI18n.default_locale#3511spec/support/faker.rbto replace'en'withI18n.default_localefor assigning locales. This change should allow for more accurate testing by using the application's specified locale."Keep this as :en. Faker doesn't have :en-GB". However,I18n.default_localeevaluates to:"en-GB"for this codebase, and all of the tests appear to be passing. Additionally, the branch that this PR is pointed at (upstream/aaron/add-email-confirmation), usesI18n.t(...)for several tests, and this change is required for those tests to pass.