diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb index 92ef10a9ef5993f166569d79e9fdaa66287b061f..0a593bd35b68cb4c90a777c9d5c1eb91ba764c57 100644 --- a/app/controllers/projects/deploy_keys_controller.rb +++ b/app/controllers/projects/deploy_keys_controller.rb @@ -46,7 +46,9 @@ def update end def enable - Projects::EnableDeployKeyService.new(@project, current_user, params).execute + key = Projects::EnableDeployKeyService.new(@project, current_user, params).execute + + return render_404 unless key respond_to do |format| format.html { redirect_to_repository_settings(@project, anchor: 'js-deploy-keys-settings') } @@ -54,19 +56,16 @@ def enable end end - # rubocop: disable CodeReuse/ActiveRecord def disable - deploy_key_project = @project.deploy_keys_projects.find_by(deploy_key_id: params[:id]) - return render_404 unless deploy_key_project + deploy_key_project = Projects::DisableDeployKeyService.new(@project, current_user, params).execute - deploy_key_project.destroy! + return render_404 unless deploy_key_project respond_to do |format| format.html { redirect_to_repository_settings(@project, anchor: 'js-deploy-keys-settings') } format.json { head :ok } end end - # rubocop: enable CodeReuse/ActiveRecord protected diff --git a/app/services/projects/disable_deploy_key_service.rb b/app/services/projects/disable_deploy_key_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..e483c0708c44a43badc50697c95fff3ff534b8f3 --- /dev/null +++ b/app/services/projects/disable_deploy_key_service.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Projects + class DisableDeployKeyService < BaseService + def execute + # rubocop: disable CodeReuse/ActiveRecord + deploy_key_project = project.deploy_keys_projects.find_by(deploy_key_id: params[:id]) + # rubocop: enable CodeReuse/ActiveRecord + + deploy_key_project&.destroy! + end + end +end diff --git a/app/services/projects/enable_deploy_key_service.rb b/app/services/projects/enable_deploy_key_service.rb index 102088e955711c9545fb9b24c97301b5064980fa..38219cacee92983627449b4f05aacb5425e7322c 100644 --- a/app/services/projects/enable_deploy_key_service.rb +++ b/app/services/projects/enable_deploy_key_service.rb @@ -2,9 +2,10 @@ module Projects class EnableDeployKeyService < BaseService - # rubocop: disable CodeReuse/ActiveRecord def execute - key = accessible_keys.find_by(id: params[:key_id] || params[:id]) + key_id = params[:key_id] || params[:id] + key = find_accessible_key(key_id) + return unless key unless project.deploy_keys.include?(key) @@ -13,12 +14,15 @@ def execute key end - # rubocop: enable CodeReuse/ActiveRecord private - def accessible_keys - current_user.accessible_deploy_keys + def find_accessible_key(key_id) + if current_user.admin? + DeployKey.find_by_id(key_id) + else + current_user.accessible_deploy_keys.find_by_id(key_id) + end end end end diff --git a/changelogs/unreleased/sh-fix-issue-53783-ce.yml b/changelogs/unreleased/sh-fix-issue-53783-ce.yml new file mode 100644 index 0000000000000000000000000000000000000000..10be1d81768dbdc826093328bec95a3b41692466 --- /dev/null +++ b/changelogs/unreleased/sh-fix-issue-53783-ce.yml @@ -0,0 +1,5 @@ +--- +title: Fix enabling project deploy key for admins +merge_request: 23043 +author: +type: fixed diff --git a/spec/controllers/projects/deploy_keys_controller_spec.rb b/spec/controllers/projects/deploy_keys_controller_spec.rb index 73bf169085fd95d78db7072ab96498cca5cee2da..4567a51b88ec7c1dc6ff2722bb021414fb609238 100644 --- a/spec/controllers/projects/deploy_keys_controller_spec.rb +++ b/spec/controllers/projects/deploy_keys_controller_spec.rb @@ -27,12 +27,8 @@ let(:project2) { create(:project, :internal)} let(:project_private) { create(:project, :private)} - let(:deploy_key_internal) do - create(:deploy_key, key: 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAgQCdMHEHyhRjbhEZVddFn6lTWdgEy5Q6Bz4nwGB76xWZI5YT/1WJOMEW+sL5zYd31kk7sd3FJ5L9ft8zWMWrr/iWXQikC2cqZK24H1xy+ZUmrRuJD4qGAaIVoyyzBL+avL+lF8J5lg6YSw8gwJY/lX64/vnJHUlWw2n5BF8IFOWhiw== dummy@gitlab.com') - end - let(:deploy_key_actual) do - create(:deploy_key, key: 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAgQDNd/UJWhPrpb+b/G5oL109y57yKuCxE+WUGJGYaj7WQKsYRJmLYh1mgjrl+KVyfsWpq4ylOxIfFSnN9xBBFN8mlb0Fma5DC7YsSsibJr3MZ19ZNBprwNcdogET7aW9I0In7Wu5f2KqI6e5W/spJHCy4JVxzVMUvk6Myab0LnJ2iQ== dummy@gitlab.com') - end + let(:deploy_key_internal) { create(:deploy_key) } + let(:deploy_key_actual) { create(:deploy_key) } let!(:deploy_key_public) { create(:deploy_key, public: true) } let!(:deploy_keys_project_internal) do @@ -63,4 +59,145 @@ end end end + + describe '/enable/:id' do + let(:deploy_key) { create(:deploy_key) } + let(:project2) { create(:project) } + let!(:deploy_keys_project_internal) do + create(:deploy_keys_project, project: project2, deploy_key: deploy_key) + end + + context 'with anonymous user' do + before do + sign_out(:user) + end + + it 'redirects to login' do + expect do + put :enable, id: deploy_key.id, namespace_id: project.namespace, project_id: project + end.not_to change { DeployKeysProject.count } + + expect(response).to have_http_status(302) + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'with user with no permission' do + before do + sign_in(create(:user)) + end + + it 'returns 404' do + expect do + put :enable, id: deploy_key.id, namespace_id: project.namespace, project_id: project + end.not_to change { DeployKeysProject.count } + + expect(response).to have_http_status(404) + end + end + + context 'with user with permission' do + before do + project2.add_maintainer(user) + end + + it 'returns 302' do + expect do + put :enable, id: deploy_key.id, namespace_id: project.namespace, project_id: project + end.to change { DeployKeysProject.count }.by(1) + + expect(DeployKeysProject.where(project_id: project.id, deploy_key_id: deploy_key.id).count).to eq(1) + expect(response).to have_http_status(302) + expect(response).to redirect_to(namespace_project_settings_repository_path(anchor: 'js-deploy-keys-settings')) + end + + it 'returns 404' do + put :enable, id: 0, namespace_id: project.namespace, project_id: project + + expect(response).to have_http_status(404) + end + end + + context 'with admin' do + before do + sign_in(create(:admin)) + end + + it 'returns 302' do + expect do + put :enable, id: deploy_key.id, namespace_id: project.namespace, project_id: project + end.to change { DeployKeysProject.count }.by(1) + + expect(DeployKeysProject.where(project_id: project.id, deploy_key_id: deploy_key.id).count).to eq(1) + expect(response).to have_http_status(302) + expect(response).to redirect_to(namespace_project_settings_repository_path(anchor: 'js-deploy-keys-settings')) + end + end + end + + describe '/disable/:id' do + let(:deploy_key) { create(:deploy_key) } + let!(:deploy_key_project) { create(:deploy_keys_project, project: project, deploy_key: deploy_key) } + + context 'with anonymous user' do + before do + sign_out(:user) + end + + it 'redirects to login' do + put :disable, id: deploy_key.id, namespace_id: project.namespace, project_id: project + + expect(response).to have_http_status(302) + expect(response).to redirect_to(new_user_session_path) + expect(DeployKey.find(deploy_key.id)).to eq(deploy_key) + end + end + + context 'with user with no permission' do + before do + sign_in(create(:user)) + end + + it 'returns 404' do + put :disable, id: deploy_key.id, namespace_id: project.namespace, project_id: project + + expect(response).to have_http_status(404) + expect(DeployKey.find(deploy_key.id)).to eq(deploy_key) + end + end + + context 'with user with permission' do + it 'returns 302' do + put :disable, id: deploy_key.id, namespace_id: project.namespace, project_id: project + + expect(response).to have_http_status(302) + expect(response).to redirect_to(namespace_project_settings_repository_path(anchor: 'js-deploy-keys-settings')) + + expect { DeployKey.find(deploy_key.id) }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'returns 404' do + put :disable, id: 0, namespace_id: project.namespace, project_id: project + + expect(response).to have_http_status(404) + end + end + + context 'with admin' do + before do + sign_in(create(:admin)) + end + + it 'returns 302' do + expect do + put :disable, id: deploy_key.id, namespace_id: project.namespace, project_id: project + end.to change { DeployKey.count }.by(-1) + + expect(response).to have_http_status(302) + expect(response).to redirect_to(namespace_project_settings_repository_path(anchor: 'js-deploy-keys-settings')) + + expect { DeployKey.find(deploy_key.id) }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end end