From 28fa3cb263375772c157f50023d7cdf6293e2116 Mon Sep 17 00:00:00 2001 From: Illya Klymov Date: Wed, 9 Mar 2022 15:02:37 +0200 Subject: [PATCH 1/4] Expose import url and status in projects API * add import_url field on project instance * add import_status field on project instance Changelog: added --- app/models/project.rb | 3 ++ doc/api/projects.md | 1 + lib/api/entities/project.rb | 4 ++- lib/api/projects.rb | 1 + spec/requests/api/project_attributes.yml | 3 +- spec/requests/api/projects_spec.rb | 42 ++++++++++++++++++++++++ 6 files changed, 52 insertions(+), 2 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 8f8f6cbf81f166..e38ee42a2da1d8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -67,6 +67,7 @@ class Project < ApplicationRecord VALID_IMPORT_PORTS = [80, 443].freeze VALID_IMPORT_PROTOCOLS = %w(http https git).freeze + INTERNAL_IMPORT_TYPES = [nil, 'gitlab_project'].freeze VALID_MIRROR_PORTS = [22, 80, 443].freeze VALID_MIRROR_PROTOCOLS = %w(http https ssh git).freeze @@ -656,7 +657,9 @@ def self.integration_association_name(name) preload(:project_feature, :route, namespace: [:route, :owner]) } + scope :created_by, -> (user) { where(creator: user) } scope :imported_from, -> (type) { where(import_type: type) } + scope :externally_imported, -> { where.not(import_type: INTERNAL_IMPORT_TYPES) } scope :with_tracing_enabled, -> { joins(:tracing_setting) } scope :with_enabled_error_tracking, -> { joins(:error_tracking_setting).where(project_error_tracking_settings: { enabled: true }) } diff --git a/doc/api/projects.md b/doc/api/projects.md index 3ca4125a2fa40d..33fbf01d3276a5 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -49,6 +49,7 @@ GET /projects | `archived` | boolean | **{dotted-circle}** No | Limit by archived status. | | `id_after` | integer | **{dotted-circle}** No | Limit results to projects with IDs greater than the specified ID. | | `id_before` | integer | **{dotted-circle}** No | Limit results to projects with IDs less than the specified ID. | +| `imported` | boolean | **{dotted-circle}** No | Limit results to projects which were imported from external systems by current user. | | `last_activity_after` | datetime | **{dotted-circle}** No | Limit results to projects with last_activity after specified time. Format: ISO 8601 (`YYYY-MM-DDTHH:MM:SSZ`) | | `last_activity_before` | datetime | **{dotted-circle}** No | Limit results to projects with last_activity before specified time. Format: ISO 8601 (`YYYY-MM-DDTHH:MM:SSZ`) | | `membership` | boolean | **{dotted-circle}** No | Limit by projects that the current user is a member of. | diff --git a/lib/api/entities/project.rb b/lib/api/entities/project.rb index 8f9a8add938677..6ebecd6bfae515 100644 --- a/lib/api/entities/project.rb +++ b/lib/api/entities/project.rb @@ -85,8 +85,10 @@ class Project < BasicProjectDetails end expose :mr_default_target_self, if: -> (project) { project.forked? } + expose :import_url do |project| + project[:import_url] + end expose :import_status - expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] } do |project| project.import_state&.last_error end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index d772079372ce7c..3c054c079b4275 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -20,6 +20,7 @@ def apply_filters(projects) projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled] projects = projects.with_statistics if params[:statistics] projects = projects.joins(:statistics) if params[:order_by].include?('project_statistics') # rubocop: disable CodeReuse/ActiveRecord + projects = projects.created_by(current_user).externally_imported.with_import_state if params[:imported] lang = params[:with_programming_language] projects = projects.with_programming_language(lang) if lang diff --git a/spec/requests/api/project_attributes.yml b/spec/requests/api/project_attributes.yml index 02d377efd95e8a..69b55622c1c256 100644 --- a/spec/requests/api/project_attributes.yml +++ b/spec/requests/api/project_attributes.yml @@ -12,7 +12,6 @@ itself: # project - hidden - import_source - import_type - - import_url - jobs_cache_index - last_repository_check_at - last_repository_check_failed @@ -63,6 +62,8 @@ itself: # project - empty_repo - forks_count - http_url_to_repo + - import_status + - import_url - name_with_namespace - open_issues_count - owner diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 877c68f8791830..953a41e18572d3 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -683,6 +683,48 @@ end end + context 'and imported=true' do + before do + project.update_attribute(:import_url, 'http://user:password@host/path') + end + + context 'when imported from github' do + before do + project.update_attribute(:import_type, 'github') + end + + it 'returns imported projects' do + get api('/projects?imported=true', user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).to eq [project.id] + end + + it 'does not expose import credentials' do + get api('/projects?imported=true', user) + + expect(json_response.first['import_url']).to eq 'http://host/path' + end + end + + context 'when imported from template' do + before do + project.update_attribute(:import_type, 'gitlab_project') + end + + it 'does not returns imported project' do + get api('/projects?imported=true', user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.length).to eq(0) + end + end + end + context 'when authenticated as a different user' do it_behaves_like 'projects response' do let(:filter) { {} } -- GitLab From c75d58691359f788d0a2c76c0d1a86337a726db7 Mon Sep 17 00:00:00 2001 From: Illya Klymov Date: Thu, 10 Mar 2022 18:55:31 +0200 Subject: [PATCH 2/4] Address reviewer comments * add extra tests --- app/models/project.rb | 4 ++-- spec/requests/api/projects_spec.rb | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index e38ee42a2da1d8..50a38a9e472e85 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -67,7 +67,7 @@ class Project < ApplicationRecord VALID_IMPORT_PORTS = [80, 443].freeze VALID_IMPORT_PROTOCOLS = %w(http https git).freeze - INTERNAL_IMPORT_TYPES = [nil, 'gitlab_project'].freeze + INTERNAL_IMPORT_TYPES = ['gitlab_project'].freeze VALID_MIRROR_PORTS = [22, 80, 443].freeze VALID_MIRROR_PROTOCOLS = %w(http https ssh git).freeze @@ -659,7 +659,7 @@ def self.integration_association_name(name) scope :created_by, -> (user) { where(creator: user) } scope :imported_from, -> (type) { where(import_type: type) } - scope :externally_imported, -> { where.not(import_type: INTERNAL_IMPORT_TYPES) } + scope :externally_imported, -> { where.not(import_type: INTERNAL_IMPORT_TYPES.concat([nil])) } scope :with_tracing_enabled, -> { joins(:tracing_setting) } scope :with_enabled_error_tracking, -> { joins(:error_tracking_setting).where(project_error_tracking_settings: { enabled: true }) } diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 953a41e18572d3..5bb7ba34c74284 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -685,6 +685,11 @@ context 'and imported=true' do before do + other_user = create(:user) + # imported project by other user + create(:project, creator: other_user, import_type: 'github', import_url: 'http://foo.com') + # project created by current user directly instead of importing + create(:project) project.update_attribute(:import_url, 'http://user:password@host/path') end @@ -693,7 +698,7 @@ project.update_attribute(:import_type, 'github') end - it 'returns imported projects' do + it 'returns only imported projects owned by current user' do get api('/projects?imported=true', user) expect(response).to have_gitlab_http_status(:ok) -- GitLab From 0b7ed6298a7a4c13e69423dc23d47329f39eca07 Mon Sep 17 00:00:00 2001 From: Illya Klymov Date: Mon, 14 Mar 2022 11:40:13 +0200 Subject: [PATCH 3/4] Expose import_type attribute * required to understand import types --- app/models/project.rb | 3 +- lib/api/entities/project.rb | 1 + lib/api/projects.rb | 3 +- spec/requests/api/project_attributes.yml | 1 - spec/requests/api/projects_spec.rb | 40 ++++++------------------ 5 files changed, 14 insertions(+), 34 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 50a38a9e472e85..c0d00408c1c5c0 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -67,7 +67,6 @@ class Project < ApplicationRecord VALID_IMPORT_PORTS = [80, 443].freeze VALID_IMPORT_PROTOCOLS = %w(http https git).freeze - INTERNAL_IMPORT_TYPES = ['gitlab_project'].freeze VALID_MIRROR_PORTS = [22, 80, 443].freeze VALID_MIRROR_PROTOCOLS = %w(http https ssh git).freeze @@ -659,7 +658,7 @@ def self.integration_association_name(name) scope :created_by, -> (user) { where(creator: user) } scope :imported_from, -> (type) { where(import_type: type) } - scope :externally_imported, -> { where.not(import_type: INTERNAL_IMPORT_TYPES.concat([nil])) } + scope :imported, -> { where.not(import_type: nil) } scope :with_tracing_enabled, -> { joins(:tracing_setting) } scope :with_enabled_error_tracking, -> { joins(:error_tracking_setting).where(project_error_tracking_settings: { enabled: true }) } diff --git a/lib/api/entities/project.rb b/lib/api/entities/project.rb index 6ebecd6bfae515..2f2714110ba166 100644 --- a/lib/api/entities/project.rb +++ b/lib/api/entities/project.rb @@ -88,6 +88,7 @@ class Project < BasicProjectDetails expose :import_url do |project| project[:import_url] end + expose :import_type expose :import_status expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] } do |project| project.import_state&.last_error diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 3c054c079b4275..b5948cb1827384 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -20,7 +20,7 @@ def apply_filters(projects) projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled] projects = projects.with_statistics if params[:statistics] projects = projects.joins(:statistics) if params[:order_by].include?('project_statistics') # rubocop: disable CodeReuse/ActiveRecord - projects = projects.created_by(current_user).externally_imported.with_import_state if params[:imported] + projects = projects.created_by(current_user).imported.with_import_state if params[:imported] lang = params[:with_programming_language] projects = projects.with_programming_language(lang) if lang @@ -126,6 +126,7 @@ def check_import_by_url_is_enabled optional :search_namespaces, type: Boolean, desc: "Include ancestor namespaces when matching search criteria" optional :owned, type: Boolean, default: false, desc: 'Limit by owned by authenticated user' optional :starred, type: Boolean, default: false, desc: 'Limit by starred status' + optional :imported, type: Boolean, default: false, desc: 'Limit by imported by authenticated user' optional :membership, type: Boolean, default: false, desc: 'Limit by projects that the current user is a member of' optional :with_issues_enabled, type: Boolean, default: false, desc: 'Limit by enabled issues feature' optional :with_merge_requests_enabled, type: Boolean, default: false, desc: 'Limit by enabled merge requests feature' diff --git a/spec/requests/api/project_attributes.yml b/spec/requests/api/project_attributes.yml index 69b55622c1c256..e55f5820b0faff 100644 --- a/spec/requests/api/project_attributes.yml +++ b/spec/requests/api/project_attributes.yml @@ -11,7 +11,6 @@ itself: # project - has_external_wiki - hidden - import_source - - import_type - jobs_cache_index - last_repository_check_at - last_repository_check_failed diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 5bb7ba34c74284..a45862976b52a5 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -691,42 +691,22 @@ # project created by current user directly instead of importing create(:project) project.update_attribute(:import_url, 'http://user:password@host/path') + project.update_attribute(:import_type, 'github') end - context 'when imported from github' do - before do - project.update_attribute(:import_type, 'github') - end - - it 'returns only imported projects owned by current user' do - get api('/projects?imported=true', user) + it 'returns only imported projects owned by current user' do + get api('/projects?imported=true', user) - expect(response).to have_gitlab_http_status(:ok) - expect(response).to include_pagination_headers - expect(json_response).to be_an Array - expect(json_response.map { |p| p['id'] }).to eq [project.id] - end - - it 'does not expose import credentials' do - get api('/projects?imported=true', user) - - expect(json_response.first['import_url']).to eq 'http://host/path' - end + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).to eq [project.id] end - context 'when imported from template' do - before do - project.update_attribute(:import_type, 'gitlab_project') - end - - it 'does not returns imported project' do - get api('/projects?imported=true', user) + it 'does not expose import credentials' do + get api('/projects?imported=true', user) - expect(response).to have_gitlab_http_status(:ok) - expect(response).to include_pagination_headers - expect(json_response).to be_an Array - expect(json_response.length).to eq(0) - end + expect(json_response.first['import_url']).to eq 'http://host/path' end end -- GitLab From 1b0211ecd8a7274b7a220c7a61943f135a9dae71 Mon Sep 17 00:00:00 2001 From: Illya Klymov Date: Wed, 16 Mar 2022 12:01:48 +0200 Subject: [PATCH 4/4] Address reviewer comments * hide import_url and import_type behind ability check --- lib/api/entities/project.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/api/entities/project.rb b/lib/api/entities/project.rb index 2f2714110ba166..60cc5167c41c52 100644 --- a/lib/api/entities/project.rb +++ b/lib/api/entities/project.rb @@ -85,10 +85,10 @@ class Project < BasicProjectDetails end expose :mr_default_target_self, if: -> (project) { project.forked? } - expose :import_url do |project| + expose :import_url, if: -> (project, options) { Ability.allowed?(options[:current_user], :admin_project, project) } do |project| project[:import_url] end - expose :import_type + expose :import_type, if: -> (project, options) { Ability.allowed?(options[:current_user], :admin_project, project) } expose :import_status expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] } do |project| project.import_state&.last_error -- GitLab