Skip to content

Add spec and fixes for possible email confirmation paths

Alex Buijs requested to merge fix-custom-email-confirmation-edge-cases into master

What does this MR do and why?

This adds a test for email confirmation during signup with various features/settings toggled.

The application settings that impact email confirmation are:

  • require_admin_approval_after_user_signup (new users cannot login until approved by admin, turned off in production)
  • send_user_confirmation_email (when turned off, new users do not need to confirm their email, turned on in production)

The feature flags that impact email confirmation are:

  • soft_email_confirmation (new users can login without confirming their email address immediately, although they will need to do so within 3 days, not turned on in production currently, but it is turned on in staging)
  • identity_verification (replaces Devise confirmation with custom code confirmation, not turned on in production currently)

This adds a spec for all possible combinations for these 4 settings.

This test identified the following problems when identity verification is enabled:

  • Devise resending confirmation instructions didn't work when identity verification was enabled
  • when admin approval was required and a confirmation email is sent:
    • the user was redirected to the identity verification page on sign in before being approved
    • on sign in after being approved, the user was redirected to the verification successful page instead of the welcome page
  • when soft_email_verification and send_user_confirmation_email enabled and require_admin_approval_after_user_signup disabled, the user receives the custom confirmation instructions email and sees the confirmation warning on every page, but has no way to verify. Also, clicking the 'Resend it' link from the warning renders a 404.

The solution to these problems was to disable identity verification when any of the following is true:

  • require_admin_approval_after_user_signup is true
  • send_user_confirmation_email is false
  • soft_email_confirmation is true

In order to be dry, the identity verification enabled logic was placed in the ::Users::EmailVerification::SendCustomConfirmationInstructionsService.enabled? method and is now called from all places where we check for identity verification.

Another bug that came up, was that confirming didn't work when signed in through soft_email_confirmation and not having completed the required_signup_info.

Issue https://gitlab.com/gitlab-org/modelops/anti-abuse/team-tasks/-/issues/115

Fixes https://gitlab.com/gitlab-org/gitlab/-/issues/377434

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Alex Buijs

Merge request reports