From 9a46f27d291d2556a65e9dfa001338876f0cabc3 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Wed, 1 Nov 2023 14:29:38 +0100 Subject: [PATCH] Add `include_hidden`, `include_pending_delete` option to Project API Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/430064 **Problem** It's not possible to list project in `pending_delete` or `hidden` state. **Solution** Add two optional disabled by default parameters `include_hidden` and `include_pending_delete` to Projects API. They can be set only by the admin. Changelog: added EE: true --- app/finders/projects_finder.rb | 8 ++++- doc/api/projects.md | 2 ++ ee/lib/ee/api/helpers.rb | 11 +++--- ee/lib/ee/api/helpers/projects_helpers.rb | 1 + ee/lib/ee/api/projects.rb | 7 ++++ ee/spec/requests/api/projects_spec.rb | 43 +++++++++++++++++++++++ lib/api/helpers.rb | 1 + lib/api/projects.rb | 1 + spec/finders/projects_finder_spec.rb | 19 +++++++++- spec/requests/api/projects_spec.rb | 26 ++++++++++++++ 10 files changed, 112 insertions(+), 7 deletions(-) diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 8f2241b3f7dcd3..1aa5245590efbc 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -77,7 +77,7 @@ def init_collection # EE would override this to add more filters def filter_projects(collection) - collection = collection.without_deleted + collection = by_deleted_status(collection) collection = by_ids(collection) collection = by_full_paths(collection) collection = by_personal(collection) @@ -155,6 +155,12 @@ def min_access_level? params[:min_access_level].present? end + def by_deleted_status(items) + return items.without_deleted unless current_user&.can?(:admin_all_resources) + + params[:include_pending_delete].present? ? items : items.without_deleted + end + # rubocop: disable CodeReuse/ActiveRecord def by_ids(items) items = items.where(id: project_ids_relation) if project_ids_relation diff --git a/doc/api/projects.md b/doc/api/projects.md index 516418a387bd56..f4a9e39693072c 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -57,6 +57,8 @@ GET /projects | `id_after` | integer | No | Limit results to projects with IDs greater than the specified ID. | | `id_before` | integer | No | Limit results to projects with IDs less than the specified ID. | | `imported` | boolean | No | Limit results to projects which were imported from external systems by current user. | +| `include_hidden` **(PREMIUM ALL)** | boolean | No | Include hidden projects. _(administrators only)_ | +| `include_pending_delete` | boolean | No | Include projects pending deletion. _(administrators only)_ | | `last_activity_after` | datetime | No | Limit results to projects with last activity after specified time. Format: ISO 8601 (`YYYY-MM-DDTHH:MM:SSZ`) | | `last_activity_before` | datetime | No | Limit results to projects with last activity before specified time. Format: ISO 8601 (`YYYY-MM-DDTHH:MM:SSZ`) | | `membership` | boolean | No | Limit by projects that the current user is a member of. | diff --git a/ee/lib/ee/api/helpers.rb b/ee/lib/ee/api/helpers.rb index 24054390b24a35..3488e2bfb57d81 100644 --- a/ee/lib/ee/api/helpers.rb +++ b/ee/lib/ee/api/helpers.rb @@ -128,11 +128,12 @@ def user_pipeline override :project_finder_params_ee def project_finder_params_ee - if params[:with_security_reports].present? - { with_security_reports: true } - else - {} - end + finder_params = {} + + finder_params[:include_hidden] = declared_params[:include_hidden] if declared_params[:include_hidden] + finder_params[:with_security_reports] = true if params[:with_security_reports].present? + + finder_params end override :send_git_archive diff --git a/ee/lib/ee/api/helpers/projects_helpers.rb b/ee/lib/ee/api/helpers/projects_helpers.rb index 80ee79b7b86251..5fed2b95986a0d 100644 --- a/ee/lib/ee/api/helpers/projects_helpers.rb +++ b/ee/lib/ee/api/helpers/projects_helpers.rb @@ -27,6 +27,7 @@ module ProjectsHelpers params :optional_filter_params_ee do optional :wiki_checksum_failed, type: Grape::API::Boolean, default: false, desc: 'Limit by projects where wiki checksum is failed' optional :repository_checksum_failed, type: Grape::API::Boolean, default: false, desc: 'Limit by projects where repository checksum is failed' + optional :include_hidden, type: Grape::API::Boolean, default: false, desc: 'Include hidden projects. Can only be set by admins' end params :optional_update_params_ee do diff --git a/ee/lib/ee/api/projects.rb b/ee/lib/ee/api/projects.rb index 084dcf12bb6bad..244611a8fc2dc4 100644 --- a/ee/lib/ee/api/projects.rb +++ b/ee/lib/ee/api/projects.rb @@ -99,6 +99,13 @@ def verify_update_project_attrs!(project, attrs) verify_merge_pipelines_attrs!(project, attrs) end + override :verify_project_filters! + def verify_project_filters!(attrs) + super + + attrs.delete(:include_hidden) unless current_user&.can_admin_all_resources? + end + def verify_mirror_attrs!(project, attrs) unless can?(current_user, :admin_mirror, project) ::Projects::UpdateService::PULL_MIRROR_ATTRIBUTES.each do |attr_name| diff --git a/ee/spec/requests/api/projects_spec.rb b/ee/spec/requests/api/projects_spec.rb index cf90efe5e9d70d..03b6bd7b5cc5a5 100644 --- a/ee/spec/requests/api/projects_spec.rb +++ b/ee/spec/requests/api/projects_spec.rb @@ -80,6 +80,49 @@ end.not_to exceed_all_query_limit(control.count) end end + + context 'when user requests hidden projects' do + let_it_be(:hidden) { create(:project, :public, :hidden) } + let(:filter_params) { { include_hidden: true } } + + context 'when user is not admin' do + before do + project.add_owner(user) + end + + it 'does not return hidden projects' do + get api('/projects', user), params: filter_params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).not_to include(hidden.id) + end + end + + context 'when user is an admin' do + let_it_be(:admin) { create(:admin) } + + it 'also returns hidden projects' do + get api("/projects", admin, admin_mode: true), params: filter_params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).to include(hidden.id) + end + + context 'when include_hidden option is off' do + let(:filter_params) { { include_hidden: nil } } + + it 'does not return hidden projects' do + get api("/projects", admin, admin_mode: true), params: filter_params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).not_to include(hidden.id) + end + end + end + end end describe 'GET /projects/:id' do diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a7e0a551283203..ba9aeba29a3837 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -764,6 +764,7 @@ def project_finder_params_ce finder_params[:id_before] = sanitize_id_param(params[:id_before]) if params[:id_before] finder_params[:updated_after] = declared_params[:updated_after] if declared_params[:updated_after] finder_params[:updated_before] = declared_params[:updated_before] if declared_params[:updated_before] + finder_params[:include_pending_delete] = declared_params[:include_pending_delete] if declared_params[:include_pending_delete] finder_params end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index d2e297f4213c21..3b80fd125ca3bc 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -159,6 +159,7 @@ def validate_projects_api_rate_limit_for_unauthenticated_users! optional :topic_id, type: Integer, desc: 'Limit results to projects with the assigned topic given by the topic ID' optional :updated_before, type: DateTime, desc: 'Return projects updated before the specified datetime. Format: ISO 8601 YYYY-MM-DDTHH:MM:SSZ' optional :updated_after, type: DateTime, desc: 'Return projects updated after the specified datetime. Format: ISO 8601 YYYY-MM-DDTHH:MM:SSZ' + optional :include_pending_delete, type: Boolean, desc: 'Include projects in pending delete state. Can only be set by admins' use :optional_filter_params_ee end diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index b8ba1176ea243a..f7afd96fa09af9 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -425,13 +425,30 @@ it { is_expected.to match_array([internal_project]) } end - describe 'always filters by without_deleted' do + describe 'filters by without_deleted by default' do let_it_be(:pending_delete_project) { create(:project, :public, pending_delete: true) } it 'returns projects that are not pending_delete' do expect(subject).not_to include(pending_delete_project) expect(subject).to include(public_project, internal_project) end + + context 'when include_pending_delete param is provided' do + let(:params) { { include_pending_delete: true } } + + it 'returns projects that are not pending_delete' do + expect(subject).not_to include(pending_delete_project) + expect(subject).to include(public_project, internal_project) + end + + context 'when user is an admin', :enable_admin_mode do + let(:current_user) { create(:admin) } + + it 'also return pending_delete projects' do + expect(subject).to include(public_project, internal_project, pending_delete_project) + end + end + end end describe 'filter by last_activity_before' do diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index ce90b9f1474fec..94ed527d9d8845 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -286,6 +286,32 @@ expect(json_response.map { |p| p['id'] }).not_to include(project.id) end + context 'when user requests pending_delete projects' do + before do + project.update!(pending_delete: true) + end + + let(:params) { { include_pending_delete: true } } + + it 'does not return projects marked for deletion' do + get api(path, user), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).not_to include(project.id) + end + + context 'when user is an admin' do + it 'returns projects marked for deletion' do + get api(path, admin, admin_mode: true), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).to include(project.id) + end + end + end + it 'does not include open_issues_count if issues are disabled' do project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED) -- GitLab