Fixes various errors when adding deploy keys caused by not exiting the control flow.

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 <remy@rymai.me>

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