diff options
-rw-r--r-- | app/assets/javascripts/lib/utils/csrf.js | 4 | ||||
-rw-r--r-- | app/assets/javascripts/profile/account/components/delete_account_modal.vue | 146 | ||||
-rw-r--r-- | app/assets/javascripts/profile/account/index.js | 21 | ||||
-rw-r--r-- | app/assets/javascripts/repo/components/repo.vue | 2 | ||||
-rw-r--r-- | app/assets/javascripts/vue_shared/components/popup_dialog.vue | 6 | ||||
-rw-r--r-- | app/assets/stylesheets/framework/modal.scss | 13 | ||||
-rw-r--r-- | app/assets/stylesheets/framework/variables.scss | 1 | ||||
-rw-r--r-- | app/controllers/registrations_controller.rb | 29 | ||||
-rw-r--r-- | app/models/user.rb | 4 | ||||
-rw-r--r-- | app/views/profiles/accounts/show.html.haml | 20 | ||||
-rw-r--r-- | changelogs/unreleased/winh-delete-account-modal.yml | 5 | ||||
-rw-r--r-- | config/webpack.config.js | 1 | ||||
-rw-r--r-- | spec/controllers/registrations_controller_spec.rb | 64 | ||||
-rw-r--r-- | spec/features/issues/issue_detail_spec.rb | 3 | ||||
-rw-r--r-- | spec/features/profile_spec.rb | 44 | ||||
-rw-r--r-- | spec/javascripts/profile/account/components/delete_account_modal_spec.js | 129 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 45 |
17 files changed, 507 insertions, 30 deletions
diff --git a/app/assets/javascripts/lib/utils/csrf.js b/app/assets/javascripts/lib/utils/csrf.js index ae41cc5e8a8..0bdb547d31a 100644 --- a/app/assets/javascripts/lib/utils/csrf.js +++ b/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; - diff --git a/app/assets/javascripts/profile/account/components/delete_account_modal.vue b/app/assets/javascripts/profile/account/components/delete_account_modal.vue new file mode 100644 index 00000000000..b2b34cb83e1 --- /dev/null +++ b/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> diff --git a/app/assets/javascripts/profile/account/index.js b/app/assets/javascripts/profile/account/index.js new file mode 100644 index 00000000000..635056e0eeb --- /dev/null +++ b/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, + }, + }); + }, +}); diff --git a/app/assets/javascripts/repo/components/repo.vue b/app/assets/javascripts/repo/components/repo.vue index d6c864cb976..cc60aa5939c 100644 --- a/app/assets/javascripts/repo/components/repo.vue +++ b/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" /> diff --git a/app/assets/javascripts/vue_shared/components/popup_dialog.vue b/app/assets/javascripts/vue_shared/components/popup_dialog.vue index 994b33bc1c9..9279b50cd55 100644 --- a/app/assets/javascripts/vue_shared/components/popup_dialog.vue +++ b/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 diff --git a/app/assets/stylesheets/framework/modal.scss b/app/assets/stylesheets/framework/modal.scss index 5b581780447..1cebd02df48 100644 --- a/app/assets/stylesheets/framework/modal.scss +++ b/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 { diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 5ab40947da9..2f9def51d91 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/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; diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 5ea3a5d5562..d9142311b6f 100644 --- a/app/controllers/registrations_controller.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 4ba9130a75a..959738ba608 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index 8abbd828032..7f79168dfb3 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/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') diff --git a/changelogs/unreleased/winh-delete-account-modal.yml b/changelogs/unreleased/winh-delete-account-modal.yml new file mode 100644 index 00000000000..f1e2710fdcc --- /dev/null +++ b/changelogs/unreleased/winh-delete-account-modal.yml @@ -0,0 +1,5 @@ +--- +title: Show confirmation modal before deleting account +merge_request: 14360 +author: +type: changed diff --git a/config/webpack.config.js b/config/webpack.config.js index c515a170d2d..8cded750a66 100644 --- a/config/webpack.config.js +++ b/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', diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 5a4ab39ab86..1d3ddfbd220 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/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 + + def expect_password_failure + expect_failure('Invalid password') + end + + 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 - post(:destroy) + context 'user requires password confirmation' do + it 'fails if password confirmation is not provided' do + post :destroy - expect(response.status).to eq(302) + 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 diff --git a/spec/features/issues/issue_detail_spec.rb b/spec/features/issues/issue_detail_spec.rb index 28b636f9359..c0c396af93f 100644 --- a/spec/features/issues/issue_detail_spec.rb +++ b/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 diff --git a/spec/features/profile_spec.rb b/spec/features/profile_spec.rb index f183dd8cb75..1cddd35fd8a 100644 --- a/spec/features/profile_spec.rb +++ b/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 diff --git a/spec/javascripts/profile/account/components/delete_account_modal_spec.js b/spec/javascripts/profile/account/components/delete_account_modal_spec.js new file mode 100644 index 00000000000..2e94948cfb2 --- /dev/null +++ b/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); + }); + }); +}); diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 9f517e4af72..995211845ce 100644 --- a/spec/models/user_spec.rb +++ b/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 |