Merge branch 'remove-deploy-key-endpoint' into 'master'

Remove deploy key endpoint

Closes #20569

See merge request !8716

changelogs/unreleased/remove-deploy-key-endpoint.yml

+---
+title: 'API: Remove /projects/:id/keys/.. endpoints'
+merge_request: 8716
+author: Robert Schilling

doc/api/v3_to_v4.md

@@ -11,3 +11,4 @@ changes are in V4:
 - `projects/:id/merge_requests?iid[]=x&iid[]=y` array filter has been renamed to `iids`
 - Endpoints under `projects/merge_request/:id` have been removed (use: `projects/merge_requests/:id`)
 - Project snippets do not return deprecated field `expires_at`
+- Endpoints under `projects/:id/keys` have been removed (use `projects/:id/deploy_keys`)

lib/api/api.rb

@@ -5,6 +5,7 @@ module API
     version %w(v3 v4), using: :path
 
     version 'v3', using: :path do
+      mount ::API::V3::DeployKeys
       mount ::API::V3::Issues
       mount ::API::V3::MergeRequests
       mount ::API::V3::Projects

lib/api/deploy_keys.rb

 module API
-  # Projects API
   class DeployKeys < Grape::API
     before { authenticate! }
 
@@ -16,107 +15,102 @@ module API
     resource :projects do
       before { authorize_admin_project }
 
-      # Routing "projects/:id/keys/..." is DEPRECATED and WILL BE REMOVED in version 9.0
-      # Use "projects/:id/deploy_keys/..." instead.
-      #
-      %w(keys deploy_keys).each do |path|
-        desc "Get a specific project's deploy keys" do
-          success Entities::SSHKey
-        end
-        get ":id/#{path}" do
-          present user_project.deploy_keys, with: Entities::SSHKey
-        end
+      desc "Get a specific project's deploy keys" do
+        success Entities::SSHKey
+      end
+      get ":id/deploy_keys" do
+        present user_project.deploy_keys, with: Entities::SSHKey
+      end
 
-        desc 'Get single deploy key' do
-          success Entities::SSHKey
-        end
-        params do
-          requires :key_id, type: Integer, desc: 'The ID of the deploy key'
-        end
-        get ":id/#{path}/:key_id" do
-          key = user_project.deploy_keys.find params[:key_id]
+      desc 'Get single deploy key' do
+        success Entities::SSHKey
+      end
+      params do
+        requires :key_id, type: Integer, desc: 'The ID of the deploy key'
+      end
+      get ":id/deploy_keys/:key_id" do
+        key = user_project.deploy_keys.find params[:key_id]
+        present key, with: Entities::SSHKey
+      end
+
+      desc 'Add new deploy key to currently authenticated user' do
+        success Entities::SSHKey
+      end
+      params do
+        requires :key, type: String, desc: 'The new deploy key'
+        requires :title, type: String, desc: 'The name of the deploy key'
+      end
+      post ":id/deploy_keys" do
+        params[:key].strip!
+
+        # Check for an existing key joined to this project
+        key = user_project.deploy_keys.find_by(key: params[:key])
+        if key
           present key, with: Entities::SSHKey
+          break
         end
 
-        desc 'Add new deploy key to currently authenticated user' do
-          success Entities::SSHKey
-        end
-        params do
-          requires :key, type: String, desc: 'The new deploy key'
-          requires :title, type: String, desc: 'The name of the deploy key'
+        # Check for available deploy keys in other projects
+        key = current_user.accessible_deploy_keys.find_by(key: params[:key])
+        if key
+          user_project.deploy_keys << key
+          present key, with: Entities::SSHKey
+          break
         end
-        post ":id/#{path}" do
-          params[:key].strip!
 
-          # Check for an existing key joined to this project
-          key = user_project.deploy_keys.find_by(key: params[: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: params[:key])
-          if key
-            user_project.deploy_keys << key
-            present key, with: Entities::SSHKey
-            break
-          end
-
-          # Create a new deploy key
-          key = DeployKey.new(declared_params(include_missing: false))
-          if key.valid? && user_project.deploy_keys << key
-            present key, with: Entities::SSHKey
-          else
-            render_validation_error!(key)
-          end
+        # Create a new deploy key
+        key = DeployKey.new(declared_params(include_missing: false))
+        if key.valid? && user_project.deploy_keys << key
+          present key, with: Entities::SSHKey
+        else
+          render_validation_error!(key)
         end
+      end
 
-        desc 'Enable a deploy key for a project' do
-          detail 'This feature was added in GitLab 8.11'
-          success Entities::SSHKey
-        end
-        params do
-          requires :key_id, type: Integer, desc: 'The ID of the deploy key'
-        end
-        post ":id/#{path}/:key_id/enable" do
-          key = ::Projects::EnableDeployKeyService.new(user_project,
-                                                        current_user, declared_params).execute
+      desc 'Enable a deploy key for a project' do
+        detail 'This feature was added in GitLab 8.11'
+        success Entities::SSHKey
+      end
+      params do
+        requires :key_id, type: Integer, desc: 'The ID of the deploy key'
+      end
+      post ":id/deploy_keys/:key_id/enable" do
+        key = ::Projects::EnableDeployKeyService.new(user_project,
+                                                      current_user, declared_params).execute
 
-          if key
-            present key, with: Entities::SSHKey
-          else
-            not_found!('Deploy Key')
-          end
+        if key
+          present key, with: Entities::SSHKey
+        else
+          not_found!('Deploy Key')
         end
+      end
 
-        desc 'Disable a deploy key for a project' do
-          detail 'This feature was added in GitLab 8.11'
-          success Entities::SSHKey
-        end
-        params do
-          requires :key_id, type: Integer, desc: 'The ID of the deploy key'
-        end
-        delete ":id/#{path}/:key_id/disable" do
-          key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
-          key.destroy
+      desc 'Disable a deploy key for a project' do
+        detail 'This feature was added in GitLab 8.11'
+        success Entities::SSHKey
+      end
+      params do
+        requires :key_id, type: Integer, desc: 'The ID of the deploy key'
+      end
+      delete ":id/deploy_keys/:key_id/disable" do
+        key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
+        key.destroy
 
-          present key.deploy_key, with: Entities::SSHKey
-        end
+        present key.deploy_key, with: Entities::SSHKey
+      end
 
-        desc 'Delete deploy key for a project' do
-          success Key
-        end
-        params do
-          requires :key_id, type: Integer, desc: 'The ID of the deploy key'
-        end
-        delete ":id/#{path}/:key_id" do
-          key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
-          if key
-            key.destroy
-          else
-            not_found!('Deploy Key')
-          end
+      desc 'Delete deploy key for a project' do
+        success Key
+      end
+      params do
+        requires :key_id, type: Integer, desc: 'The ID of the deploy key'
+      end
+      delete ":id/deploy_keys/:key_id" do
+        key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
+        if key
+          key.destroy
+        else
+          not_found!('Deploy Key')
         end
       end
     end

lib/api/v3/deploy_keys.rb

+module API
+  module V3
+    class DeployKeys < Grape::API
+      before { authenticate! }
+
+      get "deploy_keys" do
+        authenticated_as_admin!
+
+        keys = DeployKey.all
+        present keys, with: ::API::Entities::SSHKey
+      end
+
+      params do
+        requires :id, type: String, desc: 'The ID of the project'
+      end
+      resource :projects do
+        before { authorize_admin_project }
+
+        %w(keys deploy_keys).each do |path|
+          desc "Get a specific project's deploy keys" do
+            success ::API::Entities::SSHKey
+          end
+          get ":id/#{path}" do
+            present user_project.deploy_keys, with: ::API::Entities::SSHKey
+          end
+
+          desc 'Get single deploy key' do
+            success ::API::Entities::SSHKey
+          end
+          params do
+            requires :key_id, type: Integer, desc: 'The ID of the deploy key'
+          end
+          get ":id/#{path}/:key_id" do
+            key = user_project.deploy_keys.find params[:key_id]
+            present key, with: ::API::Entities::SSHKey
+          end
+
+          desc 'Add new deploy key to currently authenticated user' do
+            success ::API::Entities::SSHKey
+          end
+          params do
+            requires :key, type: String, desc: 'The new deploy key'
+            requires :title, type: String, desc: 'The name of the deploy key'
+          end
+          post ":id/#{path}" do
+            params[:key].strip!
+
+            # Check for an existing key joined to this project
+            key = user_project.deploy_keys.find_by(key: params[:key])
+            if key
+              present key, with: ::API::Entities::SSHKey
+              break
+            end
+
+            # Check for available deploy keys in other projects
+            key = current_user.accessible_deploy_keys.find_by(key: params[:key])
+            if key
+              user_project.deploy_keys << key
+              present key, with: ::API::Entities::SSHKey
+              break
+            end
+
+            # Create a new deploy key
+            key = DeployKey.new(declared_params(include_missing: false))
+            if key.valid? && user_project.deploy_keys << key
+              present key, with: ::API::Entities::SSHKey
+            else
+              render_validation_error!(key)
+            end
+          end
+
+          desc 'Enable a deploy key for a project' do
+            detail 'This feature was added in GitLab 8.11'
+            success ::API::Entities::SSHKey
+          end
+          params do
+            requires :key_id, type: Integer, desc: 'The ID of the deploy key'
+          end
+          post ":id/#{path}/:key_id/enable" do
+            key = ::Projects::EnableDeployKeyService.new(user_project,
+                                                          current_user, declared_params).execute
+
+            if key
+              present key, with: ::API::Entities::SSHKey
+            else
+              not_found!('Deploy Key')
+            end
+          end
+
+          desc 'Disable a deploy key for a project' do
+            detail 'This feature was added in GitLab 8.11'
+            success ::API::Entities::SSHKey
+          end
+          params do
+            requires :key_id, type: Integer, desc: 'The ID of the deploy key'
+          end
+          delete ":id/#{path}/:key_id/disable" do
+            key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
+            key.destroy
+
+            present key.deploy_key, with: ::API::Entities::SSHKey
+          end
+
+          desc 'Delete deploy key for a project' do
+            success Key
+          end
+          params do
+            requires :key_id, type: Integer, desc: 'The ID of the deploy key'
+          end
+          delete ":id/#{path}/:key_id" do
+            key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
+            if key
+              key.destroy
+            else
+              not_found!('Deploy Key')
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/requests/api/v3/deploy_keys_spec.rb

+require 'spec_helper'
+
+describe API::V3::DeployKeys, api: true  do
+  include ApiHelpers
+
+  let(:user)        { create(:user) }
+  let(:admin)       { create(:admin) }
+  let(:project)     { create(:empty_project, creator_id: user.id) }
+  let(:project2)    { create(:empty_project, creator_id: user.id) }
+  let(:deploy_key)  { create(:deploy_key, public: true) }
+
+  let!(:deploy_keys_project) do
+    create(:deploy_keys_project, project: project, deploy_key: deploy_key)
+  end
+
+  describe 'GET /deploy_keys' do
+    context 'when unauthenticated' do
+      it 'should return authentication error' do
+        get v3_api('/deploy_keys')
+
+        expect(response.status).to eq(401)
+      end
+    end
+
+    context 'when authenticated as non-admin user' do
+      it 'should return a 403 error' do
+        get v3_api('/deploy_keys', user)
+
+        expect(response.status).to eq(403)
+      end
+    end
+
+    context 'when authenticated as admin' do
+      it 'should return all deploy keys' do
+        get v3_api('/deploy_keys', admin)
+
+        expect(response.status).to eq(200)
+        expect(json_response).to be_an Array
+        expect(json_response.first['id']).to eq(deploy_keys_project.deploy_key.id)
+      end
+    end
+  end
+
+  %w(deploy_keys keys).each do |path|
+    describe "GET /projects/:id/#{path}" do
+      before { deploy_key }
+
+      it 'should return array of ssh keys' do
+        get v3_api("/projects/#{project.id}/#{path}", admin)
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.first['title']).to eq(deploy_key.title)
+      end
+    end
+
+    describe "GET /projects/:id/#{path}/:key_id" do
+      it 'should return a single key' do
+        get v3_api("/projects/#{project.id}/#{path}/#{deploy_key.id}", admin)
+
+        expect(response).to have_http_status(200)
+        expect(json_response['title']).to eq(deploy_key.title)
+      end
+
+      it 'should return 404 Not Found with invalid ID' do
+        get v3_api("/projects/#{project.id}/#{path}/404", admin)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    describe "POST /projects/:id/deploy_keys" do
+      it 'should not create an invalid ssh key' do
+        post v3_api("/projects/#{project.id}/#{path}", admin), { title: 'invalid key' }
+
+        expect(response).to have_http_status(400)
+        expect(json_response['error']).to eq('key is missing')
+      end
+
+      it 'should not create a key without title' do
+        post v3_api("/projects/#{project.id}/#{path}", admin), key: 'some key'
+
+        expect(response).to have_http_status(400)
+        expect(json_response['error']).to eq('title is missing')
+      end
+
+      it 'should create new ssh key' do
+        key_attrs = attributes_for :another_key
+
+        expect do
+          post v3_api("/projects/#{project.id}/#{path}", 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 v3_api("/projects/#{project.id}/#{path}", 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 v3_api("/projects/#{project2.id}/#{path}", 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/#{path}/:key_id" do
+      before { deploy_key }
+
+      it 'should delete existing key' do
+        expect do
+          delete v3_api("/projects/#{project.id}/#{path}/#{deploy_key.id}", admin)
+        end.to change{ project.deploy_keys.count }.by(-1)
+      end
+
+      it 'should return 404 Not Found with invalid ID' do
+        delete v3_api("/projects/#{project.id}/#{path}/404", admin)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    describe "POST /projects/:id/#{path}/:key_id/enable" do
+      let(:project2) { create(:empty_project) }
+
+      context 'when the user can admin the project' do
+        it 'enables the key' do
+          expect do
+            post v3_api("/projects/#{project2.id}/#{path}/#{deploy_key.id}/enable", admin)
+          end.to change { project2.deploy_keys.count }.from(0).to(1)
+
+          expect(response).to have_http_status(201)
+          expect(json_response['id']).to eq(deploy_key.id)
+        end
+      end
+
+      context 'when authenticated as non-admin user' do
+        it 'should return a 404 error' do
+          post v3_api("/projects/#{project2.id}/#{path}/#{deploy_key.id}/enable", user)
+
+          expect(response).to have_http_status(404)
+        end
+      end
+    end
+
+    describe "DELETE /projects/:id/deploy_keys/:key_id/disable" do
+      context 'when the user can admin the project' do
+        it 'disables the key' do
+          expect do
+            delete v3_api("/projects/#{project.id}/#{path}/#{deploy_key.id}/disable", admin)
+          end.to change { project.deploy_keys.count }.from(1).to(0)
+
+          expect(response).to have_http_status(200)
+          expect(json_response['id']).to eq(deploy_key.id)
+        end
+      end
+
+      context 'when authenticated as non-admin user' do
+        it 'should return a 404 error' do
+          delete v3_api("/projects/#{project.id}/#{path}/#{deploy_key.id}/disable", user)
+
+          expect(response).to have_http_status(404)
+        end
+      end
+    end
+  end
+end