Merge branch 'redirect-to-successful-verification-page-after-completed-steps' into 'master'

Redirect to successful verification page after completed steps

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111057



Merged-by: Mikołaj Wawrzyniak <mwawrzyniak@gitlab.com>
Approved-by: Eugie Limpin <elimpin@gitlab.com>
Approved-by: Scott de Jonge <sdejonge@gitlab.com>
Approved-by: Jose Ivan Vargas <jvargas@gitlab.com>
Approved-by: Jessie Young <jessieyoung@gitlab.com>
Approved-by: Mikołaj Wawrzyniak <mwawrzyniak@gitlab.com>
Reviewed-by: Jose Ivan Vargas <jvargas@gitlab.com>
Reviewed-by: Alex Buijs <abuijs@gitlab.com>
Reviewed-by: Mikołaj Wawrzyniak <mwawrzyniak@gitlab.com>
Reviewed-by: Jessie Young <jessieyoung@gitlab.com>
Reviewed-by: Scott de Jonge <sdejonge@gitlab.com>
Co-authored-by: Alex Buijs <abuijs@gitlab.com>

ee/app/assets/javascripts/users/identity_verification/components/email_verification.vue

@@ -3,7 +3,6 @@ import { GlForm, GlFormGroup, GlFormInput, GlIcon, GlLink, GlSprintf, GlButton }
 import { s__ } from '~/locale';
 import { createAlert, VARIANT_SUCCESS } from '~/flash';
 import axios from '~/lib/utils/axios_utils';
-import { visitUrl } from '~/lib/utils/url_utility';
 import {
   I18N_EMAIL_EMPTY_CODE,
   I18N_EMAIL_INVALID_CODE,
@@ -25,6 +24,13 @@ export default {
     GlButton,
   },
   inject: ['email'],
+  props: {
+    isStandalone: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
   data() {
     return {
       verificationCode: '',
@@ -78,8 +84,6 @@ export default {
     handleVerificationResponse(response) {
       if (response.data.status === SUCCESS_RESPONSE) {
         this.$emit('completed');
-
-        visitUrl(response.data.redirect_url);
       } else if (response.data.status === FAILURE_RESPONSE) {
         this.verifyError = response.data.message;
       }
@@ -107,9 +111,10 @@ export default {
     },
   },
   i18n: {
-    header: s__(
-      "IdentityVerification|For added security, you'll need to verify your identity. We've sent a verification code to %{email}",
+    headerStandalone: s__(
+      "IdentityVerification|For added security, you'll need to verify your identity.",
     ),
+    header: s__("IdentityVerification|We've sent a verification code to %{email}"),
     code: s__('IdentityVerification|Verification code'),
     noCode: s__("IdentityVerification|Didn't receive a code?"),
     resend: s__('IdentityVerification|Send a new code'),
@@ -119,14 +124,15 @@ export default {
 </script>
 <template>
   <div>
-    <p class="gl-text-center">
+    <p :class="{ 'gl-text-center': isStandalone }">
+      <span v-if="isStandalone">{{ $options.i18n.headerStandalone }}</span>
       <gl-sprintf :message="$options.i18n.header">
         <template #email>
           <b>{{ email.obfuscated }}</b>
         </template>
       </gl-sprintf>
     </p>
-    <div class="gl-p-5 gl-border gl-rounded-base">
+    <div :class="{ 'gl-p-5 gl-border gl-rounded-base': isStandalone }">
       <gl-form @submit.prevent="verify">
         <gl-form-group
           :label="$options.i18n.code"
@@ -138,7 +144,7 @@ export default {
             v-model="verificationCode"
             name="verification_code"
             :autofocus="true"
-            autocomplete="off"
+            autocomplete="one-time-code"
             inputmode="numeric"
             maxlength="6"
             :state="isValidInput"

ee/app/assets/javascripts/users/identity_verification/components/verify_phone_verification_code.vue

@@ -140,7 +140,7 @@ export default {
         name="verification_code"
         :state="form.fields.verificationCode.state"
         trim
-        autocomplete="off"
+        autocomplete="one-time-code"
         data-testid="verification-code-form-input"
         class="gl-number-as-text-input"
         @input="checkVerificationCode"

ee/app/assets/javascripts/users/identity_verification/components/wizard.vue

 <script>
 import { kebabCase } from 'lodash';
 import { s__, sprintf } from '~/locale';
+import { visitUrl } from '~/lib/utils/url_utility';
+import { REDIRECT_TIMEOUT } from '../constants';
 import EmailVerification from './email_verification.vue';
 import CreditCardVerification from './credit_card_verification.vue';
 import PhoneVerification from './phone_verification.vue';
@@ -15,7 +17,7 @@ export default {
     EmailVerification,
     VerificationStep,
   },
-  inject: ['verificationSteps', 'initialVerificationState'],
+  inject: ['verificationSteps', 'initialVerificationState', 'successfulVerificationPath'],
   data() {
     return {
       stepsVerifiedState: this.initialVerificationState,
@@ -24,12 +26,26 @@ export default {
   computed: {
     activeStep() {
       const isIncomplete = (step) => !this.stepsVerifiedState[step];
-      return this.verificationSteps.find(isIncomplete);
+      return this.orderedSteps.find(isIncomplete);
+    },
+    orderedSteps() {
+      return [...this.verificationSteps].sort(
+        (a, b) => this.stepsVerifiedState[b] - this.stepsVerifiedState[a],
+      );
+    },
+    allStepsCompleted() {
+      return !Object.entries(this.stepsVerifiedState).filter(([, completed]) => !completed).length;
+    },
+    isStandaloneEmailVerification() {
+      return this.verificationSteps.length === 1;
     },
   },
   methods: {
     onStepCompleted(step) {
       this.stepsVerifiedState[step] = true;
+      if (this.allStepsCompleted) {
+        setTimeout(() => visitUrl(this.successfulVerificationPath), REDIRECT_TIMEOUT);
+      }
     },
     methodComponent(method) {
       // eslint-disable-next-line @gitlab/require-i18n-strings
@@ -61,20 +77,21 @@ 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>
+        <p v-if="!isStandaloneEmailVerification">{{ $options.i18n.pageDescription }}</p>
       </header>
-      <component
-        :is="methodComponent(verificationSteps[0])"
-        v-if="verificationSteps.length === 1"
+      <email-verification
+        v-if="isStandaloneEmailVerification"
+        :is-standalone="true"
+        @completed="onStepCompleted(verificationSteps[0])"
       />
-      <template v-for="(step, index) in verificationSteps" v-else>
+      <template v-for="(step, index) in orderedSteps" v-else>
         <verification-step
           :key="step"
           :title="stepTitle(step, index + 1)"
           :completed="stepsVerifiedState[step]"
           :is-active="step === activeStep"
         >
-          <component :is="methodComponent(step)" @completed="() => onStepCompleted(step)" />
+          <component :is="methodComponent(step)" @completed="onStepCompleted(step)" />
         </verification-step>
       </template>
     </div>

ee/app/assets/javascripts/users/identity_verification/constants.js

@@ -29,3 +29,5 @@ export const I18N_EMAIL_RESEND_SUCCESS = s__('IdentityVerification|A new code ha
 export const I18N_GENERIC_ERROR = s__(
   'IdentityVerification|Something went wrong. Please try again.',
 );
+
+export const REDIRECT_TIMEOUT = 1500;

ee/app/assets/javascripts/users/identity_verification/index.js

@@ -15,6 +15,7 @@ export const initIdentityVerification = () => {
     phoneNumber,
     verificationState,
     verificationMethods,
+    successfulVerificationPath,
   } = convertObjectPropsToCamelCase(JSON.parse(el.dataset.data), { deep: true });
   return new Vue({
@@ -27,6 +28,7 @@ export const initIdentityVerification = () => {
       phoneNumber,
       verificationSteps: convertArrayToCamelCase(verificationMethods),
       initialVerificationState: verificationState,
+      successfulVerificationPath,
     },
     render: (createElement) => createElement(IdentityVerificationWizard),
   });

ee/app/controllers/users/identity_verification_controller.rb

@@ -7,15 +7,15 @@ class IdentityVerificationController < ApplicationController
     include Arkose::ContentSecurityPolicy
     skip_before_action :authenticate_user!
-    before_action :require_unverified_user!
+    before_action :require_verification_user!
+    before_action :require_unverified_user!, except: :success
     before_action :require_arkose_verification!, except: [:arkose_labs_challenge, :verify_arkose_labs_session]
     feature_category :authentication_and_authorization
     layout 'minimal'
-    def show
-    end
+    def show; end
     def verify_email_code
       result = verify_token
@@ -23,10 +23,7 @@ def verify_email_code
       if result[:status] == :success
         confirm_user
-        render json: {
-          status: :success,
-          redirect_url: users_successful_verification_path
-        }
+        render json: { status: :success }
       else
         log_identity_verification('Email', :failed_attempt, result[:reason])
@@ -79,12 +76,25 @@ def verify_arkose_labs_session
       redirect_to action: :show
     end
+    def success
+      return redirect_to identity_verification_path unless @user.identity_verified?
+
+      sign_in(@user)
+      session.delete(:verification_user_id)
+      @redirect_url = after_sign_in_path_for(@user)
+
+      render 'devise/sessions/successful_verification'
+    end
+
     private
-    def require_unverified_user!
+    def require_verification_user!
       @user = User.find_by_id(session[:verification_user_id])
-      access_denied! if !@user || @user.identity_verified?
+      not_found unless @user
+    end
+    def require_unverified_user!
+      access_denied! if @user.identity_verified?
     end
     def require_arkose_verification!
@@ -121,7 +131,6 @@ def verify_token
     def confirm_user
       @user.confirm
       accept_pending_invitations(user: @user)
-      sign_in(@user)
       log_identity_verification('Email', :success)
     end
@@ -139,8 +148,8 @@ def send_rate_limited?
     def send_rate_limited_error_message
       interval_in_seconds = ::Gitlab::ApplicationRateLimiter.rate_limits[:email_verification_code_send][:interval]
       email_verification_code_send_interval = distance_of_time_in_words(interval_in_seconds)
-      format(s_("IdentityVerification|You've reached the maximum amount of resends. "\
-        'Wait %{interval} and try again.'), interval: email_verification_code_send_interval)
+      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

ee/app/helpers/users/identity_verification_helper.rb

@@ -12,7 +12,8 @@ def identity_verification_data(user)
             form_id: ::Gitlab::SubscriptionPortal::REGISTRATION_VALIDATION_FORM_ID
           },
           phone_number: phone_number_verification_data(user),
-          email: email_verification_data(user)
+          email: email_verification_data(user),
+          successful_verification_path: success_identity_verification_path
         }.to_json
       }
     end

ee/config/routes/user.rb

@@ -12,6 +12,7 @@
     post :verify_phone_verification_code
     get :arkose_labs_challenge
     post :verify_arkose_labs_session
+    get :success
   end
 end

ee/spec/features/users/identity_verification_spec.rb

@@ -40,7 +40,7 @@
       it 'successfully confirms the user and shows the verification successful page' do
         verify_code confirmation_code
-        expect(page).to have_current_path(users_successful_verification_path)
+        expect(page).to have_current_path(success_identity_verification_path)
         expect(page).to have_content(s_('IdentityVerification|Verification successful'))
         expect(page).to have_selector(
           "meta[http-equiv='refresh'][content='3; url=#{users_sign_up_welcome_path}']", visible: :hidden

ee/spec/frontend/users/identity_verification/components/email_verification_spec.js

@@ -5,7 +5,6 @@ import MockAdapter from 'axios-mock-adapter';
 import { s__ } from '~/locale';
 import { createAlert, VARIANT_SUCCESS } from '~/flash';
 import { HTTP_STATUS_NOT_FOUND, HTTP_STATUS_OK } from '~/lib/utils/http_status';
-import { visitUrl } from '~/lib/utils/url_utility';
 import EmailVerification from 'ee/users/identity_verification/components/email_verification.vue';
 import {
   I18N_EMAIL_EMPTY_CODE,
@@ -15,9 +14,6 @@ import {
 } from 'ee/users/identity_verification/constants';
 jest.mock('~/flash');
-jest.mock('~/lib/utils/url_utility', () => ({
-  visitUrl: jest.fn().mockName('visitUrlMock'),
-}));
 describe('EmailVerification', () => {
   let wrapper;
@@ -75,10 +71,6 @@ describe('EmailVerification', () => {
         await axios.waitForAll();
       });
-      it('redirects to the returned redirect_url', () => {
-        expect(visitUrl).toHaveBeenCalledWith('root');
-      });
-
       it('emits completed event', () => {
         expect(wrapper.emitted('completed')).toHaveLength(1);
       });

ee/spec/frontend/users/identity_verification/components/wizard_spec.js

 import { shallowMount } from '@vue/test-utils';
 import { nextTick } from 'vue';
+import { visitUrl } from '~/lib/utils/url_utility';
 import IdentityVerificationWizard from 'ee/users/identity_verification/components/wizard.vue';
 import VerificationStep from 'ee/users/identity_verification/components/verification_step.vue';
 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';
+jest.mock('~/lib/utils/url_utility', () => ({
+  visitUrl: jest.fn().mockName('visitUrlMock'),
+}));
+
 describe('IdentityVerificationWizard', () => {
   let wrapper;
   let steps;
@@ -13,6 +18,7 @@ describe('IdentityVerificationWizard', () => {
   const DEFAULT_PROVIDE = {
     verificationSteps: ['creditCard', 'email'],
     initialVerificationState: { creditCard: false, email: false },
+    successfulVerificationPath: '/users/identity_verification/success',
   };
   const createComponent = ({ provide } = { provide: {} }) => {
@@ -73,17 +79,21 @@ describe('IdentityVerificationWizard', () => {
     });
     describe('when some steps are complete', () => {
-      it('is the first incomplete step', () => {
+      it('shows the incomplete steps at the end', () => {
         createComponent({
           provide: {
             verificationSteps: ['creditCard', 'phone', 'email'],
-            initialVerificationState: { creditCard: true, phone: false, email: false },
+            initialVerificationState: { creditCard: true, phone: false, email: true },
           },
         });
         expect(steps.at(0).props('isActive')).toBe(false);
-        expect(steps.at(1).props('isActive')).toBe(true);
-        expect(steps.at(2).props('isActive')).toBe(false);
+        expect(steps.at(1).props('isActive')).toBe(false);
+        expect(steps.at(2).props('isActive')).toBe(true);
+
+        expect(steps.at(0).props('title')).toBe('Step 1: Verify a payment method');
+        expect(steps.at(1).props('title')).toBe('Step 2: Verify email address');
+        expect(steps.at(2).props('title')).toBe('Step 3: Verify phone number');
       });
     });
@@ -119,18 +129,26 @@ describe('IdentityVerificationWizard', () => {
       createComponent();
     });
-    it('goes from first to last one step at a time', async () => {
+    it('goes from first to last one step at a time and redirects after all are completed', async () => {
+      const setTimeoutSpy = jest.spyOn(global, 'setTimeout');
+
       expectMethodToBeActive(1, steps.wrappers);
       steps.at(0).findComponent(CreditCardVerification).vm.$emit('completed');
       await nextTick();
+      expect(setTimeoutSpy).not.toHaveBeenCalled();
       expectMethodToBeActive(2, steps.wrappers);
       steps.at(1).findComponent(EmailVerification).vm.$emit('completed');
       await nextTick();
       expectNoActiveMethod(steps.wrappers);
+
+      jest.runAllTimers();
+
+      expect(setTimeoutSpy).toHaveBeenCalledTimes(1);
+      expect(visitUrl).toHaveBeenCalledWith(DEFAULT_PROVIDE.successfulVerificationPath);
     });
   });
@@ -146,5 +164,15 @@ describe('IdentityVerificationWizard', () => {
     it('renders the method component', () => {
       expect(wrapper.findComponent(EmailVerification).exists()).toBe(true);
     });
+
+    it('redirects to the successfulVerificationPath after completion', () => {
+      const setTimeoutSpy = jest.spyOn(global, 'setTimeout');
+      wrapper.findComponent(EmailVerification).vm.$emit('completed');
+
+      jest.runAllTimers();
+
+      expect(setTimeoutSpy).toHaveBeenCalledTimes(1);
+      expect(visitUrl).toHaveBeenCalledWith(DEFAULT_PROVIDE.successfulVerificationPath);
+    });
   });
 });

ee/spec/helpers/users/identity_verification_helper_spec.rb

@@ -41,7 +41,8 @@
               obfuscated: helper.obfuscated_email(user.email),
               verify_path: verify_email_code_identity_verification_path,
               resend_path: resend_email_code_identity_verification_path
-            }
+            },
+            successful_verification_path: success_identity_verification_path
           }.to_json
         )
       end
@@ -70,7 +71,8 @@
               obfuscated: helper.obfuscated_email(user.email),
               verify_path: verify_email_code_identity_verification_path,
               resend_path: resend_email_code_identity_verification_path
-            }
+            },
+            successful_verification_path: success_identity_verification_path
           }.to_json
         )
       end

ee/spec/requests/users/identity_verification_controller_spec.rb

@@ -111,14 +111,6 @@
         expect(member_invite.reload).not_to be_invite
       end
-      it 'signs in the user' do
-        stub_session(verification_user_id: unconfirmed_user.id)
-
-        do_request
-
-        expect(request.env['warden']).to be_authenticated
-      end
-
       it 'logs and tracks the successful attempt' do
         expect(Gitlab::AppLogger).to receive(:info).with(
           hash_including(
@@ -140,12 +132,12 @@
         )
       end
-      it 'renders the result as json including a redirect URL' do
+      it 'renders the result as json' do
         stub_session(verification_user_id: unconfirmed_user.id)
         do_request
-        expect(response.body).to eq(service_response.merge(redirect_url: users_successful_verification_path).to_json)
+        expect(response.body).to eq(service_response.to_json)
       end
     end
@@ -523,4 +515,40 @@
       expect(response).to render_template('arkose_labs_challenge', layout: 'minimal')
     end
   end
+
+  describe 'GET success' do
+    let(:after_sign_in_path) { '/after/sign/in' }
+    let(:user) { confirmed_user }
+
+    before do
+      allow_next_instance_of(described_class) do |controller|
+        allow(controller).to receive(:after_sign_in_path_for).and_return(after_sign_in_path)
+      end
+
+      stub_session(verification_user_id: user.id)
+      get success_identity_verification_path
+    end
+
+    context 'when not yet verified' do
+      let(:user) { unconfirmed_user }
+
+      it 'redirects back to identity_verification_path' do
+        expect(response).to redirect_to(identity_verification_path)
+      end
+    end
+
+    it 'signs in the user' do
+      expect(request.env['warden']).to be_authenticated
+    end
+
+    it 'deletes the verification_user_id from the session' do
+      expect(request.session.has_key?(:verification_user_id)).to eq(false)
+    end
+
+    it 'renders the template with the after_sign_in_path_for variable' do
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(response).to render_template('successful_verification', layout: 'minimal')
+      expect(assigns(:redirect_url)).to eq(after_sign_in_path)
+    end
+  end
 end

locale/gitlab.pot

@@ -21088,6 +21088,9 @@ msgstr ""
 msgid "IdentityVerification|For added security, you'll need to verify your identity in a few quick steps."
 msgstr ""
+msgid "IdentityVerification|For added security, you'll need to verify your identity."
+msgstr ""
+
 msgid "IdentityVerification|For added security, you'll need to verify your identity. We've sent a verification code to %{email}"
 msgstr ""
@@ -21187,6 +21190,9 @@ msgstr ""
 msgid "IdentityVerification|We sent a new code to +%{phoneNumber}"
 msgstr ""
+msgid "IdentityVerification|We've sent a verification code to %{email}"
+msgstr ""
+
 msgid "IdentityVerification|We've sent a verification code to +%{phoneNumber}"
 msgstr ""