From 0880fd8d1ef634bf425c553713ca4124046138a0 Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho <zfilho@gitlab.com> Date: Thu, 11 May 2023 16:39:59 +0200 Subject: [PATCH 1/8] Add routes, controller and view to group level dependencies. EE: true Changelog: added --- .../groups/dependencies_controller.rb | 58 ++++++ ee/app/finders/sbom/dependencies_finder.rb | 4 +- ee/app/policies/ee/group_policy.rb | 1 + ee/app/serializers/dependency_entity.rb | 8 +- .../views/groups/dependencies/index.html.haml | 11 ++ .../development/group_level_dependencies.yml | 8 + ee/config/routes/group.rb | 2 + .../groups/dependencies_controller_spec.rb | 187 ++++++++++++++++++ ee/spec/policies/group_policy_spec.rb | 2 +- ee/spec/serializers/dependency_entity_spec.rb | 10 + 10 files changed, 287 insertions(+), 4 deletions(-) create mode 100644 ee/app/controllers/groups/dependencies_controller.rb create mode 100644 ee/app/views/groups/dependencies/index.html.haml create mode 100644 ee/config/feature_flags/development/group_level_dependencies.yml create mode 100644 ee/spec/controllers/groups/dependencies_controller_spec.rb diff --git a/ee/app/controllers/groups/dependencies_controller.rb b/ee/app/controllers/groups/dependencies_controller.rb new file mode 100644 index 0000000000000000..f618df28be805c43 --- /dev/null +++ b/ee/app/controllers/groups/dependencies_controller.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +module Groups + class DependenciesController < Groups::ApplicationController + before_action :authorize_read_dependency_list! + + feature_category :dependency_management + urgency :low + + def index + respond_to do |format| + format.html do + render status: :ok + end + format.json do + render json: serializer.represent(dependencies) + end + end + end + + private + + def authorize_read_dependency_list! + return if can?(current_user, :read_dependencies, group) && Feature.enabled?(:group_level_dependencies, group) + + render_not_authorized + end + + def dependency_list_params + params.permit(:sort_by, :sort, package_managers: []) + end + + def collect_dependencies + return [] unless can?(current_user, :read_security_resource, group) + + ::Sbom::DependenciesFinder.new(group, params: dependency_list_params).execute + end + + def dependencies + @dependencies ||= ::Gitlab::ItemsCollection.new(collect_dependencies) + end + + def serializer + ::DependencyListSerializer.new(group: group, user: current_user).with_pagination(request, response) + end + + def render_not_authorized + respond_to do |format| + format.html do + render_404 + end + format.json do + render_403 + end + end + end + end +end diff --git a/ee/app/finders/sbom/dependencies_finder.rb b/ee/app/finders/sbom/dependencies_finder.rb index 03b1a8df72186737..2ae922fe1b20b993 100644 --- a/ee/app/finders/sbom/dependencies_finder.rb +++ b/ee/app/finders/sbom/dependencies_finder.rb @@ -2,6 +2,8 @@ module Sbom class DependenciesFinder + SBOM_OCCURRENCES_LIMIT = 100 + def initialize(project_or_group, params: {}) @project_or_group = project_or_group @params = params @@ -17,7 +19,7 @@ def execute sbom_occurrences.order_by_package_name(params[:sort]) else sbom_occurrences.order_by_id - end + end.limit(SBOM_OCCURRENCES_LIMIT) end private diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index e034f86e8694d111..3f05868a6df10a23 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -441,6 +441,7 @@ module GroupPolicy rule { can?(:read_group_security_dashboard) }.policy do enable :create_vulnerability_export enable :read_security_resource + enable :read_dependencies end rule { admin | owner }.policy do diff --git a/ee/app/serializers/dependency_entity.rb b/ee/app/serializers/dependency_entity.rb index c9c5bec238b6ec20..d84914e048b1812a 100644 --- a/ee/app/serializers/dependency_entity.rb +++ b/ee/app/serializers/dependency_entity.rb @@ -28,10 +28,14 @@ class LicenseEntity < Grape::Entity private def can_read_vulnerabilities? - can?(request.user, :read_security_resource, request.project) + return can?(request.user, :read_security_resource, request.project) if request.respond_to?(:project) + + false end def can_read_licenses? - can?(request.user, :read_licenses, request.project) + return can?(request.user, :read_licenses, request.project) if request.respond_to?(:project) + + false end end diff --git a/ee/app/views/groups/dependencies/index.html.haml b/ee/app/views/groups/dependencies/index.html.haml new file mode 100644 index 0000000000000000..213a99a4460012ae --- /dev/null +++ b/ee/app/views/groups/dependencies/index.html.haml @@ -0,0 +1,11 @@ +- breadcrumb_title _('Dependency list') +- page_title _('Dependency list') + +#js-dependencies-app{ + data: { + endpoint: group_dependencies_path(@group, format: :json), + documentation_path: help_page_path('user/application_security/dependency_list/index'), + support_documentation_path: help_page_path('user/application_security/dependency_scanning/index', anchor: 'supported-languages-and-package-managers'), + empty_state_svg_path: image_path('illustrations/Dependency-list-empty-state.svg') + } +} diff --git a/ee/config/feature_flags/development/group_level_dependencies.yml b/ee/config/feature_flags/development/group_level_dependencies.yml new file mode 100644 index 0000000000000000..7e5b32d06a08ddff --- /dev/null +++ b/ee/config/feature_flags/development/group_level_dependencies.yml @@ -0,0 +1,8 @@ +--- +name: group_level_dependencies +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120489 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/411257 +milestone: '16.0' +type: development +group: group::threat insights +default_enabled: false diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index 0db875d0ccf4a88b..6efe588fa02fdbc9 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -195,6 +195,8 @@ resources :compliance_framework_reports, only: [:index], constraints: { format: :csv } end + resources :dependencies, only: [:index] + resource :push_rules, only: [:update] resources :protected_branches, only: [:create, :update, :destroy] diff --git a/ee/spec/controllers/groups/dependencies_controller_spec.rb b/ee/spec/controllers/groups/dependencies_controller_spec.rb new file mode 100644 index 0000000000000000..eccc646b2eeaf627 --- /dev/null +++ b/ee/spec/controllers/groups/dependencies_controller_spec.rb @@ -0,0 +1,187 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::DependenciesController, feature_category: :dependency_management do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + + before do + sign_in(user) + end + + describe 'GET index' do + context 'with HTML format' do + subject { get :index, params: { group_id: group.to_param } } + + context 'when security dashboard feature is enabled' do + before do + stub_licensed_features(security_dashboard: true) + stub_feature_flags(group_level_dependencies: true) + end + + context 'and user is allowed to access group level dependencies' do + render_views + + before do + group.add_developer(user) + end + + it { is_expected.to have_gitlab_http_status(:ok) } + + it 'returns the correct template' do + subject + + expect(assigns(:group)).to eq(group) + expect(response).to render_template(:index) + expect(response.body).to include('data-documentation-path') + expect(response.body).to include('data-empty-state-svg-path') + expect(response.body).to include('data-endpoint') + expect(response.body).to include('data-support-documentation-path') + end + + context 'when feature flag group_level_dependencies is disabled' do + before do + stub_feature_flags(group_level_dependencies: false) + end + + it { is_expected.to have_gitlab_http_status(:not_found) } + end + end + + context 'when user is not allowed to access group level dependencies' do + it { is_expected.to have_gitlab_http_status(:not_found) } + end + end + + context 'when security dashboard feature is disabled' do + it { is_expected.to have_gitlab_http_status(:not_found) } + end + end + + context 'with JSON format' do + subject { get :index, params: params, format: :json } + + let(:params) { { group_id: group.to_param } } + + context 'when security dashboard feature is enabled' do + before do + stub_licensed_features(security_dashboard: true) + stub_feature_flags(group_level_dependencies: true) + end + + context 'and user is allowed to access group level dependencies' do + render_views + + let(:expected_response) do + { + 'report' => { + 'status' => 'job_not_set_up' + }, + 'dependencies' => [] + } + end + + before do + group.add_developer(user) + end + + it { is_expected.to have_gitlab_http_status(:ok) } + + it 'returns the expected data' do + subject + + expect(json_response).to eq(expected_response) + end + + context 'with existing dependencies' do + let_it_be(:project) { create(:project, group: group) } + let_it_be(:sbom_occurrence_npm) { create(:sbom_occurrence, project: project, packager_name: 'npm') } + let_it_be(:sbom_occurrence_bundler) { create(:sbom_occurrence, project: project, packager_name: 'bundler') } + + let(:expected_response) do + { + 'report' => { + 'status' => 'job_not_set_up' + }, + 'dependencies' => [ + { + 'location' => sbom_occurrence_npm.location.as_json, + 'name' => sbom_occurrence_npm.name, + 'packager' => sbom_occurrence_npm.packager, + 'version' => sbom_occurrence_npm.version + }, + { + 'location' => sbom_occurrence_bundler.location.as_json, + 'name' => sbom_occurrence_bundler.name, + 'packager' => sbom_occurrence_bundler.packager, + 'version' => sbom_occurrence_bundler.version + } + ] + } + end + + it 'returns the paginated data' do + subject + + expect(json_response).to eq(expected_response) + expect(response).to include_pagination_headers + end + + context 'with sorting params' do + context 'when sorted by packager' do + let(:params) { { group_id: group.to_param, sort_by: 'packager', sort: 'desc' } } + + it 'returns sorted list' do + subject + + expect(json_response['dependencies'].first['packager']).to eq('npm') + expect(json_response['dependencies'].last['packager']).to eq('bundler') + end + end + + context 'when sorted by name' do + let(:params) { { group_id: group.to_param, sort_by: 'name', sort: 'asc' } } + + it 'returns sorted list' do + subject + + expect(json_response['dependencies'].first['name']).to eq('component-1') + expect(json_response['dependencies'].last['name']).to eq('component-2') + end + end + end + + context 'with filtering params' do + context 'when filtered by package managers' do + let(:params) { { group_id: group.to_param, package_managers: ['npm'] } } + + it 'returns filtered list' do + subject + + expect(json_response['dependencies'].pluck('packager')).to eq(['npm']) + end + end + end + end + + context 'when feature flag group_level_dependencies is disabled' do + before do + stub_feature_flags(group_level_dependencies: false) + end + + it { is_expected.to have_gitlab_http_status(:forbidden) } + end + end + + context 'when user is not allowed to access group level dependencies' do + it { is_expected.to have_gitlab_http_status(:forbidden) } + end + end + + context 'when security dashboard feature is disabled' do + it { is_expected.to have_gitlab_http_status(:forbidden) } + end + end + end +end diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index a8ee33afb6c40262..a384d0fef1f3c397 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -1569,7 +1569,7 @@ def stub_group_saml_config(enabled) describe 'read_group_security_dashboard & create_vulnerability_export' do let(:abilities) do - %i[read_group_security_dashboard create_vulnerability_export read_security_resource] + %i[read_group_security_dashboard create_vulnerability_export read_security_resource read_dependencies] end before do diff --git a/ee/spec/serializers/dependency_entity_spec.rb b/ee/spec/serializers/dependency_entity_spec.rb index 63930f9bf992aca4..a79c0d29633ce6dd 100644 --- a/ee/spec/serializers/dependency_entity_spec.rb +++ b/ee/spec/serializers/dependency_entity_spec.rb @@ -32,6 +32,16 @@ it 'includes license info and vulnerabilities' do is_expected.to eq(dependency.except(:package_manager, :iid)) end + + context 'with relation to group instead project' do + before do + allow(request).to receive(:project).and_return(nil) + end + + it 'does not include license and vulnerabilities info' do + is_expected.to eq(dependency.except(:licenses, :vulnerabilities, :package_manager, :iid)) + end + end end context 'with reporter' do -- GitLab From 6ef490d6a377b473366b40998b371673907c1b9e Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho <zfilho@gitlab.com> Date: Wed, 17 May 2023 17:23:58 +0200 Subject: [PATCH 2/8] address backend and database reviews --- .../groups/dependencies_controller.rb | 16 +++++++++------- ee/app/finders/sbom/dependencies_finder.rb | 4 +--- ee/app/models/sbom/occurrence.rb | 13 ++++++++++++- ee/app/models/sbom/source.rb | 4 ++++ ee/app/serializers/dependency_entity.rb | 8 ++------ .../groups/dependencies_controller_spec.rb | 7 ------- ee/spec/serializers/dependency_entity_spec.rb | 10 ---------- 7 files changed, 28 insertions(+), 34 deletions(-) diff --git a/ee/app/controllers/groups/dependencies_controller.rb b/ee/app/controllers/groups/dependencies_controller.rb index f618df28be805c43..222957e7d7a8e92b 100644 --- a/ee/app/controllers/groups/dependencies_controller.rb +++ b/ee/app/controllers/groups/dependencies_controller.rb @@ -13,7 +13,7 @@ def index render status: :ok end format.json do - render json: serializer.represent(dependencies) + render json: serialized_dependencies end end end @@ -27,21 +27,23 @@ def authorize_read_dependency_list! end def dependency_list_params - params.permit(:sort_by, :sort, package_managers: []) + params.permit(:sort_by, :sort, :page, :per_page, package_managers: []) end def collect_dependencies return [] unless can?(current_user, :read_security_resource, group) - ::Sbom::DependenciesFinder.new(group, params: dependency_list_params).execute + @collect_dependencies ||= ::Sbom::DependenciesFinder.new(group, params: dependency_list_params).execute end - def dependencies - @dependencies ||= ::Gitlab::ItemsCollection.new(collect_dependencies) + def serialized_dependencies + DependencyListEntity.represent(collect_dependencies, entity_request) end - def serializer - ::DependencyListSerializer.new(group: group, user: current_user).with_pagination(request, response) + def entity_request + { + request: EntityRequest.new(project: nil, user: current_user) + } end def render_not_authorized diff --git a/ee/app/finders/sbom/dependencies_finder.rb b/ee/app/finders/sbom/dependencies_finder.rb index 2ae922fe1b20b993..6f90a41af731afcc 100644 --- a/ee/app/finders/sbom/dependencies_finder.rb +++ b/ee/app/finders/sbom/dependencies_finder.rb @@ -2,8 +2,6 @@ module Sbom class DependenciesFinder - SBOM_OCCURRENCES_LIMIT = 100 - def initialize(project_or_group, params: {}) @project_or_group = project_or_group @params = params @@ -19,7 +17,7 @@ def execute sbom_occurrences.order_by_package_name(params[:sort]) else sbom_occurrences.order_by_id - end.limit(SBOM_OCCURRENCES_LIMIT) + end.paginate(params[:per_page].to_i, params[:page].to_i) end private diff --git a/ee/app/models/sbom/occurrence.rb b/ee/app/models/sbom/occurrence.rb index e32591aec607a609..aafc901dcc9727ac 100644 --- a/ee/app/models/sbom/occurrence.rb +++ b/ee/app/models/sbom/occurrence.rb @@ -4,6 +4,9 @@ module Sbom class Occurrence < ApplicationRecord include EachBatch + SBOM_OCCURRENCES_MAX_LIMIT = 100 + SBOM_OCCURRENCES_DEFAULT_LIMIT = 25 + belongs_to :component, optional: false belongs_to :component_version belongs_to :project, optional: false @@ -30,7 +33,7 @@ class Occurrence < ApplicationRecord end scope :filter_by_package_managers, ->(package_managers) do - joins(:source).where("sbom_sources.source->'package_manager'->>'name' IN (?)", package_managers) + where(source_id: Sbom::Source.filter_by_package_managers(package_managers)) end scope :filter_by_component_names, ->(component_names) do @@ -46,6 +49,14 @@ def location } end + def self.paginate(per_page, page) + limit = [[per_page, SBOM_OCCURRENCES_DEFAULT_LIMIT].max, SBOM_OCCURRENCES_MAX_LIMIT].min + offset_multiplier = [page, 1].max - 1 + offset = offset_multiplier * limit + + limit(limit).offset(offset) + end + private def input_file_blob_path diff --git a/ee/app/models/sbom/source.rb b/ee/app/models/sbom/source.rb index 8fdea47cc0b74377..37d357f209e4546b 100644 --- a/ee/app/models/sbom/source.rb +++ b/ee/app/models/sbom/source.rb @@ -10,6 +10,10 @@ class Source < ApplicationRecord validates :source_type, presence: true validates :source, presence: true, json_schema: { filename: 'sbom_source' } + scope :filter_by_package_managers, ->(package_managers) do + where("source->'package_manager'->>'name' IN (?)", package_managers) + end + def packager source.dig('package_manager', 'name') end diff --git a/ee/app/serializers/dependency_entity.rb b/ee/app/serializers/dependency_entity.rb index d84914e048b1812a..c9c5bec238b6ec20 100644 --- a/ee/app/serializers/dependency_entity.rb +++ b/ee/app/serializers/dependency_entity.rb @@ -28,14 +28,10 @@ class LicenseEntity < Grape::Entity private def can_read_vulnerabilities? - return can?(request.user, :read_security_resource, request.project) if request.respond_to?(:project) - - false + can?(request.user, :read_security_resource, request.project) end def can_read_licenses? - return can?(request.user, :read_licenses, request.project) if request.respond_to?(:project) - - false + can?(request.user, :read_licenses, request.project) end end diff --git a/ee/spec/controllers/groups/dependencies_controller_spec.rb b/ee/spec/controllers/groups/dependencies_controller_spec.rb index eccc646b2eeaf627..658d59ecbb960e5b 100644 --- a/ee/spec/controllers/groups/dependencies_controller_spec.rb +++ b/ee/spec/controllers/groups/dependencies_controller_spec.rb @@ -121,13 +121,6 @@ } end - it 'returns the paginated data' do - subject - - expect(json_response).to eq(expected_response) - expect(response).to include_pagination_headers - end - context 'with sorting params' do context 'when sorted by packager' do let(:params) { { group_id: group.to_param, sort_by: 'packager', sort: 'desc' } } diff --git a/ee/spec/serializers/dependency_entity_spec.rb b/ee/spec/serializers/dependency_entity_spec.rb index a79c0d29633ce6dd..63930f9bf992aca4 100644 --- a/ee/spec/serializers/dependency_entity_spec.rb +++ b/ee/spec/serializers/dependency_entity_spec.rb @@ -32,16 +32,6 @@ it 'includes license info and vulnerabilities' do is_expected.to eq(dependency.except(:package_manager, :iid)) end - - context 'with relation to group instead project' do - before do - allow(request).to receive(:project).and_return(nil) - end - - it 'does not include license and vulnerabilities info' do - is_expected.to eq(dependency.except(:licenses, :vulnerabilities, :package_manager, :iid)) - end - end end context 'with reporter' do -- GitLab From 8529343d9686fa136f178db2e6f4ac72923db88f Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho <zfilho@gitlab.com> Date: Mon, 22 May 2023 11:15:55 +0200 Subject: [PATCH 3/8] Move specs into requests --- .../groups/dependencies_controller_spec.rb | 56 ++++++++++++++----- 1 file changed, 42 insertions(+), 14 deletions(-) rename ee/spec/{controllers => requests}/groups/dependencies_controller_spec.rb (79%) diff --git a/ee/spec/controllers/groups/dependencies_controller_spec.rb b/ee/spec/requests/groups/dependencies_controller_spec.rb similarity index 79% rename from ee/spec/controllers/groups/dependencies_controller_spec.rb rename to ee/spec/requests/groups/dependencies_controller_spec.rb index 658d59ecbb960e5b..9cb33977c914e5d0 100644 --- a/ee/spec/controllers/groups/dependencies_controller_spec.rb +++ b/ee/spec/requests/groups/dependencies_controller_spec.rb @@ -12,7 +12,7 @@ describe 'GET index' do context 'with HTML format' do - subject { get :index, params: { group_id: group.to_param } } + subject { get group_dependencies_path(group_id: group.full_path) } context 'when security dashboard feature is enabled' do before do @@ -21,13 +21,15 @@ end context 'and user is allowed to access group level dependencies' do - render_views - before do group.add_developer(user) end - it { is_expected.to have_gitlab_http_status(:ok) } + it 'returns http status :ok' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end it 'returns the correct template' do subject @@ -45,22 +47,34 @@ stub_feature_flags(group_level_dependencies: false) end - it { is_expected.to have_gitlab_http_status(:not_found) } + it 'return http status :not_found' do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end end end context 'when user is not allowed to access group level dependencies' do - it { is_expected.to have_gitlab_http_status(:not_found) } + it 'return http status :not_found' do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end end end context 'when security dashboard feature is disabled' do - it { is_expected.to have_gitlab_http_status(:not_found) } + it 'return http status :not_found' do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end end end context 'with JSON format' do - subject { get :index, params: params, format: :json } + subject { get group_dependencies_path(group_id: group.full_path), params: params, as: :json } let(:params) { { group_id: group.to_param } } @@ -71,8 +85,6 @@ end context 'and user is allowed to access group level dependencies' do - render_views - let(:expected_response) do { 'report' => { @@ -86,7 +98,11 @@ group.add_developer(user) end - it { is_expected.to have_gitlab_http_status(:ok) } + it 'returns http status :ok' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end it 'returns the expected data' do subject @@ -163,17 +179,29 @@ stub_feature_flags(group_level_dependencies: false) end - it { is_expected.to have_gitlab_http_status(:forbidden) } + it 'returns http status :forbidden' do + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end end end context 'when user is not allowed to access group level dependencies' do - it { is_expected.to have_gitlab_http_status(:forbidden) } + it 'returns http status :forbidden' do + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end end end context 'when security dashboard feature is disabled' do - it { is_expected.to have_gitlab_http_status(:forbidden) } + it 'returns http status :forbidden' do + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end end end end -- GitLab From 5d192a7168d4b370ab536a3d04dad2e17e57b4f4 Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho <zfilho@gitlab.com> Date: Wed, 24 May 2023 10:58:52 +0200 Subject: [PATCH 4/8] Update milestone --- .../feature_flags/development/group_level_dependencies.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/config/feature_flags/development/group_level_dependencies.yml b/ee/config/feature_flags/development/group_level_dependencies.yml index 7e5b32d06a08ddff..ea8f90653ef72c65 100644 --- a/ee/config/feature_flags/development/group_level_dependencies.yml +++ b/ee/config/feature_flags/development/group_level_dependencies.yml @@ -2,7 +2,7 @@ name: group_level_dependencies introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120489 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/411257 -milestone: '16.0' +milestone: '16.1' type: development group: group::threat insights default_enabled: false -- GitLab From cc24674a1e24496df81dfcc5822d29b8873e718c Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho <zfilho@gitlab.com> Date: Wed, 24 May 2023 16:32:15 +0200 Subject: [PATCH 5/8] Add specs --- .../groups/dependencies_controller.rb | 2 -- ee/app/models/sbom/occurrence.rb | 3 ++- .../finders/sbom/dependencies_finder_spec.rb | 22 +++++++++++++++++ ee/spec/models/sbom/occurrence_spec.rb | 24 +++++++++++++++++++ ee/spec/models/sbom/source_spec.rb | 11 +++++++++ .../groups/dependencies_controller_spec.rb | 15 ++++++++++++ 6 files changed, 74 insertions(+), 3 deletions(-) diff --git a/ee/app/controllers/groups/dependencies_controller.rb b/ee/app/controllers/groups/dependencies_controller.rb index 222957e7d7a8e92b..66517605d6dbffc4 100644 --- a/ee/app/controllers/groups/dependencies_controller.rb +++ b/ee/app/controllers/groups/dependencies_controller.rb @@ -31,8 +31,6 @@ def dependency_list_params end def collect_dependencies - return [] unless can?(current_user, :read_security_resource, group) - @collect_dependencies ||= ::Sbom::DependenciesFinder.new(group, params: dependency_list_params).execute end diff --git a/ee/app/models/sbom/occurrence.rb b/ee/app/models/sbom/occurrence.rb index aafc901dcc9727ac..a6f56a9cbb91861b 100644 --- a/ee/app/models/sbom/occurrence.rb +++ b/ee/app/models/sbom/occurrence.rb @@ -50,7 +50,8 @@ def location end def self.paginate(per_page, page) - limit = [[per_page, SBOM_OCCURRENCES_DEFAULT_LIMIT].max, SBOM_OCCURRENCES_MAX_LIMIT].min + per_page = per_page < 1 ? SBOM_OCCURRENCES_DEFAULT_LIMIT : per_page + limit = [per_page, SBOM_OCCURRENCES_MAX_LIMIT].min offset_multiplier = [page, 1].max - 1 offset = offset_multiplier * limit diff --git a/ee/spec/finders/sbom/dependencies_finder_spec.rb b/ee/spec/finders/sbom/dependencies_finder_spec.rb index 546375d7c5b01e84..57172b46db72406b 100644 --- a/ee/spec/finders/sbom/dependencies_finder_spec.rb +++ b/ee/spec/finders/sbom/dependencies_finder_spec.rb @@ -18,6 +18,12 @@ expect(dependencies.first.id).to eq(occurrence_1.id) expect(dependencies.last.id).to eq(occurrence_3.id) end + + it 'returns records based on the default pagination' do + query = dependencies.to_sql + + expect(query).to include('LIMIT 25 OFFSET 0') + end end context 'with params' do @@ -107,6 +113,22 @@ end end + context 'with pagination parameters' do + let_it_be(:params) do + { + per_page: 2, + page: 1 + } + end + + it 'returns records based on the pagination' do + query = dependencies.to_sql + + expect(query).to include('LIMIT 2 OFFSET 0') + expect(dependencies).to eq([occurrence_1, occurrence_2]) + end + end + context 'when params is invalid' do let_it_be(:params) do { diff --git a/ee/spec/models/sbom/occurrence_spec.rb b/ee/spec/models/sbom/occurrence_spec.rb index a6952d12ac5da55e..28e0fb6e06264618 100644 --- a/ee/spec/models/sbom/occurrence_spec.rb +++ b/ee/spec/models/sbom/occurrence_spec.rb @@ -173,4 +173,28 @@ end end end + + describe '.paginate' do + using RSpec::Parameterized::TableSyntax + let(:default_limit) { described_class::SBOM_OCCURRENCES_DEFAULT_LIMIT } + let(:max_limit) { described_class::SBOM_OCCURRENCES_MAX_LIMIT } + let(:over_limit) { described_class::SBOM_OCCURRENCES_MAX_LIMIT + 1 } + + where(:per_page, :page, :limit, :offset) do + 0 | 1 | ref(:default_limit) | 0 + 1 | 0 | 1 | 0 + 1 | 2 | 1 | 1 + ref(:max_limit) | 1 | ref(:max_limit) | 0 + ref(:max_limit) | 2 | ref(:max_limit) | ref(:max_limit) + ref(:over_limit) | 1 | ref(:max_limit) | 0 + end + + with_them do + it 'adds limit and offset to the query' do + query = described_class.paginate(per_page, page).to_sql + + expect(query).to include("LIMIT #{limit} OFFSET #{offset}") + end + end + end end diff --git a/ee/spec/models/sbom/source_spec.rb b/ee/spec/models/sbom/source_spec.rb index 17b3c7c7a318cb78..12f58df459e7c004 100644 --- a/ee/spec/models/sbom/source_spec.rb +++ b/ee/spec/models/sbom/source_spec.rb @@ -14,6 +14,17 @@ it { is_expected.to validate_presence_of(:source) } end + describe 'scope' do + describe '.filter_by_package_managers' do + let(:source_bundler) { create(:sbom_source, packager_name: 'bundler') } + let(:source_yarn) { create(:sbom_source, packager_name: 'yarn') } + + subject { described_class.filter_by_package_managers(%w[bundler npm]) } + + it { is_expected.to eq([source_bundler]) } + end + end + describe 'source validation' do subject { build(:sbom_source, source: source_attributes) } diff --git a/ee/spec/requests/groups/dependencies_controller_spec.rb b/ee/spec/requests/groups/dependencies_controller_spec.rb index 9cb33977c914e5d0..3b7332d43842e56c 100644 --- a/ee/spec/requests/groups/dependencies_controller_spec.rb +++ b/ee/spec/requests/groups/dependencies_controller_spec.rb @@ -172,6 +172,21 @@ end end end + + context 'with pagination params' do + let(:params) { { group_id: group.to_param, per_page: 1, page: 1 } } + let(:finder_params) do + ActionController::Parameters.new({ per_page: 1, page: 1 }).permit(:per_page, :page) + end + + it 'propagates paginated params' do + expect(Sbom::DependenciesFinder).to receive(:new).with(group, params: finder_params).and_call_original + + subject + + expect(json_response['dependencies'].pluck('name')).to eq([sbom_occurrence_npm.name]) + end + end end context 'when feature flag group_level_dependencies is disabled' do -- GitLab From 882b85bd66ec9ef51b0fc8b7d7d2b5c7871a646a Mon Sep 17 00:00:00 2001 From: Danger bot <group_9970_bot_58e69a7d2ace73c7373ac2bc2a5cabd0@noreply.gitlab.com> Date: Wed, 24 May 2023 14:54:04 +0000 Subject: [PATCH 6/8] add suggested changes --- ee/spec/finders/sbom/dependencies_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/finders/sbom/dependencies_finder_spec.rb b/ee/spec/finders/sbom/dependencies_finder_spec.rb index 57172b46db72406b..3fe88274e93a04fa 100644 --- a/ee/spec/finders/sbom/dependencies_finder_spec.rb +++ b/ee/spec/finders/sbom/dependencies_finder_spec.rb @@ -125,7 +125,7 @@ query = dependencies.to_sql expect(query).to include('LIMIT 2 OFFSET 0') - expect(dependencies).to eq([occurrence_1, occurrence_2]) + expect(dependencies).to match_array([occurrence_1, occurrence_2]) end end -- GitLab From f8fd465c87f12e722536751ad9e59033f0a197be Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho <zfilho@gitlab.com> Date: Thu, 25 May 2023 15:31:02 +0200 Subject: [PATCH 7/8] Extract source into another query --- ee/app/models/sbom/occurrence.rb | 2 +- ee/app/models/sbom/source.rb | 4 ++++ ee/spec/models/sbom/source_spec.rb | 9 +++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/ee/app/models/sbom/occurrence.rb b/ee/app/models/sbom/occurrence.rb index a6f56a9cbb91861b..03bb9d96c07008f0 100644 --- a/ee/app/models/sbom/occurrence.rb +++ b/ee/app/models/sbom/occurrence.rb @@ -33,7 +33,7 @@ class Occurrence < ApplicationRecord end scope :filter_by_package_managers, ->(package_managers) do - where(source_id: Sbom::Source.filter_by_package_managers(package_managers)) + where(source_id: Sbom::Source.get_ids_filtered_by_package_managers(package_managers)) end scope :filter_by_component_names, ->(component_names) do diff --git a/ee/app/models/sbom/source.rb b/ee/app/models/sbom/source.rb index 37d357f209e4546b..b76eb3523822dde4 100644 --- a/ee/app/models/sbom/source.rb +++ b/ee/app/models/sbom/source.rb @@ -14,6 +14,10 @@ class Source < ApplicationRecord where("source->'package_manager'->>'name' IN (?)", package_managers) end + def self.get_ids_filtered_by_package_managers(package_managers) + filter_by_package_managers(package_managers).pluck(:id) + end + def packager source.dig('package_manager', 'name') end diff --git a/ee/spec/models/sbom/source_spec.rb b/ee/spec/models/sbom/source_spec.rb index 12f58df459e7c004..b12d18cc2c221b6f 100644 --- a/ee/spec/models/sbom/source_spec.rb +++ b/ee/spec/models/sbom/source_spec.rb @@ -25,6 +25,15 @@ end end + describe '.get_ids_filtered_by_package_managers' do + let_it_be(:source_bundler) { create(:sbom_source, packager_name: 'bundler') } + let_it_be(:source_yarn) { create(:sbom_source, packager_name: 'yarn') } + + subject { described_class.get_ids_filtered_by_package_managers(%w[bundler npm]) } + + it { is_expected.to eq([source_bundler.id]) } + end + describe 'source validation' do subject { build(:sbom_source, source: source_attributes) } -- GitLab From bfff15b19f73ec7294fb1ea6e01253602940fa51 Mon Sep 17 00:00:00 2001 From: Zamir Martins Filho <zfilho@gitlab.com> Date: Mon, 5 Jun 2023 12:05:16 +0200 Subject: [PATCH 8/8] add index for sbom_occurrences --- ...or_sbom_occurrences_on_project_id_source_id.rb | 15 +++++++++++++++ db/schema_migrations/20230605093005 | 1 + db/structure.sql | 2 ++ 3 files changed, 18 insertions(+) create mode 100644 db/post_migrate/20230605093005_add_index_for_sbom_occurrences_on_project_id_source_id.rb create mode 100644 db/schema_migrations/20230605093005 diff --git a/db/post_migrate/20230605093005_add_index_for_sbom_occurrences_on_project_id_source_id.rb b/db/post_migrate/20230605093005_add_index_for_sbom_occurrences_on_project_id_source_id.rb new file mode 100644 index 0000000000000000..868cf2763543b678 --- /dev/null +++ b/db/post_migrate/20230605093005_add_index_for_sbom_occurrences_on_project_id_source_id.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddIndexForSbomOccurrencesOnProjectIdSourceId < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + INDEX_NAME = 'idx_sbom_occurrences_on_project_id_and_source_id' + + def up + add_concurrent_index :sbom_occurrences, [:project_id, :source_id], name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :sbom_occurrences, INDEX_NAME + end +end diff --git a/db/schema_migrations/20230605093005 b/db/schema_migrations/20230605093005 new file mode 100644 index 0000000000000000..273961a1f40f928e --- /dev/null +++ b/db/schema_migrations/20230605093005 @@ -0,0 +1 @@ +df354a2edec37d3b09bc9deebe895637f8a86c90ffb2569cefe6d791458a65ba \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 904b6a3e79c41cf3..d642460cfe8634cb 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -29664,6 +29664,8 @@ CREATE INDEX idx_repository_states_on_wiki_failure_partial ON project_repository CREATE INDEX idx_repository_states_outdated_checksums ON project_repository_states USING btree (project_id) WHERE (((repository_verification_checksum IS NULL) AND (last_repository_verification_failure IS NULL)) OR ((wiki_verification_checksum IS NULL) AND (last_wiki_verification_failure IS NULL))); +CREATE INDEX idx_sbom_occurrences_on_project_id_and_source_id ON sbom_occurrences USING btree (project_id, source_id); + CREATE UNIQUE INDEX idx_security_scans_on_build_and_scan_type ON security_scans USING btree (build_id, scan_type); CREATE INDEX idx_security_scans_on_scan_type ON security_scans USING btree (scan_type); -- GitLab