Skip to content

Draft: Ensure that email_otp_required_after is always in a valid state

Nick Malcolmrequested to merge
email-otp/570173-state-mgmt into email-otp/570173-restrictions

What does this MR do and why?

Email OTP can be mandatory, permitted, or prohibited, based on various conditions. email_otp_required_after drives whether the user experiences the Email OTP flow. This MR ensures that it is updated when relevant.

2FA (App-based TOTP or WebAuthN) can be required by either an Instance setting or at a Group setting. Email OTP can be required as a minimum by an Instance setting.

  • When users enable 2FA under a mandatory 2FA policy: Email OTP must be disabled and no longer permitted as fallback. This ensures we don't degrade security under the mandatory 2FA policy. However, until they add 2FA, the user still need to do Email-based OTP (if enrolled). This also ensures we don't leave their account less protected while under that policy.
  • When users disable 2FA, and the instance requires minimum email OTP: Email OTP must be automatically re-enabled.

Part of issue: https://gitlab.com/gitlab-org/gitlab/-/issues/570173+s

Builds on Add user enrollment restriction logic for Email... (!208412) • Nick Malcolm • 18.6 and replaces Draft: Add enrollment management for Email-base... (!207777 - closed) • Nick Malcolm • 18.6.

Implementation Details

To achieve this we set email_otp_required_after to a valid state:

  • When the User is created or updated
    • User::BuildService already sets email_otp_required_after when appropriate. (Prior MR).
    • If a user has disable_two_factor! called directly, we call update_email_otp_required_after_based_on_restrictions (to turn on Email OTP if needed)
    • Users::UpdateService calls set_email_otp_required_after_based_on_restrictions, prior to the User#save call.
  • When App-based TOTP is created or removed
    • Create: Profiles::TwoFactorAuthsController calls Users::UpdateService. Implemented there.
    • TwoFactor::DestroyService:
      • calls Users::UpdateService. Implemented there.
      • Also, in that block, calls User#disable_two_factor!. Implemented there for safety.
    • TwoFactor::DestroyOtpService: not sure what the difference is (this one might be for admins / API?) but it also calls Users::UpdateService.
  • When WebAuthN is created or removed
    • Create: As with App-based, on first create it will generate backup codes via Users::UpdateService.
    • Webauthn::DestroyService calls ::Users::UpdateService
    • Also, in that block, calls User#destroy_webauthn_device
  • Finally, during sign in as a last-minute time-of-use safeguard
    • We only do this for valid sign ins when deciding whether to show them the Email OTP flow.

Why not have a validate on the User record?

In the original MR we did. One reason this does not is due to how conditions that would make User invalid arise, namely that User can become "invalid" even when it's not touched:

  • When User changes, e.g. OTP (en|dis)abled - that'd be easy
  • When UserDetail changes - we could introduce a touch to the parent user record, but that's coupling them
  • When a WebAuthN is created or destroyed - again, we could add a touch to the parent user record but it's coupling
  • When an admin changes Gitlab::CurrentSettings.update!(require_minimum_email_based_otp_for_users_with_passwords: true) - well, then many/every user becomes invalid unless we also write a background job to go and set them to the correct state

Testing

bspec spec/models/concerns/authn/email_otp_enrollment_spec.rb \
  spec/models/user_spec.rb \
  spec/requests/verifies_with_email_spec.rb  \
  spec/services/users/update_service_spec.rb \
  spec/services/two_factor/destroy_service_spec.rb \
  spec/services/webauthn/destroy_service_spec.rb

The testing approach is a hybrid:

  • Full specs for the new Concern
  • A mix of behavioral tests for the Model, Request, and Services spec:
    • validate that the concern is called as expected, and
    • one or two representative sample specs to provide more end-to-end coverage

References

Screenshots or screen recordings

Before After

To get Email-based OTP configured (not this MR - already in master)

From https://internal.gitlab.com/handbook/security/product_security/mandatory_mfa/architecture_design/#useful-background

  1. Log in as admin and change the following settings (to match GitLab.com):
    • Under Admin > Settings > General > Sign-up restrictions:
      1. Turn off "Require admin approval for new sign-ups".
      2. Set "Email confirmation settings" to "Hard".
      3. Click "Save changes".
    • Under Admin > Settings > General > Sign-in restrictions:
      1. Turn on "Require email verification when account is locked.".
      2. Click "Save changes".

To test this MR's logic

There is no UI for enabling and disabling Email OTP yet. That will be in https://gitlab.com/gitlab-org/gitlab/-/work_items/575703.

  1. Enable the flag for a user: Feature.enable(:email_based_otp, user)
  2. Enrol them in Email-based OTP: user.update!(email_otp_required_after: Time.current)
  3. Sign in & out. Note that you have to do Email OTP. (Use /rails/letter_opener to get the code)
  4. Add user to group with 2FA enforcement
  5. Sign in - note that you still do Email OTP.
  6. Enable 2FA. Observe email_otp_required_after became nil. Sign out & in again. You have to do 2FA, not email OTP.
  7. Remove the user from the group
  8. Disable 2FA → observe Email OTP is not required (because the instance doesn't require it)
  9. Update the application setting to enforce Email-based OTP: Gitlab::CurrentSettings.update!(require_minimum_email_based_otp_for_users_with_passwords: true).
    1. Note that at this point email_otp_required_after is still nil
  10. Sign out & in. Note that Email OTP was required
  11. Disable the instance setting: Gitlab::CurrentSettings.update!(require_minimum_email_based_otp_for_users_with_passwords: false)
  12. Note the user can have any combination of email_otp_required_after and app-based TOTP / webauthn. There are no restrictions on the user.
    1. In https://gitlab.com/gitlab-org/gitlab/-/work_items/575703+s we will implement the UI

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Nick Malcolm

Merge request reports