From ce4760bbd57f3b30b0e3845493ed4e39cc463359 Mon Sep 17 00:00:00 2001 From: Joshua Welsh Date: Tue, 11 Oct 2016 10:02:51 +0100 Subject: Fixes various errors when adding deploy keys caused by not exiting the control flow. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When adding a deploy key that already exists in the project the existing key would not be returned, resulting in an attempt to create a new one, which in turn caused a 500 error due to an ActiveRecord exception. When adding a deploy key that exists within another project the key would be joined to the project, but would also attempt to create a new one, which resulted in a 400 error due to the key already existing. Fixes #22741 Fixes #21754 Signed-off-by: Rémy Coutable --- CHANGELOG.md | 1 + lib/api/deploy_keys.rb | 9 +++++++-- spec/requests/api/deploy_keys_spec.rb | 17 +++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5592625efcb..e9c97a1ec66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Please view this file on the master branch, on stable branches it's out of date. - Fixed link typo on /help/ui to Alerts section. !6915 (Sam Rose) - Simpler arguments passed to named_route on toggle_award_url helper method - Fix: Backup restore doesn't clear cache + - API: Fix project deploy keys 400 and 500 errors when adding an existing key. !6784 (Joshua Welsh) - Replace jquery.cookie plugin with js.cookie !7085 - Use MergeRequestsClosingIssues cache data on Issue#closed_by_merge_requests method - Fix Sign in page 'Forgot your password?' link overlaps on medium-large screens diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 825e05fbae3..425df2c176a 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -49,18 +49,23 @@ module API attrs = attributes_for_keys [:title, :key] attrs[:key].strip! if attrs[:key] + # Check for an existing key joined to this project key = user_project.deploy_keys.find_by(key: attrs[:key]) - present key, with: Entities::SSHKey if key + if key + present key, with: Entities::SSHKey + break + end # Check for available deploy keys in other projects key = current_user.accessible_deploy_keys.find_by(key: attrs[:key]) if key user_project.deploy_keys << key present key, with: Entities::SSHKey + break end + # Create a new deploy key key = DeployKey.new attrs - if key.valid? && user_project.deploy_keys << key present key, with: Entities::SSHKey else diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index 7d8cc45327c..65897edba7f 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -6,6 +6,7 @@ describe API::API, api: true do let(:user) { create(:user) } let(:admin) { create(:admin) } let(:project) { create(:project, creator_id: user.id) } + let(:project2) { create(:project, creator_id: user.id) } let(:deploy_key) { create(:deploy_key, public: true) } let!(:deploy_keys_project) do @@ -96,6 +97,22 @@ describe API::API, api: true do post api("/projects/#{project.id}/deploy_keys", admin), key_attrs end.to change{ project.deploy_keys.count }.by(1) end + + it 'returns an existing ssh key when attempting to add a duplicate' do + expect do + post api("/projects/#{project.id}/deploy_keys", admin), { key: deploy_key.key, title: deploy_key.title } + end.not_to change { project.deploy_keys.count } + + expect(response).to have_http_status(201) + end + + it 'joins an existing ssh key to a new project' do + expect do + post api("/projects/#{project2.id}/deploy_keys", admin), { key: deploy_key.key, title: deploy_key.title } + end.to change { project2.deploy_keys.count }.by(1) + + expect(response).to have_http_status(201) + end end describe 'DELETE /projects/:id/deploy_keys/:key_id' do -- cgit v1.2.1