diff options
33 files changed, 400 insertions, 139 deletions
diff --git a/app/assets/javascripts/deploy_keys/components/key.vue b/app/assets/javascripts/deploy_keys/components/key.vue index a9e819b8a3c..8211ba280c5 100644 --- a/app/assets/javascripts/deploy_keys/components/key.vue +++ b/app/assets/javascripts/deploy_keys/components/key.vue @@ -1,6 +1,10 @@ <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' > - {{ project.full_name }} + {{ deployKeysProject.project.full_name }} + <i + v-if="!deployKeysProject.can_push" + aria-hidden="true" + class="fa fa-lock" + > + </i> </a> </div> <div class="deploy-key-content"> diff --git a/app/controllers/admin/deploy_keys_controller.rb b/app/controllers/admin/deploy_keys_controller.rb index a7ab481519d..b0c4c31cffc 100644 --- a/app/controllers/admin/deploy_keys_controller.rb +++ b/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 diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb index e06dda1baa4..f43ef2e5f2f 100644 --- a/app/controllers/projects/deploy_keys_controller.rb +++ b/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! diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index eae5eee4fee..c2e0a5fa126 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -1,10 +1,16 @@ 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 diff --git a/app/models/deploy_keys_project.rb b/app/models/deploy_keys_project.rb index b37b9bfbdac..6eef12c4373 100644 --- a/app/models/deploy_keys_project.rb +++ b/app/models/deploy_keys_project.rb @@ -1,8 +1,14 @@ 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 diff --git a/app/presenters/projects/settings/deploy_keys_presenter.rb b/app/presenters/projects/settings/deploy_keys_presenter.rb index 229311eb6ee..c226586fba5 100644 --- a/app/presenters/projects/settings/deploy_keys_presenter.rb +++ b/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 diff --git a/app/serializers/deploy_key_entity.rb b/app/serializers/deploy_key_entity.rb index c75431a79ae..2678f99510c 100644 --- a/app/serializers/deploy_key_entity.rb +++ b/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 diff --git a/app/serializers/deploy_keys_project_entity.rb b/app/serializers/deploy_keys_project_entity.rb new file mode 100644 index 00000000000..568ef5ab75e --- /dev/null +++ b/app/serializers/deploy_keys_project_entity.rb @@ -0,0 +1,4 @@ +class DeployKeysProjectEntity < Grape::Entity + expose :can_push + expose :project, using: ProjectEntity +end diff --git a/app/views/admin/deploy_keys/index.html.haml b/app/views/admin/deploy_keys/index.html.haml index 92370034baa..1420163fd5a 100644 --- a/app/views/admin/deploy_keys/index.html.haml +++ b/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)} diff --git a/app/views/projects/deploy_keys/_form.html.haml b/app/views/projects/deploy_keys/_form.html.haml index edaa3a1119e..c363180d0db 100644 --- a/app/views/projects/deploy_keys/_form.html.haml +++ b/app/views/projects/deploy_keys/_form.html.haml @@ -10,13 +10,15 @@ %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") - .form-group - .checkbox - = f.label :can_push do - = f.check_box :can_push - %strong Write access allowed - .form-group - %p.light.append-bottom-0 - Allow this key to push to repository as well? (Default only allows pull access.) + + = f.fields_for :deploy_keys_projects do |deploy_keys_project_form| + .form-group + .checkbox + = 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 + Allow this key to push to repository as well? (Default only allows pull access.) = f.submit "Add key", class: "btn-create btn" diff --git a/app/views/shared/deploy_keys/_form.html.haml b/app/views/shared/deploy_keys/_form.html.haml index e6075c3ae3a..87c2965bb21 100644 --- a/app/views/shared/deploy_keys/_form.html.haml +++ b/app/views/shared/deploy_keys/_form.html.haml @@ -1,5 +1,6 @@ - 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 - .control-label - .col-sm-10 - = form.label :can_push do - = 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.) +- 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 + = 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.) diff --git a/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml b/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml new file mode 100644 index 00000000000..cb53a94e465 --- /dev/null +++ b/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml @@ -0,0 +1,5 @@ +--- +title: Fix writable shared deploy keys +merge_request: +author: +type: security diff --git a/db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb b/db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb new file mode 100644 index 00000000000..5dc723db9f9 --- /dev/null +++ b/db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb @@ -0,0 +1,15 @@ +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 diff --git a/db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb b/db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb new file mode 100644 index 00000000000..ec0427a8e5a --- /dev/null +++ b/db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb @@ -0,0 +1,44 @@ +# 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 diff --git a/db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb b/db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb new file mode 100644 index 00000000000..05d236f8e96 --- /dev/null +++ b/db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb @@ -0,0 +1,43 @@ +# 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 diff --git a/db/post_migrate/20171215121259_remove_can_push_from_keys.rb b/db/post_migrate/20171215121259_remove_can_push_from_keys.rb new file mode 100644 index 00000000000..0599811d986 --- /dev/null +++ b/db/post_migrate/20171215121259_remove_can_push_from_keys.rb @@ -0,0 +1,17 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index a32d20b8f28..25583170851 100644 --- a/db/schema.rb +++ b/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 diff --git a/doc/api/deploy_keys.md b/doc/api/deploy_keys.md index 273d5a56b6f..698fa22a438 100644 --- a/doc/api/deploy_keys.md +++ b/doc/api/deploy_keys.md @@ -19,15 +19,13 @@ Example response: { "id": 1, "title": "Public key", - "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": false, + "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", "created_at": "2013-10-02T10:12:29Z" }, { "id": 3, "title": "Another Public key", - "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": true, + "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", "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. diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 281269b1190..a92f17937b8 100644 --- a/lib/api/deploy_keys.rb +++ b/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) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 9618d6625bb..971cb83a2d9 100644 --- a/lib/api/entities.rb +++ b/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 diff --git a/lib/api/v3/deploy_keys.rb b/lib/api/v3/deploy_keys.rb index b90e2061da3..47e54ca85a5 100644 --- a/lib/api/v3/deploy_keys.rb +++ b/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 diff --git a/spec/factories/deploy_keys_projects.rb b/spec/factories/deploy_keys_projects.rb index 30a6d468ed3..4350652fb79 100644 --- a/spec/factories/deploy_keys_projects.rb +++ b/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 diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb index 552b4b7e06e..f0c43f3d6f5 100644 --- a/spec/factories/keys.rb +++ b/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") diff --git a/spec/features/admin/admin_deploy_keys_spec.rb b/spec/features/admin/admin_deploy_keys_spec.rb index 241c7cbc34e..cb96830cb7c 100644 --- a/spec/features/admin/admin_deploy_keys_spec.rb +++ b/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 diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb index 81b282502fc..14670e91006 100644 --- a/spec/features/projects/settings/repository_settings_spec.rb +++ b/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 diff --git a/spec/javascripts/deploy_keys/components/key_spec.js b/spec/javascripts/deploy_keys/components/key_spec.js index 2f28c5bbf01..b7aadf604a4 100644 --- a/spec/javascripts/deploy_keys/components/key_spec.js +++ b/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(); }); }); diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index a6fbec295b5..cc202ce8bca 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/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}") diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 4290fbb0087..2009a8ac48c 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/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]) } diff --git a/spec/models/deploy_keys_project_spec.rb b/spec/models/deploy_keys_project_spec.rb index 0345fefb254..fca3090ff4a 100644 --- a/spec/models/deploy_keys_project_spec.rb +++ b/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 diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index 1f1e6ea17e4..0772b3f2e64 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/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 diff --git a/spec/requests/api/v3/deploy_keys_spec.rb b/spec/requests/api/v3/deploy_keys_spec.rb index 785bc1eb4ba..501af587ad4 100644 --- a/spec/requests/api/v3/deploy_keys_spec.rb +++ b/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 diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 5e59bb0d585..bee918a20aa 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/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' diff --git a/spec/serializers/deploy_key_entity_spec.rb b/spec/serializers/deploy_key_entity_spec.rb index d3aefa2c9eb..2bd8162d1b7 100644 --- a/spec/serializers/deploy_key_entity_spec.rb +++ b/spec/serializers/deploy_key_entity_spec.rb @@ -21,18 +21,21 @@ 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: [ { - id: project.id, - name: project.name, - full_path: project_path(project), - full_name: project.full_name + can_push: false, + project: + { + id: project.id, + name: project.name, + full_path: project_path(project), + full_name: project.full_name + } } ] } |