Commit f04accf8 authored by Drew Blessing's avatar Drew Blessing Committed by Drew Blessing

GitLab.com users without password must contact to delete account

Require GitLab.com users to contact support for account deletion
when they do not have a local password set. For example, when
they created their account via OAuth sign-in. This ensures
we are able to verify ownership/identity prior to deletion.
parent a8f5ba66
...@@ -1489,6 +1489,10 @@ class User < ApplicationRecord ...@@ -1489,6 +1489,10 @@ class User < ApplicationRecord
!solo_owned_groups.present? !solo_owned_groups.present?
end end
def can_remove_self?
true
end
def ci_owned_runners def ci_owned_runners
@ci_owned_runners ||= begin @ci_owned_runners ||= begin
project_runners = Ci::RunnerProject project_runners = Ci::RunnerProject
......
...@@ -79,6 +79,11 @@ ...@@ -79,6 +79,11 @@
%strong= current_user.solo_owned_groups.map(&:name).join(', ') %strong= current_user.solo_owned_groups.map(&:name).join(', ')
%p %p
= s_('Profiles|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.')
- elsif !current_user.can_remove_self?
%p
= s_('Profiles|GitLab is unable to verify your identity automatically.')
%p
= s_('Profiles|Please email %{data_request} to begin the account deletion process.').html_safe % { data_request: mail_to('personal-data-request@gitlab.com') }
- else - else
%p %p
= s_("Profiles|You don't have access to delete this user.") = s_("Profiles|You don't have access to delete this user.")
......
...@@ -5,11 +5,23 @@ module EE ...@@ -5,11 +5,23 @@ module EE
extend ActiveSupport::Concern extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override extend ::Gitlab::Utils::Override
prepended do
before_action :ensure_can_remove_self, only: [:destroy]
end
private private
override :set_blocked_pending_approval? override :set_blocked_pending_approval?
def set_blocked_pending_approval? def set_blocked_pending_approval?
super || ::Gitlab::CurrentSettings.should_apply_user_signup_cap? super || ::Gitlab::CurrentSettings.should_apply_user_signup_cap?
end end
def ensure_can_remove_self
unless current_user&.can_remove_self?
redirect_to profile_account_path,
status: :see_other,
alert: s_('Profiles|Account could not be deleted. GitLab was unable to verify your identity.')
end
end
end end
end end
...@@ -377,6 +377,16 @@ module EE ...@@ -377,6 +377,16 @@ module EE
board_id: board_id, epic_id: epic_id) board_id: board_id, epic_id: epic_id)
end end
# GitLab.com users should not be able to remove themselves
# when they cannot verify their local password, because it
# isn't set (using third party authentication).
override :can_remove_self?
def can_remove_self?
return true unless ::Gitlab.com?
!password_automatically_set?
end
protected protected
override :password_required? override :password_required?
......
...@@ -10,12 +10,18 @@ module EE ...@@ -10,12 +10,18 @@ module EE
::Gitlab::CurrentSettings.current_application_settings.updating_name_disabled_for_users ::Gitlab::CurrentSettings.current_application_settings.updating_name_disabled_for_users
end end
condition(:can_remove_self, scope: :subject) do
@subject.can_remove_self?
end
rule { can?(:update_user) }.enable :update_name rule { can?(:update_user) }.enable :update_name
rule do rule do
updating_name_disabled_for_users & updating_name_disabled_for_users &
~admin ~admin
end.prevent :update_name end.prevent :update_name
rule { user_is_self & ~can_remove_self }.prevent :destroy_user
end end
end end
end end
---
title: GitLab.com users without password must contact to delete account
merge_request: 49626
author:
type: added
...@@ -126,4 +126,31 @@ RSpec.describe RegistrationsController do ...@@ -126,4 +126,31 @@ RSpec.describe RegistrationsController do
end end
end end
end end
describe '#destroy' do
let(:user) { create(:user) }
before do
user.update!(password_automatically_set: true)
sign_in(user)
end
context 'on GitLab.com when the password is automatically set' do
before do
stub_application_setting(password_authentication_enabled_for_web: false)
stub_application_setting(password_authentication_enabled_for_git: false)
allow(::Gitlab).to receive(:com?).and_return(true)
end
it 'redirects without deleting the account' do
expect(DeleteUserWorker).not_to receive(:perform_async)
post :destroy, params: { username: user.username }
expect(flash[:alert]).to eq 'Account could not be deleted. GitLab was unable to verify your identity.'
expect(response).to have_gitlab_http_status(:see_other)
expect(response).to redirect_to profile_account_path
end
end
end
end end
...@@ -82,4 +82,22 @@ RSpec.describe 'Profile > Account' do ...@@ -82,4 +82,22 @@ RSpec.describe 'Profile > Account' do
end end
end end
end end
describe 'Delete account' do
context "on GitLab.com when the user's password is automatically set" do
before do
allow(::Gitlab).to receive(:com?).and_return(true)
user.update!(password_automatically_set: true)
visit profile_account_path
end
it 'shows that the identity cannot be verified' do
expect(page).to have_content 'GitLab is unable to verify your identity automatically.'
end
it 'does not display a delete button' do
expect(page).not_to have_button 'Delete account'
end
end
end
end end
...@@ -1571,4 +1571,42 @@ RSpec.describe User do ...@@ -1571,4 +1571,42 @@ RSpec.describe User do
end end
end end
end end
describe '#can_remove_self?' do
let(:user) { create(:user) }
subject { user.can_remove_self? }
context 'not on GitLab.com' do
context 'when the password is not automatically set' do
it { is_expected.to eq true }
end
context 'when the password is automatically set' do
before do
user.password_automatically_set = true
end
it { is_expected.to eq true }
end
end
context 'on GitLab.com' do
before do
allow(::Gitlab).to receive(:com?).and_return(true)
end
context 'when the password is not automatically set' do
it { is_expected.to eq true }
end
context 'when the password is automatically set' do
before do
user.password_automatically_set = true
end
it { is_expected.to eq false }
end
end
end
end end
...@@ -96,4 +96,34 @@ RSpec.describe UserPolicy do ...@@ -96,4 +96,34 @@ RSpec.describe UserPolicy do
it_behaves_like 'changing a user', :update_name it_behaves_like 'changing a user', :update_name
end end
end end
describe ':destroy_user' do
context 'when user is not self', :enable_admin_mode do
let(:current_user) { create(:user, :admin) }
it { is_expected.to be_allowed(:destroy_user) }
end
context 'when user is self' do
let(:current_user) { user }
it { is_expected.to be_allowed(:destroy_user) }
context 'when the user password is automatically set' do
before do
current_user.update!(password_automatically_set: true)
end
it { is_expected.to be_allowed(:destroy_user) }
context 'on GitLab.com' do
before do
allow(::Gitlab).to receive(:com?).and_return(true)
end
it { is_expected.not_to be_allowed(:destroy_user) }
end
end
end
end
end end
...@@ -20953,6 +20953,9 @@ msgstr "" ...@@ -20953,6 +20953,9 @@ msgstr ""
msgid "Profiles|@username" msgid "Profiles|@username"
msgstr "" msgstr ""
msgid "Profiles|Account could not be deleted. GitLab was unable to verify your identity."
msgstr ""
msgid "Profiles|Account scheduled for removal." msgid "Profiles|Account scheduled for removal."
msgstr "" msgstr ""
...@@ -21055,6 +21058,9 @@ msgstr "" ...@@ -21055,6 +21058,9 @@ msgstr ""
msgid "Profiles|Full name" msgid "Profiles|Full name"
msgstr "" msgstr ""
msgid "Profiles|GitLab is unable to verify your identity automatically."
msgstr ""
msgid "Profiles|Give your individual key a title." msgid "Profiles|Give your individual key a title."
msgstr "" msgstr ""
...@@ -21103,6 +21109,9 @@ msgstr "" ...@@ -21103,6 +21109,9 @@ msgstr ""
msgid "Profiles|Path" msgid "Profiles|Path"
msgstr "" msgstr ""
msgid "Profiles|Please email %{data_request} to begin the account deletion process."
msgstr ""
msgid "Profiles|Position and size your new avatar" msgid "Profiles|Position and size your new avatar"
msgstr "" msgstr ""
......
...@@ -3024,6 +3024,14 @@ RSpec.describe User do ...@@ -3024,6 +3024,14 @@ RSpec.describe User do
end end
end end
describe '#can_remove_self?' do
let(:user) { create(:user) }
it 'returns true' do
expect(user.can_remove_self?).to eq true
end
end
describe "#recent_push" do describe "#recent_push" do
let(:user) { build(:user) } let(:user) { build(:user) }
let(:project) { build(:project) } let(:project) { build(:project) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment