Fix 2FA management on Safari

Also add GitLab UI form validation and confirmation modal

Changelog: fixed

app/assets/javascripts/authentication/two_factor_auth/components/manage_two_factor_form.vue

 <script>
-import { GlFormInput, GlFormGroup, GlButton, GlForm } from '@gitlab/ui';
+import { GlFormInput, GlFormGroup, GlButton, GlForm, GlModal } from '@gitlab/ui';
 import csrf from '~/lib/utils/csrf';
 import { __ } from '~/locale';
 
 export const i18n = {
   currentPassword: __('Current password'),
+  confirmTitle: __('Are you sure?'),
   confirmWebAuthn: __(
-    'Are you sure? This will invalidate your registered applications and U2F / WebAuthn devices.',
+    'This will invalidate your registered applications and U2F / WebAuthn devices.',
   ),
-  confirm: __('Are you sure? This will invalidate your registered applications and U2F devices.'),
+  confirm: __('This will invalidate your registered applications and U2F devices.'),
   disableTwoFactor: __('Disable two-factor authentication'),
+  disable: __('Disable'),
+  cancel: __('Cancel'),
   regenerateRecoveryCodes: __('Regenerate recovery codes'),
+  currentPasswordInvalidFeedback: __('Please enter your current password.'),
 };
 
 export default {
   name: 'ManageTwoFactorForm',
   i18n,
+  modalId: 'manage-two-factor-auth-confirm-modal',
+  modalActions: {
+    primary: {
+      text: i18n.disable,
+      attributes: {
+        variant: 'danger',
+      },
+    },
+    secondary: {
+      text: i18n.cancel,
+      attributes: {
+        variant: 'default',
+      },
+    },
+  },
   components: {
     GlForm,
     GlFormInput,
     GlFormGroup,
     GlButton,
+    GlModal,
   },
   inject: [
     'webauthnEnabled',
@@ -32,8 +52,11 @@ export default {
   ],
   data() {
     return {
-      method: '',
-      action: '#',
+      method: null,
+      action: null,
+      currentPassword: '',
+      currentPasswordState: null,
+      showConfirmModal: false,
     };
   },
   computed: {
@@ -46,9 +69,34 @@ export default {
     },
   },
   methods: {
-    handleFormSubmit(event) {
-      this.method = event.submitter.dataset.formMethod;
-      this.action = event.submitter.dataset.formAction;
+    submitForm() {
+      this.$refs.form.$el.submit();
+    },
+    async handleSubmitButtonClick({ method, action, confirm = false }) {
+      this.method = method;
+      this.action = action;
+
+      if (this.isCurrentPasswordRequired && this.currentPassword === '') {
+        this.currentPasswordState = false;
+
+        return;
+      }
+
+      this.currentPasswordState = null;
+
+      if (confirm) {
+        this.showConfirmModal = true;
+
+        return;
+      }
+
+      // Wait for form action and method to be updated
+      await this.$nextTick();
+
+      this.submitForm();
+    },
+    handleModalPrimary() {
+      this.submitForm();
     },
   },
   csrf,
@@ -57,10 +105,11 @@ export default {
 
 <template>
   <gl-form
-    class="gl-display-inline-block"
+    ref="form"
+    class="gl-sm-display-inline-block"
     method="post"
     :action="action"
-    @submit="handleFormSubmit($event)"
+    @submit.prevent
   >
     <input type="hidden" name="_method" data-testid="test-2fa-method-field" :value="method" />
     <input :value="$options.csrf.token" type="hidden" name="authenticity_token" />
@@ -69,35 +118,59 @@ export default {
       v-if="isCurrentPasswordRequired"
       :label="$options.i18n.currentPassword"
       label-for="current-password"
+      :state="currentPasswordState"
+      :invalid-feedback="$options.i18n.currentPasswordInvalidFeedback"
     >
       <gl-form-input
         id="current-password"
+        v-model="currentPassword"
         type="password"
         name="current_password"
-        required
+        :state="currentPasswordState"
         data-qa-selector="current_password_field"
       />
     </gl-form-group>
 
-    <gl-button
-      type="submit"
-      class="btn-danger gl-mr-3 gl-display-inline-block"
-      data-testid="test-2fa-disable-button"
-      variant="danger"
-      :data-confirm="confirmText"
-      :data-form-action="profileTwoFactorAuthPath"
-      :data-form-method="profileTwoFactorAuthMethod"
-    >
-      {{ $options.i18n.disableTwoFactor }}
-    </gl-button>
-    <gl-button
-      type="submit"
-      class="gl-display-inline-block"
-      data-testid="test-2fa-regenerate-codes-button"
-      :data-form-action="codesProfileTwoFactorAuthPath"
-      :data-form-method="codesProfileTwoFactorAuthMethod"
+    <div class="gl-display-flex gl-flex-wrap">
+      <gl-button
+        type="submit"
+        class="gl-sm-mr-3 gl-w-full gl-sm-w-auto"
+        data-testid="test-2fa-disable-button"
+        variant="danger"
+        @click.prevent="
+          handleSubmitButtonClick({
+            method: profileTwoFactorAuthMethod,
+            action: profileTwoFactorAuthPath,
+            confirm: true,
+          })
+        "
+      >
+        {{ $options.i18n.disableTwoFactor }}
+      </gl-button>
+      <gl-button
+        type="submit"
+        class="gl-mt-3 gl-sm-mt-0 gl-w-full gl-sm-w-auto"
+        data-testid="test-2fa-regenerate-codes-button"
+        @click.prevent="
+          handleSubmitButtonClick({
+            method: codesProfileTwoFactorAuthMethod,
+            action: codesProfileTwoFactorAuthPath,
+          })
+        "
+      >
+        {{ $options.i18n.regenerateRecoveryCodes }}
+      </gl-button>
+    </div>
+    <gl-modal
+      v-model="showConfirmModal"
+      :modal-id="$options.modalId"
+      size="sm"
+      :title="$options.i18n.confirmTitle"
+      :action-primary="$options.modalActions.primary"
+      :action-secondary="$options.modalActions.secondary"
+      @primary="handleModalPrimary"
     >
-      {{ $options.i18n.regenerateRecoveryCodes }}
-    </gl-button>
+      {{ confirmText }}
+    </gl-modal>
   </gl-form>
 </template>

locale/gitlab.pot

@@ -4585,12 +4585,6 @@ msgstr ""
 msgid "Are you sure? The device will be signed out of GitLab and all remember me tokens revoked."
 msgstr ""
 
-msgid "Are you sure? This will invalidate your registered applications and U2F / WebAuthn devices."
-msgstr ""
-
-msgid "Are you sure? This will invalidate your registered applications and U2F devices."
-msgstr ""
-
 msgid "Arrange charts"
 msgstr ""
 
@@ -25684,6 +25678,9 @@ msgstr ""
 msgid "Please enter or upload a valid license."
 msgstr ""
 
+msgid "Please enter your current password."
+msgstr ""
+
 msgid "Please fill in a descriptive name for your group."
 msgstr ""
 
@@ -35275,6 +35272,12 @@ msgstr ""
 msgid "This variable can not be masked."
 msgstr ""
 
+msgid "This will invalidate your registered applications and U2F / WebAuthn devices."
+msgstr ""
+
+msgid "This will invalidate your registered applications and U2F devices."
+msgstr ""
+
 msgid "This will redirect you to an external sign in page."
 msgstr ""
 

spec/features/profiles/two_factor_auths_spec.rb

@@ -57,7 +57,9 @@ RSpec.describe 'Two factor auths' do
 
         click_button 'Disable two-factor authentication'
 
-        page.accept_alert
+        page.within('[role="dialog"]') do
+          click_button 'Disable'
+        end
 
         expect(page).to have_content('You must provide a valid current password')
 
@@ -65,7 +67,9 @@ RSpec.describe 'Two factor auths' do
 
         click_button 'Disable two-factor authentication'
 
-        page.accept_alert
+        page.within('[role="dialog"]') do
+          click_button 'Disable'
+        end
 
         expect(page).to have_content('Two-factor authentication has been disabled successfully!')
         expect(page).to have_content('Enable two-factor authentication')
@@ -95,7 +99,9 @@ RSpec.describe 'Two factor auths' do
 
           click_button 'Disable two-factor authentication'
 
-          page.accept_alert
+          page.within('[role="dialog"]') do
+            click_button 'Disable'
+          end
 
           expect(page).to have_content('Two-factor authentication has been disabled successfully!')
           expect(page).to have_content('Enable two-factor authentication')

spec/frontend/authentication/two_factor_auth/components/manage_two_factor_form_spec.js

-import { within } from '@testing-library/dom';
-import { GlForm } from '@gitlab/ui';
-import { mount } from '@vue/test-utils';
-import { extendedWrapper } from 'helpers/vue_test_utils_helper';
+import { GlForm, GlModal } from '@gitlab/ui';
+import { mountExtended } from 'helpers/vue_test_utils_helper';
+import { stubComponent } from 'helpers/stub_component';
 import ManageTwoFactorForm, {
   i18n,
 } from '~/authentication/two_factor_auth/components/manage_two_factor_form.vue';
@@ -17,100 +16,133 @@ describe('ManageTwoFactorForm', () => {
   let wrapper;
 
   const createComponent = (options = {}) => {
-    wrapper = extendedWrapper(
-      mount(ManageTwoFactorForm, {
-        provide: {
-          ...defaultProvide,
-          webauthnEnabled: options?.webauthnEnabled ?? false,
-          isCurrentPasswordRequired: options?.currentPasswordRequired ?? true,
-        },
-      }),
-    );
+    wrapper = mountExtended(ManageTwoFactorForm, {
+      provide: {
+        ...defaultProvide,
+        webauthnEnabled: options?.webauthnEnabled ?? false,
+        isCurrentPasswordRequired: options?.currentPasswordRequired ?? true,
+      },
+      stubs: {
+        GlModal: stubComponent(GlModal, {
+          template: `
+            <div>
+              <slot name="modal-title"></slot>
+              <slot></slot>
+              <slot name="modal-footer"></slot>
+            </div>`,
+        }),
+      },
+    });
   };
 
-  const queryByText = (text, options) => within(wrapper.element).queryByText(text, options);
-  const queryByLabelText = (text, options) =>
-    within(wrapper.element).queryByLabelText(text, options);
-
   const findForm = () => wrapper.findComponent(GlForm);
   const findMethodInput = () => wrapper.findByTestId('test-2fa-method-field');
   const findDisableButton = () => wrapper.findByTestId('test-2fa-disable-button');
   const findRegenerateCodesButton = () => wrapper.findByTestId('test-2fa-regenerate-codes-button');
+  const findConfirmationModal = () => wrapper.findComponent(GlModal);
+
+  const itShowsConfirmationModal = (confirmText) => {
+    it('shows confirmation modal', async () => {
+      await wrapper.findByLabelText('Current password').setValue('foo bar');
+      await findDisableButton().trigger('click');
+
+      expect(findConfirmationModal().props('visible')).toBe(true);
+      expect(findConfirmationModal().html()).toContain(confirmText);
+    });
+  };
+
+  const itShowsValidationMessageIfCurrentPasswordFieldIsEmpty = (findButtonFunction) => {
+    it('shows validation message if `Current password` is empty', async () => {
+      await findButtonFunction().trigger('click');
+
+      expect(wrapper.findByText(i18n.currentPasswordInvalidFeedback).exists()).toBe(true);
+    });
+  };
 
   beforeEach(() => {
     createComponent();
   });
 
-  describe('Current password field', () => {
-    it('renders the current password field', () => {
-      expect(queryByLabelText(i18n.currentPassword).tagName).toEqual('INPUT');
+  describe('`Current password` field', () => {
+    describe('when required', () => {
+      it('renders the current password field', () => {
+        expect(wrapper.findByLabelText(i18n.currentPassword).exists()).toBe(true);
+      });
     });
-  });
 
-  describe('when current password is not required', () => {
-    beforeEach(() => {
-      createComponent({
-        currentPasswordRequired: false,
+    describe('when not required', () => {
+      beforeEach(() => {
+        createComponent({
+          currentPasswordRequired: false,
+        });
       });
-    });
 
-    it('does not render the current password field', () => {
-      expect(queryByLabelText(i18n.currentPassword)).toBe(null);
+      it('does not render the current password field', () => {
+        expect(wrapper.findByLabelText(i18n.currentPassword).exists()).toBe(false);
+      });
     });
   });
 
   describe('Disable button', () => {
     it('renders the component with correct attributes', () => {
       expect(findDisableButton().exists()).toBe(true);
-      expect(findDisableButton().attributes()).toMatchObject({
-        'data-confirm': i18n.confirm,
-        'data-form-action': defaultProvide.profileTwoFactorAuthPath,
-        'data-form-method': defaultProvide.profileTwoFactorAuthMethod,
-      });
     });
 
-    it('has the right confirm text', () => {
-      expect(findDisableButton().attributes('data-confirm')).toBe(i18n.confirm);
-    });
+    describe('when clicked', () => {
+      itShowsValidationMessageIfCurrentPasswordFieldIsEmpty(findDisableButton);
 
-    describe('when webauthnEnabled', () => {
-      beforeEach(() => {
-        createComponent({
-          webauthnEnabled: true,
+      itShowsConfirmationModal(i18n.confirm);
+
+      describe('when webauthnEnabled', () => {
+        beforeEach(() => {
+          createComponent({
+            webauthnEnabled: true,
+          });
         });
-      });
 
-      it('has the right confirm text', () => {
-        expect(findDisableButton().attributes('data-confirm')).toBe(i18n.confirmWebAuthn);
+        itShowsConfirmationModal(i18n.confirmWebAuthn);
       });
-    });
 
-    it('modifies the form action and method when submitted through the button', async () => {
-      const form = findForm();
-      const disableButton = findDisableButton().element;
-      const methodInput = findMethodInput();
+      it('modifies the form action and method when submitted through the button', async () => {
+        const form = findForm();
+        const methodInput = findMethodInput();
+        const submitSpy = jest.spyOn(form.element, 'submit');
+
+        await wrapper.findByLabelText('Current password').setValue('foo bar');
+        await findDisableButton().trigger('click');
+
+        expect(form.attributes('action')).toBe(defaultProvide.profileTwoFactorAuthPath);
+        expect(methodInput.attributes('value')).toBe(defaultProvide.profileTwoFactorAuthMethod);
 
-      await form.vm.$emit('submit', { submitter: disableButton });
+        findConfirmationModal().vm.$emit('primary');
 
-      expect(form.attributes('action')).toBe(defaultProvide.profileTwoFactorAuthPath);
-      expect(methodInput.attributes('value')).toBe(defaultProvide.profileTwoFactorAuthMethod);
+        expect(submitSpy).toHaveBeenCalled();
+      });
     });
   });
 
   describe('Regenerate recovery codes button', () => {
     it('renders the button', () => {
-      expect(queryByText(i18n.regenerateRecoveryCodes)).toEqual(expect.any(HTMLElement));
+      expect(findRegenerateCodesButton().exists()).toBe(true);
     });
 
-    it('modifies the form action and method when submitted through the button', async () => {
-      const form = findForm();
-      const regenerateCodesButton = findRegenerateCodesButton().element;
-      const methodInput = findMethodInput();
+    describe('when clicked', () => {
+      itShowsValidationMessageIfCurrentPasswordFieldIsEmpty(findRegenerateCodesButton);
+
+      it('modifies the form action and method when submitted through the button', async () => {
+        const form = findForm();
+        const methodInput = findMethodInput();
+        const submitSpy = jest.spyOn(form.element, 'submit');
 
-      await form.vm.$emit('submit', { submitter: regenerateCodesButton });
+        await wrapper.findByLabelText('Current password').setValue('foo bar');
+        await findRegenerateCodesButton().trigger('click');
 
-      expect(form.attributes('action')).toBe(defaultProvide.codesProfileTwoFactorAuthPath);
-      expect(methodInput.attributes('value')).toBe(defaultProvide.codesProfileTwoFactorAuthMethod);
+        expect(form.attributes('action')).toBe(defaultProvide.codesProfileTwoFactorAuthPath);
+        expect(methodInput.attributes('value')).toBe(
+          defaultProvide.codesProfileTwoFactorAuthMethod,
+        );
+        expect(submitSpy).toHaveBeenCalled();
+      });
     });
   });
 });