Merge branch 'zj-enable-deploy-keys-api' into 'master'

Enable/Disable Deploy keys for a project

Closes #20123 

## Does this MR meet the acceptance criteria?

- [X] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [X] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [X] API support added
- Tests
  - [X] Added for this feature/bug
  - [X] All builds are passing

See merge request !5647

CHANGELOG

@@ -6,6 +6,7 @@ v 8.11.0 (unreleased)
   - Fix the title of the toggle dropdown button. !5515 (herminiotorres)
   - Improve diff performance by eliminating redundant checks for text blobs
   - Convert switch icon into icon font (ClemMakesApps)
+  - API: Endpoints for enabling and disabling deploy keys
   - Remove magic comments (`# encoding: UTF-8`) from Ruby files. !5456 (winniehell)
   - Add support for relative links starting with ./ or / to RelativeLinkFilter (winniehell)
   - Ignore URLs starting with // in Markdown links !5677 (winniehell)

app/controllers/projects/deploy_keys_controller.rb

@@ -12,8 +12,7 @@ class Projects::DeployKeysController < Projects::ApplicationController
   end
 
   def new
-    redirect_to namespace_project_deploy_keys_path(@project.namespace,
-                                                   @project)
+    redirect_to namespace_project_deploy_keys_path(@project.namespace, @project)
   end
 
   def create
@@ -21,19 +20,16 @@ class Projects::DeployKeysController < Projects::ApplicationController
     set_index_vars
 
     if @key.valid? && @project.deploy_keys << @key
-      redirect_to namespace_project_deploy_keys_path(@project.namespace,
-                                                     @project)
+      redirect_to namespace_project_deploy_keys_path(@project.namespace, @project)
     else
       render "index"
     end
   end
 
   def enable
-    @key = accessible_keys.find(params[:id])
-    @project.deploy_keys << @key
+    Projects::EnableDeployKeyService.new(@project, current_user, params).execute
 
-    redirect_to namespace_project_deploy_keys_path(@project.namespace,
-                                                   @project)
+    redirect_to namespace_project_deploy_keys_path(@project.namespace, @project)
   end
 
   def disable
@@ -45,9 +41,9 @@ class Projects::DeployKeysController < Projects::ApplicationController
   protected
 
   def set_index_vars
-    @enabled_keys ||= @project.deploy_keys
+    @enabled_keys           ||= @project.deploy_keys
 
-    @available_keys         ||= accessible_keys - @enabled_keys
+    @available_keys         ||= current_user.accessible_deploy_keys - @enabled_keys
     @available_project_keys ||= current_user.project_deploy_keys - @enabled_keys
     @available_public_keys  ||= DeployKey.are_public - @enabled_keys
 
@@ -56,10 +52,6 @@ class Projects::DeployKeysController < Projects::ApplicationController
     @available_public_keys -= @available_project_keys
   end
 
-  def accessible_keys
-    @accessible_keys ||= current_user.accessible_deploy_keys
-  end
-
   def deploy_key_params
     params.require(:deploy_key).permit(:key, :title)
   end

app/services/projects/enable_deploy_key_service.rb

+module Projects
+  class EnableDeployKeyService < BaseService
+    def execute
+      key = accessible_keys.find_by(id: params[:key_id] || params[:id])
+      return unless key
+
+      project.deploy_keys << key
+      key
+    end
+
+    private
+
+    def accessible_keys
+      current_user.accessible_deploy_keys
+    end
+  end
+end

doc/api/deploy_keys.md

@@ -159,3 +159,51 @@ Example response:
    "id" : 13
 }
 ```
+
+## Enable a deploy key
+
+Enables a deploy key for a project so this can be used. Returns the enabled key, with a status code 201 when successful.
+
+```bash
+curl -X POST -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/5/deploy_keys/13/enable
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id`      | integer | yes | The ID of the project |
+| `key_id`  | integer | yes | The ID of the deploy key |
+
+Example response:
+
+```json
+{
+   "key" : "ssh-rsa AAAA...",
+   "id" : 12,
+   "title" : "My deploy key",
+   "created_at" : "2015-08-29T12:44:31.550Z"
+}
+```
+
+## Disable a deploy key
+
+Disable a deploy key for a project. Returns the disabled key.
+
+```bash
+curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/5/deploy_keys/13/disable
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id`      | integer | yes | The ID of the project |
+| `key_id`  | integer | yes | The ID of the deploy key |
+
+Example response:
+
+```json
+{
+   "key" : "ssh-rsa AAAA...",
+   "id" : 12,
+   "title" : "My deploy key",
+   "created_at" : "2015-08-29T12:44:31.550Z"
+}
+```

lib/api/deploy_keys.rb

@@ -10,6 +10,9 @@ module API
       present keys, with: Entities::SSHKey
     end
 
+    params do
+      requires :id, type: String, desc: 'The ID of the project'
+    end
     resource :projects do
       before { authorize_admin_project }
 
@@ -17,52 +20,43 @@ module API
       # Use "projects/:id/deploy_keys/..." instead.
       #
       %w(keys deploy_keys).each do |path|
-        # Get a specific project's deploy keys
-        #
-        # Example Request:
-        #   GET /projects/:id/deploy_keys
+        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
 
-        # Get single deploy key owned by currently authenticated user
-        #
-        # Example Request:
-        #   GET /projects/:id/deploy_keys/: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/#{path}/:key_id" do
           key = user_project.deploy_keys.find params[:key_id]
           present key, with: Entities::SSHKey
         end
 
-        # Add new deploy key to currently authenticated user
-        # If deploy key already exists - it will be joined to project
-        # but only if original one was accessible by same user
-        #
-        # Parameters:
-        #   key (required) - New deploy Key
-        #   title (required) - New deploy Key's title
-        # Example Request:
-        #   POST /projects/:id/deploy_keys
+        # TODO: for 9.0 we should check if params are there with the params block
+        # grape provides, at this point we'd change behaviour so we can't
+        # Behaviour now if you don't provide all required params: it renders a
+        # validation error or two.
+        desc 'Add new deploy key to currently authenticated user' do
+          success Entities::SSHKey
+        end
         post ":id/#{path}" do
           attrs = attributes_for_keys [:title, :key]
+          attrs[:key].strip! if attrs[:key]
 
-          if attrs[:key].present?
-            attrs[:key].strip!
-
-            # check if key already exist in project
-            key = user_project.deploy_keys.find_by(key: attrs[:key])
-            if key
-              present key, with: Entities::SSHKey
-              next
-            end
+          key = user_project.deploy_keys.find_by(key: attrs[:key])
+          present key, with: Entities::SSHKey if key
 
-            # 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
-              next
-            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
           end
 
           key = DeployKey.new attrs
@@ -74,12 +68,46 @@ module API
           end
         end
 
-        # Delete existing deploy key of currently authenticated user
-        #
-        # Example Request:
-        #   DELETE /projects/:id/deploy_keys/:key_id
+        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
+
+          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
+
+          present key.deploy_key, with: Entities::SSHKey
+        end
+
+        desc 'Delete existing deploy key of currently authenticated user' 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.find params[:key_id]
+          key = user_project.deploy_keys.find(params[:key_id])
           key.destroy
         end
       end

spec/requests/api/deploy_keys.rb

-require 'spec_helper'
-
-describe API::API, api: true  do
-  include ApiHelpers
-
-  let(:user)  { create(:user) }
-  let(:project) { create(:project, creator_id: user.id) }
-  let!(:deploy_keys_project) { create(:deploy_keys_project, project: project) }
-  let(:admin) { create(:admin) }
-
-  describe 'GET /deploy_keys' do
-    before { admin }
-
-    context 'when unauthenticated' do
-      it 'should return authentication error' do
-        get 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 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 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
-end

spec/requests/api/deploy_keys_spec.rb

+require 'spec_helper'
+
+describe API::API, api: true  do
+  include ApiHelpers
+
+  let(:user)        { create(:user) }
+  let(:admin)       { create(:admin) }
+  let(:project)     { create(: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 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 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 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
+
+  describe 'GET /projects/:id/deploy_keys' do
+    before { deploy_key }
+
+    it 'should return array of ssh keys' do
+      get api("/projects/#{project.id}/deploy_keys", 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/deploy_keys/:key_id' do
+    it 'should return a single key' do
+      get api("/projects/#{project.id}/deploy_keys/#{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 api("/projects/#{project.id}/deploy_keys/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 api("/projects/#{project.id}/deploy_keys", admin), { title: 'invalid key' }
+
+      expect(response).to have_http_status(400)
+      expect(json_response['message']['key']).to eq([
+        'can\'t be blank',
+        'is too short (minimum is 0 characters)',
+        'is invalid'
+      ])
+    end
+
+    it 'should not create a key without title' do
+      post api("/projects/#{project.id}/deploy_keys", admin), key: 'some key'
+
+      expect(response).to have_http_status(400)
+      expect(json_response['message']['title']).to eq([
+        'can\'t be blank',
+        'is too short (minimum is 0 characters)'
+      ])
+    end
+
+    it 'should create new ssh key' do
+      key_attrs = attributes_for :another_key
+
+      expect do
+        post api("/projects/#{project.id}/deploy_keys", admin), key_attrs
+      end.to change{ project.deploy_keys.count }.by(1)
+    end
+  end
+
+  describe 'DELETE /projects/:id/deploy_keys/:key_id' do
+    before { deploy_key }
+
+    it 'should delete existing key' do
+      expect do
+        delete api("/projects/#{project.id}/deploy_keys/#{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 api("/projects/#{project.id}/deploy_keys/404", admin)
+
+      expect(response).to have_http_status(404)
+    end
+  end
+
+  describe 'POST /projects/:id/deploy_keys/: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 api("/projects/#{project2.id}/deploy_keys/#{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 api("/projects/#{project2.id}/deploy_keys/#{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 api("/projects/#{project.id}/deploy_keys/#{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 api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}/disable", user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+end

spec/requests/api/projects_spec.rb

@@ -641,79 +641,7 @@ describe API::API, api: true  do
       expect(response).to have_http_status(404)
     end
   end
-
-  describe :deploy_keys do
-    let(:deploy_keys_project) { create(:deploy_keys_project, project: project) }
-    let(:deploy_key) { deploy_keys_project.deploy_key }
-
-    describe 'GET /projects/:id/deploy_keys' do
-      before { deploy_key }
-
-      it 'should return array of ssh keys' do
-        get api("/projects/#{project.id}/deploy_keys", user)
-        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/deploy_keys/:key_id' do
-      it 'should return a single key' do
-        get api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", user)
-        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 api("/projects/#{project.id}/deploy_keys/404", user)
-        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 api("/projects/#{project.id}/deploy_keys", user), { title: 'invalid key' }
-        expect(response).to have_http_status(400)
-        expect(json_response['message']['key']).to eq([
-          'can\'t be blank',
-          'is too short (minimum is 0 characters)',
-          'is invalid'
-        ])
-      end
-
-      it 'should not create a key without title' do
-        post api("/projects/#{project.id}/deploy_keys", user), key: 'some key'
-        expect(response).to have_http_status(400)
-        expect(json_response['message']['title']).to eq([
-          'can\'t be blank',
-          'is too short (minimum is 0 characters)'
-        ])
-      end
-
-      it 'should create new ssh key' do
-        key_attrs = attributes_for :key
-        expect do
-          post api("/projects/#{project.id}/deploy_keys", user), key_attrs
-        end.to change{ project.deploy_keys.count }.by(1)
-      end
-    end
-
-    describe 'DELETE /projects/:id/deploy_keys/:key_id' do
-      before { deploy_key }
-
-      it 'should delete existing key' do
-        expect do
-          delete api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", user)
-        end.to change{ project.deploy_keys.count }.by(-1)
-      end
-
-      it 'should return 404 Not Found with invalid ID' do
-        delete api("/projects/#{project.id}/deploy_keys/404", user)
-        expect(response).to have_http_status(404)
-      end
-    end
-  end
-
+  
   describe :fork_admin do
     let(:project_fork_target) { create(:project) }
     let(:project_fork_source) { create(:project, :public) }

spec/services/projects/enable_deploy_key_service_spec.rb

+require 'spec_helper'
+
+describe Projects::EnableDeployKeyService, services: true do
+  let(:deploy_key)  { create(:deploy_key, public: true) }
+  let(:project)     { create(:empty_project) }
+  let(:user)        { project.creator}
+  let!(:params)     { { key_id: deploy_key.id } }
+
+  it 'enables the key' do
+    expect do
+      service.execute
+    end.to change { project.deploy_keys.count }.from(0).to(1)
+  end
+
+  context 'trying to add an unaccessable key' do
+    let(:another_key) { create(:another_key) }
+    let!(:params)     { { key_id: another_key.id } }
+
+    it 'returns nil if the key cannot be added' do
+      expect(service.execute).to be nil
+    end
+  end
+
+  def service
+    Projects::EnableDeployKeyService.new(project, user, params)
+  end
+end