From 8a2a8c40a84b97bd1df668b3458cf61cadce1c2a Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Sep 2021 12:53:15 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@14-3-stable-ee --- .../components/manage_two_factor_form.vue | 98 +++++++++++++++++++++ .../authentication/two_factor_auth/index.js | 31 +++++++ .../pages/profiles/two_factor_auths/index.js | 4 +- .../profiles/two_factor_auths_controller.rb | 10 +++ app/views/profiles/two_factor_auths/show.html.haml | 13 ++- .../profile/account/two_factor_authentication.md | 6 +- locale/gitlab.pot | 3 + .../profiles/two_factor_auths_controller_spec.rb | 46 ++++++++-- spec/features/profiles/two_factor_auths_spec.rb | 88 +++++++++++++++++++ spec/features/users/login_spec.rb | 1 + .../manage_two_factor_form_spec.js.snap | 99 ++++++++++++++++++++++ .../components/manage_two_factor_form_spec.js | 98 +++++++++++++++++++++ 12 files changed, 482 insertions(+), 15 deletions(-) create mode 100644 app/assets/javascripts/authentication/two_factor_auth/components/manage_two_factor_form.vue create mode 100644 spec/features/profiles/two_factor_auths_spec.rb create mode 100644 spec/frontend/authentication/two_factor_auth/components/__snapshots__/manage_two_factor_form_spec.js.snap create mode 100644 spec/frontend/authentication/two_factor_auth/components/manage_two_factor_form_spec.js diff --git a/app/assets/javascripts/authentication/two_factor_auth/components/manage_two_factor_form.vue b/app/assets/javascripts/authentication/two_factor_auth/components/manage_two_factor_form.vue new file mode 100644 index 00000000000..280c222c380 --- /dev/null +++ b/app/assets/javascripts/authentication/two_factor_auth/components/manage_two_factor_form.vue @@ -0,0 +1,98 @@ + + + diff --git a/app/assets/javascripts/authentication/two_factor_auth/index.js b/app/assets/javascripts/authentication/two_factor_auth/index.js index 5e59c44e8cd..f663c0705e6 100644 --- a/app/assets/javascripts/authentication/two_factor_auth/index.js +++ b/app/assets/javascripts/authentication/two_factor_auth/index.js @@ -1,8 +1,39 @@ 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'); diff --git a/app/assets/javascripts/pages/profiles/two_factor_auths/index.js b/app/assets/javascripts/pages/profiles/two_factor_auths/index.js index 50835333a54..f6f136f2402 100644 --- a/app/assets/javascripts/pages/profiles/two_factor_auths/index.js +++ b/app/assets/javascripts/pages/profiles/two_factor_auths/index.js @@ -1,5 +1,5 @@ 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(); diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 5eb46421583..d1b9485f06d 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/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) diff --git a/app/views/profiles/two_factor_auths/show.html.haml b/app/views/profiles/two_factor_auths/show.html.haml index 927b6d4edef..d1d6b6301b8 100644 --- a/app/views/profiles/two_factor_auths/show.html.haml +++ b/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' } diff --git a/doc/user/profile/account/two_factor_authentication.md b/doc/user/profile/account/two_factor_authentication.md index 14e6f4dad3a..44537707db6 100644 --- a/doc/user/profile/account/two_factor_authentication.md +++ b/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. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index ae3fbe248bb..6c0b33911ca 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -39091,6 +39091,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 "" diff --git a/spec/controllers/profiles/two_factor_auths_controller_spec.rb b/spec/controllers/profiles/two_factor_auths_controller_spec.rb index 073180cbafd..a0e2cf671af 100644 --- a/spec/controllers/profiles/two_factor_auths_controller_spec.rb +++ b/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 diff --git a/spec/features/profiles/two_factor_auths_spec.rb b/spec/features/profiles/two_factor_auths_spec.rb new file mode 100644 index 00000000000..e1feca5031a --- /dev/null +++ b/spec/features/profiles/two_factor_auths_spec.rb @@ -0,0 +1,88 @@ +# 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 diff --git a/spec/features/users/login_spec.rb b/spec/features/users/login_spec.rb index afd750d02eb..79c4057a8b9 100644 --- a/spec/features/users/login_spec.rb +++ b/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' diff --git a/spec/frontend/authentication/two_factor_auth/components/__snapshots__/manage_two_factor_form_spec.js.snap b/spec/frontend/authentication/two_factor_auth/components/__snapshots__/manage_two_factor_form_spec.js.snap new file mode 100644 index 00000000000..3fe0e570a54 --- /dev/null +++ b/spec/frontend/authentication/two_factor_auth/components/__snapshots__/manage_two_factor_form_spec.js.snap @@ -0,0 +1,99 @@ +// 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`] = ` +
+ + + + +
+ +
+ + + + +
+
+ + + + +
+`; diff --git a/spec/frontend/authentication/two_factor_auth/components/manage_two_factor_form_spec.js b/spec/frontend/authentication/two_factor_auth/components/manage_two_factor_form_spec.js new file mode 100644 index 00000000000..384579c6876 --- /dev/null +++ b/spec/frontend/authentication/two_factor_auth/components/manage_two_factor_form_spec.js @@ -0,0 +1,98 @@ +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'); + }); + }); +}); -- cgit v1.2.3