diff --git a/app/assets/javascripts/sessions/new/components/email_verification.vue b/app/assets/javascripts/sessions/new/components/email_verification.vue index 87385b91c426eddabecd36a31aadec268888ce82..6a67c25b58f8bc248d362ef3e28b454f362cb889 100644 --- a/app/assets/javascripts/sessions/new/components/email_verification.vue +++ b/app/assets/javascripts/sessions/new/components/email_verification.vue @@ -12,10 +12,12 @@ import { I18N_RESEND_LINK, I18N_EMAIL_RESEND_SUCCESS, I18N_GENERIC_ERROR, + I18N_UPDATE_EMAIL, VERIFICATION_CODE_REGEX, SUCCESS_RESPONSE, FAILURE_RESPONSE, } from '../constants'; +import UpdateEmail from './update_email.vue'; export default { name: 'EmailVerification', @@ -25,6 +27,7 @@ export default { GlFormGroup, GlFormInput, GlButton, + UpdateEmail, }, props: { obfuscatedEmail: { @@ -39,12 +42,22 @@ export default { type: String, required: true, }, + isOfferEmailReset: { + type: Boolean, + required: true, + }, + updateEmailPath: { + type: String, + required: true, + }, }, data() { return { + email: this.obfuscatedEmail, verificationCode: '', submitted: false, verifyError: '', + showUpdateEmail: false, }; }, computed: { @@ -126,49 +139,73 @@ export default { this.submitted = false; this.$refs.input.$el.focus(); }, + updateEmail() { + this.showUpdateEmail = true; + }, + verifyToken(email = '') { + this.showUpdateEmail = false; + if (email.length) this.email = email; + this.$nextTick(this.resetForm); + }, }, i18n: { explanation: I18N_EXPLANATION, inputLabel: I18N_INPUT_LABEL, submitButton: I18N_SUBMIT_BUTTON, resendLink: I18N_RESEND_LINK, + updateEmail: I18N_UPDATE_EMAIL, }, }; diff --git a/app/assets/javascripts/sessions/new/components/update_email.vue b/app/assets/javascripts/sessions/new/components/update_email.vue new file mode 100644 index 0000000000000000000000000000000000000000..f63176e55131c8e424327b9bbbd7d2fd690ed169 --- /dev/null +++ b/app/assets/javascripts/sessions/new/components/update_email.vue @@ -0,0 +1,130 @@ + + + diff --git a/app/assets/javascripts/sessions/new/constants.js b/app/assets/javascripts/sessions/new/constants.js index 203a8aee1c40ae71991f37588517e4a701754f99..dec96f78232e476b526662ed5aeaa1585928d4f3 100644 --- a/app/assets/javascripts/sessions/new/constants.js +++ b/app/assets/javascripts/sessions/new/constants.js @@ -1,4 +1,4 @@ -import { s__ } from '~/locale'; +import { s__, __ } from '~/locale'; export const I18N_EXPLANATION = s__( "IdentityVerification|For added security, you'll need to verify your identity. We've sent a verification code to %{email}", @@ -13,6 +13,14 @@ export const I18N_GENERIC_ERROR = s__( 'IdentityVerification|Something went wrong. Please try again.', ); +export const I18N_EMAIL = __('Email'); +export const I18N_UPDATE_EMAIL = s__('IdentityVerification|Update email'); +export const I18N_CANCEL = __('Cancel'); +export const I18N_EMAIL_INVALID = s__('IdentityVerification|Please enter a valid email address.'); +export const I18N_UPDATE_EMAIL_SUCCESS = s__( + 'IdentityVerification|A new code has been sent to your updated email address.', +); + export const VERIFICATION_CODE_REGEX = /^\d{6}$/; export const SUCCESS_RESPONSE = 'success'; export const FAILURE_RESPONSE = 'failure'; diff --git a/app/assets/javascripts/sessions/new/index.js b/app/assets/javascripts/sessions/new/index.js index 51022a281e3d72ed4a73b36108dd18fe39848c7c..bf126b0e202cf8fd18494bf5e174d0787a36a092 100644 --- a/app/assets/javascripts/sessions/new/index.js +++ b/app/assets/javascripts/sessions/new/index.js @@ -1,4 +1,5 @@ import Vue from 'vue'; +import { parseBoolean } from '~/lib/utils/common_utils'; import EmailVerification from './components/email_verification.vue'; export default () => { @@ -8,14 +9,20 @@ export default () => { return null; } - const { obfuscatedEmail, verifyPath, resendPath } = el.dataset; + const { obfuscatedEmail, verifyPath, resendPath, offerEmailReset, updateEmailPath } = el.dataset; return new Vue({ el, name: 'EmailVerificationRoot', render(createElement) { return createElement(EmailVerification, { - props: { obfuscatedEmail, verifyPath, resendPath }, + props: { + obfuscatedEmail, + verifyPath, + resendPath, + isOfferEmailReset: parseBoolean(offerEmailReset), + updateEmailPath, + }, }); }, }); diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb index f52903fe7e960e060729d49ddcf4f4ad31654782..6affd7bb4cc1ddaa700be1204d44650b23110dc3 100644 --- a/app/controllers/concerns/verifies_with_email.rb +++ b/app/controllers/concerns/verifies_with_email.rb @@ -58,6 +58,21 @@ def resend_verification_code end end + def update_email + return unless user = find_verification_user + + log_verification(user, :email_update_requested) + result = Users::EmailVerification::UpdateEmailService.new(user: user).execute(email: email_params[:email]) + + if result[:status] == :success + send_verification_instructions(user) + else + handle_verification_failure(user, result[:reason], result[:message]) + end + + render json: result + end + def successful_verification session.delete(:verification_user_id) @redirect_url = after_sign_in_path_for(current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables @@ -88,7 +103,8 @@ def send_verification_instructions(user, reason: nil) def send_verification_instructions_email(user, token) return unless user.can?(:receive_notifications) - Notify.verification_instructions_email(user.email, token: token).deliver_later + email = verification_email(user) + Notify.verification_instructions_email(email, token: token).deliver_later log_verification(user, :instructions_sent) end @@ -129,6 +145,8 @@ 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? user.unlock_access! log_verification(user, :successful) @@ -157,6 +175,10 @@ def verification_params params.require(:user).permit(:verification_token) end + def email_params + params.require(:user).permit(:email) + end + def log_verification(user, event, reason = nil) Gitlab::AppLogger.info( message: 'Email Verification', diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index d5b642994f19d92e7af6450693ff8a69086bde54..cf5cc92587f4e97f6cd8bdf31cbed17217829ebe 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -49,11 +49,22 @@ def remember_me_enabled? Gitlab::CurrentSettings.remember_me_enabled? end + def unconfirmed_verification_email?(user) + token_valid_from = ::Users::EmailVerification::ValidateTokenService::TOKEN_VALID_FOR_MINUTES.minutes.ago + user.email_reset_offered_at.nil? && user.pending_reconfirmation? && user.confirmation_sent_at >= token_valid_from + end + + def verification_email(user) + unconfirmed_verification_email?(user) ? user.unconfirmed_email : user.email + end + def verification_data(user) { - obfuscated_email: obfuscated_email(user.email), + obfuscated_email: obfuscated_email(verification_email(user)), verify_path: session_path(:user), - resend_path: users_resend_verification_code_path + resend_path: users_resend_verification_code_path, + offer_email_reset: user.email_reset_offered_at.nil?.to_s, + update_email_path: users_update_email_path } end end diff --git a/app/models/user.rb b/app/models/user.rb index f36a58b1cdf0d749bf8b220b4cde225a29edccde..a55908c5e2371a1bd1b46fe2d918d6bf4be672ca 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -403,6 +403,7 @@ def update_tracked_fields!(request) delegate :location, :location=, to: :user_detail, allow_nil: true delegate :organization, :organization=, to: :user_detail, allow_nil: true delegate :discord, :discord=, to: :user_detail, allow_nil: true + delegate :email_reset_offered_at, :email_reset_offered_at=, to: :user_detail, allow_nil: true accepts_nested_attributes_for :user_preference, update_only: true accepts_nested_attributes_for :user_detail, update_only: true diff --git a/app/services/users/email_verification/update_email_service.rb b/app/services/users/email_verification/update_email_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..3f9b90b29607f22ac6034d22d72c19ff34365131 --- /dev/null +++ b/app/services/users/email_verification/update_email_service.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module Users + module EmailVerification + class UpdateEmailService + include ActionView::Helpers::DateHelper + + RATE_LIMIT = :email_verification_code_send + + def initialize(user:) + @user = user + end + + def execute(email:) + return failure(:rate_limited) if rate_limited? + return failure(:already_offered) if already_offered? + return failure(:no_change) if no_change?(email) + return failure(:validation_error) unless update_email + + success + end + + private + + attr_reader :user + + def rate_limited? + Gitlab::ApplicationRateLimiter.throttled?(RATE_LIMIT, scope: user) + end + + def already_offered? + user.email_reset_offered_at.present? + end + + def no_change?(email) + user.email = email + !user.will_save_change_to_email? + end + + def update_email + user.skip_confirmation_notification! + user.save + end + + def success + { status: :success } + end + + def failure(reason) + { + status: :failure, + reason: reason, + message: failure_message(reason) + } + end + + def failure_message(reason) + case reason + when :rate_limited + interval = distance_of_time_in_words(Gitlab::ApplicationRateLimiter.rate_limits[RATE_LIMIT][:interval]) + format( + s_("IdentityVerification|You've reached the maximum amount of tries. Wait %{interval} and try again."), + interval: interval + ) + when :already_offered + s_('IdentityVerification|Email update is only offered once.') + when :no_change + s_('IdentityVerification|A code has already been sent to this email address. ' \ + 'Check your spam folder or enter another email address.') + when :validation_error + user.errors.full_messages.join(' ') + end + end + end + end +end diff --git a/config/routes/user.rb b/config/routes/user.rb index 5723421cad2472163be9726a33964aaf7a1f5a76..c66f4872b210715fe97428909f7fec5e6817599e 100644 --- a/config/routes/user.rb +++ b/config/routes/user.rb @@ -55,6 +55,7 @@ def override_omniauth(provider, controller, path_prefix = '/users/auth') get '/users/almost_there' => 'confirmations#almost_there' post '/users/resend_verification_code', to: 'sessions#resend_verification_code' get '/users/successful_verification', to: 'sessions#successful_verification' + patch '/users/update_email', to: 'sessions#update_email' # Redirect on GitHub authorization request errors. E.g. it could happen when user: # 1. cancel authorization the GitLab OAuth app via GitHub to import GitHub repos diff --git a/db/migrate/20230711151845_add_email_reset_offered_at_to_user_details.rb b/db/migrate/20230711151845_add_email_reset_offered_at_to_user_details.rb new file mode 100644 index 0000000000000000000000000000000000000000..45b6a7fd57f4cbd179e9f2c4191e48de6d8b3b49 --- /dev/null +++ b/db/migrate/20230711151845_add_email_reset_offered_at_to_user_details.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddEmailResetOfferedAtToUserDetails < Gitlab::Database::Migration[2.1] + enable_lock_retries! + + def change + add_column :user_details, :email_reset_offered_at, :datetime_with_timezone + end +end diff --git a/db/schema_migrations/20230711151845 b/db/schema_migrations/20230711151845 new file mode 100644 index 0000000000000000000000000000000000000000..1a96738833c16c557ce016e2039617781143b079 --- /dev/null +++ b/db/schema_migrations/20230711151845 @@ -0,0 +1 @@ +4285dfe98db13ffe7f97a69f95e14c584b32cb20526766a8179012c616ef3d0a \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 2e66ac6efa30fee5496056ddf0ff9f4a09c91d55..79cdc7f4a9196eb1f16c8dd4ec22107f50f0cdc8 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -23823,6 +23823,7 @@ CREATE TABLE user_details ( discord text DEFAULT ''::text NOT NULL, enterprise_group_id bigint, enterprise_group_associated_at timestamp with time zone, + email_reset_offered_at timestamp with time zone, CONSTRAINT check_245664af82 CHECK ((char_length(webauthn_xid) <= 100)), CONSTRAINT check_444573ee52 CHECK ((char_length(skype) <= 500)), CONSTRAINT check_466a25be35 CHECK ((char_length(twitter) <= 500)), diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 003906aa663df18cfefe460344a3703274ff8d11..93c022878bf809293b7229d1d685343fac123e02 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -23429,6 +23429,12 @@ msgstr "" msgid "IdentityVerification|%{linkStart}Enter a new phone number%{linkEnd}" msgstr "" +msgid "IdentityVerification|A code has already been sent to this email address. Check your spam folder or enter another email address." +msgstr "" + +msgid "IdentityVerification|A new code has been sent to your updated email address." +msgstr "" + msgid "IdentityVerification|A new code has been sent." msgstr "" @@ -23450,6 +23456,9 @@ msgstr "" msgid "IdentityVerification|Didn't receive a code? %{linkStart}Send a new code%{linkEnd}" msgstr "" +msgid "IdentityVerification|Email update is only offered once." +msgstr "" + msgid "IdentityVerification|Enter a code." msgstr "" @@ -23504,6 +23513,9 @@ msgstr "" msgid "IdentityVerification|Please enter a valid code" msgstr "" +msgid "IdentityVerification|Please enter a valid email address." +msgstr "" + msgid "IdentityVerification|Resend code" msgstr "" @@ -23534,6 +23546,9 @@ msgstr "" msgid "IdentityVerification|There was a problem with the credit card details you entered. Use a different credit card and try again." msgstr "" +msgid "IdentityVerification|Update email" +msgstr "" + msgid "IdentityVerification|Verification code" msgstr "" diff --git a/spec/features/users/email_verification_on_login_spec.rb b/spec/features/users/email_verification_on_login_spec.rb index c9b1670be820b827a27c06e2c02855f72f17ee02..7675de28f86e72230ac2b1b3312bf48c4972b3c5 100644 --- a/spec/features/users/email_verification_on_login_spec.rb +++ b/spec/features/users/email_verification_on_login_spec.rb @@ -5,7 +5,9 @@ RSpec.describe 'Email Verification On Login', :clean_gitlab_redis_rate_limiting, :js, feature_category: :system_access do include EmailHelpers - let_it_be(:user) { create(:user) } + let_it_be_with_reload(:user) { create(:user) } + let_it_be(:another_user) { create(:user) } + let_it_be(:new_email) { build_stubbed(:user).email } let(:require_email_verification_enabled) { user } @@ -97,6 +99,53 @@ end end + describe 'updating the email address' do + it 'offers to update the email address' do + perform_enqueued_jobs do + # When logging in + gitlab_sign_in(user) + + # Expect an instructions email to be sent with a code + code = expect_instructions_email_and_extract_code + + # It shows an update email button + expect(page).to have_button s_('IdentityVerification|Update email') + + # Click Update email button + click_button s_('IdentityVerification|Update email') + + # Try to update with another user's email address + fill_in _('Email'), with: another_user.email + click_button s_('IdentityVerification|Update email') + expect(page).to have_content('Email has already been taken') + + # Update to a unique email address + fill_in _('Email'), with: new_email + click_button s_('IdentityVerification|Update email') + expect(page).to have_content(s_('IdentityVerification|A new code has been sent to ' \ + 'your updated email address.')) + expect_log_message('Instructions Sent', 2) + + new_code = expect_email_changed_notification_to_old_address_and_instructions_email_to_new_address + + # Verify the old code is different from the new code + expect(code).not_to eq(new_code) + verify_code(new_code) + + # Expect the user to be unlocked + expect_user_to_be_unlocked + expect_user_to_be_confirmed + + # When logging in again + gitlab_sign_out + gitlab_sign_in(user) + + # It does not show an update email button anymore + expect(page).not_to have_button s_('IdentityVerification|Update email') + end + end + end + describe 'verification errors' do it 'rate limits verifications' do perform_enqueued_jobs do @@ -339,6 +388,28 @@ def expect_user_to_be_unlocked end end + def expect_user_to_be_confirmed + aggregate_failures do + expect(user.email).to eq(new_email) + expect(user.unconfirmed_email).to be_nil + end + end + + def expect_email_changed_notification_to_old_address_and_instructions_email_to_new_address + changed_email = ActionMailer::Base.deliveries[0] + instructions_email = ActionMailer::Base.deliveries[1] + + expect(changed_email.to).to match_array([user.email]) + expect(changed_email.subject).to eq('Email Changed') + + expect(instructions_email.to).to match_array([new_email]) + expect(instructions_email.subject).to eq(s_('IdentityVerification|Verify your identity')) + + reset_delivered_emails! + + instructions_email.body.parts.first.to_s[/\d{#{Users::EmailVerification::GenerateTokenService::TOKEN_LENGTH}}/o] + end + def expect_instructions_email_and_extract_code mail = find_email_for(user) expect(mail.to).to match_array([user.email]) diff --git a/spec/frontend/sessions/new/components/email_verification_spec.js b/spec/frontend/sessions/new/components/email_verification_spec.js index 8ff139e8475aa26d3b68c034b962c9f21d663783..30ba2782f2f9c7094b249f6ad1d2da2a595ef72c 100644 --- a/spec/frontend/sessions/new/components/email_verification_spec.js +++ b/spec/frontend/sessions/new/components/email_verification_spec.js @@ -6,11 +6,13 @@ import { s__ } from '~/locale'; import { createAlert, VARIANT_SUCCESS } from '~/alert'; import { HTTP_STATUS_NOT_FOUND, HTTP_STATUS_OK } from '~/lib/utils/http_status'; import EmailVerification from '~/sessions/new/components/email_verification.vue'; +import UpdateEmail from '~/sessions/new/components/update_email.vue'; import { visitUrl } from '~/lib/utils/url_utility'; import { I18N_EMAIL_EMPTY_CODE, I18N_EMAIL_INVALID_CODE, I18N_GENERIC_ERROR, + I18N_UPDATE_EMAIL, I18N_RESEND_LINK, I18N_EMAIL_RESEND_SUCCESS, } from '~/sessions/new/constants'; @@ -29,18 +31,22 @@ describe('EmailVerification', () => { obfuscatedEmail: 'al**@g*****.com', verifyPath: '/users/sign_in', resendPath: '/users/resend_verification_code', + isOfferEmailReset: true, + updateEmailPath: '/users/update_email', }; - const createComponent = () => { + const createComponent = (props = {}) => { wrapper = mountExtended(EmailVerification, { - propsData: defaultPropsData, + propsData: { ...defaultPropsData, ...props }, }); }; const findForm = () => wrapper.findComponent(GlForm); const findCodeInput = () => wrapper.findComponent(GlFormInput); + const findUpdateEmail = () => wrapper.findComponent(UpdateEmail); const findSubmitButton = () => wrapper.find('[type="submit"]'); const findResendLink = () => wrapper.findByText(I18N_RESEND_LINK); + const findUpdateEmailLink = () => wrapper.findByText(I18N_UPDATE_EMAIL); const enterCode = (code) => findCodeInput().setValue(code); const submitForm = () => findForm().trigger('submit'); @@ -202,4 +208,44 @@ describe('EmailVerification', () => { expect(findCodeInput().element.value).toBe(''); }); }); + + describe('updating the email', () => { + it('contains the link to show the update email form', () => { + expect(findUpdateEmailLink().exists()).toBe(true); + }); + + describe('when the isOfferEmailReset property is set to false', () => { + beforeEach(() => { + createComponent({ isOfferEmailReset: false }); + }); + + it('does not contain the link to show the update email form', () => { + expect(findUpdateEmailLink().exists()).toBe(false); + }); + }); + + it('shows the UpdateEmail component when clicking the link', async () => { + expect(findUpdateEmail().exists()).toBe(false); + + await findUpdateEmailLink().trigger('click'); + + expect(findUpdateEmail().exists()).toBe(true); + }); + + describe('when the UpdateEmail component triggers verifyToken', () => { + const newEmail = 'new@ema.il'; + + beforeEach(async () => { + enterCode('123'); + await findUpdateEmailLink().trigger('click'); + findUpdateEmail().vm.$emit('verifyToken', newEmail); + }); + + it('hides the UpdateEmail component, shows the updated email address and resets the form', () => { + expect(findUpdateEmail().exists()).toBe(false); + expect(wrapper.text()).toContain(newEmail); + expect(findCodeInput().element.value).toBe(''); + }); + }); + }); }); diff --git a/spec/frontend/sessions/new/components/update_email_spec.js b/spec/frontend/sessions/new/components/update_email_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..37da8b56e9ba425bcc330a7f7ef4c53caad704df --- /dev/null +++ b/spec/frontend/sessions/new/components/update_email_spec.js @@ -0,0 +1,184 @@ +import { GlForm, GlFormInput } from '@gitlab/ui'; +import axios from 'axios'; +import MockAdapter from 'axios-mock-adapter'; +import { mountExtended } from 'helpers/vue_test_utils_helper'; +import { createAlert, VARIANT_SUCCESS } from '~/alert'; +import { HTTP_STATUS_NOT_FOUND, HTTP_STATUS_OK } from '~/lib/utils/http_status'; +import UpdateEmail from '~/sessions/new/components/update_email.vue'; +import { + I18N_CANCEL, + I18N_EMAIL_INVALID, + I18N_UPDATE_EMAIL_SUCCESS, + I18N_GENERIC_ERROR, + SUCCESS_RESPONSE, + FAILURE_RESPONSE, +} from '~/sessions/new/constants'; + +const validEmailAddress = 'valid@ema.il'; +const invalidEmailAddress = 'invalid@ema.il'; + +jest.mock('~/alert'); +jest.mock('~/lib/utils/url_utility', () => ({ + ...jest.requireActual('~/lib/utils/url_utility'), + visitUrl: jest.fn(), +})); +jest.mock('~/lib/utils/forms', () => ({ + ...jest.requireActual('~/lib/utils/forms'), + isEmail: jest.fn().mockImplementation((email) => email === validEmailAddress), +})); + +describe('EmailVerification', () => { + let wrapper; + let axiosMock; + + const defaultPropsData = { + updateEmailPath: '/users/update_email', + }; + + const createComponent = (props = {}) => { + wrapper = mountExtended(UpdateEmail, { + propsData: { ...defaultPropsData, ...props }, + }); + }; + + const findForm = () => wrapper.findComponent(GlForm); + const findEmailInput = () => wrapper.findComponent(GlFormInput); + const findSubmitButton = () => wrapper.find('[type="submit"]'); + const findCancelLink = () => wrapper.findByText(I18N_CANCEL); + const enterEmail = (email) => findEmailInput().setValue(email); + const submitForm = () => findForm().trigger('submit'); + + beforeEach(() => { + axiosMock = new MockAdapter(axios); + createComponent(); + }); + + afterEach(() => { + createAlert.mockClear(); + axiosMock.restore(); + }); + + describe('when successfully verifying the email address', () => { + beforeEach(async () => { + enterEmail(validEmailAddress); + + axiosMock + .onPatch(defaultPropsData.updateEmailPath) + .reply(HTTP_STATUS_OK, { status: SUCCESS_RESPONSE }); + + submitForm(); + await axios.waitForAll(); + }); + + it('shows a successfully updated alert', () => { + expect(createAlert).toHaveBeenCalledWith({ + message: I18N_UPDATE_EMAIL_SUCCESS, + variant: VARIANT_SUCCESS, + }); + }); + + it('emits a verifyToken event with the updated email address', () => { + expect(wrapper.emitted('verifyToken')[0]).toEqual([validEmailAddress]); + }); + }); + + describe('error messages', () => { + beforeEach(() => { + enterEmail(invalidEmailAddress); + }); + + describe('when trying to submit an invalid email address', () => { + it('shows no error message before submitting the form', () => { + expect(wrapper.text()).not.toContain(I18N_EMAIL_INVALID); + expect(findSubmitButton().props('disabled')).toBe(false); + }); + + describe('when submitting the form', () => { + beforeEach(async () => { + submitForm(); + await axios.waitForAll(); + }); + + it('shows an error message and disables the submit button', () => { + expect(wrapper.text()).toContain(I18N_EMAIL_INVALID); + expect(findSubmitButton().props('disabled')).toBe(true); + }); + + describe('when entering a valid email address', () => { + beforeEach(() => { + enterEmail(validEmailAddress); + }); + + it('hides the error message and enables the submit button again', () => { + expect(wrapper.text()).not.toContain(I18N_EMAIL_INVALID); + expect(findSubmitButton().props('disabled')).toBe(false); + }); + }); + }); + }); + + describe('when the server responds with an error message', () => { + const serverErrorMessage = 'server error message'; + + beforeEach(async () => { + enterEmail(validEmailAddress); + + axiosMock + .onPatch(defaultPropsData.updateEmailPath) + .replyOnce(HTTP_STATUS_OK, { status: FAILURE_RESPONSE, message: serverErrorMessage }); + + submitForm(); + await axios.waitForAll(); + }); + + it('shows the error message and disables the submit button', () => { + expect(wrapper.text()).toContain(serverErrorMessage); + expect(findSubmitButton().props('disabled')).toBe(true); + }); + + describe('when entering a valid email address', () => { + beforeEach(async () => { + await enterEmail(''); + enterEmail(validEmailAddress); + }); + + it('hides the error message and enables the submit button again', () => { + expect(wrapper.text()).not.toContain(serverErrorMessage); + expect(findSubmitButton().props('disabled')).toBe(false); + }); + }); + }); + + describe('when the server responds unexpectedly', () => { + it.each` + scenario | statusCode + ${'the response is undefined'} | ${HTTP_STATUS_OK} + ${'the request failed'} | ${HTTP_STATUS_NOT_FOUND} + `(`shows an alert when $scenario`, async ({ statusCode }) => { + enterEmail(validEmailAddress); + + axiosMock.onPatch(defaultPropsData.updateEmailPath).replyOnce(statusCode); + + submitForm(); + + await axios.waitForAll(); + + expect(createAlert).toHaveBeenCalledWith({ + message: I18N_GENERIC_ERROR, + captureError: true, + error: expect.any(Error), + }); + }); + }); + }); + + describe('when clicking the cancel link', () => { + beforeEach(() => { + findCancelLink().trigger('click'); + }); + + it('emits a verifyToken event without an email address', () => { + expect(wrapper.emitted('verifyToken')[0]).toEqual([]); + }); + }); +}); diff --git a/spec/helpers/sessions_helper_spec.rb b/spec/helpers/sessions_helper_spec.rb index f35b6b28de804dd1e76b57cc8314c9964f421ea1..366032100dea34b03522b69837f9208c7c7f79a9 100644 --- a/spec/helpers/sessions_helper_spec.rb +++ b/spec/helpers/sessions_helper_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe SessionsHelper do +RSpec.describe SessionsHelper, feature_category: :system_access do describe '#recently_confirmed_com?' do subject { helper.recently_confirmed_com? } @@ -51,6 +51,55 @@ end end + describe '#unconfirmed_verification_email?', :freeze_time do + using RSpec::Parameterized::TableSyntax + + let(:user) { build_stubbed(:user) } + let(:token_valid_for) { ::Users::EmailVerification::ValidateTokenService::TOKEN_VALID_FOR_MINUTES } + + subject { helper.unconfirmed_verification_email?(user) } + + where(:reset_first_offer?, :unconfirmed_email_present?, :token_valid?, :result) do + true | true | true | true + false | true | true | false + true | false | true | false + true | true | false | false + end + + with_them do + before do + user.email_reset_offered_at = 1.minute.ago unless reset_first_offer? + user.unconfirmed_email = 'unconfirmed@email' if unconfirmed_email_present? + user.confirmation_sent_at = (token_valid? ? token_valid_for - 1 : token_valid_for + 1).minutes.ago + end + + it { is_expected.to eq(result) } + end + end + + describe '#verification_email' do + let(:unconfirmed_email) { 'unconfirmed@email' } + let(:user) { build_stubbed(:user, unconfirmed_email: unconfirmed_email) } + + subject { helper.verification_email(user) } + + context 'when there is an unconfirmed verification email' do + before do + allow(helper).to receive(:unconfirmed_verification_email?).and_return(true) + end + + it { is_expected.to eq(unconfirmed_email) } + end + + context 'when there is no unconfirmed verification email' do + before do + allow(helper).to receive(:unconfirmed_verification_email?).and_return(false) + end + + it { is_expected.to eq(user.email) } + end + end + describe '#verification_data' do let(:user) { build_stubbed(:user) } @@ -58,7 +107,9 @@ expect(helper.verification_data(user)).to eq({ 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, + offer_email_reset: user.email_reset_offered_at.nil?.to_s, + update_email_path: users_update_email_path }) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b43b149157cc189c67226578d4b763da321b5e0f..5d9640440411a3f1a67ad7b758a57326e00f5fb4 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -124,6 +124,9 @@ it { is_expected.to delegate_method(:organization).to(:user_detail).allow_nil } it { is_expected.to delegate_method(:organization=).to(:user_detail).with_arguments(:args).allow_nil } + + it { is_expected.to delegate_method(:email_reset_offered_at).to(:user_detail).allow_nil } + it { is_expected.to delegate_method(:email_reset_offered_at=).to(:user_detail).with_arguments(:args).allow_nil } end describe 'associations' do diff --git a/spec/requests/verifies_with_email_spec.rb b/spec/requests/verifies_with_email_spec.rb index 1c7e1bc9217f9f8a937df7fb7a8d696a7636089a..cc85ebc7adeca0e1755c3998113940db3b401411 100644 --- a/spec/requests/verifies_with_email_spec.rb +++ b/spec/requests/verifies_with_email_spec.rb @@ -21,6 +21,16 @@ expect(mail.to).to match_array([user.email]) expect(mail.subject).to eq(s_('IdentityVerification|Verify your identity')) end + + context 'when an unconfirmed verification email exists' do + let(:new_email) { 'new@email' } + let(:user) { create(:user, unconfirmed_email: new_email, confirmation_sent_at: 1.minute.ago) } + + it 'sends a verification instructions email to the unconfirmed email address' do + mail = ActionMailer::Base.deliveries.find { |d| d.to.include?(new_email) } + expect(mail.subject).to eq(s_('IdentityVerification|Verify your identity')) + end + end end shared_examples_for 'prompt for email verification' do @@ -187,12 +197,42 @@ .and change { AuditEvent.count }.by(1) .and change { AuthenticationEvent.count }.by(1) .and change { user.last_activity_on }.to(Date.today) + .and change { user.email_reset_offered_at }.to(Time.current) end it 'returns the success status and a redirect path' do submit_token expect(json_response).to eq('status' => 'success', 'redirect_path' => users_successful_verification_path) end + + context 'when an unconfirmed verification email exists' do + before do + user.update!(email: new_email) + end + + let(:new_email) { 'new@email' } + + it 'confirms the email' do + expect { submit_token } + .to change { user.reload.email }.to(new_email) + .and change { user.confirmed_at } + .and change { user.unconfirmed_email }.from(new_email).to(nil) + 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') + end + + it 'does not change the email_reset_offered_at field' do + expect { submit_token }.not_to change { user.reload.email_reset_offered_at } + end + + it 'does not confirm the email' do + expect { submit_token }.not_to change { user.reload.email } + end + end end context 'when not completing identity verification and logging in with another account' do @@ -299,6 +339,79 @@ end end + describe 'update_email' do + let(:new_email) { 'new@email' } + + subject(:do_request) { patch(users_update_email_path(user: { email: new_email })) } + + context 'when no verification_user_id session variable exists' do + it 'returns 204 No Content' do + do_request + + expect(response).to have_gitlab_http_status(:no_content) + expect(response.body).to be_empty + end + end + + context 'when a verification_user_id session variable exists' do + before do + stub_session(verification_user_id: user.id) + end + + it 'locks the user' do + do_request + + expect(user.reload.unlock_token).not_to be_nil + expect(user.locked_at).not_to be_nil + end + + it 'sends a changed notification to the primary email and verification instructions to the unconfirmed email' do + perform_enqueued_jobs { do_request } + + sent_mails = ActionMailer::Base.deliveries.map { |mail| { mail.to[0] => mail.subject } } + + expect(sent_mails).to match_array([ + { user.reload.unconfirmed_email => s_('IdentityVerification|Verify your identity') }, + { user.email => 'Email Changed' } + ]) + end + + it 'calls the UpdateEmailService and returns a success response' do + expect_next_instance_of(Users::EmailVerification::UpdateEmailService, user: user) do |instance| + expect(instance).to receive(:execute).with(email: new_email).and_call_original + end + + do_request + + expect(json_response).to eq('status' => 'success') + end + end + + context 'when failing to update the email address' do + let(:service_response) do + { + status: 'failure', + reason: 'the reason', + message: 'the message' + } + end + + before do + stub_session(verification_user_id: user.id) + end + + it 'calls the UpdateEmailService and returns an error response' do + expect_next_instance_of(Users::EmailVerification::UpdateEmailService, user: user) do |instance| + expect(instance).to receive(:execute).with(email: new_email).and_return(service_response) + end + + do_request + + expect(json_response).to eq(service_response.with_indifferent_access) + end + end + end + describe 'successful_verification' do before do allow(user).to receive(:role_required?).and_return(true) # It skips the required signup info before_action diff --git a/spec/services/users/email_verification/update_email_service_spec.rb b/spec/services/users/email_verification/update_email_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..8b4e5b8d7b5e45017042d2e585ec4c822dc334bf --- /dev/null +++ b/spec/services/users/email_verification/update_email_service_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::EmailVerification::UpdateEmailService, feature_category: :instance_resiliency do + let_it_be_with_reload(:user) { create(:user) } + let(:email) { build_stubbed(:user).email } + + describe '#execute' do + subject(:execute_service) { described_class.new(user: user).execute(email: email) } + + context 'when successful' do + it { is_expected.to eq(status: :success) } + + it 'does not send a confirmation instructions email' do + expect { execute_service }.not_to have_enqueued_mail(DeviseMailer, :confirmation_instructions) + end + + it 'sets the unconfirmed_email and confirmation_sent_at fields', :freeze_time do + expect { execute_service } + .to change { user.unconfirmed_email }.from(nil).to(email) + .and change { user.confirmation_sent_at }.from(nil).to(Time.current) + end + end + + context 'when rate limited' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:email_verification_code_send, scope: user).and_return(true) + end + + it 'returns a failure status' do + expect(execute_service).to eq( + { + status: :failure, + reason: :rate_limited, + message: format(s_("IdentityVerification|You've reached the maximum amount of tries. " \ + 'Wait %{interval} and try again.'), interval: 'about 1 hour') + } + ) + end + end + + context 'when email reset has already been offered' do + before do + user.email_reset_offered_at = 1.minute.ago + end + + it 'returns a failure status' do + expect(execute_service).to eq( + { + status: :failure, + reason: :already_offered, + message: s_('IdentityVerification|Email update is only offered once.') + } + ) + end + end + + context 'when email is unchanged' do + let(:email) { user.email } + + it 'returns a failure status' do + expect(execute_service).to eq( + { + status: :failure, + reason: :no_change, + message: s_('IdentityVerification|A code has already been sent to this email address. ' \ + 'Check your spam folder or enter another email address.') + } + ) + end + end + + context 'when email is missing' do + let(:email) { '' } + + it 'returns a failure status' do + expect(execute_service).to eq( + { + status: :failure, + reason: :validation_error, + message: "Email can't be blank" + } + ) + end + end + + context 'when email is not valid' do + let(:email) { 'xxx' } + + it 'returns a failure status' do + expect(execute_service).to eq( + { + status: :failure, + reason: :validation_error, + message: 'Email is invalid' + } + ) + end + end + + context 'when email is already taken' do + before do + create(:user, email: email) + end + + it 'returns a failure status' do + expect(execute_service).to eq( + { + status: :failure, + reason: :validation_error, + message: 'Email has already been taken' + } + ) + end + end + end +end