diff --git a/app/assets/javascripts/vue_shared/components/gl_countdown.vue b/app/assets/javascripts/vue_shared/components/gl_countdown.vue index 1769a283d8c8581ec6e69b2850cdac97c6ad0d3a..0e8ecc36f37f47d8d696bda4009b723f9d353d55 100644 --- a/app/assets/javascripts/vue_shared/components/gl_countdown.vue +++ b/app/assets/javascripts/vue_shared/components/gl_countdown.vue @@ -30,6 +30,11 @@ export default { mounted() { const updateRemainingTime = () => { const remainingMilliseconds = calculateRemainingMilliseconds(this.endDateString); + + if (remainingMilliseconds < 1) { + this.$emit('timer-expired'); + } + this.remainingTime = formatTime(remainingMilliseconds); }; diff --git a/app/models/users/phone_number_validation.rb b/app/models/users/phone_number_validation.rb index f6521eada40df7feeddec88d5e376d0253d0a515..ffb8d3a95a2de709b483678bdd538383d76d8613 100644 --- a/app/models/users/phone_number_validation.rb +++ b/app/models/users/phone_number_validation.rb @@ -4,6 +4,11 @@ module Users class PhoneNumberValidation < ApplicationRecord include IgnorableColumns + # SMS send attempts subsequent to the first one will have wait times of 1 + # min, 3 min, 5 min after each one respectively. Wait time between the fifth + # attempt and so on will be 10 minutes. + SMS_SEND_WAIT_TIMES = [1.minute, 3.minutes, 5.minutes, 10.minutes].freeze + self.primary_key = :user_id self.table_name = 'user_phone_number_validations' @@ -62,5 +67,18 @@ def self.by_reference_id(ref_id) def validated? validated_at.present? end + + def sms_send_allowed_after + return unless Feature.enabled?(:sms_send_wait_time, user) + + # first send is allowed anytime + return if sms_send_count < 1 + return unless sms_sent_at + + max_wait_time = SMS_SEND_WAIT_TIMES.last + wait_time = SMS_SEND_WAIT_TIMES.fetch(sms_send_count - 1, max_wait_time) + + sms_sent_at + wait_time + end end end diff --git a/config/feature_flags/development/sms_send_wait_time.yml b/config/feature_flags/development/sms_send_wait_time.yml new file mode 100644 index 0000000000000000000000000000000000000000..e4cde3477caad37998a45cfd73756fd74dc4f506 --- /dev/null +++ b/config/feature_flags/development/sms_send_wait_time.yml @@ -0,0 +1,8 @@ +--- +name: sms_send_wait_time +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/137850 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/432975 +milestone: '16.8' +type: development +group: group::anti-abuse +default_enabled: false diff --git a/db/migrate/20231124022520_add_sms_sent_at_and_sms_send_count_to_phone_number_validations.rb b/db/migrate/20231124022520_add_sms_sent_at_and_sms_send_count_to_phone_number_validations.rb new file mode 100644 index 0000000000000000000000000000000000000000..40508d8da6e5217fa444c11e57b7b087d892e00c --- /dev/null +++ b/db/migrate/20231124022520_add_sms_sent_at_and_sms_send_count_to_phone_number_validations.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddSmsSentAtAndSmsSendCountToPhoneNumberValidations < Gitlab::Database::Migration[2.2] + milestone '16.8' + enable_lock_retries! + + def up + add_column :user_phone_number_validations, :sms_sent_at, :datetime_with_timezone, null: true + add_column :user_phone_number_validations, :sms_send_count, :smallint, default: 0, null: false + end + + def down + remove_column :user_phone_number_validations, :sms_sent_at, if_exists: true + remove_column :user_phone_number_validations, :sms_send_count, if_exists: true + end +end diff --git a/db/schema_migrations/20231124022520 b/db/schema_migrations/20231124022520 new file mode 100644 index 0000000000000000000000000000000000000000..408c6f976f8582b2360eaad600be88982c9dd590 --- /dev/null +++ b/db/schema_migrations/20231124022520 @@ -0,0 +1 @@ +bfa32c41d867fa4de24ac0a81d1f99f14e868b2c5bd453f799e1a3b3eebd1d51 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 55d358c89762d6214eb31bf928442eeabf56c366..c74d4b69bafe64d0fd8553e801587647bdc37046 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -24978,6 +24978,8 @@ CREATE TABLE user_phone_number_validations ( country text NOT NULL, phone_number text NOT NULL, telesign_reference_xid text, + sms_sent_at timestamp with time zone, + sms_send_count smallint DEFAULT 0 NOT NULL, CONSTRAINT check_193736da9f CHECK ((char_length(country) <= 3)), CONSTRAINT check_d2f31fc815 CHECK ((char_length(phone_number) <= 12)), CONSTRAINT check_d7af4d3eb5 CHECK ((char_length(telesign_reference_xid) <= 255)) diff --git a/ee/app/assets/javascripts/users/identity_verification/components/international_phone_input.vue b/ee/app/assets/javascripts/users/identity_verification/components/international_phone_input.vue index 263513f8d0865bd035c1e1ecb9f7c49b4fd1a644..870beb871c5822e3220f00a5ca762dfcdecd9fdd 100644 --- a/ee/app/assets/javascripts/users/identity_verification/components/international_phone_input.vue +++ b/ee/app/assets/javascripts/users/identity_verification/components/international_phone_input.vue @@ -8,7 +8,10 @@ import { GlFormInput, GlIcon, GlButton, + GlSprintf, } from '@gitlab/ui'; +import GlCountdown from '~/vue_shared/components/gl_countdown.vue'; + import { createAlert } from '~/alert'; import { s__, n__ } from '~/locale'; import axios from '~/lib/utils/axios_utils'; @@ -32,6 +35,8 @@ export default { GlFormInput, GlIcon, GlButton, + GlSprintf, + GlCountdown, }, inject: { phoneNumber: { @@ -52,6 +57,15 @@ export default { required: false, default: () => {}, }, + sendCodeAllowed: { + type: Boolean, + required: true, + }, + sendCodeAllowedAfter: { + type: String, + required: false, + default: '', + }, }, i18n: { phoneNumber: s__('IdentityVerification|Phone number'), @@ -62,6 +76,7 @@ export default { ), success: s__("IdentityVerification|We've sent a verification code to +%{phoneNumber}"), sendCode: s__('IdentityVerification|Send code'), + sendCodeIn: s__('IdentityVerification|Send code in %{timer}'), I18N_GENERIC_ERROR, }, data() { @@ -119,7 +134,10 @@ export default { }, isSubmitButtonDisabled() { return ( - this.disableSubmitButton || this.relatedToBannedUser || !this.form.fields.phoneNumber.state + this.disableSubmitButton || + this.relatedToBannedUser || + !this.form.fields.phoneNumber.state || + !this.sendCodeAllowed ); }, countryDropdownToggleText() { @@ -163,7 +181,7 @@ export default { this.isLoading = false; }); }, - handleSendCodeResponse() { + handleSendCodeResponse({ data }) { const { countryId, internationalDialCode, inputPhoneNumber } = this; this.$emit('verification-attempt'); @@ -172,10 +190,12 @@ export default { country: countryId, internationalDialCode, number: inputPhoneNumber, + sendAllowedAfter: data.send_allowed_after, }); }, handleError(error) { const reason = error.response?.data?.reason; + if (reason === UNKNOWN_TELESIGN_ERROR) { this.$emit('skip-verification'); return; @@ -194,6 +214,9 @@ export default { onCountriesSearch(searchTerm) { this.countriesSearchTerm = searchTerm.trim().toLowerCase(); }, + onTimerExpired() { + this.$emit('timer-expired'); + }, }, apollo: { countries: { @@ -279,10 +302,16 @@ export default { type="submit" block class="gl-mt-5" + data-testid="submit-btn" :disabled="isSubmitButtonDisabled" :loading="isLoading" > - {{ $options.i18n.sendCode }} + <template v-if="sendCodeAllowed">{{ $options.i18n.sendCode }}</template> + <gl-sprintf v-else :message="$options.i18n.sendCodeIn"> + <template #timer> + <gl-countdown :end-date-string="sendCodeAllowedAfter" @timer-expired="onTimerExpired" /> + </template> + </gl-sprintf> </gl-button> </gl-form> </template> diff --git a/ee/app/assets/javascripts/users/identity_verification/components/phone_verification.vue b/ee/app/assets/javascripts/users/identity_verification/components/phone_verification.vue index 20e048efcfb2e14131baca992a2ae4ebdef85d95..aaeb098a57e2687c8d591cc7e49003189c759fd7 100644 --- a/ee/app/assets/javascripts/users/identity_verification/components/phone_verification.vue +++ b/ee/app/assets/javascripts/users/identity_verification/components/phone_verification.vue @@ -1,6 +1,8 @@ <script> import { GlButton } from '@gitlab/ui'; import { s__ } from '~/locale'; +import { isValidDateString } from '~/lib/utils/datetime_range'; +import { calculateRemainingMilliseconds } from '~/lib/utils/datetime_utility'; import InternationalPhoneInput from './international_phone_input.vue'; import VerifyPhoneVerificationCode from './verify_phone_verification_code.vue'; import Captcha from './identity_verification_captcha.vue'; @@ -18,15 +20,27 @@ export default { return { stepIndex: 1, latestPhoneNumber: {}, + sendAllowedAfter: null, verificationAttempts: 0, disableSubmitButton: false, captchaData: {}, }; }, + computed: { + sendCodeAllowed() { + if (!this.sendAllowedAfter) return true; + + return calculateRemainingMilliseconds(new Date(this.sendAllowedAfter).getTime()) < 1; + }, + }, + mounted() { + this.setSendAllowedOn(this.phoneNumber?.sendAllowedAfter); + }, methods: { - goToStepTwo(phoneNumber) { + goToStepTwo({ sendAllowedAfter, ...phoneNumber }) { this.stepIndex = 2; this.latestPhoneNumber = phoneNumber; + this.setSendAllowedOn(sendAllowedAfter); }, goToStepOne() { this.stepIndex = 1; @@ -48,6 +62,12 @@ export default { this.disableSubmitButton = true; this.captchaData = {}; }, + setSendAllowedOn(sendAllowedAfter) { + this.sendAllowedAfter = isValidDateString(sendAllowedAfter) ? sendAllowedAfter : null; + }, + resetTimer() { + this.setSendAllowedOn(null); + }, }, i18n: { verifyWithCreditCard: s__('IdentityVerification|Verify with a credit card instead?'), @@ -60,6 +80,9 @@ export default { v-if="stepIndex == 1" :disable-submit-button="disableSubmitButton" :additional-request-params="captchaData" + :send-code-allowed="sendCodeAllowed" + :send-code-allowed-after="sendAllowedAfter" + @timer-expired="resetTimer" @next="goToStepTwo" @verification-attempt="increaseVerificationAttempts" @skip-verification="setVerified" @@ -82,6 +105,10 @@ export default { :latest-phone-number="latestPhoneNumber" :disable-submit-button="disableSubmitButton" :additional-request-params="captchaData" + :send-code-allowed="sendCodeAllowed" + :send-code-allowed-after="sendAllowedAfter" + @timer-expired="resetTimer" + @resent="setSendAllowedOn" @back="goToStepOne" @verification-attempt="increaseVerificationAttempts" @verified="setVerified" diff --git a/ee/app/assets/javascripts/users/identity_verification/components/verify_phone_verification_code.vue b/ee/app/assets/javascripts/users/identity_verification/components/verify_phone_verification_code.vue index 9f5d04ebc50ae644d9e41679b3064063af39f4f9..5e62685318353e4cc091c491c7172e075c4de9a5 100644 --- a/ee/app/assets/javascripts/users/identity_verification/components/verify_phone_verification_code.vue +++ b/ee/app/assets/javascripts/users/identity_verification/components/verify_phone_verification_code.vue @@ -1,5 +1,6 @@ <script> import { GlForm, GlFormGroup, GlFormInput, GlIcon, GlSprintf, GlLink, GlButton } from '@gitlab/ui'; +import GlCountdown from '~/vue_shared/components/gl_countdown.vue'; import { s__, sprintf } from '~/locale'; import { createAlert, VARIANT_SUCCESS } from '~/alert'; @@ -17,6 +18,7 @@ export default { GlSprintf, GlLink, GlButton, + GlCountdown, }, i18n: { verificationCode: s__('IdentityVerification|Verification code'), @@ -24,6 +26,9 @@ export default { noCode: s__( "IdentityVerification|Didn't receive a code? %{codeLinkStart}Send a new code%{codeLinkEnd} or %{phoneLinkStart}enter a new phone number%{phoneLinkEnd}", ), + resendCodeIn: s__( + "IdentityVerification|Didn't receive a code? Send a new code in %{timer} or %{phoneLinkStart}enter a new phone number%{phoneLinkEnd}", + ), resendSuccess: s__('IdentityVerification|We sent a new code to +%{phoneNumber}'), verifyButton: s__('IdentityVerification|Verify phone number'), }, @@ -44,6 +49,15 @@ export default { required: false, default: () => {}, }, + sendCodeAllowed: { + type: Boolean, + required: true, + }, + sendCodeAllowedAfter: { + type: String, + required: false, + default: '', + }, }, data() { return { @@ -110,8 +124,9 @@ export default { this.isLoading = false; }); }, - handleResendCodeResponse() { + handleResendCodeResponse({ data }) { this.$emit('verification-attempt'); + this.$emit('resent', data?.send_allowed_after); this.alert = createAlert({ message: sprintf(this.$options.i18n.resendSuccess, { @@ -142,6 +157,9 @@ export default { resetForm() { this.form.fields.verificationCode = { value: '', state: null, feedback: '' }; }, + onTimerExpired() { + this.$emit('timer-expired'); + }, }, }; </script> @@ -171,9 +189,17 @@ export default { <div v-if="!disableSubmitButton" class="gl-font-sm gl-text-secondary"> <gl-icon name="information-o" :size="12" class="gl-mt-2" /> - <gl-sprintf :message="$options.i18n.noCode"> + <gl-sprintf v-if="sendCodeAllowed" :message="$options.i18n.noCode"> <template #codeLink="{ content }"> - <gl-link @click="resendCode">{{ content }}</gl-link> + <gl-link data-testid="resend-code-link" @click="resendCode">{{ content }}</gl-link> + </template> + <template #phoneLink="{ content }"> + <gl-link @click="goBack">{{ content }}</gl-link> + </template> + </gl-sprintf> + <gl-sprintf v-else :message="$options.i18n.resendCodeIn"> + <template #timer> + <gl-countdown :end-date-string="sendCodeAllowedAfter" @timer-expired="onTimerExpired" /> </template> <template #phoneLink="{ content }"> <gl-link @click="goBack">{{ content }}</gl-link> diff --git a/ee/app/controllers/users/identity_verification_controller.rb b/ee/app/controllers/users/identity_verification_controller.rb index 79f152b7f4d58c5d564e0d786166908e72056832..5ed3000e1fb41b3928febeab2aafdf1a7d7aaac8 100644 --- a/ee/app/controllers/users/identity_verification_controller.rb +++ b/ee/app/controllers/users/identity_verification_controller.rb @@ -93,7 +93,8 @@ def send_phone_verification_code end log_event(:phone, :sent_phone_verification_code) - render json: { status: :success } + + render json: { status: :success }.merge(result.payload) end def verify_phone_verification_code diff --git a/ee/app/helpers/users/identity_verification_helper.rb b/ee/app/helpers/users/identity_verification_helper.rb index 089ca3640670e9928b02f5b7ade8f2bb2f7307e9..17b14f7dfdb0f0bd35a3722818975a76874350ce 100644 --- a/ee/app/helpers/users/identity_verification_helper.rb +++ b/ee/app/helpers/users/identity_verification_helper.rb @@ -72,7 +72,7 @@ def email_verification_data(user) end def phone_number_verification_data(user) - paths = { + data = { send_code_path: send_phone_verification_code_identity_verification_path, verify_code_path: verify_phone_verification_code_identity_verification_path, enable_arkose_challenge: enable_arkose_challenge?(:phone).to_s, @@ -80,14 +80,15 @@ def phone_number_verification_data(user) show_recaptcha_challenge: show_recaptcha_challenge?.to_s } - phone_number_validation = user.phone_number_validation - return paths unless phone_number_validation.present? + record = user.phone_number_validation + return data unless record - paths.merge( + data.merge( { - country: phone_number_validation.country, - international_dial_code: phone_number_validation.international_dial_code, - number: phone_number_validation.phone_number + country: record.country, + international_dial_code: record.international_dial_code, + number: record.phone_number, + send_allowed_after: record.sms_send_allowed_after } ) end diff --git a/ee/app/services/phone_verification/users/send_verification_code_service.rb b/ee/app/services/phone_verification/users/send_verification_code_service.rb index e3d4f7bc4def6b323db39fcc72ead75550ca4d0a..3db7db35308f27fe3cb699f25598b842af0fd07c 100644 --- a/ee/app/services/phone_verification/users/send_verification_code_service.rb +++ b/ee/app/services/phone_verification/users/send_verification_code_service.rb @@ -30,7 +30,13 @@ def execute return error_related_to_banned_user end - return error_rate_limited if rate_limited? + if rate_limited? + reset_sms_send_data + return error_rate_limited + end + + return error_send_not_allowed unless send_allowed? + return error_high_risk_number if related_to_banned_user? risk_result = ::PhoneVerification::TelesignClient::RiskScoreService.new( phone_number: phone_number, @@ -84,6 +90,12 @@ def error_in_params ) end + def reset_sms_send_data + return unless Feature.enabled?(:sms_send_wait_time, user) + + record.update!(sms_send_count: 0, sms_sent_at: nil) + end + def error_rate_limited interval_in_seconds = ::Gitlab::ApplicationRateLimiter.rate_limits[:phone_verification_send_code][:interval] interval = distance_of_time_in_words(interval_in_seconds) @@ -114,6 +126,15 @@ def error_related_to_banned_user ) end + def send_allowed? + sms_send_allowed_after = @record.sms_send_allowed_after + sms_send_allowed_after ? (Time.current > sms_send_allowed_after) : true + end + + def error_send_not_allowed + ServiceResponse.error(message: 'Sending not allowed at this time', reason: :send_not_allowed) + end + def error_downstream_service(result) force_verify if result.reason == TELESIGN_ERROR @@ -151,11 +172,19 @@ def success(risk_result, send_code_result) end attrs = { telesign_reference_xid: send_code_result[:telesign_reference_xid] } + + if Feature.enabled?(:sms_send_wait_time, user) + attrs.merge!({ + sms_sent_at: Time.current, + sms_send_count: record.sms_send_count + 1 + }) + end + attrs[:risk_score] = risk_result[:risk_score] if Feature.enabled?(:telesign_intelligence, type: :ops) record.update!(attrs) - ServiceResponse.success + ServiceResponse.success(payload: { send_allowed_after: record.sms_send_allowed_after }) end def store_risk_score(risk_score) diff --git a/ee/spec/frontend/users/identity_verification/components/international_phone_input_spec.js b/ee/spec/frontend/users/identity_verification/components/international_phone_input_spec.js index c0e7c264bdbf9afde6e0091e46da2c8b2e26d16f..95d447cf143ae503d23a7b7237c189b8a9528681 100644 --- a/ee/spec/frontend/users/identity_verification/components/international_phone_input_spec.js +++ b/ee/spec/frontend/users/identity_verification/components/international_phone_input_spec.js @@ -1,4 +1,4 @@ -import { GlForm, GlButton, GlFormInput, GlInputGroupText } from '@gitlab/ui'; +import { GlForm, GlFormInput, GlInputGroupText } from '@gitlab/ui'; import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import axios from 'axios'; @@ -13,6 +13,7 @@ import countriesQuery from 'ee/subscriptions/graphql/queries/countries.query.gra import countriesResolver from 'ee/subscriptions/buy_addons_shared/graphql/resolvers'; import InternationalPhoneInput from 'ee/users/identity_verification/components/international_phone_input.vue'; +import GlCountdown from '~/vue_shared/components/gl_countdown.vue'; import { I18N_PHONE_NUMBER_LENGTH_ERROR, @@ -40,6 +41,7 @@ describe('International Phone input component', () => { const findPhoneNumberFormGroup = () => wrapper.findByTestId('phone-number-form-group'); const findPhoneNumberInput = () => wrapper.findComponent(GlFormInput); const findInternationalDialCode = () => wrapper.findComponent(GlInputGroupText); + const findCountdown = () => wrapper.findComponent(GlCountdown); const countryText = (country) => `${country.flag} ${country.name} (+${country.internationalDialCode})`; @@ -47,7 +49,7 @@ describe('International Phone input component', () => { const enterPhoneNumber = (value) => findPhoneNumberInput().vm.$emit('input', value); const submitForm = () => findForm().vm.$emit('submit', { preventDefault: jest.fn() }); - const findSubmitButton = () => wrapper.findComponent(GlButton); + const findSubmitButton = () => wrapper.findByTestId('submit-btn'); const createMockApolloProvider = () => { const mockResolvers = { countriesResolver }; @@ -60,18 +62,23 @@ describe('International Phone input component', () => { return mockApollo; }; - const createComponent = (provide = {}, props = {}, mountFn = shallowMountExtended) => { + const createComponent = ( + { provide, props } = { provide: {}, props: {} }, + mountFn = shallowMountExtended, + ) => { wrapper = mountFn(InternationalPhoneInput, { apolloProvider: createMockApolloProvider(), + propsData: { + sendCodeAllowed: true, + sendCodeAllowedAfter: '2000-01-01T00:00:00Z', + ...props, + }, provide: { phoneNumber: { sendCodePath: SEND_CODE_PATH, ...provide, }, }, - propsData: { - ...props, - }, }); }; @@ -121,7 +128,7 @@ describe('International Phone input component', () => { }); it('should render international dial code', () => { - createComponent({}, {}, mountExtended); + createComponent({}, mountExtended); expect(findInternationalDialCode().text()).toBe(`+${mockCountry1.internationalDialCode}`); }); @@ -141,7 +148,7 @@ describe('International Phone input component', () => { }); it('updates country field with the name of selected country', async () => { - createComponent({}, {}, mountExtended); + createComponent({}, mountExtended); findCountrySelect().vm.$emit('select', 'AU'); await nextTick(); @@ -151,7 +158,7 @@ describe('International Phone input component', () => { }); it('should render injected value', () => { - createComponent({ country: 'AU' }); + createComponent({ provide: { country: 'AU' } }); expect(findCountrySelect().attributes('selected')).toBe('AU'); expect(findCountrySelect().props('toggleText')).toBe(countryText(mockCountry2)); @@ -200,17 +207,63 @@ describe('International Phone input component', () => { it('should render injected value', () => { const number = '555'; - createComponent({ number }); + createComponent({ provide: { number } }); expect(findPhoneNumberInput().attributes('value')).toBe(number); }); }); + describe('Submit button', () => { + describe('when send is allowed', () => { + it('does not render "Send code in" text and a GlCountdown', () => { + createComponent({}, mountExtended); + + expect(wrapper.findByText(/Send code in/).exists()).toBe(false); + expect(findCountdown().exists()).toBe(false); + }); + }); + + describe('when send is not allowed', () => { + beforeEach(() => { + jest + .spyOn(Date, 'now') + .mockImplementation(() => new Date('2000-01-01T00:00:00Z').getTime()); + + createComponent( + { props: { sendCodeAllowed: false, sendCodeAllowedAfter: '2000-01-01T01:02:03Z' } }, + mountExtended, + ); + }); + + it('renders the correct text and GlCountdown', () => { + expect(wrapper.findByText(/Send code in/).exists()).toBe(true); + expect(findCountdown().exists()).toBe(true); + }); + + it('is disabled', async () => { + enterPhoneNumber('1800134678'); + await nextTick(); + + expect(findSubmitButton().attributes('disabled')).not.toBeUndefined(); + }); + + describe('when GlCountdown emits a "timer-expired" event', () => { + it('emits re-emits the event', () => { + findCountdown().vm.$emit('timer-expired'); + + expect(wrapper.emitted('timer-expired')).toStrictEqual([[]]); + }); + }); + }); + }); + describe('Sending verification code', () => { describe('when request is successful', () => { + const data = { success: true, send_allowed_after: '2000-01-01T01:02:03Z' }; + beforeEach(() => { - axiosMock.onPost(SEND_CODE_PATH).reply(HTTP_STATUS_OK, { success: true }); + axiosMock.onPost(SEND_CODE_PATH).reply(HTTP_STATUS_OK, data); - createComponent({}, { additionalRequestParams: { captcha_token: '1234' } }); + createComponent({ props: { additionalRequestParams: { captcha_token: '1234' } } }); enterPhoneNumber('555'); submitForm(); @@ -232,14 +285,16 @@ describe('International Phone input component', () => { ); }); - it('emits next event with user entered phone number', () => { - expect(wrapper.emitted('next')).toHaveLength(1); - expect(wrapper.emitted('next')[0]).toEqual([ - { - country: 'US', - internationalDialCode: '1', - number: '555', - }, + it('emits next event with the correct payload', () => { + expect(wrapper.emitted('next')).toStrictEqual([ + [ + { + country: 'US', + internationalDialCode: '1', + number: '555', + sendAllowedAfter: data.send_allowed_after, + }, + ], ]); }); }); @@ -324,7 +379,9 @@ describe('International Phone input component', () => { describe('Captcha', () => { describe('when disableSubmitButton is true', () => { beforeEach(() => { - createComponent({}, { disableSubmitButton: true }); + createComponent({ + props: { disableSubmitButton: true }, + }); enterPhoneNumber('555'); return waitForPromises(); diff --git a/ee/spec/frontend/users/identity_verification/components/phone_verification_spec.js b/ee/spec/frontend/users/identity_verification/components/phone_verification_spec.js index fe92ed60cafbffb5ca43ff59026b55a01fdce4a3..9c2faf4fbc6164badf2d957efa0b45960dc3a9bf 100644 --- a/ee/spec/frontend/users/identity_verification/components/phone_verification_spec.js +++ b/ee/spec/frontend/users/identity_verification/components/phone_verification_spec.js @@ -5,6 +5,11 @@ import PhoneVerification from 'ee/users/identity_verification/components/phone_v import InternationalPhoneInput from 'ee/users/identity_verification/components/international_phone_input.vue'; import VerifyPhoneVerificationCode from 'ee/users/identity_verification/components/verify_phone_verification_code.vue'; import Captcha from 'ee/users/identity_verification/components/identity_verification_captcha.vue'; +import { calculateRemainingMilliseconds } from '~/lib/utils/datetime_utility'; + +jest.mock('~/lib/utils/datetime_utility', () => ({ + calculateRemainingMilliseconds: jest.fn(), +})); describe('Phone Verification component', () => { let wrapper; @@ -38,23 +43,90 @@ describe('Phone Verification component', () => { }; beforeEach(() => { + calculateRemainingMilliseconds.mockReturnValue(1000); + createComponent(); }); describe('When component loads', () => { - it('should display InternationalPhoneInput component', () => { - expect(findInternationalPhoneInput().exists()).toBe(true); + const expectedProps = { + sendCodeAllowed: true, + sendCodeAllowedAfter: null, + }; + + it('renders InternationalPhoneInput component with the correct props', () => { + const component = findInternationalPhoneInput(); + expect(component.exists()).toBe(true); + expect(component.props()).toMatchObject(expectedProps); }); - it('should hide VerifyPhoneVerificationCode component', () => { + it('does not render VerifyPhoneVerificationCode component', () => { expect(findVerifyCodeInput().exists()).toBe(false); }); + + describe('rendered InternationalPhoneInput component', () => { + const expectCorrectProps = (expected) => { + it('has the correct props', () => { + expect(findInternationalPhoneInput().props()).toMatchObject(expected); + }); + }; + + describe('when sendAllowedAfter is a valid timestamp in the future', () => { + beforeEach(() => { + createComponent({ phoneNumber: { sendAllowedAfter: '2000-01-01T01:02:03Z' } }); + }); + + expectCorrectProps({ + sendCodeAllowed: false, + sendCodeAllowedAfter: '2000-01-01T01:02:03Z', + }); + + describe('when InternationalPhoneInput emits a `timer-expired` event', () => { + beforeEach(async () => { + findInternationalPhoneInput().vm.$emit('timer-expired'); + await nextTick(); + }); + + expectCorrectProps(expectedProps); + }); + }); + + describe('when sendAllowedAfter is a valid timestamp in the past', () => { + beforeEach(() => { + calculateRemainingMilliseconds.mockReturnValue(0); + createComponent({ phoneNumber: { sendAllowedAfter: '2000-01-01T01:02:03Z' } }); + }); + + expectCorrectProps({ + sendCodeAllowed: true, + sendCodeAllowedAfter: '2000-01-01T01:02:03Z', + }); + }); + + describe('when sendAllowedAfter is not a valid timestamp', () => { + beforeEach(() => { + createComponent({ phoneNumber: { sendAllowedAfter: 'not-a-date' } }); + }); + + expectCorrectProps(expectedProps); + }); + }); }); describe('On next', () => { - beforeEach(() => { - findInternationalPhoneInput().vm.$emit('next', PHONE_NUMBER); - return nextTick(); + beforeEach(async () => { + await findInternationalPhoneInput().vm.$emit('next', { + ...PHONE_NUMBER, + sendAllowedAfter: '2000-01-01T01:02:03Z', + }); + }); + + it('updates sendCodeAllowed and sendCodeAllowedAfter props of VerifyPhoneVerificationCode', () => { + const expectedProps = { + sendCodeAllowed: false, + sendCodeAllowedAfter: '2000-01-01T01:02:03Z', + }; + expect(findVerifyCodeInput().props()).toMatchObject(expectedProps); }); it('should hide InternationalPhoneInput component', () => { @@ -66,6 +138,34 @@ describe('Phone Verification component', () => { expect(findVerifyCodeInput().props()).toMatchObject({ latestPhoneNumber: PHONE_NUMBER }); }); + describe('when VerifyPhoneVerificationCode emits a `timer-expired` event', () => { + beforeEach(async () => { + findVerifyCodeInput().vm.$emit('timer-expired'); + await nextTick(); + }); + + it('has the correct props', () => { + expect(findVerifyCodeInput().props()).toMatchObject({ + sendCodeAllowed: true, + sendCodeAllowedAfter: null, + }); + }); + }); + + describe('when VerifyPhoneVerificationCode emits a `resent` event', () => { + beforeEach(async () => { + findVerifyCodeInput().vm.$emit('resent', '2001-12-31:00:00Z'); + await nextTick(); + }); + + it('has the correct props', () => { + expect(findVerifyCodeInput().props()).toMatchObject({ + sendCodeAllowed: false, + sendCodeAllowedAfter: '2001-12-31:00:00Z', + }); + }); + }); + describe('On back', () => { beforeEach(() => { findVerifyCodeInput().vm.$emit('back'); diff --git a/ee/spec/frontend/users/identity_verification/components/verify_phone_verification_code_spec.js b/ee/spec/frontend/users/identity_verification/components/verify_phone_verification_code_spec.js index f95a78b44fa8a66033def936ded229b36e9eedc9..b8a4d18fc145c353cd2514611c3d3b0a39af8f4b 100644 --- a/ee/spec/frontend/users/identity_verification/components/verify_phone_verification_code_spec.js +++ b/ee/spec/frontend/users/identity_verification/components/verify_phone_verification_code_spec.js @@ -3,6 +3,7 @@ import { nextTick } from 'vue'; import axios from 'axios'; import MockAdapter from 'axios-mock-adapter'; +import GlCountdown from '~/vue_shared/components/gl_countdown.vue'; import { sprintf } from '~/locale'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; @@ -44,7 +45,9 @@ describe('Verify phone verification code input component', () => { const findResendCodeButton = () => wrapper.findByText('Send a new code'); const resendCode = () => findResendCodeButton().vm.$emit('click'); - const createComponent = (props = {}) => { + const findCountdown = () => wrapper.findComponent(GlCountdown); + + const createComponent = ({ props } = { props: {} }) => { wrapper = shallowMountExtended(VerifyPhoneVerificationCode, { propsData: { latestPhoneNumber: { @@ -52,6 +55,8 @@ describe('Verify phone verification code input component', () => { internationalDialCode: INTERNATIONAL_DIAL_CODE, number: NUMBER, }, + sendCodeAllowed: true, + sendCodeAllowedAfter: '2000-01-01T00:00:00Z', ...props, }, provide: { @@ -140,11 +145,49 @@ describe('Verify phone verification code input component', () => { }); describe('Re-sending code', () => { + describe('when send is allowed', () => { + it('does not render "Send a new code in" text and a GlCountdown', () => { + expect(wrapper.findByText(/Send a new code in/).exists()).toBe(false); + expect(findCountdown().exists()).toBe(false); + }); + }); + + describe('when send is not allowed', () => { + beforeEach(() => { + jest + .spyOn(Date, 'now') + .mockImplementation(() => new Date('2000-01-01T00:00:00Z').getTime()); + + createComponent({ + props: { sendCodeAllowed: false, sendCodeAllowedAfter: '2000-01-01T01:02:03Z' }, + }); + }); + + it('renders the correct text and GlCountdown', () => { + expect(wrapper.findByText(/Send a new code in/).exists()).toBe(true); + expect(findCountdown().exists()).toBe(true); + }); + + it('does not render the resend code link', () => { + expect(wrapper.findByTestId('resend-code-link').exists()).toBe(false); + }); + + describe('when GlCountdown emits a "timer-expired" event', () => { + it('emits re-emits the event', () => { + findCountdown().vm.$emit('timer-expired'); + + expect(wrapper.emitted('timer-expired')).toStrictEqual([[]]); + }); + }); + }); + describe('when request is successful', () => { + const data = { success: true, send_allowed_after: '2000-01-01T01:02:03Z' }; + beforeEach(() => { - axiosMock.onPost(SEND_CODE_PATH).reply(HTTP_STATUS_OK, { success: true }); + axiosMock.onPost(SEND_CODE_PATH).reply(HTTP_STATUS_OK, data); - createComponent({ additionalRequestParams: { captcha_token: '1234' } }); + createComponent({ props: { additionalRequestParams: { captcha_token: '1234' } } }); resendCode(); return waitForPromises(); @@ -165,6 +208,10 @@ describe('Verify phone verification code input component', () => { ); }); + it('emits `resent` event with the correct data', () => { + expect(wrapper.emitted('resent')).toStrictEqual([[data.send_allowed_after]]); + }); + it('renders success message', () => { expect(createAlert).toHaveBeenCalledWith({ message: sprintf('We sent a new code to +%{phoneNumber}', { @@ -204,7 +251,7 @@ describe('Verify phone verification code input component', () => { beforeEach(() => { axiosMock.onPost(VERIFY_CODE_PATH).reply(HTTP_STATUS_OK, { success: true }); - createComponent({ additionalRequestParams: { captcha_token: '1234' } }); + createComponent({ props: { additionalRequestParams: { captcha_token: '1234' } } }); enterCode('123'); submitForm(); @@ -279,7 +326,9 @@ describe('Verify phone verification code input component', () => { describe('when disableSubmitButton is true', () => { beforeEach(() => { createComponent({ - disableSubmitButton: true, + props: { + disableSubmitButton: true, + }, }); enterCode('000'); @@ -292,6 +341,26 @@ describe('Verify phone verification code input component', () => { expect(findResendCodeButton().exists()).toBe(false); }); }); + + describe('when arkose challenge is shown and solved', () => { + beforeEach(() => { + createComponent({ + props: { + arkoseChallengeShown: true, + arkoseChallengeSolved: true, + }, + }); + + enterCode('000'); + return waitForPromises(); + }); + + it('should enable the verify, go back and resend buttons', () => { + expect(findVerifyCodeButton().attributes('disabled')).toBe(undefined); + expect(findGoBackLink().exists()).toBe(true); + expect(findResendCodeButton().exists()).toBe(true); + }); + }); }); }); }); diff --git a/ee/spec/helpers/users/identity_verification_helper_spec.rb b/ee/spec/helpers/users/identity_verification_helper_spec.rb index ae94e69c7c8d172b219237088846c3e118382163..0083537a2824f57f31bcff12c4c73d0f7fcbea8b 100644 --- a/ee/spec/helpers/users/identity_verification_helper_spec.rb +++ b/ee/spec/helpers/users/identity_verification_helper_spec.rb @@ -39,13 +39,14 @@ end context 'when phone number for user exists' do - let_it_be(:phone_number_validation) { create(:phone_number_validation, user: user) } + let_it_be(:record) { create(:phone_number_validation, user: user) } it 'returns the expected data' do phone_number_data = expected_data[:phone_number].merge({ - country: phone_number_validation.country, - international_dial_code: phone_number_validation.international_dial_code, - number: phone_number_validation.phone_number + country: record.country, + international_dial_code: record.international_dial_code, + number: record.phone_number, + send_allowed_after: record.sms_send_allowed_after }) expect(data[:data]).to eq(expected_data.merge({ phone_number: phone_number_data }).to_json) diff --git a/ee/spec/requests/users/identity_verification_controller_spec.rb b/ee/spec/requests/users/identity_verification_controller_spec.rb index b17df0ebeb3e5df8f37fe17cb859b2d375cb0f03..f3f9e8bfbe4e118ff3d8f5d0b2329c590564d0dc 100644 --- a/ee/spec/requests/users/identity_verification_controller_spec.rb +++ b/ee/spec/requests/users/identity_verification_controller_spec.rb @@ -638,7 +638,7 @@ describe 'POST send_phone_verification_code' do let_it_be(:unconfirmed_user) { create(:user, :medium_risk) } let_it_be(:user) { unconfirmed_user } - let_it_be(:service_response) { ServiceResponse.success } + let_it_be(:service_response) { ServiceResponse.success(payload: { container: 'contents' }) } let_it_be(:params) do { identity_verification: { country: 'US', international_dial_code: '1', phone_number: '555' } } end @@ -663,7 +663,8 @@ it 'responds with status 200 OK' do do_request - expect(response.body).to eq({ status: :success }.to_json) + expected_json = { status: :success }.merge(service_response.payload).to_json + expect(response.body).to eq(expected_json) end it_behaves_like 'logs and tracks the event', :phone, :sent_phone_verification_code diff --git a/ee/spec/services/phone_verification/users/send_verification_code_service_spec.rb b/ee/spec/services/phone_verification/users/send_verification_code_service_spec.rb index 5c37978e558b50d299650eda028a8115f1444d89..00574cf4e52e2b7b190077b85c613a0b17bd474d 100644 --- a/ee/spec/services/phone_verification/users/send_verification_code_service_spec.rb +++ b/ee/spec/services/phone_verification/users/send_verification_code_service_spec.rb @@ -46,13 +46,27 @@ end context 'when user has reached max verification attempts' do - let(:record) { create(:phone_number_validation, user: user) } + let_it_be(:record) { create(:phone_number_validation, sms_send_count: 1, sms_sent_at: Time.current, user: user) } before do allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) .with(:phone_verification_send_code, scope: user).and_return(true) end + it 'resets sms_send_count and sms_sent_at' do + expect { service.execute }.to change { [record.reload.sms_send_count, record.reload.sms_sent_at] }.to([0, nil]) + end + + context 'when sms_send_wait_time feature flag is disabled' do + before do + stub_feature_flags(sms_send_wait_time: false) + end + + it 'does not reset sms_send_count and sms_sent_at' do + expect { service.execute }.not_to change { [record.reload.sms_send_count, record.reload.sms_sent_at] } + end + end + it 'returns an error', :aggregate_failures do response = service.execute @@ -277,6 +291,15 @@ end end + shared_examples 'it returns a success response' do + it 'returns a success response', :aggregate_failures do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_success + end + end + context 'when verification code is sent successfully' do let_it_be(:risk_score) { 10 } let_it_be(:telesign_reference_xid) { '123' } @@ -319,14 +342,9 @@ end end - it 'returns a success response', :aggregate_failures do - response = service.execute - - expect(response).to be_a(ServiceResponse) - expect(response).to be_success - end + it_behaves_like 'it returns a success response' - it 'saves the risk score, telesign_reference_xid and increases verification attempts', :aggregate_failures do + it 'saves the risk score and telesign_reference_xid', :aggregate_failures do service.execute record = user.phone_number_validation @@ -339,6 +357,63 @@ service.execute end + + it 'updates sms_send_count and sms_sent_at', :freeze_time do + service.execute + record = user.phone_number_validation + expect(record.sms_send_count).to eq(1) + expect(record.sms_sent_at).to eq(Time.current) + end + + context 'when sms_send_wait_time feature flag is disabled' do + before do + stub_feature_flags(sms_send_wait_time: false) + end + + it 'does not update sms_send_count and sms_sent_at', :freeze_time, :aggregate_failures do + service.execute + record = user.phone_number_validation + expect(record.sms_send_count).to eq(0) + expect(record.sms_sent_at).to be_nil + end + end + + context 'when send is allowed', :freeze_time do + let_it_be(:record) do + create(:phone_number_validation, user: user, sms_send_count: 1, sms_sent_at: Time.current) + end + + let!(:old_sms_sent_at) { record.sms_sent_at } + + before do + travel_to(5.minutes.from_now) + end + + it_behaves_like 'it returns a success response' + + it 'increments sms_send_count and sets sms_sent_at' do + old_values = [1, old_sms_sent_at.to_i] + new_values = [2, (old_sms_sent_at + 5.minutes).to_i] + + expect { service.execute }.to change { [record.reload.sms_send_count, record.reload.sms_sent_at.to_i] } + .from(old_values).to(new_values) + end + end + + context 'when send is not allowed', :freeze_time do + let_it_be(:record) do + create(:phone_number_validation, user: user, sms_send_count: 1, sms_sent_at: Time.current) + end + + it 'returns an error', :aggregate_failures do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq('Sending not allowed at this time') + expect(response.reason).to eq(:send_not_allowed) + end + end end context 'when telesign_intelligence feature flag is disabled' do @@ -354,12 +429,7 @@ stub_feature_flags(telesign_intelligence: false) end - it 'returns a success response', :aggregate_failures do - response = service.execute - - expect(response).to be_a(ServiceResponse) - expect(response).to be_success - end + it_behaves_like 'it returns a success response' it 'does not save the risk_score' do service.execute diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 80b35158b51e4b1492bc29aa9680115280bd0e0e..25dd9aa512f96daf469cb8bfeff6ebe340047249 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -24692,6 +24692,9 @@ msgstr "" msgid "IdentityVerification|Didn't receive a code? %{codeLinkStart}Send a new code%{codeLinkEnd} or %{phoneLinkStart}enter a new phone number%{phoneLinkEnd}" msgstr "" +msgid "IdentityVerification|Didn't receive a code? Send a new code in %{timer} or %{phoneLinkStart}enter a new phone number%{phoneLinkEnd}" +msgstr "" + msgid "IdentityVerification|Email update is only offered once." msgstr "" @@ -24761,6 +24764,9 @@ msgstr "" msgid "IdentityVerification|Send code" msgstr "" +msgid "IdentityVerification|Send code in %{timer}" +msgstr "" + msgid "IdentityVerification|Something went wrong. Please try again." msgstr "" diff --git a/spec/frontend/vue_shared/components/gl_countdown_spec.js b/spec/frontend/vue_shared/components/gl_countdown_spec.js index 38d54eff87247253d38770c7dd711234710e1928..a755f35332f9cb861f69fee16a5bd9fc8d60eb50 100644 --- a/spec/frontend/vue_shared/components/gl_countdown_spec.js +++ b/spec/frontend/vue_shared/components/gl_countdown_spec.js @@ -44,6 +44,10 @@ describe('GlCountdown', () => { it('displays 00:00:00', () => { expect(wrapper.text()).toContain('00:00:00'); }); + + it('emits `timer-expired` event', () => { + expect(wrapper.emitted('timer-expired')).toStrictEqual([[]]); + }); }); describe('when an invalid date is passed', () => { diff --git a/spec/models/users/phone_number_validation_spec.rb b/spec/models/users/phone_number_validation_spec.rb index 788df05763c1e59dc4a1752c9d4fb01cf1a9e0cf..eb73fc31dac5481ffcda25635b0377aed2a6c025 100644 --- a/spec/models/users/phone_number_validation_spec.rb +++ b/spec/models/users/phone_number_validation_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Users::PhoneNumberValidation, feature_category: :instance_resiliency do + using RSpec::Parameterized::TableSyntax + let_it_be(:user) { create(:user) } let_it_be(:banned_user) { create(:user, :banned) } @@ -250,4 +252,43 @@ it { is_expected.to be_nil } end end + + describe '.sms_send_allowed_after' do + let_it_be(:record) { create(:phone_number_validation, sms_send_count: 0) } + + subject(:result) { record.sms_send_allowed_after } + + context 'when there are no attempts yet' do + it { is_expected.to be_nil } + end + + context 'when sms_send_wait_time feature flag is disabled' do + let_it_be(:record) { create(:phone_number_validation, sms_send_count: 1) } + + before do + stub_feature_flags(sms_send_wait_time: false) + end + + it { is_expected.to be_nil } + end + + where(:attempt_number, :expected_delay) do + 2 | 1.minute + 3 | 3.minutes + 4 | 5.minutes + 5 | 10.minutes + 6 | 10.minutes + end + + with_them do + it 'returns the correct delayed timestamp value' do + freeze_time do + record.update!(sms_send_count: attempt_number - 1, sms_sent_at: Time.current) + + expected_result = Time.current + expected_delay + expect(result).to eq expected_result + end + end + end + end end