From 59d910f2a136a52748326977fcefe2ec16c35d09 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 28 Mar 2018 18:53:52 +0200 Subject: [PATCH 1/4] Set user when adding deploy key to project using API --- lib/api/deploy_keys.rb | 23 +++++++++++------------ spec/requests/api/deploy_keys_spec.rb | 4 ++++ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index b0b7b50998f..70d43ac1d79 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -54,7 +54,7 @@ module API present key, with: Entities::DeployKeysProject end - desc 'Add new deploy key to currently authenticated user' do + desc 'Add new deploy key to a project' do success Entities::DeployKeysProject end params do @@ -66,33 +66,32 @@ module API params[:key].strip! # Check for an existing key joined to this project - key = user_project.deploy_keys_projects + deploy_key_project = user_project.deploy_keys_projects .joins(:deploy_key) .find_by(keys: { key: params[:key] }) - if key - present key, with: Entities::DeployKeysProject + if deploy_key_project + present deploy_key_project, with: Entities::DeployKeysProject break end # Check for available deploy keys in other projects key = current_user.accessible_deploy_keys.find_by(key: params[:key]) if key - added_key = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push]) + deploy_key_project = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push]) - present added_key, with: Entities::DeployKeysProject + present deploy_key_project, with: Entities::DeployKeysProject break end # Create a new deploy key - key_attributes = { can_push: !!params[:can_push], - deploy_key_attributes: declared_params.except(:can_push) } - key = add_deploy_keys_project(user_project, key_attributes) + deploy_key_attributes = declared_params.except(:can_push).merge(user: current_user) + deploy_key_project = add_deploy_keys_project(user_project, deploy_key_attributes: deploy_key_attributes, can_push: !!params[:can_push]) - if key.valid? - present key, with: Entities::DeployKeysProject + if deploy_key_project.valid? + present deploy_key_project, with: Entities::DeployKeysProject else - render_validation_error!(key) + render_validation_error!(deploy_key_project) end end diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index 0772b3f2e64..ae9c0e9c304 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -91,6 +91,10 @@ describe API::DeployKeys do expect do post api("/projects/#{project.id}/deploy_keys", admin), key_attrs end.to change { project.deploy_keys.count }.by(1) + + new_key = project.deploy_keys.last + expect(new_key.key).to eq(key_attrs[:key]) + expect(new_key.user).to eq(admin) end it 'returns an existing ssh key when attempting to add a duplicate' do From 7bca902a23d0cccaa3bce422fce7fdc71e722db4 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 28 Mar 2018 18:54:15 +0200 Subject: [PATCH 2/4] Fall back on ghost user when deploy key user is not set --- app/models/deploy_key.rb | 4 ++++ lib/gitlab/git_access.rb | 8 ++++---- spec/models/deploy_key_spec.rb | 21 +++++++++++++++++++++ 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index c2e0a5fa126..89a74b7dcb1 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -27,6 +27,10 @@ class DeployKey < Key self.private? end + def user + super || User.ghost + end + def has_access_to?(project) deploy_keys_project_for(project).present? end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 6400089a22f..6beecea7e73 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -99,8 +99,6 @@ module Gitlab end def check_active_user! - return if deploy_key? - if user && !user_access.allowed? raise UnauthorizedError, ERROR_MESSAGES[:account_blocked] end @@ -219,7 +217,7 @@ module Gitlab raise UnauthorizedError, ERROR_MESSAGES[:read_only] end - if deploy_key + if deploy_key? unless deploy_key.can_push_to?(project) raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload] end @@ -309,8 +307,10 @@ module Gitlab case actor when User actor + when DeployKey + nil when Key - actor.user unless actor.is_a?(DeployKey) + actor.user when :ci nil end diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb index 3d7283e2164..41440c6d288 100644 --- a/spec/models/deploy_key_spec.rb +++ b/spec/models/deploy_key_spec.rb @@ -17,4 +17,25 @@ describe DeployKey, :mailer do should_not_email(user) end end + + describe '#user' do + let(:deploy_key) { create(:deploy_key) } + let(:user) { create(:user) } + + context 'when user is set' do + before do + deploy_key.user = user + end + + it 'returns the user' do + expect(deploy_key.user).to be(user) + end + end + + context 'when user is not set' do + it 'returns the ghost user' do + expect(deploy_key.user).to eq(User.ghost) + end + end + end end From 931151972189d0d203e2bacedb78638f256a5055 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 28 Mar 2018 18:54:31 +0200 Subject: [PATCH 3/4] =?UTF-8?q?Don=E2=80=99t=20delete=20deploy=20key=20whe?= =?UTF-8?q?n=20user=20who=20created=20it=20is=20deleted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/models/user.rb | 7 ++----- spec/models/user_spec.rb | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index b8c55205ab8..8d5d0bbbfa0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -82,11 +82,8 @@ class User < ActiveRecord::Base has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, inverse_of: :owner, autosave: true # rubocop:disable Cop/ActiveRecordDependent # Profile - has_many :keys, -> do - type = Key.arel_table[:type] - where(type.not_eq('DeployKey').or(type.eq(nil))) - end, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_many :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :keys, -> { where(type: ['Key', nil]) }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :nullify # rubocop:disable Cop/ActiveRecordDependent has_many :gpg_keys has_many :emails, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c61674fff13..e29d4ff1ea0 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -25,7 +25,7 @@ describe User do it { is_expected.to have_many(:group_members) } it { is_expected.to have_many(:groups) } it { is_expected.to have_many(:keys).dependent(:destroy) } - it { is_expected.to have_many(:deploy_keys).dependent(:destroy) } + it { is_expected.to have_many(:deploy_keys).dependent(:nullify) } it { is_expected.to have_many(:events).dependent(:destroy) } it { is_expected.to have_many(:issues).dependent(:destroy) } it { is_expected.to have_many(:notes).dependent(:destroy) } From d6624fe80f756f92810c88d9f51df19a43796fae Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 28 Mar 2018 18:56:26 +0200 Subject: [PATCH 4/4] Add changelog --- changelogs/unreleased/dm-deploy-keys-default-user.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/dm-deploy-keys-default-user.yml diff --git a/changelogs/unreleased/dm-deploy-keys-default-user.yml b/changelogs/unreleased/dm-deploy-keys-default-user.yml new file mode 100644 index 00000000000..b82d67d028c --- /dev/null +++ b/changelogs/unreleased/dm-deploy-keys-default-user.yml @@ -0,0 +1,5 @@ +--- +title: Ensure hooks run when a deploy key without a user pushes +merge_request: +author: +type: fixed