From 4ea1635b98003b21011649617990738110bbb8df Mon Sep 17 00:00:00 2001
From: Andrew Fontaine <afontaine@gitlab.com>
Date: Wed, 6 Dec 2023 14:29:18 +0000
Subject: [PATCH] Add endpoints to paginate deploy keys by type
This will give the frontend the ability to fetch a specific set of
deploy keys at at time, reducing page load times and DB query time.
---
.../projects/deploy_keys_controller.rb | 41 +++++++
app/finders/deploy_keys/deploy_keys_finder.rb | 50 +++++++++
app/models/deploy_key.rb | 1 +
.../deploy_keys/deploy_key_serializer.rb | 1 +
config/routes/project.rb | 6 +
.../projects/deploy_keys_controller_spec.rb | 104 ++++++++++++------
.../deploy_keys/deploy_keys_finder_spec.rb | 78 +++++++++++++
7 files changed, 246 insertions(+), 35 deletions(-)
create mode 100644 app/finders/deploy_keys/deploy_keys_finder.rb
create mode 100644 spec/finders/deploy_keys/deploy_keys_finder_spec.rb
diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb
index 9cdbd2a30f64..65aa836c5621 100644
--- a/app/controllers/projects/deploy_keys_controller.rb
+++ b/app/controllers/projects/deploy_keys_controller.rb
@@ -22,6 +22,34 @@ def index
end
end
+ def enabled_keys
+ respond_to do |format|
+ format.json do
+ enabled_keys = find_keys(filter: :enabled_keys)
+ render json: serialize(enabled_keys)
+ end
+ end
+ end
+
+ def available_project_keys
+ respond_to do |format|
+ format.json do
+ available_project_keys = find_keys(filter: :available_project_keys)
+ render json: serialize(available_project_keys)
+ end
+ end
+ end
+
+ def available_public_keys
+ respond_to do |format|
+ format.json do
+ available_public_keys = find_keys(filter: :available_public_keys)
+
+ render json: serialize(available_public_keys)
+ end
+ end
+ end
+
def new
redirect_to_repository
end
@@ -108,4 +136,17 @@ def authorize_update_deploy_key!
def redirect_to_repository
redirect_to_repository_settings(@project, anchor: 'js-deploy-keys-settings')
end
+
+ def find_keys(params)
+ DeployKeys::DeployKeysFinder.new(@project, current_user, params)
+ .execute
+ end
+
+ def serialize(keys)
+ opts = { user: current_user, project: project }
+
+ DeployKeys::DeployKeySerializer.new
+ .with_pagination(request, response)
+ .represent(keys, opts)
+ end
end
diff --git a/app/finders/deploy_keys/deploy_keys_finder.rb b/app/finders/deploy_keys/deploy_keys_finder.rb
new file mode 100644
index 000000000000..5924a6568016
--- /dev/null
+++ b/app/finders/deploy_keys/deploy_keys_finder.rb
@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+module DeployKeys
+ class DeployKeysFinder
+ attr_reader :project, :current_user, :params
+
+ def initialize(project, current_user, params = {})
+ @project = project
+ @current_user = current_user
+ @params = params
+ end
+
+ def execute
+ return empty unless can_admin_project?
+
+ case params[:filter]
+ when :enabled_keys
+ enabled_keys
+ when :available_project_keys
+ available_project_keys
+ when :available_public_keys
+ available_public_keys
+ else
+ empty
+ end
+ end
+
+ private
+
+ def enabled_keys
+ project.deploy_keys.with_projects
+ end
+
+ def available_project_keys
+ current_user.project_deploy_keys.with_projects.not_in(enabled_keys)
+ end
+
+ def available_public_keys
+ DeployKey.are_public.with_projects.not_in(enabled_keys)
+ end
+
+ def empty
+ DeployKey.none
+ end
+
+ def can_admin_project?
+ current_user.can?(:admin_project, project)
+ end
+ end
+end
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index 9ede494cfccd..5923497a1a3c 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -25,6 +25,7 @@ class DeployKey < Key
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) }
+ scope :not_in, ->(keys) { where.not(id: keys.select(:id)) }
accepts_nested_attributes_for :deploy_keys_projects, reject_if: :reject_deploy_keys_projects?
diff --git a/app/serializers/deploy_keys/deploy_key_serializer.rb b/app/serializers/deploy_keys/deploy_key_serializer.rb
index b00ef65696f9..2e6291a95f20 100644
--- a/app/serializers/deploy_keys/deploy_key_serializer.rb
+++ b/app/serializers/deploy_keys/deploy_key_serializer.rb
@@ -3,5 +3,6 @@
module DeployKeys
class DeployKeySerializer < BaseSerializer
entity DeployKeyEntity
+ include WithPagination
end
end
diff --git a/config/routes/project.rb b/config/routes/project.rb
index 9016a683404f..7064e1e19dbb 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -186,6 +186,12 @@
end
resources :deploy_keys, constraints: { id: /\d+/ }, only: [:index, :new, :create, :edit, :update] do
+ collection do
+ get :enabled_keys
+ get :available_project_keys
+ get :available_public_keys
+ end
+
member do
put :enable
put :disable
diff --git a/spec/controllers/projects/deploy_keys_controller_spec.rb b/spec/controllers/projects/deploy_keys_controller_spec.rb
index 96addb4b6c5d..43f089cede90 100644
--- a/spec/controllers/projects/deploy_keys_controller_spec.rb
+++ b/spec/controllers/projects/deploy_keys_controller_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe Projects::DeployKeysController do
+RSpec.describe Projects::DeployKeysController, feature_category: :continuous_delivery do
let(:project) { create(:project, :repository) }
let(:user) { create(:user) }
let(:admin) { create(:admin) }
@@ -13,60 +13,94 @@
sign_in(user)
end
- describe 'GET index' do
- let(:params) do
- { namespace_id: project.namespace, project_id: project }
- end
+ describe 'GET actions' do
+ let_it_be(:project) { create(:project, :repository) }
+ let_it_be(:user) { create(:user) }
- context 'when html requested' do
- it 'redirects to project settings with the correct anchor' do
- get :index, params: params
+ let_it_be(:accessible_project) { create(:project, :internal).tap { |p| p.add_developer(user) } }
+ let_it_be(:inaccessible_project) { create(:project, :internal) }
+ let_it_be(:project_private) { create(:project, :private) }
- expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-deploy-keys-settings'))
- end
+ let_it_be(:deploy_key_for_target_project) do
+ create(:deploy_keys_project, project: project, deploy_key: create(:deploy_key))
end
- context 'when json requested' do
- let(:project2) { create(:project, :internal) }
- let(:project_private) { create(:project, :private) }
+ let_it_be(:deploy_key_for_accessible_project) do
+ create(:deploy_keys_project, project: accessible_project, deploy_key: create(:deploy_key))
+ end
- let(:deploy_key_internal) { create(:deploy_key) }
- let(:deploy_key_actual) { create(:deploy_key) }
- let!(:deploy_key_public) { create(:deploy_key, public: true) }
+ let_it_be(:deploy_key_for_inaccessible_project) do
+ create(:deploy_keys_project, project: inaccessible_project, deploy_key: create(:deploy_key))
+ end
- let!(:deploy_keys_project_internal) do
- create(:deploy_keys_project, project: project2, deploy_key: deploy_key_internal)
- end
+ let_it_be(:deploy_keys_project_private) do
+ create(:deploy_keys_project, project: project_private, deploy_key: create(:another_deploy_key))
+ end
- let!(:deploy_keys_project_actual) do
- create(:deploy_keys_project, project: project, deploy_key: deploy_key_actual)
- end
+ let_it_be(:deploy_key_public) { create(:deploy_key, public: true) }
- let!(:deploy_keys_project_private) do
- create(:deploy_keys_project, project: project_private, deploy_key: create(:another_deploy_key))
+ describe 'GET index' do
+ let(:params) do
+ { namespace_id: project.namespace, project_id: project }
end
- context 'when user has access to all projects where deploy keys are used' do
- before do
- project2.add_developer(user)
+ context 'when html requested' do
+ it 'redirects to project settings with the correct anchor' do
+ get :index, params: params
+
+ expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-deploy-keys-settings'))
end
+ end
+ context 'when json requested' do
it 'returns json in a correct format' do
get :index, params: params.merge(format: :json)
expect(json_response.keys).to match_array(%w[enabled_keys available_project_keys public_keys])
- expect(json_response['enabled_keys'].count).to eq(1)
- expect(json_response['available_project_keys'].count).to eq(1)
- expect(json_response['public_keys'].count).to eq(1)
+ expect(json_response['enabled_keys'].pluck('id')).to match_array(
+ [deploy_key_for_target_project.deploy_key_id]
+ )
+ expect(json_response['available_project_keys'].pluck('id')).to match_array(
+ [deploy_key_for_accessible_project.deploy_key_id]
+ )
+ expect(json_response['public_keys'].pluck('id')).to match_array([deploy_key_public.id])
end
end
+ end
- context 'when user has no access to all projects where deploy keys are used' do
- it 'returns json in a correct format' do
- get :index, params: params.merge(format: :json)
+ describe 'GET enabled_keys' do
+ let(:params) do
+ { namespace_id: project.namespace, project_id: project }
+ end
- expect(json_response['available_project_keys'].count).to eq(0)
- end
+ it 'returns only enabled keys' do
+ get :enabled_keys, params: params.merge(format: :json)
+
+ expect(json_response.pluck("id")).to match_array([deploy_key_for_target_project.deploy_key_id])
+ end
+ end
+
+ describe 'GET available_project_keys' do
+ let(:params) do
+ { namespace_id: project.namespace, project_id: project }
+ end
+
+ it 'returns available project keys' do
+ get :available_project_keys, params: params.merge(format: :json)
+
+ expect(json_response.pluck("id")).to match_array([deploy_key_for_accessible_project.deploy_key_id])
+ end
+ end
+
+ describe 'GET available_public_keys' do
+ let(:params) do
+ { namespace_id: project.namespace, project_id: project }
+ end
+
+ it 'returns available public keys' do
+ get :available_public_keys, params: params.merge(format: :json)
+
+ expect(json_response.pluck("id")).to match_array([deploy_key_public.id])
end
end
end
diff --git a/spec/finders/deploy_keys/deploy_keys_finder_spec.rb b/spec/finders/deploy_keys/deploy_keys_finder_spec.rb
new file mode 100644
index 000000000000..f0d3935cc957
--- /dev/null
+++ b/spec/finders/deploy_keys/deploy_keys_finder_spec.rb
@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe DeployKeys::DeployKeysFinder, feature_category: :continuous_delivery do
+ describe '#execute' do
+ let_it_be(:user) { create(:user) }
+ let_it_be(:project) { create(:project, :repository) }
+
+ let_it_be(:accessible_project) { create(:project, :internal).tap { |p| p.add_developer(user) } }
+ let_it_be(:inaccessible_project) { create(:project, :internal) }
+ let_it_be(:project_private) { create(:project, :private) }
+
+ let_it_be(:deploy_key_for_target_project) do
+ create(:deploy_keys_project, project: project, deploy_key: create(:deploy_key))
+ end
+
+ let_it_be(:deploy_key_for_accessible_project) do
+ create(:deploy_keys_project, project: accessible_project, deploy_key: create(:deploy_key))
+ end
+
+ let_it_be(:deploy_key_for_inaccessible_project) do
+ create(:deploy_keys_project, project: inaccessible_project, deploy_key: create(:deploy_key))
+ end
+
+ let_it_be(:deploy_keys_project_private) do
+ create(:deploy_keys_project, project: project_private, deploy_key: create(:another_deploy_key))
+ end
+
+ let_it_be(:deploy_key_public) { create(:deploy_key, public: true) }
+
+ let(:params) { {} }
+
+ subject(:result) { described_class.new(project, user, params).execute }
+
+ context 'with access' do
+ before_all do
+ project.add_maintainer(user)
+ end
+
+ context 'when filtering for enabled_keys' do
+ let(:params) { { filter: :enabled_keys } }
+
+ it 'returns the correct result' do
+ expect(result.map(&:id)).to match_array([deploy_key_for_target_project.deploy_key_id])
+ end
+ end
+
+ context 'when filtering for available project keys' do
+ let(:params) { { filter: :available_project_keys } }
+
+ it 'returns the correct result' do
+ expect(result.map(&:id)).to match_array([deploy_key_for_accessible_project.deploy_key_id])
+ end
+ end
+
+ context 'when filtering for available public keys' do
+ let(:params) { { filter: :available_public_keys } }
+
+ it 'returns the correct result' do
+ expect(result.map(&:id)).to match_array([deploy_key_public.id])
+ end
+ end
+
+ context 'when there are no set filters' do
+ it 'returns an empty collection' do
+ expect(result).to eq DeployKey.none
+ end
+ end
+ end
+
+ context 'without access' do
+ it 'returns an empty collection' do
+ expect(result).to eq DeployKey.none
+ end
+ end
+ end
+end
--
GitLab