Merge branch 'winh-delete-account-modal' into 'master'

Show confirmation modal before deleting account

Closes #13482 and #38754

See merge request gitlab-org/gitlab-ce!14360

app/assets/javascripts/lib/utils/csrf.js

@@ -14,6 +14,9 @@ If you need to compose a headers object, use the spread operator:
     someOtherHeader: '12345',
   }
 ```
+
+see also http://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf
+and https://github.com/rails/jquery-rails/blob/v4.3.1/vendor/assets/javascripts/jquery_ujs.js#L59-L62
  */
 
 const csrf = {
@@ -53,4 +56,3 @@ if ($.rails) {
 }
 
 export default csrf;
-

app/assets/javascripts/profile/account/components/delete_account_modal.vue

@@ -0,0 +1,146 @@
+<script>
+  import popupDialog from '../../../vue_shared/components/popup_dialog.vue';
+  import { __, s__, sprintf } from '../../../locale';
+  import csrf from '../../../lib/utils/csrf';
+
+  export default {
+    props: {
+      actionUrl: {
+        type: String,
+        required: true,
+      },
+      confirmWithPassword: {
+        type: Boolean,
+        required: true,
+      },
+      username: {
+        type: String,
+        required: true,
+      },
+    },
+    data() {
+      return {
+        enteredPassword: '',
+        enteredUsername: '',
+        isOpen: false,
+      };
+    },
+    components: {
+      popupDialog,
+    },
+    computed: {
+      csrfToken() {
+        return csrf.token;
+      },
+      inputLabel() {
+        let confirmationValue;
+        if (this.confirmWithPassword) {
+          confirmationValue = __('password');
+        } else {
+          confirmationValue = __('username');
+        }
+
+        confirmationValue = `<code>${confirmationValue}</code>`;
+
+        return sprintf(
+          s__('Profiles|Type your %{confirmationValue} to confirm:'),
+          { confirmationValue },
+          false,
+        );
+      },
+      text() {
+        return sprintf(
+          s__(`Profiles|
+You are about to permanently delete %{yourAccount}, and all of the issues, merge requests, and groups linked to your account.
+Once you confirm %{deleteAccount}, it cannot be undone or recovered.`),
+          {
+            yourAccount: `<strong>${s__('Profiles|your account')}</strong>`,
+            deleteAccount: `<strong>${s__('Profiles|Delete Account')}</strong>`,
+          },
+          false,
+        );
+      },
+    },
+    methods: {
+      canSubmit() {
+        if (this.confirmWithPassword) {
+          return this.enteredPassword !== '';
+        }
+
+        return this.enteredUsername === this.username;
+      },
+      onSubmit(status) {
+        if (status) {
+          if (!this.canSubmit()) {
+            return;
+          }
+
+          this.$refs.form.submit();
+        }
+
+        this.toggleOpen(false);
+      },
+      toggleOpen(isOpen) {
+        this.isOpen = isOpen;
+      },
+    },
+  };
+</script>
+
+<template>
+  <div>
+    <popup-dialog
+      v-if="isOpen"
+      :title="s__('Profiles|Delete your account?')"
+      :text="text"
+      :kind="`danger ${!canSubmit() && 'disabled'}`"
+      :primary-button-label="s__('Profiles|Delete account')"
+      @toggle="toggleOpen"
+      @submit="onSubmit">
+
+      <template slot="body" scope="props">
+        <p v-html="props.text"></p>
+
+        <form
+          ref="form"
+          :action="actionUrl"
+          method="post">
+
+          <input
+            type="hidden"
+            name="_method"
+            value="delete" />
+          <input
+            type="hidden"
+            name="authenticity_token"
+            :value="csrfToken" />
+
+          <p id="input-label" v-html="inputLabel"></p>
+
+          <input
+            v-if="confirmWithPassword"
+            name="password"
+            class="form-control"
+            type="password"
+            v-model="enteredPassword"
+            aria-labelledby="input-label" />
+          <input
+            v-else
+            name="username"
+            class="form-control"
+            type="text"
+            v-model="enteredUsername"
+            aria-labelledby="input-label" />
+        </form>
+      </template>
+
+    </popup-dialog>
+
+    <button
+      type="button"
+      class="btn btn-danger"
+      @click="toggleOpen(true)">
+      {{ s__('Profiles|Delete account') }}
+    </button>
+  </div>
+</template>

app/assets/javascripts/profile/account/index.js

@@ -0,0 +1,21 @@
+import Vue from 'vue';
+
+import deleteAccountModal from './components/delete_account_modal.vue';
+
+const deleteAccountModalEl = document.getElementById('delete-account-modal');
+// eslint-disable-next-line no-new
+new Vue({
+  el: deleteAccountModalEl,
+  components: {
+    deleteAccountModal,
+  },
+  render(createElement) {
+    return createElement('delete-account-modal', {
+      props: {
+        actionUrl: deleteAccountModalEl.dataset.actionUrl,
+        confirmWithPassword: !!deleteAccountModalEl.dataset.confirmWithPassword,
+        username: deleteAccountModalEl.dataset.username,
+      },
+    });
+  },
+});

app/assets/javascripts/repo/components/repo.vue

@@ -62,7 +62,7 @@ export default {
       :primary-button-label="__('Discard changes')"
       kind="warning"
       :title="__('Are you sure?')"
-      :body="__('Are you sure you want to discard your changes?')"
+      :text="__('Are you sure you want to discard your changes?')"
       @toggle="toggleDialogOpen"
       @submit="dialogSubmitted"
     />

app/assets/javascripts/vue_shared/components/popup_dialog.vue

@@ -7,7 +7,7 @@ export default {
       type: String,
       required: true,
     },
-    body: {
+    text: {
       type: String,
       required: true,
     },
@@ -63,7 +63,9 @@ export default {
         <h4 class="modal-title">{{this.title}}</h4>
       </div>
       <div class="modal-body">
-        <p>{{this.body}}</p>
+        <slot name="body" :text="text">
+          <p>{{text}}</p>
+        </slot>
       </div>
       <div class="modal-footer">
         <button

app/assets/stylesheets/framework/modal.scss

@@ -1,10 +1,17 @@
+.modal-header {
+  padding: #{3 * $grid-size} #{2 * $grid-size};
+
+  .page-title {
+    margin-top: 0;
+  }
+}
+
 .modal-body {
   position: relative;
-  padding: 15px;
+  padding: #{3 * $grid-size} #{2 * $grid-size};
 
   .form-actions {
-    margin: -$gl-padding + 1;
-    margin-top: 15px;
+    margin: #{2 * $grid-size} #{-2 * $grid-size} #{-2 * $grid-size};
   }
 
   .text-danger {

app/assets/stylesheets/framework/variables.scss

@@ -1,6 +1,7 @@
 /*
  * Layout
  */
+$grid-size: 8px;
 $gutter_collapsed_width: 62px;
 $gutter_width: 290px;
 $gutter_inner_width: 250px;

app/controllers/registrations_controller.rb

@@ -25,18 +25,33 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def destroy
-    current_user.delete_async(deleted_by: current_user)
-
-    respond_to do |format|
-      format.html do
-        session.try(:destroy)
-        redirect_to new_user_session_path, status: 302, notice: "Account scheduled for removal."
-      end
+    if destroy_confirmation_valid?
+      current_user.delete_async(deleted_by: current_user)
+      session.try(:destroy)
+      redirect_to new_user_session_path, status: 303, notice: s_('Profiles|Account scheduled for removal.')
+    else
+      redirect_to profile_account_path, status: 303, alert: destroy_confirmation_failure_message
     end
   end
 
   protected
 
+  def destroy_confirmation_valid?
+    if current_user.confirm_deletion_with_password?
+      current_user.valid_password?(params[:password])
+    else
+      current_user.username == params[:username]
+    end
+  end
+
+  def destroy_confirmation_failure_message
+    if current_user.confirm_deletion_with_password?
+      s_('Profiles|Invalid password')
+    else
+      s_('Profiles|Invalid username')
+    end
+  end
+
   def build_resource(hash = nil)
     super
   end

app/models/user.rb

@@ -654,6 +654,10 @@ class User < ActiveRecord::Base
     Ability.allowed?(self, action, subject)
   end
 
+  def confirm_deletion_with_password?
+    !password_automatically_set? && allow_password_authentication?
+  end
+
   def first_name
     name.split.first unless name.blank?
   end

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

@@ -97,21 +97,29 @@
 .row.prepend-top-default
   .col-lg-4.profile-settings-sidebar
     %h4.prepend-top-0.danger-title
-      Remove account
+      = s_('Profiles|Delete account')
   .col-lg-8
     - if @user.can_be_removed? && can?(current_user, :destroy_user, @user)
       %p
-        Deleting an account has the following effects:
+        = s_('Profiles|Deleting an account has the following effects:')
       = render 'users/deletion_guidance', user: current_user
-      = link_to 'Delete account', user_registration_path, data: { confirm: "REMOVE #{current_user.name}? Are you sure?" }, method: :delete, class: "btn btn-remove"
+
+      #delete-account-modal{ data: { action_url: user_registration_path,
+        confirm_with_password: ('true' if current_user.confirm_deletion_with_password?),
+        username: current_user.username } }
+        %button.btn.btn-danger.disabled
+          = s_('Profiles|Delete account')
     - else
       - if @user.solo_owned_groups.present?
         %p
-          Your account is currently an owner in these groups:
+          = s_('Profiles|Your account is currently an owner in these groups:')
           %strong= @user.solo_owned_groups.map(&:name).join(', ')
         %p
-          You must transfer ownership or delete these groups before you can delete your account.
+          = s_('Profiles|You must transfer ownership or delete these groups before you can delete your account.')
       - else
         %p
-          You don't have access to delete this user.
+          = s_("Profiles|You don't have access to delete this user.")
 .append-bottom-default
+
+- content_for :page_specific_javascripts do
+  = webpack_bundle_tag('account')

changelogs/unreleased/winh-delete-account-modal.yml

@@ -0,0 +1,5 @@
+---
+title: Show confirmation modal before deleting account
+merge_request: 14360
+author:
+type: changed

config/webpack.config.js

@@ -26,6 +26,7 @@ var config = {
   },
   context: path.join(ROOT_PATH, 'app/assets/javascripts'),
   entry: {
+    account:              './profile/account/index.js',
     balsamiq_viewer:      './blob/balsamiq_viewer.js',
     blob:                 './blob_edit/blob_bundle.js',
     boards:               './boards/boards_bundle.js',

spec/controllers/registrations_controller_spec.rb

@@ -76,12 +76,68 @@ describe RegistrationsController do
       sign_in(user)
     end
 
-    it 'schedules the user for destruction' do
-      expect(DeleteUserWorker).to receive(:perform_async).with(user.id, user.id, {})
+    def expect_failure(message)
+      expect(flash[:alert]).to eq(message)
+      expect(response.status).to eq(303)
+      expect(response).to redirect_to profile_account_path
+    end
 
-      post(:destroy)
+    def expect_password_failure
+      expect_failure('Invalid password')
+    end
 
-      expect(response.status).to eq(302)
+    def expect_username_failure
+      expect_failure('Invalid username')
+    end
+
+    def expect_success
+      expect(flash[:notice]).to eq 'Account scheduled for removal.'
+      expect(response.status).to eq(303)
+      expect(response).to redirect_to new_user_session_path
+    end
+
+    context 'user requires password confirmation' do
+      it 'fails if password confirmation is not provided' do
+        post :destroy
+
+        expect_password_failure
+      end
+
+      it 'fails if password confirmation is wrong' do
+        post :destroy, password: 'wrong password'
+
+        expect_password_failure
+      end
+
+      it 'succeeds if password is confirmed' do
+        post :destroy, password: '12345678'
+
+        expect_success
+      end
+    end
+
+    context 'user does not require password confirmation' do
+      before do
+        stub_application_setting(password_authentication_enabled: false)
+      end
+
+      it 'fails if username confirmation is not provided' do
+        post :destroy
+
+        expect_username_failure
+      end
+
+      it 'fails if username confirmation is wrong' do
+        post :destroy, username: 'wrong username'
+
+        expect_username_failure
+      end
+
+      it 'succeeds if username is confirmed' do
+        post :destroy, username: user.username
+
+        expect_success
+      end
     end
   end
 end

spec/features/issues/issue_detail_spec.rb

@@ -28,8 +28,7 @@ feature 'Issue Detail', :js do
       fill_in 'issue-title', with: 'issue title'
       click_button 'Save'
 
-      visit profile_account_path
-      click_link 'Delete account'
+      Users::DestroyService.new(user).execute(user)
 
       visit project_issue_path(project, issue)
     end

spec/features/profile_spec.rb

@@ -12,11 +12,47 @@ describe 'Profile account page' do
       visit profile_account_path
     end
 
-    it { expect(page).to have_content('Remove account') }
+    it { expect(page).to have_content('Delete account') }
 
-    it 'deletes the account' do
-      expect { click_link 'Delete account' }.to change { User.where(id: user.id).count }.by(-1)
-      expect(current_path).to eq(new_user_session_path)
+    it 'does not immediately delete the account' do
+      click_button 'Delete account'
+
+      expect(User.exists?(user.id)).to be_truthy
+    end
+
+    it 'deletes user', :js do
+      click_button 'Delete account'
+
+      fill_in 'password', with: '12345678'
+
+      page.within '.popup-dialog' do
+        click_button 'Delete account'
+      end
+
+      expect(page).to have_content('Account scheduled for removal')
+      expect(User.exists?(user.id)).to be_falsy
+    end
+
+    it 'shows invalid password flash message', :js do
+      click_button 'Delete account'
+
+      fill_in 'password', with: 'testing123'
+
+      page.within '.popup-dialog' do
+        click_button 'Delete account'
+      end
+
+      expect(page).to have_content('Invalid password')
+    end
+
+    it 'does not show delete button when user owns a group' do
+      group = create(:group)
+      group.add_owner(user)
+
+      visit profile_account_path
+
+      expect(page).not_to have_button('Delete account')
+      expect(page).to have_content("Your account is currently an owner in these groups: #{group.name}")
     end
   end
 

spec/javascripts/profile/account/components/delete_account_modal_spec.js

@@ -0,0 +1,129 @@
+import Vue from 'vue';
+
+import deleteAccountModal from '~/profile/account/components/delete_account_modal.vue';
+
+import mountComponent from '../../../helpers/vue_mount_component_helper';
+
+describe('DeleteAccountModal component', () => {
+  const actionUrl = `${gl.TEST_HOST}/delete/user`;
+  const username = 'hasnoname';
+  let Component;
+  let vm;
+
+  beforeEach(() => {
+    Component = Vue.extend(deleteAccountModal);
+  });
+
+  afterEach(() => {
+    vm.$destroy();
+  });
+
+  const findElements = () => {
+    const confirmation = vm.confirmWithPassword ? 'password' : 'username';
+    return {
+      form: vm.$refs.form,
+      input: vm.$el.querySelector(`[name="${confirmation}"]`),
+      submitButton: vm.$el.querySelector('.btn-danger'),
+    };
+  };
+
+  describe('with password confirmation', () => {
+    beforeEach((done) => {
+      vm = mountComponent(Component, {
+        actionUrl,
+        confirmWithPassword: true,
+        username,
+      });
+
+      vm.isOpen = true;
+
+      Vue.nextTick()
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('does not accept empty password', (done) => {
+      const { form, input, submitButton } = findElements();
+      spyOn(form, 'submit');
+      input.value = '';
+      input.dispatchEvent(new Event('input'));
+
+      Vue.nextTick()
+        .then(() => {
+          expect(vm.enteredPassword).toBe(input.value);
+          expect(submitButton).toHaveClass('disabled');
+          submitButton.click();
+          expect(form.submit).not.toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('submits form with password', (done) => {
+      const { form, input, submitButton } = findElements();
+      spyOn(form, 'submit');
+      input.value = 'anything';
+      input.dispatchEvent(new Event('input'));
+
+      Vue.nextTick()
+        .then(() => {
+          expect(vm.enteredPassword).toBe(input.value);
+          expect(submitButton).not.toHaveClass('disabled');
+          submitButton.click();
+          expect(form.submit).toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
+
+  describe('with username confirmation', () => {
+    beforeEach((done) => {
+      vm = mountComponent(Component, {
+        actionUrl,
+        confirmWithPassword: false,
+        username,
+      });
+
+      vm.isOpen = true;
+
+      Vue.nextTick()
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('does not accept wrong username', (done) => {
+      const { form, input, submitButton } = findElements();
+      spyOn(form, 'submit');
+      input.value = 'this is wrong';
+      input.dispatchEvent(new Event('input'));
+
+      Vue.nextTick()
+        .then(() => {
+          expect(vm.enteredUsername).toBe(input.value);
+          expect(submitButton).toHaveClass('disabled');
+          submitButton.click();
+          expect(form.submit).not.toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('submits form with correct username', (done) => {
+      const { form, input, submitButton } = findElements();
+      spyOn(form, 'submit');
+      input.value = username;
+      input.dispatchEvent(new Event('input'));
+
+      Vue.nextTick()
+        .then(() => {
+          expect(vm.enteredUsername).toBe(input.value);
+          expect(submitButton).not.toHaveClass('disabled');
+          submitButton.click();
+          expect(form.submit).toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
+});

spec/models/user_spec.rb

@@ -2282,4 +2282,49 @@ describe User do
       end
     end
   end
+
+  describe '#confirm_deletion_with_password?' do
+    where(
+      password_automatically_set: [true, false],
+      ldap_user: [true, false],
+      password_authentication_disabled: [true, false]
+    )
+
+    with_them do
+      let!(:user) { create(:user, password_automatically_set: password_automatically_set) }
+      let!(:identity) { create(:identity, user: user) if ldap_user }
+
+      # Only confirm deletion with password if all inputs are false
+      let(:expected) { !(password_automatically_set || ldap_user || password_authentication_disabled) }
+
+      before do
+        stub_application_setting(password_authentication_enabled: !password_authentication_disabled)
+      end
+
+      it 'returns false unless all inputs are true' do
+        expect(user.confirm_deletion_with_password?).to eq(expected)
+      end
+    end
+  end
+
+  describe '#delete_async' do
+    let(:user) { create(:user) }
+    let(:deleted_by) { create(:user) }
+
+    it 'blocks the user then schedules them for deletion if a hard delete is specified' do
+      expect(DeleteUserWorker).to receive(:perform_async).with(deleted_by.id, user.id, hard_delete: true)
+
+      user.delete_async(deleted_by: deleted_by, params: { hard_delete: true })
+
+      expect(user).to be_blocked
+    end
+
+    it 'schedules user for deletion without blocking them' do
+      expect(DeleteUserWorker).to receive(:perform_async).with(deleted_by.id, user.id, {})
+
+      user.delete_async(deleted_by: deleted_by)
+
+      expect(user).not_to be_blocked
+    end
+  end
 end