Merge branch 'dm-deploy-keys-default-user' into 'master'
Ensure hooks run when a deploy key without a user pushes Closes #44317 See merge request gitlab-org/gitlab-ce!18057
This commit is contained in:
commit
869b7b31cf
8 changed files with 52 additions and 22 deletions
|
@ -27,6 +27,10 @@ class DeployKey < Key
|
||||||
self.private?
|
self.private?
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def user
|
||||||
|
super || User.ghost
|
||||||
|
end
|
||||||
|
|
||||||
def has_access_to?(project)
|
def has_access_to?(project)
|
||||||
deploy_keys_project_for(project).present?
|
deploy_keys_project_for(project).present?
|
||||||
end
|
end
|
||||||
|
|
|
@ -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
|
has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, inverse_of: :owner, autosave: true # rubocop:disable Cop/ActiveRecordDependent
|
||||||
|
|
||||||
# Profile
|
# Profile
|
||||||
has_many :keys, -> do
|
has_many :keys, -> { where(type: ['Key', nil]) }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
|
||||||
type = Key.arel_table[:type]
|
has_many :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :nullify # rubocop:disable Cop/ActiveRecordDependent
|
||||||
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 :gpg_keys
|
has_many :gpg_keys
|
||||||
|
|
||||||
has_many :emails, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
|
has_many :emails, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
|
||||||
|
|
5
changelogs/unreleased/dm-deploy-keys-default-user.yml
Normal file
5
changelogs/unreleased/dm-deploy-keys-default-user.yml
Normal file
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Ensure hooks run when a deploy key without a user pushes
|
||||||
|
merge_request:
|
||||||
|
author:
|
||||||
|
type: fixed
|
|
@ -54,7 +54,7 @@ module API
|
||||||
present key, with: Entities::DeployKeysProject
|
present key, with: Entities::DeployKeysProject
|
||||||
end
|
end
|
||||||
|
|
||||||
desc 'Add new deploy key to currently authenticated user' do
|
desc 'Add new deploy key to a project' do
|
||||||
success Entities::DeployKeysProject
|
success Entities::DeployKeysProject
|
||||||
end
|
end
|
||||||
params do
|
params do
|
||||||
|
@ -66,33 +66,32 @@ module API
|
||||||
params[:key].strip!
|
params[:key].strip!
|
||||||
|
|
||||||
# Check for an existing key joined to this project
|
# 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)
|
.joins(:deploy_key)
|
||||||
.find_by(keys: { key: params[:key] })
|
.find_by(keys: { key: params[:key] })
|
||||||
|
|
||||||
if key
|
if deploy_key_project
|
||||||
present key, with: Entities::DeployKeysProject
|
present deploy_key_project, with: Entities::DeployKeysProject
|
||||||
break
|
break
|
||||||
end
|
end
|
||||||
|
|
||||||
# Check for available deploy keys in other projects
|
# Check for available deploy keys in other projects
|
||||||
key = current_user.accessible_deploy_keys.find_by(key: params[:key])
|
key = current_user.accessible_deploy_keys.find_by(key: params[:key])
|
||||||
if 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
|
break
|
||||||
end
|
end
|
||||||
|
|
||||||
# Create a new deploy key
|
# Create a new deploy key
|
||||||
key_attributes = { can_push: !!params[:can_push],
|
deploy_key_attributes = declared_params.except(:can_push).merge(user: current_user)
|
||||||
deploy_key_attributes: declared_params.except(:can_push) }
|
deploy_key_project = add_deploy_keys_project(user_project, deploy_key_attributes: deploy_key_attributes, can_push: !!params[:can_push])
|
||||||
key = add_deploy_keys_project(user_project, key_attributes)
|
|
||||||
|
|
||||||
if key.valid?
|
if deploy_key_project.valid?
|
||||||
present key, with: Entities::DeployKeysProject
|
present deploy_key_project, with: Entities::DeployKeysProject
|
||||||
else
|
else
|
||||||
render_validation_error!(key)
|
render_validation_error!(deploy_key_project)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -99,8 +99,6 @@ module Gitlab
|
||||||
end
|
end
|
||||||
|
|
||||||
def check_active_user!
|
def check_active_user!
|
||||||
return if deploy_key?
|
|
||||||
|
|
||||||
if user && !user_access.allowed?
|
if user && !user_access.allowed?
|
||||||
raise UnauthorizedError, ERROR_MESSAGES[:account_blocked]
|
raise UnauthorizedError, ERROR_MESSAGES[:account_blocked]
|
||||||
end
|
end
|
||||||
|
@ -215,7 +213,7 @@ module Gitlab
|
||||||
raise UnauthorizedError, ERROR_MESSAGES[:read_only]
|
raise UnauthorizedError, ERROR_MESSAGES[:read_only]
|
||||||
end
|
end
|
||||||
|
|
||||||
if deploy_key
|
if deploy_key?
|
||||||
unless deploy_key.can_push_to?(project)
|
unless deploy_key.can_push_to?(project)
|
||||||
raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload]
|
raise UnauthorizedError, ERROR_MESSAGES[:deploy_key_upload]
|
||||||
end
|
end
|
||||||
|
@ -305,8 +303,10 @@ module Gitlab
|
||||||
case actor
|
case actor
|
||||||
when User
|
when User
|
||||||
actor
|
actor
|
||||||
|
when DeployKey
|
||||||
|
nil
|
||||||
when Key
|
when Key
|
||||||
actor.user unless actor.is_a?(DeployKey)
|
actor.user
|
||||||
when :ci
|
when :ci
|
||||||
nil
|
nil
|
||||||
end
|
end
|
||||||
|
|
|
@ -17,4 +17,25 @@ describe DeployKey, :mailer do
|
||||||
should_not_email(user)
|
should_not_email(user)
|
||||||
end
|
end
|
||||||
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
|
end
|
||||||
|
|
|
@ -25,7 +25,7 @@ describe User do
|
||||||
it { is_expected.to have_many(:group_members) }
|
it { is_expected.to have_many(:group_members) }
|
||||||
it { is_expected.to have_many(:groups) }
|
it { is_expected.to have_many(:groups) }
|
||||||
it { is_expected.to have_many(:keys).dependent(:destroy) }
|
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(:events).dependent(:destroy) }
|
||||||
it { is_expected.to have_many(:issues).dependent(:destroy) }
|
it { is_expected.to have_many(:issues).dependent(:destroy) }
|
||||||
it { is_expected.to have_many(:notes).dependent(:destroy) }
|
it { is_expected.to have_many(:notes).dependent(:destroy) }
|
||||||
|
|
|
@ -91,6 +91,10 @@ describe API::DeployKeys do
|
||||||
expect do
|
expect do
|
||||||
post api("/projects/#{project.id}/deploy_keys", admin), key_attrs
|
post api("/projects/#{project.id}/deploy_keys", admin), key_attrs
|
||||||
end.to change { project.deploy_keys.count }.by(1)
|
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
|
end
|
||||||
|
|
||||||
it 'returns an existing ssh key when attempting to add a duplicate' do
|
it 'returns an existing ssh key when attempting to add a duplicate' do
|
||||||
|
|
Loading…
Reference in a new issue