diff --git a/app/models/users/phone_number_validation.rb b/app/models/users/phone_number_validation.rb index f6123c01fd03ec5bb6611f2d5988015e0eb67858..b9e4e908ddda94106839cbdd2480a2e766e867cd 100644 --- a/app/models/users/phone_number_validation.rb +++ b/app/models/users/phone_number_validation.rb @@ -31,11 +31,17 @@ class PhoneNumberValidation < ApplicationRecord validates :telesign_reference_xid, length: { maximum: 255 } + scope :for_user, -> (user_id) { where(user_id: user_id) } + def self.related_to_banned_user?(international_dial_code, phone_number) joins(:banned_user).where( international_dial_code: international_dial_code, phone_number: phone_number ).exists? end + + def validated? + validated_at.present? + end end end diff --git a/ee/app/assets/javascripts/users/identity_verification/components/email_verification.vue b/ee/app/assets/javascripts/users/identity_verification/components/email_verification.vue index 2d22c07011ddf6cce35bc6f9dbf9ec89b011d763..1705f41a892b2aa4fcf6179c16aa313c02358bcf 100644 --- a/ee/app/assets/javascripts/users/identity_verification/components/email_verification.vue +++ b/ee/app/assets/javascripts/users/identity_verification/components/email_verification.vue @@ -7,7 +7,7 @@ import { visitUrl } from '~/lib/utils/url_utility'; import { I18N_EMAIL_EMPTY_CODE, I18N_EMAIL_INVALID_CODE, - I18N_EMAIL_REQUEST_ERROR, + I18N_GENERIC_ERROR, I18N_EMAIL_RESEND_SUCCESS, } from '../constants'; @@ -96,7 +96,7 @@ export default { }, handleError(error) { createAlert({ - message: I18N_EMAIL_REQUEST_ERROR, + message: I18N_GENERIC_ERROR, captureError: true, error, }); 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 19b55d382b3884ab1a59ab35597da6069fe1fdef..766217160f9460498c44ad316633b52d0cf083f5 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 @@ -1,40 +1,80 @@ <script> -import { GlFormGroup, GlFormInput, GlFormSelect, GlIcon } from '@gitlab/ui'; -import { s__ } from '~/locale'; +import { GlForm, GlFormGroup, GlFormInput, GlFormSelect, GlIcon, GlButton } from '@gitlab/ui'; +import { createAlert, VARIANT_SUCCESS } from '~/flash'; +import axios from '~/lib/utils/axios_utils'; +import { s__, sprintf } from '~/locale'; + import countriesQuery from 'ee/subscriptions/graphql/queries/countries.query.graphql'; import { validatePhoneNumber } from '../validations'; -import { PHONE_NUMBER_LABEL, COUNTRY_LABEL } from '../constants'; + +import { DEFAULT_COUNTRY, DEFAULT_INTERNATIONAL_DIAL_CODE, I18N_GENERIC_ERROR } from '../constants'; export default { name: 'InternationalPhoneInput', components: { + GlForm, GlFormGroup, GlFormInput, GlFormSelect, GlIcon, + GlButton, + }, + inject: { + phoneNumber: { + default: { + country: DEFAULT_COUNTRY, + internationalDialCode: DEFAULT_INTERNATIONAL_DIAL_CODE, + number: '', + }, + }, }, i18n: { - PHONE_NUMBER_LABEL, - COUNTRY_LABEL, + phoneNumber: s__('IdentityVerification|Phone number'), + dialCode: s__('IdentityVerification|International dial code'), infoText: s__( 'IdentityVerification|You will receive a text containing a code. Standard charges may apply.', ), + success: s__("IdentityVerification|We've sent a verification code to +%{phoneNumber}"), + sendCode: s__('IdentityVerification|Send code'), + I18N_GENERIC_ERROR, }, data() { return { form: { fields: { - phoneNumber: { value: '', state: null, feedback: '' }, - country: { value: 'US+1', state: null, feedback: '' }, + phoneNumber: { + value: this.phoneNumber.number, + state: null, + feedback: '', + }, + country: { + value: `${this.phoneNumber.country || DEFAULT_COUNTRY}+${ + this.phoneNumber.internationalDialCode || DEFAULT_INTERNATIONAL_DIAL_CODE + }`, + state: null, + feedback: '', + }, }, }, countries: [], + isLoading: false, }; }, computed: { countriesWithInternationalDialCode() { return this.countries.filter((country) => country.internationalDialCode); }, + internationalPhoneNumber() { + const internationalDialCode = this.form.fields.country.value.split('+')[1]; + const phoneNumber = this.form.fields.phoneNumber.value; + + return `${internationalDialCode}${phoneNumber}`; + }, + }, + mounted() { + if (this.form.fields.phoneNumber.value) { + this.checkPhoneNumber(); + } }, methods: { checkPhoneNumber() { @@ -42,6 +82,38 @@ export default { this.form.fields.phoneNumber.feedback = errorMessage; this.form.fields.phoneNumber.state = errorMessage.length <= 0; }, + sendVerificationCode() { + this.isLoading = true; + + const [country, internationalDialCode] = this.form.fields.country.value.split('+'); + + axios + .post(this.phoneNumber.sendCodePath, { + country, + international_dial_code: internationalDialCode, + phone_number: this.form.fields.phoneNumber.value, + }) + .then(this.handleSendCodeResponse) + .catch(this.handleError) + .finally(() => { + this.isLoading = false; + }); + }, + handleSendCodeResponse() { + createAlert({ + message: sprintf(this.$options.i18n.success, { + phoneNumber: this.internationalPhoneNumber, + }), + variant: VARIANT_SUCCESS, + }); + }, + handleError(error) { + createAlert({ + message: error.response?.data?.message || this.$options.i18n.I18N_GENERIC_ERROR, + captureError: true, + error, + }); + }, }, apollo: { countries: { @@ -51,10 +123,10 @@ export default { }; </script> <template> - <div> + <gl-form data-testid="send-verification-code-form" @submit.prevent="sendVerificationCode"> <gl-form-group v-if="!$apollo.loading.countries" - :label="$options.i18n.COUNTRY_LABEL" + :label="$options.i18n.dialCode" label-for="country" data-testid="country-form-group" > @@ -77,7 +149,7 @@ export default { </gl-form-group> <gl-form-group - :label="$options.i18n.PHONE_NUMBER_LABEL" + :label="$options.i18n.phoneNumber" label-for="phone_number" :state="form.fields.phoneNumber.state" :invalid-feedback="form.fields.phoneNumber.feedback" @@ -91,7 +163,8 @@ export default { trim autocomplete="off" data-testid="phone-number-form-input" - @blur="checkPhoneNumber" + debounce="250" + @input="checkPhoneNumber" /> </gl-form-group> @@ -99,5 +172,16 @@ export default { <gl-icon name="information-o" :size="12" class="gl-mt-2" /> <span>{{ $options.i18n.infoText }}</span> </div> - </div> + + <gl-button + variant="confirm" + type="submit" + block + class="gl-mt-5" + :disabled="!form.fields.phoneNumber.state" + :loading="isLoading" + > + {{ $options.i18n.sendCode }} + </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 54c5c20d3e19cfc84b98e7a5cec442e2233c466b..181371051e43f0f35e819ab0deaef9181c8dd241 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,26 +1,13 @@ <script> -import { GlForm, GlButton } from '@gitlab/ui'; -import { s__ } from '~/locale'; import InternationalPhoneInput from './international_phone_input.vue'; export default { name: 'PhoneVerification', components: { - GlForm, - GlButton, InternationalPhoneInput, }, - i18n: { - sendCode: s__('IdentityVerification|Send code'), - }, }; </script> <template> - <gl-form> - <international-phone-input /> - - <gl-button variant="confirm" type="submit" class="gl-w-full! gl-mt-5"> - {{ $options.i18n.sendCode }} - </gl-button> - </gl-form> + <international-phone-input /> </template> diff --git a/ee/app/assets/javascripts/users/identity_verification/components/wizard.vue b/ee/app/assets/javascripts/users/identity_verification/components/wizard.vue index 9c7ee17b45d89da23a69a085096272d69a99ebc9..b33d720d592e20faeeac692f9bf6cd7f36222bba 100644 --- a/ee/app/assets/javascripts/users/identity_verification/components/wizard.vue +++ b/ee/app/assets/javascripts/users/identity_verification/components/wizard.vue @@ -1,7 +1,7 @@ <script> import { kebabCase } from 'lodash'; import { s__, sprintf } from '~/locale'; -import { PAGE_TITLE } from '../constants'; + import EmailVerification from './email_verification.vue'; import CreditCardVerification from './credit_card_verification.vue'; import PhoneVerification from './phone_verification.vue'; @@ -46,7 +46,10 @@ export default { }, }, i18n: { - pageTitle: PAGE_TITLE, + pageTitle: s__('IdentityVerification|Help us keep GitLab secure'), + pageDescription: s__( + "IdentityVerification|For added security, you'll need to verify your identity in a few quick steps.", + ), ccStep: s__('IdentityVerification|Step %{stepNumber}: Verify a payment method'), phoneStep: s__('IdentityVerification|Step %{stepNumber}: Verify phone number'), emailStep: s__('IdentityVerification|Step %{stepNumber}: Verify email address'), @@ -58,6 +61,7 @@ export default { <div class="gl-flex-grow-1 gl-max-w-62"> <header class="gl-text-center"> <h2>{{ $options.i18n.pageTitle }}</h2> + <p>{{ $options.i18n.pageDescription }}</p> </header> <component :is="methodComponent(verificationSteps[0])" diff --git a/ee/app/assets/javascripts/users/identity_verification/constants.js b/ee/app/assets/javascripts/users/identity_verification/constants.js index 2ac42f167fe4436cabb970c5c1a2218a77cb5d0d..8b6c9ab0dfd2ec9539d4932fc1148f500a858d0e 100644 --- a/ee/app/assets/javascripts/users/identity_verification/constants.js +++ b/ee/app/assets/javascripts/users/identity_verification/constants.js @@ -1,21 +1,15 @@ import { s__, sprintf } from '~/locale'; -export const PAGE_TITLE = s__('IdentityVerification|Help us keep GitLab secure'); -export const PAGE_SUBTITLE = s__( - "IdentityVerification|For added security, you'll need to verify your identity in a few quick steps.", -); - -export const PHONE_NUMBER_LABEL = s__('IdentityVerification|Phone number'); -export const COUNTRY_LABEL = s__('IdentityVerification|International dial code'); - // follows E.164 standard - https://en.wikipedia.org/wiki/E.164 export const MAX_PHONE_NUMBER_LENGTH = 12; +export const DEFAULT_COUNTRY = 'US'; +export const DEFAULT_INTERNATIONAL_DIAL_CODE = '1'; -export const PHONE_NUMBER_BLANK_ERROR = s__("IdentityVerification|Phone number can't be blank."); -export const PHONE_NUMBER_NAN_ERROR = s__( +export const I18N_PHONE_NUMBER_BLANK_ERROR = s__('IdentityVerification|Phone number is required.'); +export const I18N_PHONE_NUMBER_NAN_ERROR = s__( 'IdentityVerification|Phone number must contain only digits.', ); -export const PHONE_NUMBER_LENGTH_ERROR = sprintf( +export const I18N_PHONE_NUMBER_LENGTH_ERROR = sprintf( s__(`IdentityVerification|Phone number must be %{maxLength} digits or fewer.`), { maxLength: MAX_PHONE_NUMBER_LENGTH, @@ -25,6 +19,6 @@ export const PHONE_NUMBER_LENGTH_ERROR = sprintf( export const I18N_EMAIL_EMPTY_CODE = s__('IdentityVerification|Enter a code.'); export const I18N_EMAIL_INVALID_CODE = s__('IdentityVerification|Enter a valid code.'); export const I18N_EMAIL_RESEND_SUCCESS = s__('IdentityVerification|A new code has been sent.'); -export const I18N_EMAIL_REQUEST_ERROR = s__( +export const I18N_GENERIC_ERROR = s__( 'IdentityVerification|Something went wrong. Please try again.', ); diff --git a/ee/app/assets/javascripts/users/identity_verification/index.js b/ee/app/assets/javascripts/users/identity_verification/index.js index eb877f46ba27c59ba0c695926fb8f168c7b6f829..89ade5aeb4041c2d0ed43477e2f3e2e74b189cf3 100644 --- a/ee/app/assets/javascripts/users/identity_verification/index.js +++ b/ee/app/assets/javascripts/users/identity_verification/index.js @@ -12,6 +12,7 @@ export const initIdentityVerification = () => { const { email, creditCard, + phoneNumber, verificationState, verificationMethods, } = convertObjectPropsToCamelCase(JSON.parse(el.dataset.data), { deep: true }); @@ -23,6 +24,7 @@ export const initIdentityVerification = () => { provide: { email, creditCard, + phoneNumber, verificationSteps: convertArrayToCamelCase(verificationMethods), initialVerificationState: verificationState, }, diff --git a/ee/app/assets/javascripts/users/identity_verification/validations.js b/ee/app/assets/javascripts/users/identity_verification/validations.js index ce804ccfd62f033f0893a7e0322829363c18b5ce..a6489fb9af5877ee7f3d82132caaea56823c4872 100644 --- a/ee/app/assets/javascripts/users/identity_verification/validations.js +++ b/ee/app/assets/javascripts/users/identity_verification/validations.js @@ -1,22 +1,22 @@ import { - PHONE_NUMBER_BLANK_ERROR, - PHONE_NUMBER_NAN_ERROR, + I18N_PHONE_NUMBER_BLANK_ERROR, + I18N_PHONE_NUMBER_NAN_ERROR, + I18N_PHONE_NUMBER_LENGTH_ERROR, MAX_PHONE_NUMBER_LENGTH, - PHONE_NUMBER_LENGTH_ERROR, } from './constants'; export const validatePhoneNumber = (phoneNumber) => { if (!phoneNumber && phoneNumber !== 0) { - return PHONE_NUMBER_BLANK_ERROR; + return I18N_PHONE_NUMBER_BLANK_ERROR; } const numbersOnlyRegex = /^[0-9]+$/; if (!numbersOnlyRegex.test(phoneNumber)) { - return PHONE_NUMBER_NAN_ERROR; + return I18N_PHONE_NUMBER_NAN_ERROR; } if (phoneNumber.length > MAX_PHONE_NUMBER_LENGTH) { - return PHONE_NUMBER_LENGTH_ERROR; + return I18N_PHONE_NUMBER_LENGTH_ERROR; } return ''; diff --git a/ee/app/controllers/users/identity_verification_controller.rb b/ee/app/controllers/users/identity_verification_controller.rb index 3eafcc4494aba486c5b7c642fec92d3cba54b0e2..e34704cdeeec64e892225c76b6d0752f74f7b479 100644 --- a/ee/app/controllers/users/identity_verification_controller.rb +++ b/ee/app/controllers/users/identity_verification_controller.rb @@ -43,6 +43,17 @@ def resend_email_code end end + def send_phone_verification_code + result = ::PhoneVerification::Users::SendVerificationCodeService.new(@user, phone_verification_params).execute + + unless result.success? + log_identity_verification(:failed_phone_verification_attempt, result.reason) + return render status: :bad_request, json: { message: result.message } + end + + render json: { status: :success } + end + private def require_unconfirmed_user! @@ -93,5 +104,9 @@ def send_rate_limited_error_message format(s_("IdentityVerification|You've reached the maximum amount of resends. "\ 'Wait %{interval} and try again.'), interval: email_verification_code_send_interval) end + + def phone_verification_params + params.require(:identity_verification).permit(:country, :international_dial_code, :phone_number) + end end end diff --git a/ee/app/helpers/users/identity_verification_helper.rb b/ee/app/helpers/users/identity_verification_helper.rb index 4d01aa6c8b6a2deddb281eefcb7af63cf30bc65f..12c7a8f87543c0a49af3c3acb936108ed0523c6a 100644 --- a/ee/app/helpers/users/identity_verification_helper.rb +++ b/ee/app/helpers/users/identity_verification_helper.rb @@ -11,6 +11,7 @@ def identity_verification_data(user) user_id: user.id, form_id: ::Gitlab::SubscriptionPortal::REGISTRATION_VALIDATION_FORM_ID }, + phone_number: phone_number_verification_data(user), email: email_verification_data(user) }.to_json } @@ -25,5 +26,22 @@ def email_verification_data(user) resend_path: resend_email_code_identity_verification_path } end + + def phone_number_verification_data(user) + paths = { + send_code_path: send_phone_verification_code_identity_verification_path + } + + phone_number_validation = user.phone_number_validation + return paths unless phone_number_validation.present? + + paths.merge( + { + country: phone_number_validation.country, + international_dial_code: phone_number_validation.international_dial_code, + number: phone_number_validation.phone_number + } + ) + end end end diff --git a/ee/app/models/concerns/identity_verifiable.rb b/ee/app/models/concerns/identity_verifiable.rb index ef2cbb065c36e09b32044fa5c49d3cfa43167a5c..f7fb0494bb9b5928c23472afaaef5f9e45167fc4 100644 --- a/ee/app/models/concerns/identity_verifiable.rb +++ b/ee/app/models/concerns/identity_verifiable.rb @@ -56,7 +56,7 @@ def verification_state end def phone_verified? - phone_number_validation.present? + phone_number_validation.present? && phone_number_validation.validated? end def email_verified? 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 new file mode 100644 index 0000000000000000000000000000000000000000..9c7ac2eb3c3f926aadca86e863e3d76492a45101 --- /dev/null +++ b/ee/app/services/phone_verification/users/send_verification_code_service.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +module PhoneVerification + module Users + class SendVerificationCodeService + include ActionView::Helpers::DateHelper + + def initialize(user, params = {}) + @user = user + @params = params + + @record = ::Users::PhoneNumberValidation.for_user(user.id).first_or_initialize + @record.assign_attributes(params) + end + + def execute + return error_in_params unless valid? + return error_rate_limited if rate_limited? + return error_high_risk_number if related_to_banned_user? + + risk_result = ::PhoneVerification::TelesignClient::RiskScoreService.new( + phone_number: phone_number, + user: user + ).execute + + return error_downstream_service(risk_result) unless risk_result.success? + + send_code_result = ::PhoneVerification::TelesignClient::SendVerificationCodeService.new( + phone_number: phone_number, + user: user + ).execute + + return error_downstream_service(send_code_result) unless send_code_result.success? + + success(risk_result, send_code_result) + + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e, user_id: user.id) + error + end + + private + + attr_reader :user, :params, :record + + def phone_number + params[:international_dial_code].to_s + params[:phone_number].to_s + end + + def valid? + record.valid? + end + + def rate_limited? + ::Gitlab::ApplicationRateLimiter.throttled?(:phone_verification_send_code, scope: user) + end + + def related_to_banned_user? + ::Users::PhoneNumberValidation.related_to_banned_user?( + params[:international_dial_code], params[:phone_number] + ) + end + + def error_in_params + ServiceResponse.error( + message: record.errors.first.full_message, + reason: :bad_params + ) + 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) + + ServiceResponse.error( + message: format( + s_( + 'PhoneVerification|You\'ve reached the maximum number of tries. '\ + 'Wait %{interval} and try again.' + ), + interval: interval + ), + reason: :rate_limited + ) + end + + def error_high_risk_number + ServiceResponse.error( + message: s_( + 'PhoneVerification|There was a problem with the phone number you entered. '\ + 'Enter a different phone number and try again.' + ), + reason: :related_to_banned_user + ) + end + + def error_downstream_service(result) + ServiceResponse.error( + message: result.message, + reason: result.reason + ) + end + + def error + ServiceResponse.error( + message: s_('PhoneVerification|Something went wrong. Please try again.'), + reason: :internal_server_error + ) + end + + def success(risk_result, send_code_result) + record.update!( + risk_score: risk_result[:risk_score], + telesign_reference_xid: send_code_result[:telesign_reference_xid] + ) + + ServiceResponse.success + end + end + end +end diff --git a/ee/config/routes/user.rb b/ee/config/routes/user.rb index 7cff60b1fb0e4af44f2f080f2f67ae7bd32fe6b6..9385e7e123d5ab4ecc5154a3538989f5862bbcc3 100644 --- a/ee/config/routes/user.rb +++ b/ee/config/routes/user.rb @@ -8,6 +8,7 @@ resource :identity_verification, controller: :identity_verification, only: :show do post :verify_email_code post :resend_email_code + post :send_phone_verification_code end end diff --git a/ee/spec/frontend/users/identity_verification/components/email_verification_spec.js b/ee/spec/frontend/users/identity_verification/components/email_verification_spec.js index 5386fc957ef7e6e457e3167b42cbc0ba985b6eec..84ad7b29df0137027659ae10b13144ac2e5e7f72 100644 --- a/ee/spec/frontend/users/identity_verification/components/email_verification_spec.js +++ b/ee/spec/frontend/users/identity_verification/components/email_verification_spec.js @@ -9,7 +9,7 @@ import EmailVerification from 'ee/users/identity_verification/components/email_v import { I18N_EMAIL_EMPTY_CODE, I18N_EMAIL_INVALID_CODE, - I18N_EMAIL_REQUEST_ERROR, + I18N_GENERIC_ERROR, I18N_EMAIL_RESEND_SUCCESS, } from 'ee/users/identity_verification/constants'; @@ -141,7 +141,7 @@ describe('EmailVerification', () => { await axios.waitForAll(); expect(createAlert).toHaveBeenCalledWith({ - message: I18N_EMAIL_REQUEST_ERROR, + message: I18N_GENERIC_ERROR, captureError: true, error: expect.any(Error), }); @@ -176,7 +176,7 @@ describe('EmailVerification', () => { alertObject = { message: response.message }; } else { alertObject = { - message: I18N_EMAIL_REQUEST_ERROR, + message: I18N_GENERIC_ERROR, captureError: true, error: expect.any(Error), }; 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 57fae6d80d68fd94af0bd8b53ed4d602bf890e65..422fd9f0b6cbe3d373f707da1bf8974cdfa93861 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,7 +1,14 @@ +import { GlForm } from '@gitlab/ui'; import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; +import axios from 'axios'; +import MockAdapter from 'axios-mock-adapter'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; +import waitForPromises from 'helpers/wait_for_promises'; +import { createAlert, VARIANT_SUCCESS } from '~/flash'; +import { sprintf } from '~/locale'; +import httpStatusCodes from '~/lib/utils/http_status'; import countriesQuery from 'ee/subscriptions/graphql/queries/countries.query.graphql'; import countriesResolver from 'ee/subscriptions/buy_addons_shared/graphql/resolvers'; @@ -9,19 +16,24 @@ import countriesResolver from 'ee/subscriptions/buy_addons_shared/graphql/resolv import InternationalPhoneInput from 'ee/users/identity_verification/components/international_phone_input.vue'; import { - PHONE_NUMBER_LABEL, - COUNTRY_LABEL, - PHONE_NUMBER_LENGTH_ERROR, - PHONE_NUMBER_NAN_ERROR, - PHONE_NUMBER_BLANK_ERROR, + I18N_PHONE_NUMBER_LENGTH_ERROR, + I18N_PHONE_NUMBER_NAN_ERROR, + I18N_PHONE_NUMBER_BLANK_ERROR, } from 'ee/users/identity_verification/constants'; import { COUNTRIES, mockCountry1, mockCountry2 } from '../mock_data'; Vue.use(VueApollo); +jest.mock('~/flash'); + describe('International Phone input component', () => { let wrapper; + let axiosMock; + + const SEND_CODE_PATH = '/users/identity_verification/send_phone_verification_code'; + + const findForm = () => wrapper.findComponent(GlForm); const findCountryFormGroup = () => wrapper.findByTestId('country-form-group'); const findCountrySelect = () => wrapper.findByTestId('country-form-select'); @@ -33,6 +45,9 @@ describe('International Phone input component', () => { `${country.flag} ${country.name} +${country.internationalDialCode}`; const expectedCountryValue = (country) => `${country.id}+${country.internationalDialCode}`; + const enterPhoneNumber = (value) => findPhoneNumberInput().vm.$emit('input', value); + const submitForm = () => findForm().vm.$emit('submit', { preventDefault: jest.fn() }); + const createMockApolloProvider = () => { const mockResolvers = { countriesResolver }; const mockApollo = createMockApollo([], mockResolvers); @@ -44,17 +59,27 @@ describe('International Phone input component', () => { return mockApollo; }; - const createComponent = ({ props } = { props: {} }) => { + const createComponent = (provide = {}) => { wrapper = shallowMountExtended(InternationalPhoneInput, { apolloProvider: createMockApolloProvider(), - propsData: { - ...props, + provide: { + phoneNumber: { + sendCodePath: SEND_CODE_PATH, + ...provide, + }, }, }); }; + beforeEach(() => { + createComponent(); + axiosMock = new MockAdapter(axios); + }); + afterEach(() => { wrapper.destroy(); + axiosMock.restore(); + createAlert.mockClear(); }); describe('Country select field', () => { @@ -63,7 +88,7 @@ describe('International Phone input component', () => { }); it('should have label', () => { - expect(findCountryFormGroup().attributes('label')).toBe(COUNTRY_LABEL); + expect(findCountryFormGroup().attributes('label')).toBe(wrapper.vm.$options.i18n.dialCode); }); it('should filter out options without international dial code', () => { @@ -88,6 +113,11 @@ describe('International Phone input component', () => { expectedCountryValue(mockCountry2), ); }); + + it('should render injected value', () => { + createComponent({ country: 'AU', internationalDialCode: '61' }); + expect(findCountrySelect().attributes('value')).toBe(expectedCountryValue(mockCountry2)); + }); }); describe('Phone number input field', () => { @@ -96,7 +126,9 @@ describe('International Phone input component', () => { }); it('should have label', () => { - expect(findPhoneNumberFormGroup().attributes('label')).toBe(PHONE_NUMBER_LABEL); + expect(findPhoneNumberFormGroup().attributes('label')).toBe( + wrapper.vm.$options.i18n.phoneNumber, + ); }); it('should be of type tel', () => { @@ -107,15 +139,14 @@ describe('International Phone input component', () => { value | valid | errorMessage ${'1800134678'} | ${true} | ${''} ${'123456789012'} | ${true} | ${''} - ${'1234567890123'} | ${false} | ${PHONE_NUMBER_LENGTH_ERROR} - ${'1300-123-123'} | ${false} | ${PHONE_NUMBER_NAN_ERROR} - ${'abc'} | ${false} | ${PHONE_NUMBER_NAN_ERROR} - ${''} | ${false} | ${PHONE_NUMBER_BLANK_ERROR} + ${'1234567890123'} | ${false} | ${I18N_PHONE_NUMBER_LENGTH_ERROR} + ${'1300-123-123'} | ${false} | ${I18N_PHONE_NUMBER_NAN_ERROR} + ${'abc'} | ${false} | ${I18N_PHONE_NUMBER_NAN_ERROR} + ${''} | ${false} | ${I18N_PHONE_NUMBER_BLANK_ERROR} `( 'when the input has a value of $value, then its validity should be $valid', async ({ value, valid, errorMessage }) => { - findPhoneNumberInput().vm.$emit('input', value); - findPhoneNumberInput().vm.$emit('blur'); + enterPhoneNumber(value); await nextTick(); @@ -127,5 +158,52 @@ describe('International Phone input component', () => { expect(findPhoneNumberInput().attributes('state')).toBe(expectedState); }, ); + + it('should render injected value', () => { + const number = '555'; + createComponent({ number }); + expect(findPhoneNumberInput().attributes('value')).toBe(number); + }); + }); + + describe('Sending verification code', () => { + describe('when request is successful', () => { + beforeEach(() => { + axiosMock.onPost(SEND_CODE_PATH).reply(httpStatusCodes.OK, { success: true }); + + enterPhoneNumber('555'); + submitForm(); + return waitForPromises(); + }); + + it('renders success message', () => { + expect(createAlert).toHaveBeenCalledWith({ + message: sprintf(wrapper.vm.$options.i18n.success, { phoneNumber: '1555' }), + variant: VARIANT_SUCCESS, + }); + }); + }); + + describe('when request is unsuccessful', () => { + const errorMessage = 'Invalid phone number'; + + beforeEach(() => { + axiosMock + .onPost(SEND_CODE_PATH) + .reply(httpStatusCodes.BAD_REQUEST, { message: errorMessage }); + + enterPhoneNumber('555'); + submitForm(); + return waitForPromises(); + }); + + it('renders error message', () => { + expect(createAlert).toHaveBeenCalledWith({ + message: errorMessage, + captureError: true, + error: expect.any(Error), + }); + }); + }); }); }); 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 f4ba65a001291f0b49c2048d04f9f3f51bf524b6..664380023c50d3c830f9cf241e9d111ef17dd482 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 @@ -16,14 +16,16 @@ describe('Phone Verification component', () => { }); }; + beforeEach(() => { + createComponent(); + }); + afterEach(() => { wrapper.destroy(); }); describe('International Phone input', () => { it('should render InternationalPhoneInput component', () => { - createComponent(); - expect(findInternationalPhoneInput().exists()).toBe(true); }); }); diff --git a/ee/spec/frontend/users/identity_verification/components/wizard_spec.js b/ee/spec/frontend/users/identity_verification/components/wizard_spec.js index 92a2f119787ec9fef2c9f5ea33156cf4111dc21f..f751228c7441280e90157e70fde441a0c7fd9d61 100644 --- a/ee/spec/frontend/users/identity_verification/components/wizard_spec.js +++ b/ee/spec/frontend/users/identity_verification/components/wizard_spec.js @@ -5,7 +5,6 @@ import VerificationStep from 'ee/users/identity_verification/components/verifica import CreditCardVerification from 'ee/users/identity_verification/components/credit_card_verification.vue'; import PhoneVerification from 'ee/users/identity_verification/components/phone_verification.vue'; import EmailVerification from 'ee/users/identity_verification/components/email_verification.vue'; -import { PAGE_TITLE } from 'ee/users/identity_verification/constants'; describe('IdentityVerificationWizard', () => { let wrapper; @@ -25,6 +24,7 @@ describe('IdentityVerificationWizard', () => { }; const findHeader = () => wrapper.find('h2'); + const findDescription = () => wrapper.find('p'); afterEach(() => { wrapper.destroy(); @@ -41,7 +41,11 @@ describe('IdentityVerificationWizard', () => { }); it('displays the header', () => { - expect(findHeader().text()).toBe(PAGE_TITLE); + expect(findHeader().text()).toBe(wrapper.vm.$options.i18n.pageTitle); + }); + + it('displays the description', () => { + expect(findDescription().text()).toBe(wrapper.vm.$options.i18n.pageDescription); }); it('renders the correct verification method components in order', () => { diff --git a/ee/spec/helpers/users/identity_verification_helper_spec.rb b/ee/spec/helpers/users/identity_verification_helper_spec.rb index 341a70a30b9f812b89413d4562005872833ecae9..68cf158e2523fa1cf38634f365bf79554f570160 100644 --- a/ee/spec/helpers/users/identity_verification_helper_spec.rb +++ b/ee/spec/helpers/users/identity_verification_helper_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Users::IdentityVerificationHelper do - let_it_be(:user) { build(:user) } + let_it_be_with_reload(:user) { create(:user) } describe '#identity_verification_data' do let(:mock_required_identity_verification_methods) { ['email'] } @@ -23,22 +23,55 @@ subject(:data) { helper.identity_verification_data(user) } - it 'returns the expected data' do - expect(data[:data]).to eq( - { - verification_methods: mock_required_identity_verification_methods, - verification_state: mock_identity_verification_state, - credit_card: { - user_id: user.id, - form_id: ::Gitlab::SubscriptionPortal::REGISTRATION_VALIDATION_FORM_ID - }, - email: { - obfuscated: helper.obfuscated_email(user.email), - verify_path: verify_email_code_identity_verification_path, - resend_path: resend_email_code_identity_verification_path - } - }.to_json - ) + context 'when no phone number for user exists' do + it 'returns the expected data' do + expect(data[:data]).to eq( + { + verification_methods: mock_required_identity_verification_methods, + verification_state: mock_identity_verification_state, + credit_card: { + user_id: user.id, + form_id: ::Gitlab::SubscriptionPortal::REGISTRATION_VALIDATION_FORM_ID + }, + phone_number: { + send_code_path: send_phone_verification_code_identity_verification_path + }, + email: { + obfuscated: helper.obfuscated_email(user.email), + verify_path: verify_email_code_identity_verification_path, + resend_path: resend_email_code_identity_verification_path + } + }.to_json + ) + end + end + + context 'when phone number for user exists' do + let_it_be(:phone_number_validation) { create(:phone_number_validation, user: user) } + + it 'returns the expected data with saved phone number' do + expect(data[:data]).to eq( + { + verification_methods: mock_required_identity_verification_methods, + verification_state: mock_identity_verification_state, + credit_card: { + user_id: user.id, + form_id: ::Gitlab::SubscriptionPortal::REGISTRATION_VALIDATION_FORM_ID + }, + phone_number: { + send_code_path: send_phone_verification_code_identity_verification_path, + country: phone_number_validation.country, + international_dial_code: phone_number_validation.international_dial_code, + number: phone_number_validation.phone_number + }, + email: { + obfuscated: helper.obfuscated_email(user.email), + verify_path: verify_email_code_identity_verification_path, + resend_path: resend_email_code_identity_verification_path + } + }.to_json + ) + end end end end diff --git a/ee/spec/models/concerns/identity_verifiable_spec.rb b/ee/spec/models/concerns/identity_verifiable_spec.rb index 549452f18b5d43ff90ffd79176aed1488036a60a..6cb07b314153d83e2f87316155893c0762a541b2 100644 --- a/ee/spec/models/concerns/identity_verifiable_spec.rb +++ b/ee/spec/models/concerns/identity_verifiable_spec.rb @@ -109,16 +109,31 @@ def add_user_risk_band(value) subject { user.identity_verification_state['phone'] } - context 'when user has not verified a phone number' do + context 'when user has no phone number' do let(:user) { create(:user, phone_number_validation: nil) } it { is_expected.to eq false } end + context 'when user has not verified a phone number' do + let(:validation) { create(:phone_number_validation) } + let(:user) { create(:user, phone_number_validation: validation) } + + before do + allow(validation).to receive(:validated?).and_return(false) + end + + it { is_expected.to eq false } + end + context 'when user has verified a phone number' do let(:validation) { create(:phone_number_validation) } let(:user) { create(:user, phone_number_validation: validation) } + before do + allow(validation).to receive(:validated?).and_return(true) + end + it { is_expected.to eq true } end end diff --git a/ee/spec/requests/users/identity_verification_controller_spec.rb b/ee/spec/requests/users/identity_verification_controller_spec.rb index e7a68e8afcaf604762533d2c2c13ada0880d202d..8056652793f47d9bc2bbf836b9549d0b6a2936e1 100644 --- a/ee/spec/requests/users/identity_verification_controller_spec.rb +++ b/ee/spec/requests/users/identity_verification_controller_spec.rb @@ -229,4 +229,53 @@ end end end + + describe '#send_phone_verification_code' do + let_it_be(:params) do + { identity_verification: { country: 'US', international_dial_code: '1', phone_number: '555' } } + end + + subject(:do_request) { post send_phone_verification_code_identity_verification_path(params) } + + before do + allow_next_instance_of(::PhoneVerification::Users::SendVerificationCodeService) do |service| + allow(service).to receive(:execute).and_return(service_response) + end + stub_session(verification_user_id: unconfirmed_user.id) + end + + context 'when sending the code is successful' do + let_it_be(:service_response) { ServiceResponse.success } + + it 'responds with status 200 OK' do + do_request + + expect(response.body).to eq({ status: :success }.to_json) + end + end + + context 'when sending the code is unsuccessful' do + let_it_be(:service_response) { ServiceResponse.error(message: 'message', reason: 'reason') } + + it 'logs the failed attempt' do + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + message: 'Identity Verification', + event: 'Failed Phone Verification Attempt', + username: unconfirmed_user.username, + reason: service_response.reason + ) + ) + + do_request + end + + it 'responds with error message' do + do_request + + expect(response).to have_gitlab_http_status(:bad_request) + expect(response.body).to eq({ message: service_response.message }.to_json) + end + end + end end 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 new file mode 100644 index 0000000000000000000000000000000000000000..71d00ba4a61e1a281e1af7ab7575dd4b3735b7a7 --- /dev/null +++ b/ee/spec/services/phone_verification/users/send_verification_code_service_spec.rb @@ -0,0 +1,186 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe PhoneVerification::Users::SendVerificationCodeService do + let_it_be_with_reload(:user) { create(:user) } + let(:params) { { country: 'US', international_dial_code: 1, phone_number: '555' } } + + subject(:service) { described_class.new(user, params) } + + describe '#execute', :aggregate_failures do + before do + allow_next_instance_of(PhoneVerification::TelesignClient::RiskScoreService) do |instance| + allow(instance).to receive(:execute).and_return(risk_service_response) + end + + allow_next_instance_of(PhoneVerification::TelesignClient::SendVerificationCodeService) do |instance| + allow(instance).to receive(:execute).and_return(send_verification_code_response) + end + end + + context 'when params are invalid' do + let(:params) { { country: 'US', international_dial_code: 1 } } + + it 'returns an error' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq('Phone number can\'t be blank') + expect(response.reason).to eq(:bad_params) + end + end + + context 'when user has reached max verification attempts' do + let(:record) { create(:phone_number_validation, user: user) } + + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:phone_verification_send_code, scope: user).and_return(true) + end + + it 'returns an error' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq( + 'You\'ve reached the maximum number of tries. ' \ + 'Wait about 1 hour and try again.' + ) + expect(response.reason).to eq(:rate_limited) + end + end + + context 'when phone number is linked to an already banned user' do + let(:banned_user) { create(:user, :banned) } + let(:record) { create(:phone_number_validation, user: banned_user) } + + let(:params) do + { + country: 'AU', + international_dial_code: record.international_dial_code, + phone_number: record.phone_number + } + end + + it 'returns an error' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq( + 'There was a problem with the phone number you entered. '\ + 'Enter a different phone number and try again.' + ) + expect(response.reason).to eq(:related_to_banned_user) + end + end + + context 'when phone number is high risk' do + let_it_be(:risk_service_response) do + ServiceResponse.error(message: 'Downstream error message', reason: :invalid_phone_number) + end + + it 'returns an error' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq('Downstream error message') + expect(response.reason).to eq(:invalid_phone_number) + end + end + + context 'when there is a client error in sending the verification code' do + let_it_be(:risk_service_response) { ServiceResponse.success } + + let_it_be(:send_verification_code_response) do + ServiceResponse.error(message: 'Downstream error message', reason: :bad_request) + end + + it 'returns an error' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq('Downstream error message') + expect(response.reason).to eq(:bad_request) + end + end + + context 'when there is a server error in sending the verification code' do + let_it_be(:risk_service_response) { ServiceResponse.success } + + let_it_be(:send_verification_code_response) do + ServiceResponse.error(message: 'Downstream error message', reason: :internal_server_error) + end + + it 'returns an error' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq('Downstream error message') + expect(response.reason).to eq(:internal_server_error) + end + end + + context 'when there is an unknown exception' do + let(:exception) { StandardError.new } + + before do + allow(Gitlab::ErrorTracking).to receive(:track_exception) + allow_next_instance_of(PhoneVerification::TelesignClient::RiskScoreService) do |instance| + allow(instance).to receive(:execute).and_raise(exception) + end + end + + it 'returns an error ServiceResponse' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq('Something went wrong. Please try again.') + expect(response.reason).to be(:internal_server_error) + end + + it 'tracks the exception' do + service.execute + + expect(Gitlab::ErrorTracking).to have_received(:track_exception).with( + exception, user_id: user.id + ) + end + end + + context 'when verification code is sent successfully' do + let_it_be(:risk_score) { 10 } + let_it_be(:telesign_reference_xid) { '123' } + + let_it_be(:risk_service_response) do + ServiceResponse.success(payload: { risk_score: risk_score }) + end + + let_it_be(:send_verification_code_response) do + ServiceResponse.success(payload: { telesign_reference_xid: telesign_reference_xid }) + end + + it 'returns a success response' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_success + end + + it 'saves the risk score, telesign_reference_xid and increases verification attempts' do + service.execute + record = user.phone_number_validation + + expect(record.risk_score).to eq(risk_score) + expect(record.telesign_reference_xid).to eq(telesign_reference_xid) + end + end + end +end diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index 507f94d87a53cbbd23d974127f1582a550eaa54e..d78722605bcf6a664c96751716806c98e429aace 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -51,6 +51,7 @@ def rate_limits # rubocop:disable Metrics/AbcSize project_testing_integration: { threshold: 5, interval: 1.minute }, email_verification: { threshold: 10, interval: 10.minutes }, email_verification_code_send: { threshold: 10, interval: 1.hour }, + phone_verification_send_code: { threshold: 10, interval: 1.hour }, namespace_exists: { threshold: 20, interval: 1.minute }, fetch_google_ip_list: { threshold: 10, interval: 1.minute } }.freeze diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 711920a0689018178ad32149435adf1e845de2f2..dbca0dcaca4a7ef146fb781a6747b1acff331f6c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -20431,7 +20431,7 @@ msgstr "" msgid "IdentityVerification|Phone number" msgstr "" -msgid "IdentityVerification|Phone number can't be blank." +msgid "IdentityVerification|Phone number is required." msgstr "" msgid "IdentityVerification|Phone number must be %{maxLength} digits or fewer." @@ -20488,6 +20488,9 @@ msgstr "" msgid "IdentityVerification|Verify your identity" msgstr "" +msgid "IdentityVerification|We've sent a verification code to +%{phoneNumber}" +msgstr "" + msgid "IdentityVerification|You can always verify your account at a later time to create a group." msgstr "" @@ -29755,6 +29758,9 @@ msgstr "" msgid "PhoneVerification|You've reached the maximum number of tries. Request a new code and try again." msgstr "" +msgid "PhoneVerification|You've reached the maximum number of tries. Wait %{interval} and try again." +msgstr "" + msgid "Pick a name" msgstr "" diff --git a/spec/models/users/phone_number_validation_spec.rb b/spec/models/users/phone_number_validation_spec.rb index 2f0fd1d3ac9c5762cafcdcb059ce6707288ff8c4..7ab461a4346bb0fa7c2f001f046c2c0207680831 100644 --- a/spec/models/users/phone_number_validation_spec.rb +++ b/spec/models/users/phone_number_validation_spec.rb @@ -78,4 +78,42 @@ it { is_expected.to eq(false) } end end + + describe '#for_user' do + let_it_be(:user_1) { create(:user) } + let_it_be(:user_2) { create(:user) } + + let_it_be(:phone_number_record_1) { create(:phone_number_validation, user: user_1) } + let_it_be(:phone_number_record_2) { create(:phone_number_validation, user: user_2) } + + context 'when multiple records exist for multiple users' do + it 'returns the correct phone number record for user' do + records = described_class.for_user(user_1.id) + + expect(records.count).to be(1) + expect(records.first).to eq(phone_number_record_1) + end + end + end + + describe '#validated?' do + let_it_be(:user) { create(:user) } + let_it_be(:phone_number_record) { create(:phone_number_validation, user: user) } + + context 'when phone number record is not validated' do + it 'returns false' do + expect(phone_number_record.validated?).to be(false) + end + end + + context 'when phone number record is validated' do + before do + phone_number_record.update!(validated_at: Time.now.utc) + end + + it 'returns true' do + expect(phone_number_record.validated?).to be(true) + end + end + end end