Merge branch '342152-bugfix-2fa_for_users_without_password' into 'master'

Fix 2FA setup for users with no password

See merge request gitlab-org/gitlab!71694

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

@@ -24,6 +24,7 @@ export default {
   },
   inject: [
     'webauthnEnabled',
+    'isCurrentPasswordRequired',
     'profileTwoFactorAuthPath',
     'profileTwoFactorAuthMethod',
     'codesProfileTwoFactorAuthPath',
@@ -64,7 +65,11 @@ export default {
     <input type="hidden" name="_method" data-testid="test-2fa-method-field" :value="method" />
     <input :value="$options.csrf.token" type="hidden" name="authenticity_token" />
 
-    <gl-form-group :label="$options.i18n.currentPassword" label-for="current-password">
+    <gl-form-group
+      v-if="isCurrentPasswordRequired"
+      :label="$options.i18n.currentPassword"
+      label-for="current-password"
+    >
       <gl-form-input
         id="current-password"
         type="password"

app/assets/javascripts/authentication/two_factor_auth/index.js

 import Vue from 'vue';
+import { parseBoolean } from '~/lib/utils/common_utils';
 import { updateHistory, removeParams } from '~/lib/utils/url_utility';
 import ManageTwoFactorForm from './components/manage_two_factor_form.vue';
 import RecoveryCodes from './components/recovery_codes.vue';
@@ -13,16 +14,20 @@ export const initManageTwoFactorForm = () => {
 
   const {
     webauthnEnabled = false,
+    currentPasswordRequired,
     profileTwoFactorAuthPath = '',
     profileTwoFactorAuthMethod = '',
     codesProfileTwoFactorAuthPath = '',
     codesProfileTwoFactorAuthMethod = '',
   } = el.dataset;
 
+  const isCurrentPasswordRequired = parseBoolean(currentPasswordRequired);
+
   return new Vue({
     el,
     provide: {
       webauthnEnabled,
+      isCurrentPasswordRequired,
       profileTwoFactorAuthPath,
       profileTwoFactorAuthMethod,
       codesProfileTwoFactorAuthPath,

app/controllers/profiles/two_factor_auths_controller.rb

@@ -3,7 +3,9 @@
 class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
   skip_before_action :check_two_factor_requirement
   before_action :ensure_verified_primary_email, only: [:show, :create]
-  before_action :validate_current_password, only: [:create, :codes, :destroy]
+  before_action :validate_current_password, only: [:create, :codes, :destroy], if: :current_password_required?
+
+  helper_method :current_password_required?
 
   before_action do
     push_frontend_feature_flag(:webauthn)
@@ -144,6 +146,10 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
     redirect_to profile_two_factor_auth_path, alert: _('You must provide a valid current password')
   end
 
+  def current_password_required?
+    !current_user.password_automatically_set?
+  end
+
   def build_qr_code
     uri = current_user.otp_provisioning_uri(account_string, issuer: issuer_host)
     RQRCode.render_qrcode(uri, :svg, level: :m, unit: 3)

app/views/profiles/two_factor_auths/show.html.haml

@@ -17,7 +17,7 @@
           = _("You've already enabled two-factor authentication using one time password authenticators. In order to register a different device, you must first disable two-factor authentication.")
         %p
           = _('If you lose your recovery codes you can generate new ones, invalidating all previous codes.')
-        .js-manage-two-factor-form{ data: { webauthn_enabled: webauthn_enabled, profile_two_factor_auth_path: profile_two_factor_auth_path, profile_two_factor_auth_method: 'delete', codes_profile_two_factor_auth_path: codes_profile_two_factor_auth_path, codes_profile_two_factor_auth_method: 'post' } }
+        .js-manage-two-factor-form{ data: { webauthn_enabled: webauthn_enabled, current_password_required: current_password_required?.to_s, profile_two_factor_auth_path: profile_two_factor_auth_path, profile_two_factor_auth_method: 'delete', codes_profile_two_factor_auth_path: codes_profile_two_factor_auth_path, codes_profile_two_factor_auth_method: 'post' } }
 
       - else
         %p
@@ -47,6 +47,7 @@
           .form-group
             = label_tag :pin_code, _('Pin code'), class: "label-bold"
             = text_field_tag :pin_code, nil, class: "form-control gl-form-input", required: true, data: { qa_selector: 'pin_code_field' }
+          - if current_password_required?
             .form-group
               = label_tag :current_password, _('Current password'), class: 'label-bold'
               = password_field_tag :current_password, nil, required: true, class: 'form-control gl-form-input', data: { qa_selector: 'current_password_field' }

spec/controllers/profiles/two_factor_auths_controller_spec.rb

@@ -31,11 +31,12 @@ RSpec.describe Profiles::TwoFactorAuthsController do
 
   shared_examples 'user must enter a valid current password' do
     let(:current_password) { '123' }
+    let(:redirect_path) { profile_two_factor_auth_path }
 
     it 'requires the current password', :aggregate_failures do
       go
 
-      expect(response).to redirect_to(profile_two_factor_auth_path)
+      expect(response).to redirect_to(redirect_path)
       expect(flash[:alert]).to eq(_('You must provide a valid current password'))
     end
 
@@ -48,6 +49,19 @@ RSpec.describe Profiles::TwoFactorAuthsController do
         expect(user.reload).to be_access_locked
       end
     end
+
+    context 'when user authenticates with an external service' do
+      before do
+        allow(user).to receive(:password_automatically_set?).and_return(true)
+      end
+
+      it 'does not require the current password', :aggregate_failures do
+        go
+
+        expect(response).not_to redirect_to(redirect_path)
+        expect(flash[:alert]).to be_nil
+      end
+    end
   end
 
   describe 'GET show' do
@@ -188,7 +202,9 @@ RSpec.describe Profiles::TwoFactorAuthsController do
   end
 
   describe 'DELETE destroy' do
-    subject { delete :destroy, params: { current_password: current_password } }
+    def go
+      delete :destroy, params: { current_password: current_password }
+    end
 
     let(:current_password) { user.password }
 
@@ -196,40 +212,38 @@ RSpec.describe Profiles::TwoFactorAuthsController do
       let_it_be_with_reload(:user) { create(:user, :two_factor) }
 
       it 'disables two factor' do
-        subject
+        go
 
         expect(user.reload.two_factor_enabled?).to eq(false)
       end
 
       it 'redirects to profile_account_path' do
-        subject
+        go
 
         expect(response).to redirect_to(profile_account_path)
       end
 
       it 'displays a notice on success' do
-        subject
+        go
 
         expect(flash[:notice])
           .to eq _('Two-factor authentication has been disabled successfully!')
       end
 
-      it_behaves_like 'user must enter a valid current password' do
-        let(:go) { delete :destroy, params: { current_password: current_password } }
-      end
+      it_behaves_like 'user must enter a valid current password'
     end
 
     context 'for a user that does not have 2FA enabled' do
       let_it_be_with_reload(:user) { create(:user) }
 
       it 'redirects to profile_account_path' do
-        subject
+        go
 
         expect(response).to redirect_to(profile_account_path)
       end
 
       it 'displays an alert on failure' do
-        subject
+        go
 
         expect(flash[:alert])
           .to eq _('Two-factor authentication is not enabled for this user')

spec/features/profiles/two_factor_auths_spec.rb

@@ -5,20 +5,16 @@ require 'spec_helper'
 RSpec.describe 'Two factor auths' do
   context 'when signed in' do
     before do
-      allow(Gitlab).to receive(:com?) { true }
+      sign_in(user)
     end
 
     context 'when user has two-factor authentication disabled' do
-      let(:user) { create(:user ) }
-
-      before do
-        sign_in(user)
-      end
+      let_it_be(:user) { create(:user ) }
 
       it 'requires the current password to set up two factor authentication', :js do
         visit profile_two_factor_auth_path
 
-        register_2fa(user.reload.current_otp, '123')
+        register_2fa(user.current_otp, '123')
 
         expect(page).to have_content('You must provide a valid current password')
 
@@ -31,15 +27,29 @@ RSpec.describe 'Two factor auths' do
 
         expect(page).to have_content('Status: Enabled')
       end
-    end
 
-    context 'when user has two-factor authentication enabled' do
-      let(:user) { create(:user, :two_factor) }
+      context 'when user authenticates with an external service' do
+        let_it_be(:user) { create(:omniauth_user) }
 
-      before do
-        sign_in(user)
+        it 'does not require the current password to set up two factor authentication', :js do
+          visit profile_two_factor_auth_path
+
+          fill_in 'pin_code', with: user.current_otp
+          click_button 'Register with two-factor app'
+
+          expect(page).to have_content('Please copy, download, or print your recovery codes before proceeding.')
+
+          click_button 'Copy codes'
+          click_link 'Proceed'
+
+          expect(page).to have_content('Status: Enabled')
+        end
+      end
     end
 
+    context 'when user has two-factor authentication enabled' do
+      let_it_be(:user) { create(:user, :two_factor) }
+
       it 'requires the current_password to disable two-factor authentication', :js do
         visit profile_two_factor_auth_path
 
@@ -61,7 +71,7 @@ RSpec.describe 'Two factor auths' do
         expect(page).to have_content('Enable two-factor authentication')
       end
 
-      it 'requires the current_password to regernate recovery codes', :js do
+      it 'requires the current_password to regenerate recovery codes', :js do
         visit profile_two_factor_auth_path
 
         fill_in 'current_password', with: '123'
@@ -76,6 +86,29 @@ RSpec.describe 'Two factor auths' do
 
         expect(page).to have_content('Please copy, download, or print your recovery codes before proceeding.')
       end
+
+      context 'when user authenticates with an external service' do
+        let_it_be(:user) { create(:omniauth_user, :two_factor) }
+
+        it 'does not require the current_password to disable two-factor authentication', :js do
+          visit profile_two_factor_auth_path
+
+          click_button 'Disable two-factor authentication'
+
+          page.accept_alert
+
+          expect(page).to have_content('Two-factor authentication has been disabled successfully!')
+          expect(page).to have_content('Enable two-factor authentication')
+        end
+
+        it 'does not require the current_password to regenerate recovery codes', :js do
+          visit profile_two_factor_auth_path
+
+          click_button 'Regenerate recovery codes'
+
+          expect(page).to have_content('Please copy, download, or print your recovery codes before proceeding.')
+        end
+      end
     end
 
     def register_2fa(pin, password)

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

-// Jest Snapshot v1, https://goo.gl/fbAQLP
-
-exports[`ManageTwoFactorForm Disable button renders the component correctly 1`] = `
-VueWrapper {
-  "_emitted": Object {},
-  "_emittedByOrder": Array [],
-  "isFunctionalComponent": undefined,
-}
-`;
-
-exports[`ManageTwoFactorForm Disable button renders the component correctly 2`] = `
-<form
-  action="#"
-  class="gl-display-inline-block"
-  method="post"
->
-  <input
-    data-testid="test-2fa-method-field"
-    name="_method"
-    type="hidden"
-  />
-   
-  <input
-    name="authenticity_token"
-    type="hidden"
-  />
-   
-  <div
-    class="form-group gl-form-group"
-    id="__BVID__15"
-    role="group"
-  >
-    <label
-      class="d-block col-form-label"
-      for="current-password"
-      id="__BVID__15__BV_label_"
-    >
-      Current password
-    </label>
-    <div
-      class="bv-no-focus-ring"
-    >
-      <input
-        aria-required="true"
-        class="gl-form-input form-control"
-        data-qa-selector="current_password_field"
-        id="current-password"
-        name="current_password"
-        required="required"
-        type="password"
-      />
-      <!---->
-      <!---->
-      <!---->
-    </div>
-  </div>
-   
-  <button
-    class="btn btn-danger gl-mr-3 gl-display-inline-block btn-danger btn-md gl-button"
-    data-confirm="Are you sure? This will invalidate your registered applications and U2F devices."
-    data-form-action="2fa_auth_path"
-    data-form-method="2fa_auth_method"
-    data-testid="test-2fa-disable-button"
-    type="submit"
-  >
-    <!---->
-     
-    <!---->
-      
-    <span
-      class="gl-button-text"
-    >
-      
-    Disable two-factor authentication
-  
-    </span>
-  </button>
-   
-  <button
-    class="btn gl-display-inline-block btn-default btn-md gl-button"
-    data-form-action="2fa_codes_path"
-    data-form-method="2fa_codes_method"
-    data-testid="test-2fa-regenerate-codes-button"
-    type="submit"
-  >
-    <!---->
-     
-    <!---->
-      
-    <span
-      class="gl-button-text"
-    >
-      
-    Regenerate recovery codes
-  
-    </span>
-  </button>
-</form>
-`;

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 ManageTwoFactorForm, {
   i18n,
 } from '~/authentication/two_factor_auth/components/manage_two_factor_form.vue';
 
+const defaultProvide = {
+  profileTwoFactorAuthPath: '2fa_auth_path',
+  profileTwoFactorAuthMethod: '2fa_auth_method',
+  codesProfileTwoFactorAuthPath: '2fa_codes_path',
+  codesProfileTwoFactorAuthMethod: '2fa_codes_method',
+};
+
 describe('ManageTwoFactorForm', () => {
   let wrapper;
 
@@ -12,11 +20,9 @@ describe('ManageTwoFactorForm', () => {
     wrapper = extendedWrapper(
       mount(ManageTwoFactorForm, {
         provide: {
-          webauthnEnabled: options?.webauthnEnabled || false,
-          profileTwoFactorAuthPath: '2fa_auth_path',
-          profileTwoFactorAuthMethod: '2fa_auth_method',
-          codesProfileTwoFactorAuthPath: '2fa_codes_path',
-          codesProfileTwoFactorAuthMethod: '2fa_codes_method',
+          ...defaultProvide,
+          webauthnEnabled: options?.webauthnEnabled ?? false,
+          isCurrentPasswordRequired: options?.currentPasswordRequired ?? true,
         },
       }),
     );
@@ -26,6 +32,11 @@ describe('ManageTwoFactorForm', () => {
   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');
+
   beforeEach(() => {
     createComponent();
   });
@@ -36,16 +47,30 @@ describe('ManageTwoFactorForm', () => {
     });
   });
 
+  describe('when current password is not required', () => {
+    beforeEach(() => {
+      createComponent({
+        currentPasswordRequired: false,
+      });
+    });
+
+    it('does not render the current password field', () => {
+      expect(queryByLabelText(i18n.currentPassword)).toBe(null);
+    });
+  });
+
   describe('Disable button', () => {
-    it('renders the component correctly', () => {
-      expect(wrapper).toMatchSnapshot();
-      expect(wrapper.element).toMatchSnapshot();
+    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(wrapper.findByTestId('test-2fa-disable-button').element.dataset.confirm).toEqual(
-        i18n.confirm,
-      );
+      expect(findDisableButton().attributes('data-confirm')).toBe(i18n.confirm);
     });
 
     describe('when webauthnEnabled', () => {
@@ -56,23 +81,19 @@ describe('ManageTwoFactorForm', () => {
       });
 
       it('has the right confirm text', () => {
-        expect(wrapper.findByTestId('test-2fa-disable-button').element.dataset.confirm).toEqual(
-          i18n.confirmWebAuthn,
-        );
+        expect(findDisableButton().attributes('data-confirm')).toBe(i18n.confirmWebAuthn);
       });
     });
 
     it('modifies the form action and method when submitted through the button', async () => {
-      const form = wrapper.find('form');
-      const disableButton = wrapper.findByTestId('test-2fa-disable-button').element;
-      const methodInput = wrapper.findByTestId('test-2fa-method-field').element;
+      const form = findForm();
+      const disableButton = findDisableButton().element;
+      const methodInput = findMethodInput();
 
-      form.trigger('submit', { submitter: disableButton });
+      await form.vm.$emit('submit', { submitter: disableButton });
 
-      await wrapper.vm.$nextTick();
-
-      expect(form.element.getAttribute('action')).toEqual('2fa_auth_path');
-      expect(methodInput.getAttribute('value')).toEqual('2fa_auth_method');
+      expect(form.attributes('action')).toBe(defaultProvide.profileTwoFactorAuthPath);
+      expect(methodInput.attributes('value')).toBe(defaultProvide.profileTwoFactorAuthMethod);
     });
   });
 
@@ -82,17 +103,14 @@ describe('ManageTwoFactorForm', () => {
     });
 
     it('modifies the form action and method when submitted through the button', async () => {
-      const form = wrapper.find('form');
-      const regenerateCodesButton = wrapper.findByTestId('test-2fa-regenerate-codes-button')
-        .element;
-      const methodInput = wrapper.findByTestId('test-2fa-method-field').element;
-
-      form.trigger('submit', { submitter: regenerateCodesButton });
+      const form = findForm();
+      const regenerateCodesButton = findRegenerateCodesButton().element;
+      const methodInput = findMethodInput();
 
-      await wrapper.vm.$nextTick();
+      await form.vm.$emit('submit', { submitter: regenerateCodesButton });
 
-      expect(form.element.getAttribute('action')).toEqual('2fa_codes_path');
-      expect(methodInput.getAttribute('value')).toEqual('2fa_codes_method');
+      expect(form.attributes('action')).toBe(defaultProvide.codesProfileTwoFactorAuthPath);
+      expect(methodInput.attributes('value')).toBe(defaultProvide.codesProfileTwoFactorAuthMethod);
     });
   });
 });