Merge branch '338825-add-password-verification-to-2FA' into 'master'

Require password for 2FA changes

See merge request gitlab-org/security/gitlab!1713

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

+<script>
+import { GlFormInput, GlFormGroup, GlButton, GlForm } from '@gitlab/ui';
+import csrf from '~/lib/utils/csrf';
+import { __ } from '~/locale';
+
+export const i18n = {
+  currentPassword: __('Current password'),
+  confirmWebAuthn: __(
+    'Are you sure? This will invalidate your registered applications and U2F / WebAuthn devices.',
+  ),
+  confirm: __('Are you sure? This will invalidate your registered applications and U2F devices.'),
+  disableTwoFactor: __('Disable two-factor authentication'),
+  regenerateRecoveryCodes: __('Regenerate recovery codes'),
+};
+
+export default {
+  name: 'ManageTwoFactorForm',
+  i18n,
+  components: {
+    GlForm,
+    GlFormInput,
+    GlFormGroup,
+    GlButton,
+  },
+  inject: [
+    'webauthnEnabled',
+    'profileTwoFactorAuthPath',
+    'profileTwoFactorAuthMethod',
+    'codesProfileTwoFactorAuthPath',
+    'codesProfileTwoFactorAuthMethod',
+  ],
+  data() {
+    return {
+      method: '',
+      action: '#',
+    };
+  },
+  computed: {
+    confirmText() {
+      if (this.webauthnEnabled) {
+        return i18n.confirmWebAuthn;
+      }
+
+      return i18n.confirm;
+    },
+  },
+  methods: {
+    handleFormSubmit(event) {
+      this.method = event.submitter.dataset.formMethod;
+      this.action = event.submitter.dataset.formAction;
+    },
+  },
+  csrf,
+};
+</script>
+
+<template>
+  <gl-form
+    class="gl-display-inline-block"
+    method="post"
+    :action="action"
+    @submit="handleFormSubmit($event)"
+  >
+    <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-input
+        id="current-password"
+        type="password"
+        name="current_password"
+        required
+        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"
+    >
+      {{ $options.i18n.regenerateRecoveryCodes }}
+    </gl-button>
+  </gl-form>
+</template>

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

 import Vue from 'vue';
 import { updateHistory, removeParams } from '~/lib/utils/url_utility';
+import ManageTwoFactorForm from './components/manage_two_factor_form.vue';
 import RecoveryCodes from './components/recovery_codes.vue';
 import { SUCCESS_QUERY_PARAM } from './constants';
 
+export const initManageTwoFactorForm = () => {
+  const el = document.querySelector('.js-manage-two-factor-form');
+
+  if (!el) {
+    return false;
+  }
+
+  const {
+    webauthnEnabled = false,
+    profileTwoFactorAuthPath = '',
+    profileTwoFactorAuthMethod = '',
+    codesProfileTwoFactorAuthPath = '',
+    codesProfileTwoFactorAuthMethod = '',
+  } = el.dataset;
+
+  return new Vue({
+    el,
+    provide: {
+      webauthnEnabled,
+      profileTwoFactorAuthPath,
+      profileTwoFactorAuthMethod,
+      codesProfileTwoFactorAuthPath,
+      codesProfileTwoFactorAuthMethod,
+    },
+    render(createElement) {
+      return createElement(ManageTwoFactorForm);
+    },
+  });
+};
+
 export const initRecoveryCodes = () => {
   const el = document.querySelector('.js-2fa-recovery-codes');
 

app/assets/javascripts/pages/profiles/two_factor_auths/index.js

 import { mount2faRegistration } from '~/authentication/mount_2fa';
-import { initRecoveryCodes } from '~/authentication/two_factor_auth';
+import { initRecoveryCodes, initManageTwoFactorForm } from '~/authentication/two_factor_auth';
 import { parseBoolean } from '~/lib/utils/common_utils';
 
 const twoFactorNode = document.querySelector('.js-two-factor-auth');
@@ -14,3 +14,5 @@ if (skippable) {
 mount2faRegistration();
 
 initRecoveryCodes();
+
+initManageTwoFactorForm();

app/controllers/profiles/two_factor_auths_controller.rb

@@ -3,6 +3,8 @@
 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 do
     push_frontend_feature_flag(:webauthn)
   end
@@ -134,6 +136,14 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
 
   private
 
+  def validate_current_password
+    return if current_user.valid_password?(params[:current_password])
+
+    current_user.increment_failed_attempts!
+
+    redirect_to profile_two_factor_auth_path, alert: _('You must provide a valid current password')
+  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,13 +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.')
-        %div
-          = link_to _('Disable two-factor authentication'), profile_two_factor_auth_path,
-                method: :delete,
-                data: { confirm: webauthn_enabled ? _('Are you sure? This will invalidate your registered applications and U2F / WebAuthn devices.') : _('Are you sure? This will invalidate your registered applications and U2F devices.') },
-                class: 'gl-button btn btn-danger gl-mr-3'
-          = form_tag codes_profile_two_factor_auth_path, {style: 'display: inline-block', method: :post} do |f|
-            = submit_tag _('Regenerate recovery codes'), class: 'gl-button btn btn-default'
+        .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' } }
 
       - else
         %p
@@ -53,6 +47,11 @@
           .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' }
+          .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' }
+            %p.form-text.text-muted
+              = _('Your current password is required to register a two-factor authenticator app.')
           .gl-mt-3
             = submit_tag _('Register with two-factor app'), class: 'gl-button btn btn-confirm', data: { qa_selector: 'register_2fa_app_button' }
 

doc/user/profile/account/two_factor_authentication.md

@@ -75,6 +75,7 @@ To enable 2FA:
 1. **In GitLab:**
    1. Enter the six-digit pin number from the entry on your device into the **Pin
       code** field.
+   1. Enter your current password.
    1. Select **Submit**.
 
 If the pin you entered was correct, a message displays indicating that
@@ -365,7 +366,8 @@ If you ever need to disable 2FA:
 1. Sign in to your GitLab account.
 1. Go to your [**User settings**](../index.md#access-your-user-settings).
 1. Go to **Account**.
-1. Click **Disable**, under **Two-Factor Authentication**.
+1. Select **Manage two-factor authentication**.
+1. Under **Two-Factor Authentication**, enter your current password and select **Disable**.
 
 This clears all your two-factor authentication registrations, including mobile
 applications and U2F / WebAuthn devices.
@@ -460,7 +462,7 @@ To regenerate 2FA recovery codes, you need access to a desktop browser:
 1. Go to your [**User settings**](../index.md#access-your-user-settings).
 1. Select **Account > Two-Factor Authentication (2FA)**.
 1. If you've already configured 2FA, click **Manage two-factor authentication**.
-1. In the **Register Two-Factor Authenticator** pane, click **Regenerate recovery codes**.
+1. In the **Register Two-Factor Authenticator** pane, enter your current password and select **Regenerate recovery codes**.
 
 NOTE:
 If you regenerate 2FA recovery codes, save them. You can't use any previously created 2FA codes.

ee/spec/features/account_recovery_regular_check_spec.rb

@@ -29,6 +29,7 @@ RSpec.describe 'Account recovery regular check callout' do
           visit profile_two_factor_auth_path
 
           fill_in 'pin_code', with: user_two_factor_disabled.reload.current_otp
+          fill_in 'current_password', with: user_two_factor_disabled.password
 
           click_button 'Register with two-factor app'
 

locale/gitlab.pot

@@ -39235,6 +39235,9 @@ msgstr ""
 msgid "Your commit email is used for web based operations, such as edits and merges."
 msgstr ""
 
+msgid "Your current password is required to register a two-factor authenticator app."
+msgstr ""
+
 msgid "Your dashboard has been copied. You can %{web_ide_link_start}edit it here%{web_ide_link_end}."
 msgstr ""
 

spec/controllers/profiles/two_factor_auths_controller_spec.rb

@@ -35,6 +35,27 @@ RSpec.describe Profiles::TwoFactorAuthsController do
     end
   end
 
+  shared_examples 'user must enter a valid current password' do
+    let(:current_password) { '123' }
+
+    it 'requires the current password', :aggregate_failures do
+      go
+
+      expect(response).to redirect_to(profile_two_factor_auth_path)
+      expect(flash[:alert]).to eq(_('You must provide a valid current password'))
+    end
+
+    context 'when the user is on the last sign in attempt' do
+      it do
+        user.update!(failed_attempts: User.maximum_attempts.pred)
+
+        go
+
+        expect(user.reload).to be_access_locked
+      end
+    end
+  end
+
   describe 'GET show' do
     let_it_be_with_reload(:user) { create(:user) }
 
@@ -69,9 +90,10 @@ RSpec.describe Profiles::TwoFactorAuthsController do
     let_it_be_with_reload(:user) { create(:user) }
 
     let(:pin) { 'pin-code' }
+    let(:current_password) { user.password }
 
     def go
-      post :create, params: { pin_code: pin }
+      post :create, params: { pin_code: pin, current_password: current_password }
     end
 
     context 'with valid pin' do
@@ -136,21 +158,25 @@ RSpec.describe Profiles::TwoFactorAuthsController do
       end
     end
 
+    it_behaves_like 'user must enter a valid current password'
+
     it_behaves_like 'user must first verify their primary email address'
   end
 
   describe 'POST codes' do
     let_it_be_with_reload(:user) { create(:user, :two_factor) }
 
+    let(:current_password) { user.password }
+
     it 'presents plaintext codes for the user to save' do
       expect(user).to receive(:generate_otp_backup_codes!).and_return(%w(a b c))
 
-      post :codes
+      post :codes, params: { current_password: current_password }
       expect(assigns[:codes]).to match_array %w(a b c)
     end
 
     it 'persists the generated codes' do
-      post :codes
+      post :codes, params: { current_password: current_password }
 
       user.reload
       expect(user.otp_backup_codes).not_to be_empty
@@ -159,12 +185,18 @@ RSpec.describe Profiles::TwoFactorAuthsController do
     it 'dismisses the `TWO_FACTOR_AUTH_RECOVERY_SETTINGS_CHECK` callout' do
       expect(controller.helpers).to receive(:dismiss_two_factor_auth_recovery_settings_check)
 
-      post :codes
+      post :codes, params: { current_password: current_password }
+    end
+
+    it_behaves_like 'user must enter a valid current password' do
+      let(:go) { post :codes, params: { current_password: current_password } }
     end
   end
 
   describe 'DELETE destroy' do
-    subject { delete :destroy }
+    subject { delete :destroy, params: { current_password: current_password } }
+
+    let(:current_password) { user.password }
 
     context 'for a user that has 2FA enabled' do
       let_it_be_with_reload(:user) { create(:user, :two_factor) }
@@ -187,6 +219,10 @@ RSpec.describe Profiles::TwoFactorAuthsController do
         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
     end
 
     context 'for a user that does not have 2FA enabled' do

spec/features/profiles/two_factor_auths_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Two factor auths' do
+  context 'when signed in' do
+    before do
+      allow(Gitlab).to receive(:com?) { true }
+    end
+
+    context 'when user has two-factor authentication disabled' do
+      let(:user) { create(:user ) }
+
+      before do
+        sign_in(user)
+      end
+
+      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')
+
+        expect(page).to have_content('You must provide a valid current password')
+
+        register_2fa(user.reload.current_otp, user.password)
+
+        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
+
+    context 'when user has two-factor authentication enabled' do
+      let(:user) { create(:user, :two_factor) }
+
+      before do
+        sign_in(user)
+      end
+
+      it 'requires the current_password to disable two-factor authentication', :js do
+        visit profile_two_factor_auth_path
+
+        fill_in 'current_password', with: '123'
+
+        click_button 'Disable two-factor authentication'
+
+        page.accept_alert
+
+        expect(page).to have_content('You must provide a valid current password')
+
+        fill_in 'current_password', with: user.password
+
+        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 'requires the current_password to regernate recovery codes', :js do
+        visit profile_two_factor_auth_path
+
+        fill_in 'current_password', with: '123'
+
+        click_button 'Regenerate recovery codes'
+
+        expect(page).to have_content('You must provide a valid current password')
+
+        fill_in 'current_password', with: user.password
+
+        click_button 'Regenerate recovery codes'
+
+        expect(page).to have_content('Please copy, download, or print your recovery codes before proceeding.')
+      end
+    end
+
+    def register_2fa(pin, password)
+      fill_in 'pin_code', with: pin
+      fill_in 'current_password', with: password
+
+      click_button 'Register with two-factor app'
+    end
+  end
+end

spec/features/users/login_spec.rb

@@ -807,6 +807,7 @@ RSpec.describe 'Login', :clean_gitlab_redis_shared_state do
           expect(current_path).to eq(profile_two_factor_auth_path)
 
           fill_in 'pin_code', with: user.reload.current_otp
+          fill_in 'current_password', with: user.password
 
           click_button 'Register with two-factor app'
           click_button 'Copy codes'

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 { 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';
+
+describe('ManageTwoFactorForm', () => {
+  let wrapper;
+
+  const createComponent = (options = {}) => {
+    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',
+        },
+      }),
+    );
+  };
+
+  const queryByText = (text, options) => within(wrapper.element).queryByText(text, options);
+  const queryByLabelText = (text, options) =>
+    within(wrapper.element).queryByLabelText(text, options);
+
+  beforeEach(() => {
+    createComponent();
+  });
+
+  describe('Current password field', () => {
+    it('renders the current password field', () => {
+      expect(queryByLabelText(i18n.currentPassword).tagName).toEqual('INPUT');
+    });
+  });
+
+  describe('Disable button', () => {
+    it('renders the component correctly', () => {
+      expect(wrapper).toMatchSnapshot();
+      expect(wrapper.element).toMatchSnapshot();
+    });
+
+    it('has the right confirm text', () => {
+      expect(wrapper.findByTestId('test-2fa-disable-button').element.dataset.confirm).toEqual(
+        i18n.confirm,
+      );
+    });
+
+    describe('when webauthnEnabled', () => {
+      beforeEach(() => {
+        createComponent({
+          webauthnEnabled: true,
+        });
+      });
+
+      it('has the right confirm text', () => {
+        expect(wrapper.findByTestId('test-2fa-disable-button').element.dataset.confirm).toEqual(
+          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;
+
+      form.trigger('submit', { submitter: disableButton });
+
+      await wrapper.vm.$nextTick();
+
+      expect(form.element.getAttribute('action')).toEqual('2fa_auth_path');
+      expect(methodInput.getAttribute('value')).toEqual('2fa_auth_method');
+    });
+  });
+
+  describe('Regenerate recovery codes button', () => {
+    it('renders the button', () => {
+      expect(queryByText(i18n.regenerateRecoveryCodes)).toEqual(expect.any(HTMLElement));
+    });
+
+    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 });
+
+      await wrapper.vm.$nextTick();
+
+      expect(form.element.getAttribute('action')).toEqual('2fa_codes_path');
+      expect(methodInput.getAttribute('value')).toEqual('2fa_codes_method');
+    });
+  });
+});