Commit 7009eca0 authored by Nick Thomas's avatar Nick Thomas

Merge branch '23079-write-permission-global-deploy-keys' into 'master'

Allow maintainers to toggle write permission for public deploy keys

See merge request gitlab-org/gitlab!17210
parents 67eb6663 42ab8edd
......@@ -73,6 +73,10 @@ class Projects::DeployKeysController < Projects::ApplicationController
@deploy_key ||= DeployKey.find(params[:id])
end
def deploy_keys_project
@deploy_keys_project ||= deploy_key.deploy_keys_project_for(@project)
end
def create_params
create_params = params.require(:deploy_key)
.permit(:key, :title, deploy_keys_projects_attributes: [:can_push])
......@@ -81,10 +85,16 @@ class Projects::DeployKeysController < Projects::ApplicationController
end
def update_params
params.require(:deploy_key).permit(:title, deploy_keys_projects_attributes: [:id, :can_push])
permitted_params = [deploy_keys_projects_attributes: [:id, :can_push]]
permitted_params << :title if can?(current_user, :update_deploy_key, deploy_key)
params.require(:deploy_key).permit(*permitted_params)
end
def authorize_update_deploy_key!
access_denied! unless can?(current_user, :update_deploy_key, deploy_key)
if !can?(current_user, :update_deploy_key, deploy_key) &&
!can?(current_user, :update_deploy_keys_project, deploy_keys_project)
access_denied!
end
end
end
# frozen_string_literal: true
class DeployKeysProjectPolicy < BasePolicy
delegate { @subject.project }
with_options scope: :subject, score: 0
condition(:public_deploy_key) { @subject.deploy_key.public? }
rule { public_deploy_key & can?(:admin_project) }.enable :update_deploy_keys_project
end
......@@ -40,7 +40,7 @@ module Projects
def as_json
serializer = DeployKeySerializer.new # rubocop: disable CodeReuse/Serializer
opts = { user: current_user }
opts = { user: current_user, project: project }
{
enabled_keys: serializer.represent(enabled_keys.with_projects, opts),
......
......@@ -20,6 +20,7 @@ class DeployKeyEntity < Grape::Entity
private
def can_edit
Ability.allowed?(options[:user], :update_deploy_key, object)
Ability.allowed?(options[:user], :update_deploy_key, object) ||
Ability.allowed?(options[:user], :update_deploy_keys_project, object.deploy_keys_project_for(options[:project]))
end
end
......@@ -6,7 +6,7 @@
.form-group
= form.label :title, class: 'col-form-label col-sm-2'
.col-sm-10= form.text_field :title, class: 'form-control'
.col-sm-10= form.text_field :title, class: 'form-control', readonly: ('readonly' unless can?(current_user, :update_deploy_key, deploy_key))
.form-group
- if deploy_key.new_record?
......
---
title: Allow maintainers to toggle write permission for public deploy keys
merge_request: 17210
author:
type: fixed
......@@ -115,14 +115,20 @@ module API
put ":id/deploy_keys/:key_id" do
deploy_keys_project = find_by_deploy_key(user_project, params[:key_id])
authorize!(:update_deploy_key, deploy_keys_project.deploy_key)
if !can?(current_user, :update_deploy_key, deploy_keys_project.deploy_key) &&
!can?(current_user, :update_deploy_keys_project, deploy_keys_project)
forbidden!(nil)
end
update_params = {}
update_params[:can_push] = params[:can_push] if params.key?(:can_push)
update_params[:deploy_key_attributes] = { id: params[:key_id] }
can_push = params[:can_push].nil? ? deploy_keys_project.can_push : params[:can_push]
title = params[:title] || deploy_keys_project.deploy_key.title
if can?(current_user, :update_deploy_key, deploy_keys_project.deploy_key)
update_params[:deploy_key_attributes][:title] = params[:title] if params.key?(:title)
end
result = deploy_keys_project.update(can_push: can_push,
deploy_key_attributes: { id: params[:key_id],
title: title })
result = deploy_keys_project.update(update_params)
if result
present deploy_keys_project, with: Entities::DeployKeysProject
......
......@@ -5,6 +5,7 @@ require 'spec_helper'
describe Projects::DeployKeysController do
let(:project) { create(:project, :repository) }
let(:user) { create(:user) }
let(:admin) { create(:admin) }
before do
project.add_maintainer(user)
......@@ -37,7 +38,7 @@ describe Projects::DeployKeysController do
create(:deploy_keys_project, project: project2, deploy_key: deploy_key_internal)
end
let!(:deploy_keys_actual_project) do
let!(:deploy_keys_project_actual) do
create(:deploy_keys_project, project: project, deploy_key: deploy_key_actual)
end
......@@ -154,7 +155,7 @@ describe Projects::DeployKeysController do
context 'with admin' do
before do
sign_in(create(:admin))
sign_in(admin)
end
it 'returns 302' do
......@@ -219,7 +220,7 @@ describe Projects::DeployKeysController do
context 'with admin' do
before do
sign_in(create(:admin))
sign_in(admin)
end
it 'returns 302' do
......@@ -234,4 +235,80 @@ describe Projects::DeployKeysController do
end
end
end
describe 'PUT update' do
let(:extra_params) { {} }
subject do
put :update, params: extra_params.reverse_merge(id: deploy_key.id,
namespace_id: project.namespace,
project_id: project)
end
def deploy_key_params(title, can_push)
deploy_keys_projects_attributes = { '0' => { id: deploy_keys_project, can_push: can_push } }
{ deploy_key: { title: title, deploy_keys_projects_attributes: deploy_keys_projects_attributes } }
end
let(:deploy_key) { create(:deploy_key, public: true) }
let(:project) { create(:project) }
let!(:deploy_keys_project) do
create(:deploy_keys_project, project: project, deploy_key: deploy_key)
end
context 'with project maintainer' do
before do
project.add_maintainer(user)
end
context 'public deploy key attached to project' do
let(:extra_params) { deploy_key_params('updated title', '1') }
it 'does not update the title of the deploy key' do
expect { subject }.not_to change { deploy_key.reload.title }
end
it 'updates can_push of deploy_keys_project' do
expect { subject }.to change { deploy_keys_project.reload.can_push }.from(false).to(true)
end
end
end
context 'with admin' do
before do
sign_in(admin)
end
context 'public deploy key attached to project' do
let(:extra_params) { deploy_key_params('updated title', '1') }
it 'updates the title of the deploy key' do
expect { subject }.to change { deploy_key.reload.title }.to('updated title')
end
it 'updates can_push of deploy_keys_project' do
expect { subject }.to change { deploy_keys_project.reload.can_push }.from(false).to(true)
end
end
end
context 'with admin as project maintainer' do
before do
sign_in(admin)
project.add_maintainer(admin)
end
context 'public deploy key attached to project' do
let(:extra_params) { deploy_key_params('updated title', '1') }
it 'updates the title of the deploy key' do
expect { subject }.to change { deploy_key.reload.title }.to('updated title')
end
it 'updates can_push of deploy_keys_project' do
expect { subject }.to change { deploy_keys_project.reload.can_push }.from(false).to(true)
end
end
end
end
end
......@@ -66,6 +66,19 @@ describe 'Projects > Settings > Repository settings' do
expect(page).to have_content('Write access allowed')
end
it 'edit an existing public deploy key to be writable' do
project.deploy_keys << public_deploy_key
visit project_settings_repository_path(project)
find('.deploy-key', text: public_deploy_key.title).find('.ic-pencil').click
check 'deploy_key_deploy_keys_projects_attributes_0_can_push'
click_button 'Save changes'
expect(page).to have_content('public_deploy_key')
expect(page).to have_content('Write access allowed')
end
it 'edit a deploy key from projects user has access to' do
project2 = create(:project_empty_repo)
project2.add_role(user, role)
......
# frozen_string_literal: true
require 'spec_helper'
describe DeployKeysProjectPolicy do
subject { described_class.new(current_user, deploy_key.deploy_keys_project_for(project)) }
describe 'updating a deploy_keys_project' do
context 'when a project maintainer' do
let(:current_user) { create(:user) }
context 'tries to update private deploy key attached to project' do
let(:deploy_key) { create(:deploy_key, public: false) }
let(:project) { create(:project_empty_repo) }
before do
project.add_maintainer(current_user)
project.deploy_keys << deploy_key
end
it { is_expected.to be_disallowed(:update_deploy_keys_project) }
end
context 'tries to update public deploy key attached to project' do
let(:deploy_key) { create(:deploy_key, public: true) }
let(:project) { create(:project_empty_repo) }
before do
project.add_maintainer(current_user)
project.deploy_keys << deploy_key
end
it { is_expected.to be_allowed(:update_deploy_keys_project) }
end
end
context 'when a non-maintainer project member' do
let(:current_user) { create(:user) }
let(:project) { create(:project_empty_repo) }
before do
project.add_developer(current_user)
project.deploy_keys << deploy_key
end
context 'tries to update private deploy key attached to project' do
let(:deploy_key) { create(:deploy_key, public: false) }
it { is_expected.to be_disallowed(:update_deploy_keys_project) }
end
context 'tries to update public deploy key attached to project' do
let(:deploy_key) { create(:deploy_key, public: true) }
it { is_expected.to be_disallowed(:update_deploy_keys_project) }
end
end
context 'when a user is not a project member' do
let(:current_user) { create(:user) }
let(:project) { create(:project_empty_repo) }
let(:deploy_key) { create(:deploy_key, public: true) }
before do
project.deploy_keys << deploy_key
end
context 'tries to update public deploy key attached to project' do
it { is_expected.to be_disallowed(:update_deploy_keys_project) }
end
end
end
end
......@@ -2,6 +2,7 @@ require 'spec_helper'
describe API::DeployKeys do
let(:user) { create(:user) }
let(:maintainer) { create(:user) }
let(:admin) { create(:admin) }
let(:project) { create(:project, creator_id: user.id) }
let(:project2) { create(:project, creator_id: user.id) }
......@@ -124,45 +125,109 @@ describe API::DeployKeys do
end
describe 'PUT /projects/:id/deploy_keys/:key_id' do
let(:private_deploy_key) { create(:another_deploy_key, public: false) }
let(:project_private_deploy_key) do
create(:deploy_keys_project, project: project, deploy_key: private_deploy_key)
let(:extra_params) { {} }
subject do
put api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", api_user), params: extra_params
end
it 'updates a public deploy key as admin' do
expect do
put api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin), params: { title: 'new title' }
end.not_to change(deploy_key, :title)
context 'with non-admin' do
let(:api_user) { user }
expect(response).to have_gitlab_http_status(200)
it 'does not update a public deploy key' do
expect { subject }.not_to change(deploy_key, :title)
expect(response).to have_gitlab_http_status(404)
end
end
it 'does not update a public deploy key as non admin' do
expect do
put api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", user), params: { title: 'new title' }
end.not_to change(deploy_key, :title)
context 'with admin' do
let(:api_user) { admin }
expect(response).to have_gitlab_http_status(404)
context 'public deploy key attached to project' do
let(:extra_params) { { title: 'new title', can_push: true } }
it 'updates the title of the deploy key' do
expect { subject }.to change { deploy_key.reload.title }.to 'new title'
expect(response).to have_gitlab_http_status(200)
end
it 'updates can_push of deploy_keys_project' do
expect { subject }.to change { deploy_keys_project.reload.can_push }.from(false).to(true)
expect(response).to have_gitlab_http_status(200)
end
end
context 'private deploy key' do
let(:deploy_key) { create(:another_deploy_key, public: false) }
let(:deploy_keys_project) do
create(:deploy_keys_project, project: project, deploy_key: deploy_key)
end
let(:extra_params) { { title: 'new title', can_push: true } }
it 'updates the title of the deploy key' do
expect { subject }.to change { deploy_key.reload.title }.to 'new title'
expect(response).to have_gitlab_http_status(200)
end
it 'updates can_push of deploy_keys_project' do
expect { subject }.to change { deploy_keys_project.reload.can_push }.from(false).to(true)
expect(response).to have_gitlab_http_status(200)
end
context 'invalid title' do
let(:extra_params) { { title: '' } }
it 'does not update the title of the deploy key' do
expect { subject }.not_to change { deploy_key.reload.title }
expect(response).to have_gitlab_http_status(400)
end
end
end
end
it 'does not update a private key with invalid title' do
project_private_deploy_key
context 'with admin as project maintainer' do
let(:api_user) { admin }
expect do
put api("/projects/#{project.id}/deploy_keys/#{private_deploy_key.id}", admin), params: { title: '' }
end.not_to change(deploy_key, :title)
before do
project.add_maintainer(admin)
end
expect(response).to have_gitlab_http_status(400)
context 'public deploy key attached to project' do
let(:extra_params) { { title: 'new title', can_push: true } }
it 'updates the title of the deploy key' do
expect { subject }.to change { deploy_key.reload.title }.to 'new title'
expect(response).to have_gitlab_http_status(200)
end
it 'updates can_push of deploy_keys_project' do
expect { subject }.to change { deploy_keys_project.reload.can_push }.from(false).to(true)
expect(response).to have_gitlab_http_status(200)
end
end
end
it 'updates a private ssh key with correct attributes' do
project_private_deploy_key
context 'with maintainer' do
let(:api_user) { maintainer }
put api("/projects/#{project.id}/deploy_keys/#{private_deploy_key.id}", admin), params: { title: 'new title', can_push: true }
before do
project.add_maintainer(maintainer)
end
expect(json_response['id']).to eq(private_deploy_key.id)
expect(json_response['title']).to eq('new title')
expect(json_response['can_push']).to eq(true)
context 'public deploy key attached to project' do
let(:extra_params) { { title: 'new title', can_push: true } }
it 'does not update the title of the deploy key' do
expect { subject }.not_to change { deploy_key.reload.title }
expect(response).to have_gitlab_http_status(200)
end
it 'updates can_push of deploy_keys_project' do
expect { subject }.to change { deploy_keys_project.reload.can_push }.from(false).to(true)
expect(response).to have_gitlab_http_status(200)
end
end
end
end
......
......@@ -8,14 +8,15 @@ describe DeployKeyEntity do
let(:user) { create(:user) }
let(:project) { create(:project, :internal)}
let(:project_private) { create(:project, :private)}
let!(:project_pending_delete) { create(:project, :internal, pending_delete: true) }
let(:deploy_key) { create(:deploy_key) }
let!(:deploy_key_internal) { create(:deploy_keys_project, project: project, deploy_key: deploy_key) }
let!(:deploy_key_private) { create(:deploy_keys_project, project: project_private, deploy_key: deploy_key) }
let!(:deploy_key_pending_delete) { create(:deploy_keys_project, project: project_pending_delete, deploy_key: deploy_key) }
let(:entity) { described_class.new(deploy_key, user: user) }
before do
project.deploy_keys << deploy_key
project_private.deploy_keys << deploy_key
end
describe 'returns deploy keys with projects a user can read' do
let(:expected_result) do
{
......@@ -46,17 +47,30 @@ describe DeployKeyEntity do
it { expect(entity.as_json).to eq(expected_result) }
end
describe 'returns can_edit true if user is a maintainer of project' do
context 'user is an admin' do
let(:user) { create(:user, :admin) }
it { expect(entity.as_json).to include(can_edit: true) }
end
context 'user is a project maintainer' do
before do
project.add_maintainer(user)
end
it { expect(entity.as_json).to include(can_edit: true) }
end
context 'project deploy key' do
it { expect(entity.as_json).to include(can_edit: true) }
end
describe 'returns can_edit true if a user admin' do
let(:user) { create(:user, :admin) }
context 'public deploy key' do
let(:deploy_key_public) { create(:deploy_key, public: true) }
let(:entity_public) { described_class.new(deploy_key_public, { user: user, project: project }) }
it { expect(entity.as_json).to include(can_edit: true) }
before do
project.deploy_keys << deploy_key_public
end
it { expect(entity_public.as_json).to include(can_edit: true) }
end
end
end
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