Merge branch 'add-password-confirmation-on-user-email-change' into 'master'

Require password confirmation when user changes their primary email

See merge request gitlab-org/gitlab!69439

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

@@ -2,6 +2,7 @@ import $ from 'jquery';
 import '~/profile/gl_crop';
 import Profile from '~/profile/profile';
 import initSearchSettings from '~/search_settings';
+import initPasswordPrompt from './password_prompt';
 
 // eslint-disable-next-line func-names
 $(document).on('input.ssh_key', '#key_key', function () {
@@ -19,3 +20,4 @@ $(document).on('input.ssh_key', '#key_key', function () {
 new Profile(); // eslint-disable-line no-new
 
 initSearchSettings();
+initPasswordPrompt();

app/assets/javascripts/pages/profiles/password_prompt/constants.js

+import { __, s__ } from '~/locale';
+
+export const I18N_PASSWORD_PROMPT_TITLE = s__('PasswordPrompt|Confirm password to continue');
+export const I18N_PASSWORD_PROMPT_FORM_LABEL = s__(
+  'PasswordPrompt|Please enter your password to confirm',
+);
+export const I18N_PASSWORD_PROMPT_ERROR_MESSAGE = s__('PasswordPrompt|Password is required');
+export const I18N_PASSWORD_PROMPT_CONFIRM_BUTTON = s__('PasswordPrompt|Confirm password');
+export const I18N_PASSWORD_PROMPT_CANCEL_BUTTON = __('Cancel');

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

+import Vue from 'vue';
+import Translate from '~/vue_shared/translate';
+import PasswordPromptModal from './password_prompt_modal.vue';
+
+Vue.use(Translate);
+
+const emailFieldSelector = '#user_email';
+const editFormSelector = '.js-password-prompt-form';
+const passwordPromptFieldSelector = '.js-password-prompt-field';
+const passwordPromptBtnSelector = '.js-password-prompt-btn';
+
+const passwordPromptModalId = 'password-prompt-modal';
+
+const getEmailValue = () => document.querySelector(emailFieldSelector).value.trim();
+const passwordPromptButton = document.querySelector(passwordPromptBtnSelector);
+const field = document.querySelector(passwordPromptFieldSelector);
+const form = document.querySelector(editFormSelector);
+
+const handleConfirmPassword = (pw) => {
+  // update the validation_password field
+  field.value = pw;
+  // submit the form
+  form.submit();
+};
+
+export default () => {
+  const passwordPromptModalEl = document.getElementById(passwordPromptModalId);
+
+  if (passwordPromptModalEl && field) {
+    return new Vue({
+      el: passwordPromptModalEl,
+      data() {
+        return {
+          initialEmail: '',
+        };
+      },
+      mounted() {
+        this.initialEmail = getEmailValue();
+        passwordPromptButton.addEventListener('click', this.handleSettingsUpdate);
+      },
+      methods: {
+        handleSettingsUpdate(ev) {
+          const email = getEmailValue();
+          if (email !== this.initialEmail) {
+            ev.preventDefault();
+            this.$root.$emit('bv::show::modal', passwordPromptModalId, passwordPromptBtnSelector);
+          }
+        },
+      },
+      render(createElement) {
+        return createElement(PasswordPromptModal, {
+          props: { handleConfirmPassword },
+        });
+      },
+    });
+  }
+  return null;
+};

app/assets/javascripts/pages/profiles/password_prompt/password_prompt_modal.vue

+<script>
+import { GlModal, GlForm, GlFormGroup, GlFormInput } from '@gitlab/ui';
+import {
+  I18N_PASSWORD_PROMPT_TITLE,
+  I18N_PASSWORD_PROMPT_FORM_LABEL,
+  I18N_PASSWORD_PROMPT_ERROR_MESSAGE,
+  I18N_PASSWORD_PROMPT_CANCEL_BUTTON,
+  I18N_PASSWORD_PROMPT_CONFIRM_BUTTON,
+} from './constants';
+
+export default {
+  components: {
+    GlModal,
+    GlForm,
+    GlFormGroup,
+    GlFormInput,
+  },
+  props: {
+    handleConfirmPassword: {
+      type: Function,
+      required: true,
+    },
+  },
+  data() {
+    return {
+      passwordCheck: '',
+    };
+  },
+  computed: {
+    isValid() {
+      return Boolean(this.passwordCheck.length);
+    },
+    primaryProps() {
+      return {
+        text: I18N_PASSWORD_PROMPT_CONFIRM_BUTTON,
+        attributes: [{ variant: 'danger' }, { category: 'primary' }, { disabled: !this.isValid }],
+      };
+    },
+  },
+  methods: {
+    onConfirmPassword() {
+      this.handleConfirmPassword(this.passwordCheck);
+    },
+  },
+  cancelProps: {
+    text: I18N_PASSWORD_PROMPT_CANCEL_BUTTON,
+  },
+  i18n: {
+    title: I18N_PASSWORD_PROMPT_TITLE,
+    formLabel: I18N_PASSWORD_PROMPT_FORM_LABEL,
+    errorMessage: I18N_PASSWORD_PROMPT_ERROR_MESSAGE,
+  },
+};
+</script>
+
+<template>
+  <gl-modal
+    data-testid="password-prompt-modal"
+    modal-id="password-prompt-modal"
+    :title="$options.i18n.title"
+    :action-primary="primaryProps"
+    :action-cancel="$options.cancelProps"
+    @primary="onConfirmPassword"
+  >
+    <gl-form @submit.prevent="onConfirmPassword">
+      <gl-form-group
+        :label="$options.i18n.formLabel"
+        label-for="password-prompt-confirmation"
+        :invalid-feedback="$options.i18n.errorMessage"
+        :state="isValid"
+      >
+        <gl-form-input
+          id="password-prompt-confirmation"
+          v-model="passwordCheck"
+          name="password-confirmation"
+          type="password"
+          data-testid="password-prompt-field"
+        />
+      </gl-form-group>
+    </gl-form>
+  </gl-modal>
+</template>

app/controllers/profiles_controller.rb

@@ -18,7 +18,7 @@ class ProfilesController < Profiles::ApplicationController
 
   def update
     respond_to do |format|
-      result = Users::UpdateService.new(current_user, user_params.merge(user: @user)).execute
+      result = Users::UpdateService.new(current_user, user_params.merge(user: @user)).execute(check_password: true)
 
       if result[:status] == :success
         message = s_("Profiles|Profile was successfully updated")
@@ -129,6 +129,7 @@ class ProfilesController < Profiles::ApplicationController
       :job_title,
       :pronouns,
       :pronunciation,
+      :validation_password,
       status: [:emoji, :message, :availability]
     ]
   end

app/services/users/update_service.rb

@@ -5,15 +5,18 @@ module Users
     include NewUserNotifier
     attr_reader :user, :identity_params
 
+    ATTRS_REQUIRING_PASSWORD_CHECK = %w[email].freeze
+
     def initialize(current_user, params = {})
       @current_user = current_user
+      @validation_password = params.delete(:validation_password)
       @user = params.delete(:user)
       @status_params = params.delete(:status)
       @identity_params = params.slice(*identity_attributes)
       @params = params.dup
     end
 
-    def execute(validate: true, &block)
+    def execute(validate: true, check_password: false, &block)
       yield(@user) if block_given?
 
       user_exists = @user.persisted?
@@ -21,6 +24,11 @@ module Users
 
       discard_read_only_attributes
       assign_attributes
+
+      if check_password && require_password_check? && !@user.valid_password?(@validation_password)
+        return error(s_("Profiles|Invalid password"))
+      end
+
       assign_identity
       build_canonical_email
 
@@ -32,8 +40,8 @@ module Users
       end
     end
 
-    def execute!(*args, &block)
-      result = execute(*args, &block)
+    def execute!(*args, **kargs, &block)
+      result = execute(*args, **kargs, &block)
 
       raise ActiveRecord::RecordInvalid, @user unless result[:status] == :success
 
@@ -42,6 +50,14 @@ module Users
 
     private
 
+    def require_password_check?
+      return false unless @user.persisted?
+      return false if @user.password_automatically_set?
+
+      changes = @user.changed
+      ATTRS_REQUIRING_PASSWORD_CHECK.any? { |param| changes.include?(param) }
+    end
+
     def build_canonical_email
       return unless @user.email_changed?
 

app/views/profiles/_email_settings.html.haml

@@ -3,8 +3,11 @@
 - email_change_disabled = local_assigns.fetch(:email_change_disabled, nil)
 - read_only_help_text = readonly ? s_("Profiles|Your email address was automatically set based on your %{provider_label} account") % { provider_label: attribute_provider_label(:email) } : user_email_help_text(@user)
 - help_text = email_change_disabled ? s_("Your account uses dedicated credentials for the \"%{group_name}\" group and can only be updated through SSO.") % { group_name: @user.managing_group.name } : read_only_help_text
+- password_automatically_set = @user.password_automatically_set?
 
 = form.text_field :email, required: true, class: 'input-lg gl-form-input', value: (@user.email unless @user.temp_oauth_email?), help: help_text.html_safe, readonly: readonly || email_change_disabled
+- unless password_automatically_set
+  = hidden_field_tag 'user[validation_password]', :validation_password, class: 'js-password-prompt-field', help: s_("Profiles|Enter your password to confirm the email change")
 = form.select :public_email, options_for_select(@user.public_verified_emails, selected: @user.public_email),
   { help: s_("Profiles|This email will be displayed on your public profile"), include_blank: s_("Profiles|Do not show on profile") },
   control_class: 'select2 input-lg', disabled: email_change_disabled

app/views/profiles/show.html.haml

@@ -5,7 +5,7 @@
 - availability = availability_values
 - custom_emoji = show_status_emoji?(@user.status)
 
-= bootstrap_form_for @user, url: profile_path, method: :put, html: { multipart: true, class: 'edit-user gl-mt-3 js-quick-submit gl-show-field-errors' }, authenticity_token: true do |f|
+= bootstrap_form_for @user, url: profile_path, method: :put, html: { multipart: true, class: 'edit-user gl-mt-3 js-quick-submit gl-show-field-errors js-password-prompt-form', remote: true }, authenticity_token: true do |f|
   = form_errors(@user)
 
   .row.js-search-settings-section
@@ -124,9 +124,11 @@
       .help-block
         = s_("Profiles|Choose to show contributions of private projects on your public profile without any project, repository or organization information")
   %hr
-  = f.submit s_("Profiles|Update profile settings"), class: 'gl-button btn btn-confirm gl-mr-3'
+  = f.submit s_("Profiles|Update profile settings"), class: 'gl-button btn btn-confirm gl-mr-3 js-password-prompt-btn'
   = link_to _("Cancel"), user_path(current_user), class: 'gl-button btn btn-default btn-cancel'
 
+#password-prompt-modal
+
 .modal.modal-profile-crop{ data: { cropper_css_path: ActionController::Base.helpers.stylesheet_path('lazy_bundles/cropper.css') } }
   .modal-dialog
     .modal-content

lib/api/users.rb

@@ -1085,7 +1085,6 @@ module API
         attrs = declared_params(include_missing: false)
 
         service = ::UserPreferences::UpdateService.new(current_user, attrs).execute
-
         if service.success?
           present preferences, with: Entities::UserPreferences
         else

locale/gitlab.pot

@@ -24487,6 +24487,18 @@ msgstr ""
 msgid "Password was successfully updated. Please sign in again."
 msgstr ""
 
+msgid "PasswordPrompt|Confirm password"
+msgstr ""
+
+msgid "PasswordPrompt|Confirm password to continue"
+msgstr ""
+
+msgid "PasswordPrompt|Password is required"
+msgstr ""
+
+msgid "PasswordPrompt|Please enter your password to confirm"
+msgstr ""
+
 msgid "Passwords should be unique and not used for any other sites or services."
 msgstr ""
 
@@ -25933,6 +25945,9 @@ msgstr ""
 msgid "Profiles|Enter your name, so people you know can recognize you"
 msgstr ""
 
+msgid "Profiles|Enter your password to confirm the email change"
+msgstr ""
+
 msgid "Profiles|Enter your pronouns to let people know how to refer to you"
 msgstr ""
 

spec/controllers/profiles_controller_spec.rb

@@ -3,7 +3,8 @@
 require('spec_helper')
 
 RSpec.describe ProfilesController, :request_store do
-  let(:user) { create(:user) }
+  let(:password) { 'longsecret987!' }
+  let(:user) { create(:user, password: password) }
 
   describe 'POST update' do
     it 'does not update password' do
@@ -23,7 +24,7 @@ RSpec.describe ProfilesController, :request_store do
       sign_in(user)
 
       put :update,
-          params: { user: { email: "john@gmail.com", name: "John" } }
+          params: { user: { email: "john@gmail.com", name: "John", validation_password: password } }
 
       user.reload
 

spec/factories/users.rb

@@ -139,6 +139,8 @@ FactoryBot.define do
     end
 
     factory :omniauth_user do
+      password_automatically_set { true }
+
       transient do
         extern_uid { '123456' }
         provider { 'ldapmain' }

spec/features/admin/admin_mode/login_spec.rb

@@ -121,7 +121,7 @@ RSpec.describe 'Admin Mode Login' do
       end
 
       context 'when logging in via omniauth' do
-        let(:user) { create(:omniauth_user, :admin, :two_factor, extern_uid: 'my-uid', provider: 'saml')}
+        let(:user) { create(:omniauth_user, :admin, :two_factor, extern_uid: 'my-uid', provider: 'saml', password_automatically_set: false)}
         let(:mock_saml_response) do
           File.read('spec/fixtures/authentication/saml_response.xml')
         end

spec/features/profiles/user_edit_profile_spec.rb

@@ -19,6 +19,17 @@ RSpec.describe 'User edit profile' do
     wait_for_requests if respond_to?(:wait_for_requests)
   end
 
+  def update_user_email
+    fill_in 'user_email', with: 'new-email@example.com'
+    click_button 'Update profile settings'
+  end
+
+  def confirm_password(password)
+    fill_in 'password-confirmation', with: password
+    click_button 'Confirm password'
+    wait_for_requests if respond_to?(:wait_for_requests)
+  end
+
   def visit_user
     visit user_path(user)
     wait_for_requests
@@ -88,16 +99,42 @@ RSpec.describe 'User edit profile' do
     expect(page).to have_content('Website url is not a valid URL')
   end
 
-  describe 'when I change my email' do
+  describe 'when I change my email', :js do
     before do
       user.send_reset_password_instructions
     end
 
+    it 'will prompt to confirm my password' do
+      expect(user.reset_password_token?).to be true
+
+      update_user_email
+
+      expect(page).to have_selector('[data-testid="password-prompt-modal"]')
+    end
+
+    context 'when prompted to confirm password' do
+      before do
+        update_user_email
+      end
+
+      it 'with the correct password successfully updates' do
+        confirm_password(user.password)
+
+        expect(page).to have_text("Profile was successfully updated")
+      end
+
+      it 'with the incorrect password fails to update' do
+        confirm_password("Fake password")
+
+        expect(page).to have_text("Invalid password")
+      end
+    end
+
     it 'clears the reset password token' do
       expect(user.reset_password_token?).to be true
 
-      fill_in 'user_email', with: 'new-email@example.com'
-      submit_settings
+      update_user_email
+      confirm_password(user.password)
 
       user.reload
       expect(user.confirmation_token).not_to be_nil

spec/features/users/login_spec.rb

@@ -874,7 +874,7 @@ RSpec.describe 'Login', :clean_gitlab_redis_shared_state do
       end
     end
 
-    context 'when the user does not have an email configured' do
+    context 'when the user does not have an email configured', :js do
       let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'saml', email: 'temp-email-for-oauth-user@gitlab.localhost') }
 
       before do

spec/frontend/pages/profiles/password_prompt/password_prompt_modal_spec.js

+import { GlModal } from '@gitlab/ui';
+import { shallowMountExtended } from 'helpers/vue_test_utils_helper';
+import {
+  I18N_PASSWORD_PROMPT_CANCEL_BUTTON,
+  I18N_PASSWORD_PROMPT_CONFIRM_BUTTON,
+} from '~/pages/profiles/password_prompt/constants';
+import PasswordPromptModal from '~/pages/profiles/password_prompt/password_prompt_modal.vue';
+
+const createComponent = ({ props }) => {
+  return shallowMountExtended(PasswordPromptModal, {
+    propsData: {
+      ...props,
+    },
+  });
+};
+
+describe('Password prompt modal', () => {
+  let wrapper;
+
+  const mockPassword = 'not+fake+shady+password';
+  const mockEvent = { preventDefault: jest.fn() };
+  const handleConfirmPasswordSpy = jest.fn();
+
+  const findField = () => wrapper.findByTestId('password-prompt-field');
+  const findModal = () => wrapper.findComponent(GlModal);
+  const findConfirmBtn = () => findModal().props('actionPrimary');
+  const findConfirmBtnDisabledState = () =>
+    findModal().props('actionPrimary').attributes[2].disabled;
+
+  const findCancelBtn = () => findModal().props('actionCancel');
+
+  const submitModal = () => findModal().vm.$emit('primary', mockEvent);
+  const setPassword = (newPw) => findField().vm.$emit('input', newPw);
+
+  beforeEach(() => {
+    wrapper = createComponent({
+      props: {
+        handleConfirmPassword: handleConfirmPasswordSpy,
+      },
+    });
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  it('renders the password field', () => {
+    expect(findField().exists()).toBe(true);
+  });
+
+  it('renders the confirm button', () => {
+    expect(findConfirmBtn().text).toEqual(I18N_PASSWORD_PROMPT_CONFIRM_BUTTON);
+  });
+
+  it('renders the cancel button', () => {
+    expect(findCancelBtn().text).toEqual(I18N_PASSWORD_PROMPT_CANCEL_BUTTON);
+  });
+
+  describe('confirm button', () => {
+    describe('with a valid password', () => {
+      it('calls the `handleConfirmPassword` method when clicked', async () => {
+        setPassword(mockPassword);
+        submitModal();
+
+        await wrapper.vm.$nextTick();
+
+        expect(handleConfirmPasswordSpy).toHaveBeenCalledTimes(1);
+        expect(handleConfirmPasswordSpy).toHaveBeenCalledWith(mockPassword);
+      });
+
+      it('enables the confirm button', async () => {
+        setPassword(mockPassword);
+
+        expect(findConfirmBtnDisabledState()).toBe(true);
+
+        await wrapper.vm.$nextTick();
+
+        expect(findConfirmBtnDisabledState()).toBe(false);
+      });
+    });
+
+    it('without a valid password is disabled', async () => {
+      setPassword('');
+
+      expect(findConfirmBtnDisabledState()).toBe(true);
+
+      await wrapper.vm.$nextTick();
+
+      expect(findConfirmBtnDisabledState()).toBe(true);
+    });
+  });
+});

spec/services/users/update_service_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Users::UpdateService do
-  let(:user) { create(:user) }
+  let(:password) { 'longsecret987!' }
+  let(:user) { create(:user, password: password, password_confirmation: password) }
 
   describe '#execute' do
     it 'updates time preferences' do
@@ -18,7 +19,7 @@ RSpec.describe Users::UpdateService do
     it 'returns an error result when record cannot be updated' do
       result = {}
       expect do
-        result = update_user(user, { email: 'invalid' })
+        result = update_user(user, { email: 'invalid', validation_password: password })
       end.not_to change { user.reload.email }
       expect(result[:status]).to eq(:error)
       expect(result[:message]).to eq('Email is invalid')
@@ -65,7 +66,7 @@ RSpec.describe Users::UpdateService do
     context 'updating canonical email' do
       context 'if email was changed' do
         subject do
-          update_user(user, email: 'user+extrastuff@example.com')
+          update_user(user, email: 'user+extrastuff@example.com', validation_password: password)
         end
 
         it 'calls canonicalize_email' do
@@ -75,15 +76,68 @@ RSpec.describe Users::UpdateService do
 
           subject
         end
+
+        context 'when check_password is true' do
+          def update_user(user, opts)
+            described_class.new(user, opts.merge(user: user)).execute(check_password: true)
+          end
+
+          it 'returns error if no password confirmation was passed', :aggregate_failures do
+            result = {}
+
+            expect do
+              result = update_user(user, { email: 'example@example.com' })
+            end.not_to change { user.reload.unconfirmed_email }
+            expect(result[:status]).to eq(:error)
+            expect(result[:message]).to eq('Invalid password')
+          end
+
+          it 'returns error if wrong password confirmation was passed', :aggregate_failures do
+            result = {}
+
+            expect do
+              result = update_user(user, { email: 'example@example.com', validation_password: 'wrongpassword' })
+            end.not_to change { user.reload.unconfirmed_email }
+            expect(result[:status]).to eq(:error)
+            expect(result[:message]).to eq('Invalid password')
+          end
+
+          it 'does not require password if it was automatically set', :aggregate_failures do
+            user.update!(password_automatically_set: true)
+            result = {}
+
+            expect do
+              result = update_user(user, { email: 'example@example.com' })
+            end.to change { user.reload.unconfirmed_email }
+            expect(result[:status]).to eq(:success)
+          end
+
+          it 'does not require a password if the attribute changed does not require it' do
+            result = {}
+
+            expect do
+              result = update_user(user, { job_title: 'supreme leader of the universe' })
+            end.to change { user.reload.job_title }
+            expect(result[:status]).to eq(:success)
+          end
+        end
       end
 
-      context 'if email was NOT changed' do
-        subject do
-          update_user(user, job_title: 'supreme leader of the universe')
+      context 'when check_password is left to false' do
+        it 'does not require a password check', :aggregate_failures do
+          result = {}
+          expect do
+            result = update_user(user, { email: 'example@example.com' })
+          end.to change { user.reload.unconfirmed_email }
+          expect(result[:status]).to eq(:success)
         end
+      end
 
+      context 'if email was NOT changed' do
         it 'skips update canonicalize email service call' do
-          expect { subject }.not_to change { user.user_canonical_email }
+          expect do
+            update_user(user, job_title: 'supreme leader of the universe')
+          end.not_to change { user.user_canonical_email }
         end
       end
     end
@@ -106,7 +160,7 @@ RSpec.describe Users::UpdateService do
 
     it 'raises an error when record cannot be updated' do
       expect do
-        update_user(user, email: 'invalid')
+        update_user(user, email: 'invalid', validation_password: password)
       end.to raise_error(ActiveRecord::RecordInvalid)
     end