From d220eeda0e588d8823177b4ba3db213b811e43b8 Mon Sep 17 00:00:00 2001 From: Jennifer Li Date: Mon, 8 Sep 2025 13:50:12 -0700 Subject: [PATCH 1/5] Commit partial changes --- .../new/components/email_verification.vue | 56 +++++++++++++++++++ .../javascripts/sessions/new/constants.js | 1 + app/assets/javascripts/sessions/new/index.js | 15 ++++- .../concerns/verifies_with_email.rb | 2 + 4 files changed, 73 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/sessions/new/components/email_verification.vue b/app/assets/javascripts/sessions/new/components/email_verification.vue index 02f0d3c2f96e3d..5f8f057cbd531a 100644 --- a/app/assets/javascripts/sessions/new/components/email_verification.vue +++ b/app/assets/javascripts/sessions/new/components/email_verification.vue @@ -11,6 +11,7 @@ import { I18N_EMAIL_INVALID_CODE, I18N_SUBMIT_BUTTON, I18N_RESEND_LINK, + I18N_SKIP_FOR_NOW_BUTTON, I18N_EMAIL_RESEND_SUCCESS, I18N_GENERIC_ERROR, I18N_HELP_TEXT, @@ -55,6 +56,19 @@ export default { type: String, required: true, }, + skipPath: { + type: String, + required: true, + }, + isInGracePeriod: { + type: Boolean, + required: true, + default: true + }, + gracePeriodEndsAt: { + type: String, + required: false, + }, }, data() { return { @@ -87,6 +101,18 @@ export default { return this.verifyError; }, + showSkipButton() { + return this.isInGracePeriod && this.skipPath; + }, + gracePeriodMessage() { + if (!this.isInGracePeriod || !this.gracePeriodEndsAt) { + return ''; + } + + const endDate = new Date(this.gracePeriodEndsAt); + const formattedGracePeriodEndDate = endDate.toLocaleDateString(); + return `You can skip verification until ${formattedGracePeriodEndDate}`; + }, }, watch: { verificationCode() { @@ -124,6 +150,23 @@ export default { this.verifyToken(email); this.resend(email); }, + skip() { + if (!this.skipPath) return; + + axios + .post(this.skipPath) + .then(this.handleSkipResponse) + .catch(this.handleError); + }, + handleSkipResponse(response) { + if (response.data.status === undefined) { + this.handleError(); + } else if (response.data.status === SUCCESS_RESPONSE) { + visitUrl(response.data.redirect_path); + } else if (response.data.status === FAILURE_RESPONSE) { + createAlert({ message: response.data.message }); + } + }, handleResendResponse(response) { if (response.data.status === undefined) { this.handleError(); @@ -162,6 +205,7 @@ export default { helpText: I18N_HELP_TEXT, sendToSecondaryEmailButtonText: I18N_SEND_TO_SECONDARY_EMAIL_BUTTON_TEXT, sendToSecondaryEmailGuide: I18N_SEND_TO_SECONDARY_EMAIL_GUIDE, + skipForNowButton: I18N_SKIP_FOR_NOW_BUTTON, supportUrl: SUPPORT_URL, }, forms: { @@ -188,6 +232,9 @@ export default { {{ email }} +
+ {{ gracePeriodMessage }} +
{{ $options.i18n.resendLink }} + + {{ $options.i18n.skipForNowButton }} +

diff --git a/app/assets/javascripts/sessions/new/constants.js b/app/assets/javascripts/sessions/new/constants.js index 8e4367b0a02d88..4572681bef9e2f 100644 --- a/app/assets/javascripts/sessions/new/constants.js +++ b/app/assets/javascripts/sessions/new/constants.js @@ -10,6 +10,7 @@ export const I18N_EMAIL_EMPTY_CODE = s__('IdentityVerification|Enter a code.'); export const I18N_EMAIL_INVALID_CODE = s__('IdentityVerification|Please enter a valid code'); export const I18N_SUBMIT_BUTTON = s__('IdentityVerification|Verify code'); export const I18N_RESEND_LINK = s__('IdentityVerification|Resend code'); +export const I18N_SKIP_FOR_NOW_BUTTON = s__('Skip for now'); export const I18N_EMAIL_RESEND_SUCCESS = s__('IdentityVerification|A new code has been sent.'); export const I18N_GENERIC_ERROR = s__( 'IdentityVerification|Something went wrong. Please try again.', diff --git a/app/assets/javascripts/sessions/new/index.js b/app/assets/javascripts/sessions/new/index.js index f7679b28e040f6..bbba2d20f4e817 100644 --- a/app/assets/javascripts/sessions/new/index.js +++ b/app/assets/javascripts/sessions/new/index.js @@ -8,7 +8,17 @@ export default () => { return null; } - const { username, obfuscatedEmail, verifyPath, resendPath } = el.dataset; + const { + username, + obfuscatedEmail, + verifyPath, + resendPath, + skipPath, + isInGracePeriod, + gracePeriodEndsAt + } = el.dataset; + + console.log(el.dataset) return new Vue({ el, @@ -20,6 +30,9 @@ export default () => { obfuscatedEmail, verifyPath, resendPath, + skipPath, + isInGracePeriod, + gracePeriodEndsAt }, }); }, diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb index b97aaa47aba4a5..21719801c7066b 100644 --- a/app/controllers/concerns/verifies_with_email.rb +++ b/app/controllers/concerns/verifies_with_email.rb @@ -212,6 +212,8 @@ def require_email_based_otp?(user) user.email_otp_required_after <= Time.zone.now end + def email_otp_grace_period?; end + def verify_token(user, token) # Account locks take precendent over Email-based OTP. If the account # is locked, they need to receive and enter their unlock token. -- GitLab From ea6b49eadac64b229ceba20f02e6ae6f038ceab5 Mon Sep 17 00:00:00 2001 From: Jennifer Li Date: Thu, 11 Sep 2025 10:29:28 -0700 Subject: [PATCH 2/5] Finish skip email otp workflow --- .../new/components/email_verification.vue | 23 ++++++++++---- .../javascripts/sessions/new/constants.js | 2 +- .../concerns/verifies_with_email.rb | 30 +++++++++++++++++++ app/helpers/sessions_helper.rb | 4 ++- .../devise/sessions/_skip_verification.haml | 10 +++++++ .../devise/sessions/skip_verification.haml | 1 + config/routes/user.rb | 2 ++ 7 files changed, 64 insertions(+), 8 deletions(-) create mode 100644 app/views/devise/sessions/_skip_verification.haml create mode 100644 app/views/devise/sessions/skip_verification.haml diff --git a/app/assets/javascripts/sessions/new/components/email_verification.vue b/app/assets/javascripts/sessions/new/components/email_verification.vue index 5f8f057cbd531a..277f60948a77d7 100644 --- a/app/assets/javascripts/sessions/new/components/email_verification.vue +++ b/app/assets/javascripts/sessions/new/components/email_verification.vue @@ -63,7 +63,7 @@ export default { isInGracePeriod: { type: Boolean, required: true, - default: true + default: false }, gracePeriodEndsAt: { type: String, @@ -153,6 +153,7 @@ export default { skip() { if (!this.skipPath) return; + console.log(this.skipPath) axios .post(this.skipPath) .then(this.handleSkipResponse) @@ -241,6 +242,7 @@ export default { label-for="verification-code" :state="inputValidation.state" :invalid-feedback="inputValidation.message" + class="gl-mb-1" > + + +

+ Didn't get the code? + {{ + $options.i18n.resendLink + }} +
+
{{ $options.i18n.submitButton }} - {{ - $options.i18n.resendLink - }} + + {{ $options.i18n.skipForNowButton }}
+

-

+
{{ gracePeriodMessage }}
@@ -258,7 +258,7 @@ export default {
- Didn't get the code? + {{ $options.i18n.didntGetCode }} {{ $options.i18n.resendLink }} @@ -270,13 +270,7 @@ export default { }} - + {{ $options.i18n.skipForNowButton }} diff --git a/app/assets/javascripts/sessions/new/constants.js b/app/assets/javascripts/sessions/new/constants.js index 00606b30bf2ed3..8bf731bb88ab9c 100644 --- a/app/assets/javascripts/sessions/new/constants.js +++ b/app/assets/javascripts/sessions/new/constants.js @@ -10,12 +10,14 @@ export const I18N_EMAIL_EMPTY_CODE = s__('IdentityVerification|Enter a code.'); export const I18N_EMAIL_INVALID_CODE = s__('IdentityVerification|Please enter a valid code'); export const I18N_SUBMIT_BUTTON = s__('IdentityVerification|Verify code'); export const I18N_RESEND_LINK = s__('IdentityVerification|Resend.'); -export const I18N_SKIP_FOR_NOW_BUTTON = s__('Skip for now'); +export const I18N_SKIP_FOR_NOW_BUTTON = s__('IdentityVerification|Skip for now'); export const I18N_EMAIL_RESEND_SUCCESS = s__('IdentityVerification|A new code has been sent.'); export const I18N_GENERIC_ERROR = s__( 'IdentityVerification|Something went wrong. Please try again.', ); +export const I18N_DIDNT_GET_CODE = s__("IdentityVerification|Didn't get the code?"); + export const I18N_EMAIL = __('Email'); export const I18N_SEND_TO_SECONDARY_EMAIL_BUTTON_TEXT = s__( 'IdentityVerification|send a code to another address associated with this account', diff --git a/app/assets/javascripts/sessions/new/index.js b/app/assets/javascripts/sessions/new/index.js index bbba2d20f4e817..cfaaa899fb7078 100644 --- a/app/assets/javascripts/sessions/new/index.js +++ b/app/assets/javascripts/sessions/new/index.js @@ -14,12 +14,10 @@ export default () => { verifyPath, resendPath, skipPath, - isInGracePeriod, - gracePeriodEndsAt + inEmailOtpGracePeriod, + emailOtpRequiredAfter, } = el.dataset; - console.log(el.dataset) - return new Vue({ el, name: 'EmailVerificationRoot', @@ -31,8 +29,8 @@ export default () => { verifyPath, resendPath, skipPath, - isInGracePeriod, - gracePeriodEndsAt + inEmailOtpGracePeriod, + emailOtpRequiredAfter, }, }); }, diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb index 755be5d69898bc..0aca9c7b613322 100644 --- a/app/controllers/concerns/verifies_with_email.rb +++ b/app/controllers/concerns/verifies_with_email.rb @@ -90,21 +90,13 @@ def successful_verification def skip_verification_for_now return unless user = find_verification_user - # Clear the verification session session.delete(:verification_user_id) - - # Sign in the user (since they're skipping verification) sign_in(user) + user.update(email_otp: nil) if user.email_otp.present? - # Log the skip event log_verification(user, :skipped, 'user chose to skip verification in grace period') - # log_audit_event(current_user, user, with: 'email_verification_skip_for_now') log_user_activity(user) - # Clear any pending verification tokens since they're skipping - user.update(email_otp: nil) if user.email_otp.present? - - # Return JSON with redirect path to confirmation page render json: { status: :success, redirect_path: users_skip_verification_confirmation_path } end @@ -113,7 +105,7 @@ def skip_verification_confirmation layout: 'minimal', locals: { redirect_url: after_sign_in_path_for(current_user), - grace_period_ends_at: current_user.email_otp_required_after + email_otp_required_after: current_user.email_otp_required_after } end @@ -239,11 +231,9 @@ def require_email_based_otp?(user) # Devise::Confirmable user.last_sign_in_at.present? && user.email_otp_required_after.present? && - user.email_otp_required_after <= Time.zone.now + (user.email_otp_required_after <= Time.zone.now || user.in_email_otp_grace_period?) end - def email_otp_grace_period?; end - def verify_token(user, token) # Account locks take precendent over Email-based OTP. If the account # is locked, they need to receive and enter their unlock token. diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index b60ebce999fa1c..4024f29448855d 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -28,7 +28,8 @@ def verification_data(user) verify_path: session_path(:user), resend_path: users_resend_verification_code_path, skip_path: users_skip_verification_for_now_path, - is_in_grace_period: true + in_email_otp_grace_period: user.in_email_otp_grace_period?, + email_otp_required_after: user.email_otp_required_after } end end diff --git a/app/models/user.rb b/app/models/user.rb index 6ff5caeab76ebc..44258fbe97c67f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -756,6 +756,16 @@ def inactive_message end end + def in_email_otp_grace_period? + now = Time.zone.now + return false unless email_otp_required_after.present? && email_otp_required_after > now + + now.between?( + email_otp_required_after - 30.days, + email_otp_required_after + ) + end + def self.with_visible_profile(user) return with_public_profile if user.nil? diff --git a/app/views/devise/sessions/_skip_verification.haml b/app/views/devise/sessions/_skip_verification.haml index 10fe998e5d967f..65392f29ffa8d5 100644 --- a/app/views/devise/sessions/_skip_verification.haml +++ b/app/views/devise/sessions/_skip_verification.haml @@ -7,4 +7,4 @@ %h2 = s_('IdentityVerification|You will be automatically redirected') %p.gl-pt-2 - = html_escape(s_("IdentityVerification|You can skip email verification for now. Starting on %{date}, email verification will be mandatory.")) % { date: grace_period_ends_at.strftime('%B %d, %Y') } + = html_escape(s_("IdentityVerification|You can skip email verification for now. Starting on %{date}, email verification will be mandatory.")) % { date: email_otp_required_after.strftime('%B %d, %Y') } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 2a3806ef876cb1..7cde1c5aad8c9e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -33213,6 +33213,9 @@ msgstr "" msgid "IdentityVerification|Country or region" msgstr "" +msgid "IdentityVerification|Didn't get the code?" +msgstr "" + msgid "IdentityVerification|Email Verification" msgstr "" @@ -33291,7 +33294,7 @@ msgstr "" msgid "IdentityVerification|Please enter a valid email address." msgstr "" -msgid "IdentityVerification|Resend code" +msgid "IdentityVerification|Resend." msgstr "" msgid "IdentityVerification|Select country or region" @@ -33303,6 +33306,9 @@ msgstr "" msgid "IdentityVerification|Send code in %{timer}" msgstr "" +msgid "IdentityVerification|Skip for now" +msgstr "" + msgid "IdentityVerification|Something went wrong. Please try again." msgstr "" @@ -33378,6 +33384,12 @@ msgstr "" msgid "IdentityVerification|You are signed in as %{username}. For added security, you'll need to verify your identity. We've sent a verification code to %{email}" msgstr "" +msgid "IdentityVerification|You can skip email verification for now. Starting on %{date}, email verification will be mandatory." +msgstr "" + +msgid "IdentityVerification|You will be automatically redirected" +msgstr "" + msgid "IdentityVerification|You will receive a text containing a code. Standard charges may apply." msgstr "" -- GitLab From 03a7f82ca00b09574ce90b03af5ff35aa55dc603 Mon Sep 17 00:00:00 2001 From: Jennifer Li Date: Fri, 12 Sep 2025 13:42:40 -0700 Subject: [PATCH 4/5] Update failing specs Remove unused prop --- .../new/components/email_verification.vue | 37 +++++++------------ app/models/user.rb | 1 + .../users/email_verification_on_login_spec.rb | 4 +- spec/helpers/sessions_helper_spec.rb | 7 +++- 4 files changed, 22 insertions(+), 27 deletions(-) diff --git a/app/assets/javascripts/sessions/new/components/email_verification.vue b/app/assets/javascripts/sessions/new/components/email_verification.vue index 0ff1475d58e713..6e8102dd10ced5 100644 --- a/app/assets/javascripts/sessions/new/components/email_verification.vue +++ b/app/assets/javascripts/sessions/new/components/email_verification.vue @@ -64,12 +64,7 @@ export default { inEmailOtpGracePeriod: { type: Boolean, required: true, - default: false, - }, - emailOtpRequiredAfter: { - type: String, - required: false, - default: '', + default: false }, }, data() { @@ -106,15 +101,6 @@ export default { showSkipButton() { return this.inEmailOtpGracePeriod; }, - gracePeriodMessage() { - if (!this.inEmailOtpGracePeriod || !this.emailOtpRequiredAfter) { - return ''; - } - - const endDate = new Date(this.emailOtpRequiredAfter); - const formattedGracePeriodEndDate = endDate.toLocaleDateString(); - return this.$sprintf(this.$options.i18n.gracePeriodMessage, { formattedGracePeriodEndDate }); - }, }, watch: { verificationCode() { @@ -155,7 +141,10 @@ export default { skip() { if (!this.skipPath) return; - axios.post(this.skipPath).then(this.handleSkipResponse).catch(this.handleError); + axios + .post(this.skipPath) + .then(this.handleSkipResponse) + .catch(this.handleError); }, handleSkipResponse(response) { if (response.data.status === undefined) { @@ -206,8 +195,7 @@ export default { sendToSecondaryEmailGuide: I18N_SEND_TO_SECONDARY_EMAIL_GUIDE, skipForNowButton: I18N_SKIP_FOR_NOW_BUTTON, supportUrl: SUPPORT_URL, - didntGetCode: I18N_DIDNT_GET_CODE, - gracePeriodMessage: 'You can skip verification until %{formattedGracePeriodEndDate}', + didntGetCode: I18N_DIDNT_GET_CODE }, forms: { verifyTokenForm: VERIFY_TOKEN_FORM, @@ -233,9 +221,6 @@ export default { {{ email }} -
- {{ gracePeriodMessage }} -
- {{ $options.i18n.didntGetCode }} + {{ $options.i18n.didntGetCode }} {{ $options.i18n.resendLink }} @@ -270,7 +255,13 @@ export default { }} - + {{ $options.i18n.skipForNowButton }} diff --git a/app/models/user.rb b/app/models/user.rb index 44258fbe97c67f..cae51f59f653cf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -760,6 +760,7 @@ def in_email_otp_grace_period? now = Time.zone.now return false unless email_otp_required_after.present? && email_otp_required_after > now + # Reminder that `between` is inclusive now.between?( email_otp_required_after - 30.days, email_otp_required_after diff --git a/spec/features/users/email_verification_on_login_spec.rb b/spec/features/users/email_verification_on_login_spec.rb index b898de8912c9f0..03ae0d0e6e7da5 100644 --- a/spec/features/users/email_verification_on_login_spec.rb +++ b/spec/features/users/email_verification_on_login_spec.rb @@ -132,7 +132,7 @@ it 'rate limits resends' do gitlab_sign_in(user) - expect(page).to have_button s_('IdentityVerification|Resend code') + expect(page).to have_button s_('IdentityVerification|Resend') # Resend more than the rate limited amount of times 10.times { click_request_new_code_button } @@ -465,6 +465,6 @@ def perform_verification_with_code(code) end def click_request_new_code_button - click_button s_('IdentityVerification|Resend code') + click_button s_('IdentityVerification|Resend') end end diff --git a/spec/helpers/sessions_helper_spec.rb b/spec/helpers/sessions_helper_spec.rb index e6ddad0ebaa617..631959df04329d 100644 --- a/spec/helpers/sessions_helper_spec.rb +++ b/spec/helpers/sessions_helper_spec.rb @@ -21,11 +21,14 @@ let(:user) { build_stubbed(:user) } it 'returns the expected data' do - expect(helper.verification_data(user)).to eq({ + expect(helper.verification_data(user)).to match({ username: user.username, obfuscated_email: obfuscated_email(user.email), verify_path: helper.session_path(:user), - resend_path: users_resend_verification_code_path + resend_path: users_resend_verification_code_path, + skip_path: users_skip_verification_for_now_path, + email_otp_required_after: user.email_otp_required_after, + in_email_otp_grace_period: user.in_email_otp_grace_period? }) end end -- GitLab From cb6c135206081a8431dbbd30a8cebd0889e06384 Mon Sep 17 00:00:00 2001 From: Jennifer Li Date: Fri, 12 Sep 2025 16:04:36 -0700 Subject: [PATCH 5/5] Finish sessions helper --- .../new/components/email_verification.vue | 23 ++-- app/assets/javascripts/sessions/new/index.js | 4 +- .../concerns/verifies_with_email.rb | 16 +-- app/helpers/sessions_helper.rb | 23 +++- spec/helpers/sessions_helper_spec.rb | 101 +++++++++++++++++- 5 files changed, 133 insertions(+), 34 deletions(-) diff --git a/app/assets/javascripts/sessions/new/components/email_verification.vue b/app/assets/javascripts/sessions/new/components/email_verification.vue index 6e8102dd10ced5..e958ad723ccb0d 100644 --- a/app/assets/javascripts/sessions/new/components/email_verification.vue +++ b/app/assets/javascripts/sessions/new/components/email_verification.vue @@ -61,10 +61,10 @@ export default { type: String, required: true, }, - inEmailOtpGracePeriod: { + permittedToSkipEmailOtpInGracePeriod: { type: Boolean, required: true, - default: false + default: false, }, }, data() { @@ -99,7 +99,7 @@ export default { return this.verifyError; }, showSkipButton() { - return this.inEmailOtpGracePeriod; + return this.permittedToSkipEmailOtpInGracePeriod; }, }, watch: { @@ -141,10 +141,7 @@ export default { skip() { if (!this.skipPath) return; - axios - .post(this.skipPath) - .then(this.handleSkipResponse) - .catch(this.handleError); + axios.post(this.skipPath).then(this.handleSkipResponse).catch(this.handleError); }, handleSkipResponse(response) { if (response.data.status === undefined) { @@ -195,7 +192,7 @@ export default { sendToSecondaryEmailGuide: I18N_SEND_TO_SECONDARY_EMAIL_GUIDE, skipForNowButton: I18N_SKIP_FOR_NOW_BUTTON, supportUrl: SUPPORT_URL, - didntGetCode: I18N_DIDNT_GET_CODE + didntGetCode: I18N_DIDNT_GET_CODE, }, forms: { verifyTokenForm: VERIFY_TOKEN_FORM, @@ -243,7 +240,7 @@ export default {
- {{ $options.i18n.didntGetCode }} + {{ $options.i18n.didntGetCode }} {{ $options.i18n.resendLink }} @@ -255,13 +252,7 @@ export default { }} - + {{ $options.i18n.skipForNowButton }} diff --git a/app/assets/javascripts/sessions/new/index.js b/app/assets/javascripts/sessions/new/index.js index cfaaa899fb7078..8945a66e73d176 100644 --- a/app/assets/javascripts/sessions/new/index.js +++ b/app/assets/javascripts/sessions/new/index.js @@ -14,7 +14,7 @@ export default () => { verifyPath, resendPath, skipPath, - inEmailOtpGracePeriod, + permittedToSkipEmailOtpInGracePeriod, emailOtpRequiredAfter, } = el.dataset; @@ -29,7 +29,7 @@ export default () => { verifyPath, resendPath, skipPath, - inEmailOtpGracePeriod, + permittedToSkipEmailOtpInGracePeriod, emailOtpRequiredAfter, }, }); diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb index 0aca9c7b613322..6b442844d980cd 100644 --- a/app/controllers/concerns/verifies_with_email.rb +++ b/app/controllers/concerns/verifies_with_email.rb @@ -88,7 +88,8 @@ def successful_verification end def skip_verification_for_now - return unless user = find_verification_user + user = find_verification_user + return unless user && permitted_to_skip_email_otp_in_grace_period?(user) session.delete(:verification_user_id) sign_in(user) @@ -166,15 +167,6 @@ def requires_verify_email?(user) treat_as_locked?(user) || !trusted_ip_address?(user) || require_email_based_otp?(user) end - def treat_as_locked?(user) - # A user can have #access_locked? return false, but we still want - # to treat as locked during sign in if they were sent an unlock - # token in the past. - # See https://docs.gitlab.com/security/unlock_user/#gitlabcom-users - # and https://gitlab.com/gitlab-org/gitlab/-/issues/560080. - user.access_locked? || user.unlock_token - end - def verify_email(user) return true unless requires_verify_email?(user) @@ -300,10 +292,6 @@ def handle_verification_success(user) verify_known_sign_in end - def trusted_ip_address?(user) - AuthenticationEvent.initial_login_or_known_ip_address?(user, request.ip) - end - def prompt_for_email_verification(user) session[:verification_user_id] = user.id self.resource = user diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index 4024f29448855d..6b31213ec6bbd6 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -28,8 +28,29 @@ def verification_data(user) verify_path: session_path(:user), resend_path: users_resend_verification_code_path, skip_path: users_skip_verification_for_now_path, - in_email_otp_grace_period: user.in_email_otp_grace_period?, + permitted_to_skip_email_otp_in_grace_period: permitted_to_skip_email_otp_in_grace_period?(user), email_otp_required_after: user.email_otp_required_after } end + + # Used by frontend to decide if we should render the "skip for now" button + def permitted_to_skip_email_otp_in_grace_period?(user) + Feature.enabled?(:email_based_mfa, user) && + trusted_ip_address?(user) && + !treat_as_locked?(user) && + user.in_email_otp_grace_period? + end + + def trusted_ip_address?(user) + AuthenticationEvent.initial_login_or_known_ip_address?(user, request.ip) + end + + def treat_as_locked?(user) + # A user can have #access_locked? return false, but we still want + # to treat as locked during sign in if they were sent an unlock + # token in the past. + # See https://docs.gitlab.com/security/unlock_user/#gitlabcom-users + # and https://gitlab.com/gitlab-org/gitlab/-/issues/560080. + user.access_locked? || user.unlock_token + end end diff --git a/spec/helpers/sessions_helper_spec.rb b/spec/helpers/sessions_helper_spec.rb index 631959df04329d..abd2fb9f353655 100644 --- a/spec/helpers/sessions_helper_spec.rb +++ b/spec/helpers/sessions_helper_spec.rb @@ -28,7 +28,7 @@ resend_path: users_resend_verification_code_path, skip_path: users_skip_verification_for_now_path, email_otp_required_after: user.email_otp_required_after, - in_email_otp_grace_period: user.in_email_otp_grace_period? + permitted_to_skip_email_otp_in_grace_period: permitted_to_skip_email_otp_in_grace_period?(user) }) end end @@ -87,4 +87,103 @@ it { is_expected.to be false } end end + + describe '#trusted_ip_address' do + let(:trusted) { false } + + before do + allow(AuthenticationEvent) + .to receive(:initial_login_or_known_ip_address?) + .and_return(trusted) + end + + context 'when AuthenticationEvent.initial_login_or_known_ip_address returns false' do + it { is_expected.to be false } + end + + context 'when AuthenticationEvent.initial_login_or_known_ip_address returns true' do + it { is_expected.to be true } + end + end + + describe '#treat_as_locked' do + let(:user_access_locked) { false } + let(:user_unlock_token) { nil } + let(:user) do + build_stubbed(:user, access_locked?: user_access_locked, unlock_token: user_unlock_token) + end + + context 'when user access is not locked and has no unlock token' do + it { is_expected.to be false } + end + + context 'when user access is locked' do + let(:user_access_locked?) { true } + + it { is_expected.to be true } + end + + context 'when user unlock token is present' do + let(:user_unlock_token) { 'some token' } + + it { is_expected.to be true } + end + end + + describe '#permitted_to_skip_email_otp_in_grace_period?' do + let(:trusted_ip) { true } + let(:user_access_locked) { false } + let(:user_unlock_token) { nil } + let(:in_grace_period) { true } + + let(:user) do + build_stubbed(:user, + access_locked?: user_access_locked, + unlock_token: user_unlock_token + ) + end + + before do + allow(AuthenticationEvent) + .to receive(:initial_login_or_known_ip_address?) + .and_return(trusted_ip) + allow(user).to receive(:in_email_otp_grace_period?).and_return(in_grace_period) + end + + context 'when all conditions are met' do + it { is_expected.to be true } + end + + context 'when email_based_mfa feature is disabled' do + before do + stub_feature_flags(email_based_mfa: false) + end + + it { is_expected.to be false } + end + + context 'when IP address is not trusted' do + let(:trusted_ip) { false } + + it { is_expected.to be false } + end + + context 'when user is treated as locked due to access_locked' do + let(:user_access_locked) { true } + + it { is_expected.to be false } + end + + context 'when user is treated as locked due to unlock token' do + let(:user_unlock_token) { 'some_token' } + + it { is_expected.to be false } + end + + context 'when user is not in email OTP grace period' do + let(:in_grace_period) { false } + + it { is_expected.to be false } + end + end end -- GitLab