Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3'

[10.3] Migrate `can_push` column from `keys` to `deploy_keys_project`

See merge request gitlab/gitlabhq!2276

(cherry picked from commit f6ca52d31bac350a23938e0aebf717c767b4710c)

1f2bd3c0 Backport to 10.3

app/assets/javascripts/deploy_keys/components/key.vue

 <script>
   import actionBtn from './action_btn.vue';
+<<<<<<< HEAD
   import { getTimeago } from '../../lib/utils/datetime_utility';
+=======
+  import tooltip from '../../vue_shared/directives/tooltip';
+>>>>>>> Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3'
 
   export default {
     components: {
@@ -20,6 +24,15 @@
         required: true,
       },
     },
+<<<<<<< HEAD
+=======
+    directives: {
+      tooltip,
+    },
+    components: {
+      actionBtn,
+    },
+>>>>>>> Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3'
     computed: {
       timeagoDate() {
         return getTimeago().format(this.deployKey.created_at);
@@ -32,6 +45,9 @@
       isEnabled(id) {
         return this.store.findEnabledKey(id) !== undefined;
       },
+      tooltipTitle(project) {
+        return project.can_push ? 'Write access allowed' : 'Read access only';
+      },
     },
   };
 </script>
@@ -52,21 +68,29 @@
       <div class="description">
         {{ deployKey.fingerprint }}
       </div>
-      <div
-        v-if="deployKey.can_push"
-        class="write-access-allowed"
-      >
-        Write access allowed
-      </div>
     </div>
     <div class="deploy-key-content prepend-left-default deploy-key-projects">
       <a
+<<<<<<< HEAD
         v-for="(project, i) in deployKey.projects"
         class="label deploy-project-label"
         :href="project.full_path"
         :key="i"
+=======
+        v-for="deployKeysProject in deployKey.deploy_keys_projects"
+        class="label deploy-project-label"
+        :href="deployKeysProject.project.full_path"
+        :title="tooltipTitle(deployKeysProject)"
+        v-tooltip
+>>>>>>> Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3'
+      >
+        {{ deployKeysProject.project.full_name }}
+        <i
+          v-if="!deployKeysProject.can_push"
+          aria-hidden="true"
+          class="fa fa-lock"
         >
-        {{ project.full_name }}
+        </i>
       </a>
     </div>
     <div class="deploy-key-content">

app/controllers/admin/deploy_keys_controller.rb

@@ -50,10 +50,10 @@ class Admin::DeployKeysController < Admin::ApplicationController
   end
 
   def create_params
-    params.require(:deploy_key).permit(:key, :title, :can_push)
+    params.require(:deploy_key).permit(:key, :title)
   end
 
   def update_params
-    params.require(:deploy_key).permit(:title, :can_push)
+    params.require(:deploy_key).permit(:title)
   end
 end

app/controllers/projects/deploy_keys_controller.rb

@@ -24,7 +24,7 @@ class Projects::DeployKeysController < Projects::ApplicationController
   def create
     @key = DeployKeys::CreateService.new(current_user, create_params).execute
 
-    unless @key.valid? && @project.deploy_keys << @key
+    unless @key.valid?
       flash[:alert] = @key.errors.full_messages.join(', ').html_safe
     end
 
@@ -71,11 +71,14 @@ class Projects::DeployKeysController < Projects::ApplicationController
   end
 
   def create_params
-    params.require(:deploy_key).permit(:key, :title, :can_push)
+    create_params = params.require(:deploy_key)
+                          .permit(:key, :title, deploy_keys_projects_attributes: [:can_push])
+    create_params.dig(:deploy_keys_projects_attributes, '0')&.merge!(project_id: @project.id)
+    create_params
   end
 
   def update_params
-    params.require(:deploy_key).permit(:title, :can_push)
+    params.require(:deploy_key).permit(:title, deploy_keys_projects_attributes: [:id, :can_push])
   end
 
   def authorize_update_deploy_key!

app/models/deploy_key.rb

 class DeployKey < Key
-  has_many :deploy_keys_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+  include IgnorableColumn
+
+  has_many :deploy_keys_projects, inverse_of: :deploy_key, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :projects, through: :deploy_keys_projects
 
   scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) }
   scope :are_public,  -> { where(public: true) }
 
+  ignore_column :can_push
+
+  accepts_nested_attributes_for :deploy_keys_projects
+
   def private?
     !public?
   end
@@ -22,10 +28,18 @@ class DeployKey < Key
   end
 
   def has_access_to?(project)
-    projects.include?(project)
+    deploy_keys_project_for(project).present?
   end
 
   def can_push_to?(project)
-    can_push? && has_access_to?(project)
+    !!deploy_keys_project_for(project)&.can_push?
+  end
+
+  def deploy_keys_project_for(project)
+    deploy_keys_projects.find_by(project: project)
+  end
+
+  def projects_with_write_access
+    Project.preload(:route).where(id: deploy_keys_projects.with_write_access.select(:project_id))
   end
 end

app/models/deploy_keys_project.rb

 class DeployKeysProject < ActiveRecord::Base
   belongs_to :project
-  belongs_to :deploy_key
+  belongs_to :deploy_key, inverse_of: :deploy_keys_projects
 
-  validates :deploy_key_id, presence: true
+  scope :without_project_deleted,  -> { joins(:project).where(projects: { pending_delete: false }) }
+  scope :in_project, ->(project) { where(project: project) }
+  scope :with_write_access, -> { where(can_push: true) }
+
+  accepts_nested_attributes_for :deploy_key
+
+  validates :deploy_key, presence: true
   validates :deploy_key_id, uniqueness: { scope: [:project_id], message: "already exists in project" }
   validates :project_id, presence: true
 

app/presenters/projects/settings/deploy_keys_presenter.rb

@@ -7,7 +7,7 @@ module Projects
       delegate :size, to: :available_public_keys, prefix: true
 
       def new_key
-        @key ||= DeployKey.new
+        @key ||= DeployKey.new.tap { |dk| dk.deploy_keys_projects.build }
       end
 
       def enabled_keys

app/serializers/deploy_key_entity.rb

@@ -3,19 +3,20 @@ class DeployKeyEntity < Grape::Entity
   expose :user_id
   expose :title
   expose :fingerprint
-  expose :can_push
   expose :destroyed_when_orphaned?, as: :destroyed_when_orphaned
   expose :almost_orphaned?, as: :almost_orphaned
   expose :created_at
   expose :updated_at
-  expose :projects, using: ProjectEntity do |deploy_key|
-    deploy_key.projects.without_deleted.select { |project| options[:user].can?(:read_project, project) }
+  expose :deploy_keys_projects, using: DeployKeysProjectEntity do |deploy_key|
+    deploy_key.deploy_keys_projects
+              .without_project_deleted
+              .select { |deploy_key_project| Ability.allowed?(options[:user], :read_project, deploy_key_project.project) }
   end
   expose :can_edit
 
   private
 
   def can_edit
-    options[:user].can?(:update_deploy_key, object)
+    Ability.allowed?(options[:user], :update_deploy_key, object)
   end
 end

app/serializers/deploy_keys_project_entity.rb

+class DeployKeysProjectEntity < Grape::Entity
+  expose :can_push
+  expose :project, using: ProjectEntity
+end

app/views/admin/deploy_keys/index.html.haml

@@ -12,7 +12,7 @@
         %tr
           %th.col-sm-2 Title
           %th.col-sm-4 Fingerprint
-          %th.col-sm-2 Write access allowed
+          %th.col-sm-2 Projects with write access
           %th.col-sm-2 Added at
           %th.col-sm-2
       %tbody
@@ -23,10 +23,8 @@
             %td
               %code.key-fingerprint= deploy_key.fingerprint
             %td
-              - if deploy_key.can_push?
-                Yes
-              - else
-                No
+              - deploy_key.projects_with_write_access.each do |project|
+                = link_to project.full_name, admin_project_path(project), class: 'label deploy-project-label'
             %td
               %span.cgray
                 added #{time_ago_with_tooltip(deploy_key.created_at)}

app/views/projects/deploy_keys/_form.html.haml

@@ -10,10 +10,12 @@
     %p.light.append-bottom-0
       Paste a machine public key here. Read more about how to generate it
       = link_to "here", help_page_path("ssh/README")
+
+  = f.fields_for :deploy_keys_projects do |deploy_keys_project_form|
     .form-group
       .checkbox
-      = f.label :can_push do
-        = f.check_box :can_push
+        = deploy_keys_project_form.label :can_push do
+          = deploy_keys_project_form.check_box :can_push
           %strong Write access allowed
     .form-group
       %p.light.append-bottom-0

app/views/shared/deploy_keys/_form.html.haml

 - form = local_assigns.fetch(:form)
 - deploy_key = local_assigns.fetch(:deploy_key)
+- deploy_keys_project = deploy_key.deploy_keys_project_for(@project)
 
 = form_errors(deploy_key)
 
@@ -20,11 +21,13 @@
     .col-sm-10
       = form.text_field :fingerprint, class: 'form-control', readonly: 'readonly'
 
-.form-group
+- if deploy_keys_project.present?
+  = form.fields_for :deploy_keys_projects, deploy_keys_project do |deploy_keys_project_form|
+    .form-group
       .control-label
       .col-sm-10
-    = form.label :can_push do
-      = form.check_box :can_push
+        = deploy_keys_project_form.label :can_push do
+          = deploy_keys_project_form.check_box :can_push
           %strong Write access allowed
         %p.light.append-bottom-0
           Allow this key to push to repository as well? (Default only allows pull access.)

changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml

+---
+title: Fix writable shared deploy keys
+merge_request:
+author:
+type: security

db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb

+class AddCanPushToDeployKeysProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :deploy_keys_projects, :can_push, :boolean, default: false, allow_null: false
+  end
+
+  def down
+    remove_column :deploy_keys_projects, :can_push
+  end
+end

db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class PopulateCanPushFromDeployKeysProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  class DeploysKeyProject < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'deploy_keys_projects'
+  end
+
+  def up
+    DeploysKeyProject.each_batch(of: 10_000) do |batch|
+      start_id, end_id = batch.pluck('MIN(id), MAX(id)').first
+
+      execute <<-EOF
+        UPDATE deploy_keys_projects
+        SET can_push = keys.can_push
+        FROM keys
+        WHERE deploy_key_id = keys.id
+        AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id}
+      EOF
+    end
+  end
+
+  def down
+    DeploysKeyProject.each_batch(of: 10_000) do |batch|
+      start_id, end_id = batch.pluck('MIN(id), MAX(id)').first
+
+      execute <<-EOF
+        UPDATE keys
+        SET can_push = deploy_keys_projects.can_push
+        FROM deploy_keys_projects
+        WHERE deploy_keys_projects.deploy_key_id = keys.id
+        AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id}
+      EOF
+    end
+  end
+end

db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class PostPopulateCanPushFromDeployKeysProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  class DeploysKeyProject < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'deploy_keys_projects'
+  end
+
+  def up
+    DeploysKeyProject.each_batch(of: 10_000) do |batch|
+      start_id, end_id = batch.pluck('MIN(id), MAX(id)').first
+
+      execute <<-EOF
+        UPDATE deploy_keys_projects
+        SET can_push = keys.can_push
+        FROM keys
+        WHERE deploy_key_id = keys.id
+        AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id}
+      EOF
+    end
+  end
+
+  def down
+    DeploysKeyProject.each_batch(of: 10_000) do |batch|
+      start_id, end_id = batch.pluck('MIN(id), MAX(id)').first
+
+      execute <<-EOF
+        UPDATE keys
+        SET can_push = deploy_keys_projects.can_push
+        FROM deploy_keys_projects
+        WHERE deploy_keys_projects.deploy_key_id = keys.id
+        AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id}
+      EOF
+    end
+  end
+end

db/post_migrate/20171215121259_remove_can_push_from_keys.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveCanPushFromKeys < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    remove_column :keys, :can_push
+  end
+
+  def down
+    add_column_with_default :keys, :can_push, :boolean, default: false, allow_null: false
+  end
+end

db/schema.rb

@@ -11,7 +11,11 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
+<<<<<<< HEAD
 ActiveRecord::Schema.define(version: 20180105212544) do
+=======
+ActiveRecord::Schema.define(version: 20171215121259) do
+>>>>>>> Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3'
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -626,6 +630,7 @@ ActiveRecord::Schema.define(version: 20180105212544) do
     t.integer "project_id", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.boolean "can_push", default: false, null: false
   end
 
   add_index "deploy_keys_projects", ["project_id"], name: "index_deploy_keys_projects_on_project_id", using: :btree
@@ -896,7 +901,6 @@ ActiveRecord::Schema.define(version: 20180105212544) do
     t.string "type"
     t.string "fingerprint"
     t.boolean "public", default: false, null: false
-    t.boolean "can_push", default: false, null: false
     t.datetime "last_used_at"
   end
 

doc/api/deploy_keys.md

@@ -20,14 +20,12 @@ Example response:
     "id": 1,
     "title": "Public key",
     "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=",    
-    "can_push": false,
     "created_at": "2013-10-02T10:12:29Z"
   },
   {
     "id": 3,
     "title": "Another Public key",
     "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=",    
-    "can_push": true,
     "created_at": "2013-10-02T11:12:29Z"
   }
 ]
@@ -57,15 +55,15 @@ Example response:
     "id": 1,
     "title": "Public key",
     "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=",
-    "can_push": false,
-    "created_at": "2013-10-02T10:12:29Z"
+    "created_at": "2013-10-02T10:12:29Z",
+    "can_push": false
   },
   {
     "id": 3,
     "title": "Another Public key",
     "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=",
-    "can_push": false,
-    "created_at": "2013-10-02T11:12:29Z"
+    "created_at": "2013-10-02T11:12:29Z",
+    "can_push": false
   }
 ]
 ```
@@ -96,8 +94,8 @@ Example response:
   "id": 1,
   "title": "Public key",
   "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=",
-  "can_push": false,
-  "created_at": "2013-10-02T10:12:29Z"
+  "created_at": "2013-10-02T10:12:29Z",
+  "can_push": false
 }
 ```
 
@@ -135,6 +133,36 @@ Example response:
 }
 ```
 
+## Update deploy key
+
+Updates a deploy key for a project.
+
+```
+PUT /projects/:id/deploy_keys/:key_id
+```
+
+| Attribute  | Type | Required | Description |
+| ---------  | ---- | -------- | ----------- |
+| `id`       | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `title`    | string  | no | New deploy key's title |
+| `can_push` | boolean | no  | Can deploy key push to the project's repository |
+
+```bash
+curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" --header "Content-Type: application/json" --data '{"title": "New deploy key", "can_push": true}' "https://gitlab.example.com/api/v4/projects/5/deploy_keys/11"
+```
+
+Example response:
+
+```json
+{
+   "id": 11,
+   "title": "New deploy key",
+   "key": "ssh-rsa AAAA...",
+   "created_at": "2015-08-29T12:44:31.550Z",
+   "can_push": true
+}
+```
+
 ## Delete deploy key
 
 Removes a deploy key from the project. If the deploy key is used only for this project, it will be deleted from the system.

lib/api/deploy_keys.rb

@@ -4,6 +4,16 @@ module API
 
     before { authenticate! }
 
+    helpers do
+      def add_deploy_keys_project(project, attrs = {})
+        project.deploy_keys_projects.create(attrs)
+      end
+
+      def find_by_deploy_key(project, key_id)
+        project.deploy_keys_projects.find_by!(deploy_key: key_id)
+      end
+    end
+
     desc 'Return all deploy keys'
     params do
       use :pagination
@@ -21,28 +31,31 @@ module API
       before { authorize_admin_project }
 
       desc "Get a specific project's deploy keys" do
-        success Entities::SSHKey
+        success Entities::DeployKeysProject
       end
       params do
         use :pagination
       end
       get ":id/deploy_keys" do
-        present paginate(user_project.deploy_keys), with: Entities::SSHKey
+        keys = user_project.deploy_keys_projects.preload(:deploy_key)
+
+        present paginate(keys), with: Entities::DeployKeysProject
       end
 
       desc 'Get single deploy key' do
-        success Entities::SSHKey
+        success Entities::DeployKeysProject
       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
+        key = find_by_deploy_key(user_project, params[:key_id])
+
+        present key, with: Entities::DeployKeysProject
       end
 
       desc 'Add new deploy key to currently authenticated user' do
-        success Entities::SSHKey
+        success Entities::DeployKeysProject
       end
       params do
         requires :key, type: String, desc: 'The new deploy key'
@@ -53,24 +66,31 @@ module API
         params[:key].strip!
 
         # Check for an existing key joined to this project
-        key = user_project.deploy_keys.find_by(key: params[:key])
+        key = user_project.deploy_keys_projects
+                          .joins(:deploy_key)
+                          .find_by(keys: { key: params[:key] })
+
         if key
-          present key, with: Entities::SSHKey
+          present key, with: Entities::DeployKeysProject
           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
+          added_key = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push])
+
+          present added_key, with: Entities::DeployKeysProject
           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
+        key_attributes = { can_push: !!params[:can_push],
+                           deploy_key_attributes: declared_params.except(:can_push) }
+        key = add_deploy_keys_project(user_project, key_attributes)
+
+        if key.valid?
+          present key, with: Entities::DeployKeysProject
         else
           render_validation_error!(key)
         end
@@ -86,14 +106,20 @@ module API
         at_least_one_of :title, :can_push
       end
       put ":id/deploy_keys/:key_id" do
-        key = DeployKey.find(params.delete(:key_id))
+        deploy_keys_project = find_by_deploy_key(user_project, params[:key_id])
 
-        authorize!(:update_deploy_key, key)
+        authorize!(:update_deploy_key, deploy_keys_project.deploy_key)
 
-        if key.update_attributes(declared_params(include_missing: false))
-          present key, with: Entities::SSHKey
+        can_push = params[:can_push].nil? ? deploy_keys_project.can_push : params[:can_push]
+        title = params[:title] || deploy_keys_project.deploy_key.title
+
+        result = deploy_keys_project.update_attributes(can_push: can_push,
+                                                       deploy_key_attributes: { id: params[:key_id],
+                                                                                title: title })
+        if result
+          present deploy_keys_project, with: Entities::DeployKeysProject
         else
-          render_validation_error!(key)
+          render_validation_error!(deploy_keys_project)
         end
       end
 
@@ -122,7 +148,7 @@ module API
         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])
+        key = user_project.deploy_keys.find(params[:key_id])
         not_found!('Deploy Key') unless key
 
         destroy_conditionally!(key)

lib/api/entities.rb

@@ -554,13 +554,18 @@ module API
     end
 
     class SSHKey < Grape::Entity
-      expose :id, :title, :key, :created_at, :can_push
+      expose :id, :title, :key, :created_at
     end
 
     class SSHKeyWithUser < SSHKey
       expose :user, using: Entities::UserPublic
     end
 
+    class DeployKeysProject < Grape::Entity
+      expose :deploy_key, merge: true, using: Entities::SSHKey
+      expose :can_push
+    end
+
     class GPGKey < Grape::Entity
       expose :id, :key, :created_at
     end

lib/api/v3/deploy_keys.rb

@@ -3,6 +3,16 @@ module API
     class DeployKeys < Grape::API
       before { authenticate! }
 
+      helpers do
+        def add_deploy_keys_project(project, attrs = {})
+          project.deploy_keys_projects.create(attrs)
+        end
+
+        def find_by_deploy_key(project, key_id)
+          project.deploy_keys_projects.find_by!(deploy_key: key_id)
+        end
+      end
+
       get "deploy_keys" do
         authenticated_as_admin!
 
@@ -18,25 +28,28 @@ module API
 
         %w(keys deploy_keys).each do |path|
           desc "Get a specific project's deploy keys" do
-            success ::API::Entities::SSHKey
+            success ::API::Entities::DeployKeysProject
           end
           get ":id/#{path}" do
-            present user_project.deploy_keys, with: ::API::Entities::SSHKey
+            keys = user_project.deploy_keys_projects.preload(:deploy_key)
+
+            present keys, with: ::API::Entities::DeployKeysProject
           end
 
           desc 'Get single deploy key' do
-            success ::API::Entities::SSHKey
+            success ::API::Entities::DeployKeysProject
           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
+            key = find_by_deploy_key(user_project, params[:key_id])
+
+            present key, with: ::API::Entities::DeployKeysProject
           end
 
           desc 'Add new deploy key to currently authenticated user' do
-            success ::API::Entities::SSHKey
+            success ::API::Entities::DeployKeysProject
           end
           params do
             requires :key, type: String, desc: 'The new deploy key'
@@ -47,24 +60,31 @@ module API
             params[:key].strip!
 
             # Check for an existing key joined to this project
-            key = user_project.deploy_keys.find_by(key: params[:key])
+            key = user_project.deploy_keys_projects
+                              .joins(:deploy_key)
+                              .find_by(keys: { key: params[:key] })
+
             if key
-              present key, with: ::API::Entities::SSHKey
+              present key, with: ::API::Entities::DeployKeysProject
               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
+              added_key = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push])
+
+              present added_key, with: ::API::Entities::DeployKeysProject
               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
+            key_attributes = { can_push: !!params[:can_push],
+                               deploy_key_attributes: declared_params.except(:can_push) }
+            key = add_deploy_keys_project(user_project, key_attributes)
+
+            if key.valid?
+              present key, with: ::API::Entities::DeployKeysProject
             else
               render_validation_error!(key)
             end

spec/factories/deploy_keys_projects.rb

@@ -2,5 +2,9 @@ FactoryBot.define do
   factory :deploy_keys_project do
     deploy_key
     project
+
+    trait :write_access do
+      can_push true
+    end
   end
 end

spec/factories/keys.rb

@@ -15,10 +15,6 @@ FactoryBot.define do
       factory :another_deploy_key, class: 'DeployKey'
     end
 
-    factory :write_access_key, class: 'DeployKey' do
-      can_push true
-    end
-
     factory :rsa_key_2048 do
       key do
         <<~KEY.delete("\n")

spec/features/admin/admin_deploy_keys_spec.rb

@@ -17,6 +17,16 @@ RSpec.describe 'admin deploy keys' do
     end
   end
 
+  it 'shows all the projects the deploy key has write access' do
+    write_key = create(:deploy_keys_project, :write_access, deploy_key: deploy_key)
+
+    visit admin_deploy_keys_path
+
+    page.within(find('.deploy-keys-list', match: :first)) do
+      expect(page).to have_content(write_key.project.full_name)
+    end
+  end
+
   describe 'create a new deploy key' do
     let(:new_ssh_key) { attributes_for(:key)[:key] }
 
@@ -28,14 +38,12 @@ RSpec.describe 'admin deploy keys' do
     it 'creates a new deploy key' do
       fill_in 'deploy_key_title', with: 'laptop'
       fill_in 'deploy_key_key', with: new_ssh_key
-      check 'deploy_key_can_push'
       click_button 'Create'
 
       expect(current_path).to eq admin_deploy_keys_path
 
       page.within(find('.deploy-keys-list', match: :first)) do
         expect(page).to have_content('laptop')
-        expect(page).to have_content('Yes')
       end
     end
   end
@@ -48,14 +56,12 @@ RSpec.describe 'admin deploy keys' do
 
     it 'updates an existing deploy key' do
       fill_in 'deploy_key_title', with: 'new-title'
-      check 'deploy_key_can_push'
       click_button 'Save changes'
 
       expect(current_path).to eq admin_deploy_keys_path
 
       page.within(find('.deploy-keys-list', match: :first)) do
         expect(page).to have_content('new-title')
-        expect(page).to have_content('Yes')
       end
     end
   end

spec/features/projects/settings/repository_settings_spec.rb

@@ -43,7 +43,7 @@ feature 'Repository settings' do
 
         fill_in 'deploy_key_title', with: 'new_deploy_key'
         fill_in 'deploy_key_key', with: new_ssh_key
-        check 'deploy_key_can_push'
+        check 'deploy_key_deploy_keys_projects_attributes_0_can_push'
         click_button 'Add key'
 
         expect(page).to have_content('new_deploy_key')
@@ -57,7 +57,7 @@ feature 'Repository settings' do
         find('li', text: private_deploy_key.title).click_link('Edit')
 
         fill_in 'deploy_key_title', with: 'updated_deploy_key'
-        check 'deploy_key_can_push'
+        check 'deploy_key_deploy_keys_projects_attributes_0_can_push'
         click_button 'Save changes'
 
         expect(page).to have_content('updated_deploy_key')
@@ -74,11 +74,9 @@ feature 'Repository settings' do
         find('li', text: private_deploy_key.title).click_link('Edit')
 
         fill_in 'deploy_key_title', with: 'updated_deploy_key'
-        check 'deploy_key_can_push'
         click_button 'Save changes'
 
         expect(page).to have_content('updated_deploy_key')
-        expect(page).to have_content('Write access allowed')
       end
 
       scenario 'remove an existing deploy key' do

spec/javascripts/deploy_keys/components/key_spec.js

@@ -53,18 +53,24 @@ describe('Deploy keys key', () => {
       ).toBe('Remove');
     });
 
-    it('shows write access text when key has write access', (done) => {
-      vm.deployKey.can_push = true;
+    it('shows write access title when key has write access', (done) => {
+      vm.deployKey.deploy_keys_projects[0].can_push = true;
 
       Vue.nextTick(() => {
         expect(
-          vm.$el.querySelector('.write-access-allowed'),
-        ).not.toBeNull();
-
-        expect(
-          vm.$el.querySelector('.write-access-allowed').textContent.trim(),
+          vm.$el.querySelector('.deploy-project-label').getAttribute('data-original-title'),
         ).toBe('Write access allowed');
+        done();
+      });
+    });
+
+    it('does not show write access title when key has write access', (done) => {
+      vm.deployKey.deploy_keys_projects[0].can_push = false;
 
+      Vue.nextTick(() => {
+        expect(
+          vm.$el.querySelector('.deploy-project-label').getAttribute('data-original-title'),
+        ).toBe('Read access only');
         done();
       });
     });

spec/lib/gitlab/auth_spec.rb

@@ -136,8 +136,8 @@ describe Gitlab::Auth do
 
       it 'grants deploy key write permissions' do
         project = create(:project)
-        key = create(:deploy_key, can_push: true)
-        create(:deploy_keys_project, deploy_key: key, project: project)
+        key = create(:deploy_key)
+        create(:deploy_keys_project, :write_access, deploy_key: key, project: project)
         token = Gitlab::LfsToken.new(key).token
 
         expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}")
@@ -146,7 +146,7 @@ describe Gitlab::Auth do
 
       it 'does not grant deploy key write permissions' do
         project = create(:project)
-        key = create(:deploy_key, can_push: true)
+        key = create(:deploy_key)
         token = Gitlab::LfsToken.new(key).token
 
         expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}")

spec/lib/gitlab/git_access_spec.rb

@@ -51,12 +51,12 @@ describe Gitlab::GitAccess do
     context 'when the project exists' do
       context 'when actor exists' do
         context 'when actor is a DeployKey' do
-          let(:deploy_key) { create(:deploy_key, user: user, can_push: true) }
+          let(:deploy_key) { create(:deploy_key, user: user) }
           let(:actor) { deploy_key }
 
           context 'when the DeployKey has access to the project' do
             before do
-              deploy_key.projects << project
+              deploy_key.deploy_keys_projects.create(project: project, can_push: true)
             end
 
             it 'allows push and pull access' do
@@ -696,15 +696,13 @@ describe Gitlab::GitAccess do
   end
 
   describe 'deploy key permissions' do
-    let(:key) { create(:deploy_key, user: user, can_push: can_push) }
+    let(:key) { create(:deploy_key, user: user) }
     let(:actor) { key }
 
     context 'when deploy_key can push' do
-      let(:can_push) { true }
-
       context 'when project is authorized' do
         before do
-          key.projects << project
+          key.deploy_keys_projects.create(project: project, can_push: true)
         end
 
         it { expect { push_access_check }.not_to raise_error }
@@ -732,11 +730,9 @@ describe Gitlab::GitAccess do
     end
 
     context 'when deploy_key cannot push' do
-      let(:can_push) { false }
-
       context 'when project is authorized' do
         before do
-          key.projects << project
+          key.deploy_keys_projects.create(project: project, can_push: false)
         end
 
         it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:deploy_key_upload]) }

spec/models/deploy_keys_project_spec.rb

@@ -8,7 +8,7 @@ describe DeployKeysProject do
 
   describe "Validation" do
     it { is_expected.to validate_presence_of(:project_id) }
-    it { is_expected.to validate_presence_of(:deploy_key_id) }
+    it { is_expected.to validate_presence_of(:deploy_key) }
   end
 
   describe "Destroying" do

spec/requests/api/deploy_keys_spec.rb

@@ -110,7 +110,7 @@ describe API::DeployKeys do
     end
 
     it 'accepts can_push parameter' do
-      key_attrs = attributes_for :write_access_key
+      key_attrs = attributes_for(:another_key).merge(can_push: true)
 
       post api("/projects/#{project.id}/deploy_keys", admin), key_attrs
 
@@ -160,16 +160,6 @@ describe API::DeployKeys do
       expect(json_response['title']).to eq('new title')
       expect(json_response['can_push']).to eq(true)
     end
-
-    it 'updates a private ssh key from projects user has access with correct attributes' do
-      create(:deploy_keys_project, project: project2, deploy_key: private_deploy_key)
-
-      put api("/projects/#{project.id}/deploy_keys/#{private_deploy_key.id}", admin), { title: 'new title', can_push: true }
-
-      expect(json_response['id']).to eq(private_deploy_key.id)
-      expect(json_response['title']).to eq('new title')
-      expect(json_response['can_push']).to eq(true)
-    end
   end
 
   describe 'DELETE /projects/:id/deploy_keys/:key_id' do

spec/requests/api/v3/deploy_keys_spec.rb

@@ -107,7 +107,7 @@ describe API::V3::DeployKeys do
       end
 
       it 'accepts can_push parameter' do
-        key_attrs = attributes_for :write_access_key
+        key_attrs = attributes_for(:another_key).merge(can_push: true)
 
         post v3_api("/projects/#{project.id}/#{path}", admin), key_attrs
 

spec/requests/lfs_http_spec.rb

@@ -781,11 +781,11 @@ describe 'Git LFS API and storage' do
         end
 
         context 'when deploy key has project push access' do
-          let(:key) { create(:deploy_key, can_push: true) }
+          let(:key) { create(:deploy_key) }
           let(:authorization) { authorize_deploy_key }
 
           let(:update_user_permissions) do
-            project.deploy_keys << key
+            project.deploy_keys_projects.create(deploy_key: key, can_push: true)
           end
 
           it_behaves_like 'pushes new LFS objects'

spec/serializers/deploy_key_entity_spec.rb

@@ -21,19 +21,22 @@ describe DeployKeyEntity do
         user_id: deploy_key.user_id,
         title: deploy_key.title,
         fingerprint: deploy_key.fingerprint,
-        can_push: deploy_key.can_push,
         destroyed_when_orphaned: true,
         almost_orphaned: false,
         created_at: deploy_key.created_at,
         updated_at: deploy_key.updated_at,
         can_edit: false,
-        projects: [
+        deploy_keys_projects: [
+          {
+            can_push: false,
+            project:
             {
               id: project.id,
               name: project.name,
               full_path: project_path(project),
               full_name: project.full_name
             }
+          }
         ]
       }
     end