From 593d55c05b49e9b73063ed045b480ed2b29b3c36 Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Wed, 11 Jun 2025 13:27:27 +0200 Subject: [PATCH] Put offer to reset email behind a feature flag Put the one-time offer to reset email during email verification behind a by default disabled feature flag. --- .../concerns/verifies_with_email.rb | 12 ++++- app/helpers/sessions_helper.rb | 8 +++- .../feature_flags/ops/offer_email_reset.yml | 8 ++++ spec/helpers/sessions_helper_spec.rb | 22 ++++++++- spec/requests/verifies_with_email_spec.rb | 48 ++++++++++++++++--- 5 files changed, 87 insertions(+), 11 deletions(-) create mode 100644 config/feature_flags/ops/offer_email_reset.yml diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb index 8e011357e1f78c..3ded2b4d265c07 100644 --- a/app/controllers/concerns/verifies_with_email.rb +++ b/app/controllers/concerns/verifies_with_email.rb @@ -6,6 +6,7 @@ module VerifiesWithEmail extend ActiveSupport::Concern include ActionView::Helpers::DateHelper + include SessionsHelper included do prepend_before_action :verify_with_email, only: :create, unless: -> { skip_verify_with_email? } @@ -52,6 +53,7 @@ def resend_verification_code def update_email return unless user = find_verification_user + return render json: { status: :error, message: 'Feature not available' } unless offer_email_reset_enabled?(user) log_verification(user, :email_update_requested) result = Users::EmailVerification::UpdateEmailService.new(user: user).execute(email: email_params[:email]) @@ -150,8 +152,7 @@ def handle_verification_failure(user, reason, message) end def handle_verification_success(user) - user.confirm if unconfirmed_verification_email?(user) - user.email_reset_offered_at = Time.current if user.email_reset_offered_at.nil? + handle_email_reset_on_verification(user) user.unlock_access! log_verification(user, :successful) @@ -162,6 +163,13 @@ def handle_verification_success(user) verify_known_sign_in end + def handle_email_reset_on_verification(user) + return unless offer_email_reset_enabled?(user) + + user.confirm if unconfirmed_verification_email?(user) + user.email_reset_offered_at = Time.current + end + def trusted_ip_address?(user) AuthenticationEvent.initial_login_or_known_ip_address?(user, request.ip) end diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index 17b0629c70240b..6c9e85b37446d7 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -36,8 +36,14 @@ def verification_data(user) obfuscated_email: obfuscated_email(verification_email(user)), verify_path: session_path(:user), resend_path: users_resend_verification_code_path, - offer_email_reset: user.email_reset_offered_at.nil?.to_s, + offer_email_reset: offer_email_reset_enabled?(user).to_s, update_email_path: users_update_email_path } end + + private + + def offer_email_reset_enabled?(user) + Feature.enabled?(:offer_email_reset, :instance) && !user.email_reset_offered_at + end end diff --git a/config/feature_flags/ops/offer_email_reset.yml b/config/feature_flags/ops/offer_email_reset.yml new file mode 100644 index 00000000000000..8060ace65f9b1b --- /dev/null +++ b/config/feature_flags/ops/offer_email_reset.yml @@ -0,0 +1,8 @@ +--- +name: offer_email_reset +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/194151 +rollout_issue_url: +milestone: "18.1" +type: ops +group: group::authorization +default_enabled: false diff --git a/spec/helpers/sessions_helper_spec.rb b/spec/helpers/sessions_helper_spec.rb index 006dbac812cd8f..23216645dc61e1 100644 --- a/spec/helpers/sessions_helper_spec.rb +++ b/spec/helpers/sessions_helper_spec.rb @@ -75,10 +75,30 @@ obfuscated_email: obfuscated_email(user.email), verify_path: helper.session_path(:user), resend_path: users_resend_verification_code_path, - offer_email_reset: user.email_reset_offered_at.nil?.to_s, + offer_email_reset: 'true', update_email_path: users_update_email_path }) end + + context 'when email reset has already been offered' do + before do + user.email_reset_offered_at = Time.now + end + + it 'returns offer_email_reset as `false`' do + expect(helper.verification_data(user)).to include(offer_email_reset: 'false') + end + end + + context 'when the `offer_email_reset` feature flag is disabled' do + before do + stub_feature_flags(offer_email_reset: false) + end + + it 'returns offer_email_reset as `false`' do + expect(helper.verification_data(user)).to include(offer_email_reset: 'false') + end + end end describe '#obfuscated_email' do diff --git a/spec/requests/verifies_with_email_spec.rb b/spec/requests/verifies_with_email_spec.rb index edb03e1be4bd16..ffd3dd3729419f 100644 --- a/spec/requests/verifies_with_email_spec.rb +++ b/spec/requests/verifies_with_email_spec.rb @@ -227,17 +227,33 @@ end end - context 'when email reset has already been offered' do - before do - user.update!(email_reset_offered_at: 1.hour.ago, email: 'new@email') + context 'when email reset functionality is disabled' do + shared_examples 'does not perform email reset actions' do + before do + user.update!(email: 'new@email') + end + + it 'does not confirm the email or change email_reset_offered_at' do + expect { submit_token } + .to not_change { user.reload.email } + .and not_change { user.reload.email_reset_offered_at } + end end - it 'does not change the email_reset_offered_at field' do - expect { submit_token }.not_to change { user.reload.email_reset_offered_at } + context 'when email reset has already been offered' do + before do + user.update!(email_reset_offered_at: 1.hour.ago) + end + + it_behaves_like 'does not perform email reset actions' end - it 'does not confirm the email' do - expect { submit_token }.not_to change { user.reload.email } + context 'when the `offer_email_reset` feature flag is disabled' do + before do + stub_feature_flags(offer_email_reset: false) + end + + it_behaves_like 'does not perform email reset actions' end end end @@ -444,6 +460,24 @@ expect(json_response).to eq('status' => 'success') end + + context 'when the `offer_email_reset` feature flag is disabled' do + before do + stub_feature_flags(offer_email_reset: false) + end + + it 'returns an error response' do + do_request + + expect(json_response).to eq('status' => 'error', 'message' => 'Feature not available') + end + + it 'does not call the UpdateEmailService' do + expect(Users::EmailVerification::UpdateEmailService).not_to receive(:new) + + do_request + end + end end context 'when failing to update the email address' do -- GitLab