From 5251ae2a8b4995db5db008a166baf691304edad4 Mon Sep 17 00:00:00 2001 From: afzal442 Date: Sat, 29 Apr 2023 17:58:48 +0000 Subject: [PATCH 1/9] Added Read only Deploy keys --- app/models/deploy_key.rb | 3 ++ app/models/deploy_keys_project.rb | 1 + doc/api/deploy_keys.md | 18 ++++++- lib/api/entities/deploy_key.rb | 1 + spec/models/deploy_key_spec.rb | 68 ++++++++++++++++++++++++++- spec/requests/api/deploy_keys_spec.rb | 27 +++++++++++ 6 files changed, 115 insertions(+), 3 deletions(-) diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index ef31bedc3a8251..7678cbbef11706 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -10,12 +10,15 @@ class DeployKey < Key has_many :projects, through: :deploy_keys_projects has_many :deploy_keys_projects_with_write_access, -> { with_write_access }, class_name: "DeployKeysProject", inverse_of: :deploy_key + has_many :deploy_keys_projects_with_readonly_access, -> { with_readonly_access }, class_name: "DeployKeysProject", inverse_of: :deploy_key has_many :projects_with_write_access, -> { includes(:route) }, class_name: 'Project', through: :deploy_keys_projects_with_write_access, source: :project + has_many :projects_with_readonly_access, -> { includes(:route) }, class_name: 'Project', through: :deploy_keys_projects_with_readonly_access, source: :project has_many :protected_branch_push_access_levels, class_name: '::ProtectedBranch::PushAccessLevel', inverse_of: :deploy_key has_many :protected_tag_create_access_levels, class_name: '::ProtectedTag::CreateAccessLevel', inverse_of: :deploy_key scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where(deploy_keys_projects: { project_id: projects }) } scope :with_write_access, -> { joins(:deploy_keys_projects).merge(DeployKeysProject.with_write_access) } + scope :with_readonly_access, -> { joins(:deploy_keys_projects).merge(DeployKeysProject.with_readonly_access) } scope :are_public, -> { where(public: true) } scope :with_projects, -> { includes(deploy_keys_projects: { project: [:route, namespace: :route] }) } scope :including_projects_with_write_access, -> { includes(:projects_with_write_access) } diff --git a/app/models/deploy_keys_project.rb b/app/models/deploy_keys_project.rb index 363ef0b1c9a074..e114b7297ebda0 100644 --- a/app/models/deploy_keys_project.rb +++ b/app/models/deploy_keys_project.rb @@ -5,6 +5,7 @@ class DeployKeysProject < ApplicationRecord belongs_to :deploy_key, inverse_of: :deploy_keys_projects scope :in_project, ->(project) { where(project: project) } scope :with_write_access, -> { where(can_push: true) } + scope :with_readonly_access, -> { where(can_push: false) } accepts_nested_attributes_for :deploy_key diff --git a/doc/api/deploy_keys.md b/doc/api/deploy_keys.md index 7bf9fb0827a4b7..afca944f37c2f7 100644 --- a/doc/api/deploy_keys.md +++ b/doc/api/deploy_keys.md @@ -13,6 +13,8 @@ The deploy keys API can return in responses fingerprints of the public key in th ## List all deploy keys **(FREE SELF)** +> `projects_with_readonly_access` [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/119147) in GitLab 16.0. + Get a list of all deploy keys across all projects of the GitLab instance. This endpoint requires administrator access and is not available on GitLab.com. @@ -63,7 +65,8 @@ Example response: "path_with_namespace": "sidney_jones/project3", "created_at": "2021-10-25T18:33:17.666Z" } - ] + ], + "projects_with_readonly_access": [] }, { "id": 3, @@ -73,7 +76,18 @@ Example response: "fingerprint_sha256": "SHA256:lGI/Ys/Wx7PfMhUO1iuBH92JQKYN+3mhJZvWO4Q5ims", "created_at": "2013-10-02T11:12:29Z", "expires_at": null, - "projects_with_write_access": [] + "projects_with_write_access": [], + "projects_with_readonly_access": [ + { + "id": 74, + "description": null, + "name": "project3", + "name_with_namespace": "Sidney Jones / project3", + "path": "project3", + "path_with_namespace": "sidney_jones/project3", + "created_at": "2021-10-25T18:33:17.666Z" + } + ] } ] ``` diff --git a/lib/api/entities/deploy_key.rb b/lib/api/entities/deploy_key.rb index 1bcd06f2c8839e..0e82d9abb63da2 100644 --- a/lib/api/entities/deploy_key.rb +++ b/lib/api/entities/deploy_key.rb @@ -14,6 +14,7 @@ class DeployKey < Entities::SSHKey documentation: { type: 'string', example: 'SHA256:Jrs3LD1Ji30xNLtTVf9NDCj7kkBgPBb2pjvTZ3HfIgU' } expose :projects_with_write_access, using: Entities::ProjectIdentity, if: -> (_, options) { options[:include_projects_with_write_access] } + expose :projects_with_readonly_access, using: Entities::ProjectIdentity, if: -> (_, options) { options[:include_projects_with_readonly_access] } end end end diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb index 337fa40b4ba2bd..27ccc2d426bd2a 100644 --- a/spec/models/deploy_key_spec.rb +++ b/spec/models/deploy_key_spec.rb @@ -20,6 +20,20 @@ .source(:project) end + it do + is_expected.to have_many(:deploy_keys_projects_with_readonly_access) + .conditions(can_push: true) + .class_name('DeployKeysProject') + .inverse_of(:deploy_key) + end + + it do + is_expected.to have_many(:projects_with_readonly_access) + .class_name('Project') + .through(:deploy_keys_projects_with_readonly_access) + .source(:project) + end + it { is_expected.to have_many(:projects) } it { is_expected.to have_many(:protected_branch_push_access_levels).inverse_of(:deploy_key) } it { is_expected.to have_many(:protected_tag_create_access_levels).inverse_of(:deploy_key) } @@ -95,7 +109,7 @@ it { is_expected.to be_empty } end - context 'and this deploy key has not write access to the project' do + context 'and this deploy key has no write access to the project' do let(:specific_deploy_key) { create(:deploy_key, deploy_keys_projects: [create(:deploy_keys_project, project: project)]) } it { is_expected.to be_empty } @@ -110,6 +124,58 @@ end end + describe '.with_readonly_access_for_project' do + let_it_be(:project) { create(:project, :private) } + + subject { described_class.with_readonly_access_for_project(project) } + + context 'when no project is passed in' do + let(:project) { nil } + + it { is_expected.to be_empty } + end + + context 'when a project is passed in' do + let_it_be(:deploy_keys_project) { create(:deploy_keys_project, :readonly_access, project: project) } + let_it_be(:deploy_key) { deploy_keys_project.deploy_key } + + it 'only returns deploy keys with readonly access' do + create(:deploy_keys_project, project: project) + + is_expected.to contain_exactly(deploy_key) + end + + it 'returns deploy keys only for this project' do + other_project = create(:project) + create(:deploy_keys_project, :readonly_access, project: other_project) + + is_expected.to contain_exactly(deploy_key) + end + + context 'and a specific deploy key is passed in' do + subject { described_class.with_readonly_access_for_project(project, deploy_key: specific_deploy_key) } + + context 'and this deploy key is not linked to the project' do + let(:specific_deploy_key) { create(:deploy_key) } + + it { is_expected.to be_empty } + end + + context 'and this deploy key has no readonly access to the project' do + let(:specific_deploy_key) { create(:deploy_key, deploy_keys_projects: [create(:deploy_keys_project, project: project)]) } + + it { is_expected.to be_empty } + end + + context 'and this deploy key has readonly access to the project' do + let(:specific_deploy_key) { create(:deploy_key, deploy_keys_projects: [create(:deploy_keys_project, :readonly_access, project: project)]) } + + it { is_expected.to contain_exactly(specific_deploy_key) } + end + end + end + end + describe 'PolicyActor methods' do let_it_be(:user) { create(:user) } let_it_be(:deploy_key) { create(:deploy_key, user: user) } diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index 18a9211df3ef0d..d97583ed2ca850 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -59,6 +59,17 @@ def make_api_request(params = {}) expect { make_api_request }.not_to exceed_all_query_limit(control) end + it 'avoids N+1 database queries', :use_sql_query_cache, :request_store do + create(:deploy_keys_project, :readonly_access, project: project2, deploy_key: deploy_key) + + control = ActiveRecord::QueryRecorder.new(skip_cached: false) { make_api_request } + + deploy_key2 = create(:deploy_key, public: true) + create(:deploy_keys_project, :readonly_access, project: project3, deploy_key: deploy_key2) + + expect { make_api_request }.not_to exceed_all_query_limit(control) + end + context 'when `public` parameter is `true`' do it 'only returns public deploy keys' do make_api_request({ public: true }) @@ -81,6 +92,20 @@ def make_api_request(params = {}) expect(response_projects_with_write_access[1]['id']).to eq(project3.id) end end + + context 'projects_with_readonly_access' do + let!(:deploy_keys_project2) { create(:deploy_keys_project, :readonly_access, project: project2, deploy_key: deploy_key) } + let!(:deploy_keys_project3) { create(:deploy_keys_project, :readonly_access, project: project3, deploy_key: deploy_key) } + + it 'returns projects with readonly access' do + make_api_request + + response_projects_with_readonly_access = json_response.first['projects_with_readonly_access'] + + expect(response_projects_with_readonly_access[0]['id']).to eq(project2.id) + expect(response_projects_with_readonly_access[1]['id']).to eq(project3.id) + end + end end end @@ -103,6 +128,7 @@ def perform_request expect(json_response).to be_an Array expect(json_response.first['title']).to eq(deploy_key.title) expect(json_response.first).not_to have_key(:projects_with_write_access) + expect(json_response.first).not_to have_key(:projects_with_readonly_access) end it 'returns multiple deploy keys without N + 1' do @@ -129,6 +155,7 @@ def perform_request expect(json_response['title']).to eq(deploy_key.title) expect(json_response).not_to have_key(:projects_with_write_access) + expect(json_response).not_to have_key(:projects_with_readonly_access) end it 'returns 404 Not Found with invalid ID' do -- GitLab From bc3bf29e670ff4e1027eedff4ee8de877b42ae50 Mon Sep 17 00:00:00 2001 From: afzal442 Date: Thu, 1 Jun 2023 05:04:40 +0000 Subject: [PATCH 2/9] Add trait readonly_access --- spec/factories/deploy_keys_projects.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/factories/deploy_keys_projects.rb b/spec/factories/deploy_keys_projects.rb index 2a429bf8e56aa3..3d5038a6e17b78 100644 --- a/spec/factories/deploy_keys_projects.rb +++ b/spec/factories/deploy_keys_projects.rb @@ -8,5 +8,10 @@ trait :write_access do can_push { true } end + + trait :readonly_access do + can_push { false } + end + end end -- GitLab From d6aaf631492c18967a6aa79585353d915f8b9293 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Thu, 1 Jun 2023 08:15:06 +0000 Subject: [PATCH 3/9] Minor fix to identation --- spec/factories/deploy_keys_projects.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/factories/deploy_keys_projects.rb b/spec/factories/deploy_keys_projects.rb index 3d5038a6e17b78..118336913299c2 100644 --- a/spec/factories/deploy_keys_projects.rb +++ b/spec/factories/deploy_keys_projects.rb @@ -12,6 +12,5 @@ trait :readonly_access do can_push { false } end - end end -- GitLab From 5eb2d443368a8f1b8a739f1fc8604e28debacb38 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Thu, 1 Jun 2023 14:29:28 +0000 Subject: [PATCH 4/9] Adds `.with_readonly_access` method --- app/models/deploy_key.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index 7678cbbef11706..c748c0e910ba69 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -72,6 +72,13 @@ def self.with_write_access_for_project(project, deploy_key: nil) query end + def self.with_readonly_access_for_project(project, deploy_key: nil) + query = in_projects(project).with_readonly_access + query = query.where(id: deploy_key) if deploy_key + + query + end + # This is used for the internal logic of AuditEvents::BuildService. def impersonated? false -- GitLab From 20f3877affb0748cf7891fcd16aa1f36cc141b3f Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Thu, 1 Jun 2023 15:08:01 +0000 Subject: [PATCH 5/9] Minor fixes extra space --- app/models/deploy_key.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index c748c0e910ba69..93fc608783bc28 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -77,7 +77,7 @@ def self.with_readonly_access_for_project(project, deploy_key: nil) query = query.where(id: deploy_key) if deploy_key query - end + end # This is used for the internal logic of AuditEvents::BuildService. def impersonated? -- GitLab From 797ad62bcc92199a2854189818d30b4a86b8d816 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Thu, 1 Jun 2023 18:16:59 +0000 Subject: [PATCH 6/9] Revert the deploy key readonly access method --- app/models/deploy_key.rb | 7 ----- spec/models/deploy_key_spec.rb | 52 ---------------------------------- 2 files changed, 59 deletions(-) diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index 93fc608783bc28..7678cbbef11706 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -72,13 +72,6 @@ def self.with_write_access_for_project(project, deploy_key: nil) query end - def self.with_readonly_access_for_project(project, deploy_key: nil) - query = in_projects(project).with_readonly_access - query = query.where(id: deploy_key) if deploy_key - - query - end - # This is used for the internal logic of AuditEvents::BuildService. def impersonated? false diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb index 27ccc2d426bd2a..69995878dc06e3 100644 --- a/spec/models/deploy_key_spec.rb +++ b/spec/models/deploy_key_spec.rb @@ -124,58 +124,6 @@ end end - describe '.with_readonly_access_for_project' do - let_it_be(:project) { create(:project, :private) } - - subject { described_class.with_readonly_access_for_project(project) } - - context 'when no project is passed in' do - let(:project) { nil } - - it { is_expected.to be_empty } - end - - context 'when a project is passed in' do - let_it_be(:deploy_keys_project) { create(:deploy_keys_project, :readonly_access, project: project) } - let_it_be(:deploy_key) { deploy_keys_project.deploy_key } - - it 'only returns deploy keys with readonly access' do - create(:deploy_keys_project, project: project) - - is_expected.to contain_exactly(deploy_key) - end - - it 'returns deploy keys only for this project' do - other_project = create(:project) - create(:deploy_keys_project, :readonly_access, project: other_project) - - is_expected.to contain_exactly(deploy_key) - end - - context 'and a specific deploy key is passed in' do - subject { described_class.with_readonly_access_for_project(project, deploy_key: specific_deploy_key) } - - context 'and this deploy key is not linked to the project' do - let(:specific_deploy_key) { create(:deploy_key) } - - it { is_expected.to be_empty } - end - - context 'and this deploy key has no readonly access to the project' do - let(:specific_deploy_key) { create(:deploy_key, deploy_keys_projects: [create(:deploy_keys_project, project: project)]) } - - it { is_expected.to be_empty } - end - - context 'and this deploy key has readonly access to the project' do - let(:specific_deploy_key) { create(:deploy_key, deploy_keys_projects: [create(:deploy_keys_project, :readonly_access, project: project)]) } - - it { is_expected.to contain_exactly(specific_deploy_key) } - end - end - end - end - describe 'PolicyActor methods' do let_it_be(:user) { create(:user) } let_it_be(:deploy_key) { create(:deploy_key, user: user) } -- GitLab From 02e2f6788ca17e45cde8c480074b473937bc4fd1 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Fri, 2 Jun 2023 05:49:31 +0000 Subject: [PATCH 7/9] Fix readonly_access specs --- app/models/deploy_key.rb | 1 + lib/api/deploy_keys.rb | 4 +++- spec/models/deploy_key_spec.rb | 2 +- spec/requests/api/deploy_keys_spec.rb | 5 +++-- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index 7678cbbef11706..f9fa4bd212c2fa 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -22,6 +22,7 @@ class DeployKey < Key scope :are_public, -> { where(public: true) } scope :with_projects, -> { includes(deploy_keys_projects: { project: [:route, namespace: :route] }) } scope :including_projects_with_write_access, -> { includes(:projects_with_write_access) } + scope :including_projects_with_readonly_access, -> { includes(:projects_with_readonly_access) } accepts_nested_attributes_for :deploy_keys_projects, reject_if: :reject_deploy_keys_projects? diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 634d6052b997c3..f8379392531c87 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -41,8 +41,10 @@ def find_by_deploy_key(project, key_id) authenticated_as_admin! deploy_keys = params[:public] ? DeployKey.are_public : DeployKey.all + deploy_keys = deploy_keys.including_projects_with_write_access.including_projects_with_readonly_access - present paginate(deploy_keys.including_projects_with_write_access), with: Entities::DeployKey, include_projects_with_write_access: true + present paginate(deploy_keys), + with: Entities::DeployKey, include_projects_with_write_access: true, include_projects_with_readonly_access: true end params do diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb index 69995878dc06e3..528b36babc6898 100644 --- a/spec/models/deploy_key_spec.rb +++ b/spec/models/deploy_key_spec.rb @@ -22,7 +22,7 @@ it do is_expected.to have_many(:deploy_keys_projects_with_readonly_access) - .conditions(can_push: true) + .conditions(can_push: false) .class_name('DeployKeysProject') .inverse_of(:deploy_key) end diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index d97583ed2ca850..30c345ef4584b3 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -102,8 +102,9 @@ def make_api_request(params = {}) response_projects_with_readonly_access = json_response.first['projects_with_readonly_access'] - expect(response_projects_with_readonly_access[0]['id']).to eq(project2.id) - expect(response_projects_with_readonly_access[1]['id']).to eq(project3.id) + expect(response_projects_with_readonly_access[0]['id']).to eq(project.id) + expect(response_projects_with_readonly_access[1]['id']).to eq(project2.id) + expect(response_projects_with_readonly_access[2]['id']).to eq(project3.id) end end end -- GitLab From a82453ad0b0522d5c940ce619924d3901949ffac Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Fri, 2 Jun 2023 15:03:00 +0000 Subject: [PATCH 8/9] Add readonly_access json --- spec/fixtures/api/schemas/public_api/v4/deploy_key.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/fixtures/api/schemas/public_api/v4/deploy_key.json b/spec/fixtures/api/schemas/public_api/v4/deploy_key.json index 4f8b5c8422e83a..ad6891830498b7 100644 --- a/spec/fixtures/api/schemas/public_api/v4/deploy_key.json +++ b/spec/fixtures/api/schemas/public_api/v4/deploy_key.json @@ -52,5 +52,11 @@ } } }, + "projects_with_readonly_access": { + "type": "array", + "items": { + "$ref": "project/identity.json" + } + }, "additionalProperties": false } \ No newline at end of file -- GitLab From 28c69a6b212fa078f08242e0e559c374cce629c7 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Fri, 2 Jun 2023 15:04:25 +0000 Subject: [PATCH 9/9] Add readonly_access json --- .../api/schemas/public_api/v4/deploy_key.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/fixtures/api/schemas/public_api/v4/deploy_key.json b/spec/fixtures/api/schemas/public_api/v4/deploy_key.json index ad6891830498b7..77e533488f0fc3 100644 --- a/spec/fixtures/api/schemas/public_api/v4/deploy_key.json +++ b/spec/fixtures/api/schemas/public_api/v4/deploy_key.json @@ -50,12 +50,12 @@ "items": { "$ref": "project/identity.json" } - } - }, - "projects_with_readonly_access": { - "type": "array", - "items": { - "$ref": "project/identity.json" + }, + "projects_with_readonly_access": { + "type": "array", + "items": { + "$ref": "project/identity.json" + } } }, "additionalProperties": false -- GitLab