From 6da6c1e1d8eb048b4dec8d53e19da8277b590114 Mon Sep 17 00:00:00 2001 From: mo khan Date: Mon, 6 Nov 2023 14:15:07 -0700 Subject: [PATCH 1/4] Bind vulnerability_feedback* permissions to custom role permissions * Enable security dashboard in vulnerability feedback specs * Bind read_vulnerability_scanner to read_vulnerability * Use maintainer role in specs --- ee/app/policies/ee/project_policy.rb | 18 ++++++---- .../projects/issues_controller_spec.rb | 1 + .../vulnerability_feedback_controller_spec.rb | 7 ++-- ee/spec/policies/project_policy_spec.rb | 33 +++++++++++++++++-- .../vulnerabilities/feedback_policy_spec.rb | 8 +++++ .../merge_request_presenter_spec.rb | 6 +++- .../api/vulnerability_findings_spec.rb | 2 +- .../admin_vulnerability/request_spec.rb | 4 +-- .../merge_request_widget_entity_spec.rb | 1 + .../vulnerabilities/feedback_entity_spec.rb | 3 +- .../compare_security_reports_service_spec.rb | 10 +++--- .../update_service_spec.rb | 5 +-- ...destroy_dismissal_feedback_service_spec.rb | 6 +++- .../create_service_spec.rb | 2 +- 14 files changed, 78 insertions(+), 28 deletions(-) diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 29dd3d025ce37b..731cd6f77f76aa 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -384,10 +384,6 @@ module ProjectPolicy rule { can?(:developer_access) }.policy do enable :admin_issue_board - enable :read_vulnerability_feedback - enable :create_vulnerability_feedback - enable :destroy_vulnerability_feedback - enable :update_vulnerability_feedback enable :admin_feature_flags_issue_links enable :read_project_audit_events enable :read_product_analytics @@ -427,7 +423,6 @@ module ProjectPolicy rule { security_dashboard_enabled & can?(:developer_access) }.policy do enable :read_security_resource - enable :read_vulnerability_scanner end rule { coverage_fuzzing_enabled & can?(:developer_access) }.policy do @@ -469,6 +464,18 @@ module ProjectPolicy enable :admin_vulnerability end + rule { can?(:admin_vulnerability) }.policy do + enable :read_vulnerability + enable :create_vulnerability_feedback + enable :destroy_vulnerability_feedback + enable :update_vulnerability_feedback + end + + rule { can?(:read_vulnerability) }.policy do + enable :read_vulnerability_feedback + enable :read_vulnerability_scanner + end + rule { security_and_compliance_disabled }.policy do prevent :admin_vulnerability prevent :read_vulnerability @@ -542,7 +549,6 @@ module ProjectPolicy rule { auditor & security_dashboard_enabled }.policy do enable :read_security_resource - enable :read_vulnerability_scanner end rule { auditor & oncall_schedules_available }.policy do diff --git a/ee/spec/controllers/projects/issues_controller_spec.rb b/ee/spec/controllers/projects/issues_controller_spec.rb index 5f118835e4b0d6..7e5244699aec85 100644 --- a/ee/spec/controllers/projects/issues_controller_spec.rb +++ b/ee/spec/controllers/projects/issues_controller_spec.rb @@ -131,6 +131,7 @@ def perform(method, action, opts = {}) before do stub_licensed_features(security_dashboard: true) + namespace.add_maintainer(user) end it 'overwrites the default fields' do diff --git a/ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb b/ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb index c804728194bcb0..cc8226f31a97cc 100644 --- a/ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb +++ b/ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb @@ -9,7 +9,8 @@ let_it_be(:guest) { create(:user) } before do - group.add_developer(user) + stub_licensed_features(security_dashboard: true) + group.add_maintainer(user) end describe 'GET #count' do @@ -281,10 +282,6 @@ def list_feedbacks(params = {}) } end - before do - stub_licensed_features(security_dashboard: true) - end - shared_examples 'vulnerability response' do let_it_be(:vulnerability) { create(:vulnerability) } let_it_be(:finding) { create(:vulnerabilities_finding, vulnerability: vulnerability) } diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index f6f3052f4c2964..6e38d511f6f243 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -702,6 +702,10 @@ end describe 'vulnerability feedback permissions' do + before do + stub_licensed_features(security_dashboard: true) + end + where(permission: %i[ read_vulnerability_feedback create_vulnerability_feedback @@ -737,7 +741,19 @@ context 'with developer' do let(:current_user) { developer } - it { is_expected.to be_allowed(permission) } + if params[:permission] == :read_vulnerability_feedback + it { is_expected.to be_allowed(permission) } + else + it { is_expected.to be_disallowed(permission) } + end + + context "with `disable_developer_access_to_admin_vulnerability` disabled" do + before do + stub_feature_flags(disable_developer_access_to_admin_vulnerability: false) + end + + it { is_expected.to be_allowed(permission) } + end end context 'with reporter' do @@ -2683,7 +2699,9 @@ def create_member_role(member, abilities = member_role_abilities) :access_security_and_compliance, :create_vulnerability_export, :read_security_resource, - :read_vulnerability + :read_vulnerability, + :read_vulnerability_feedback, + :read_vulnerability_scanner ] end @@ -2696,7 +2714,16 @@ def create_member_role(member, abilities = member_role_abilities) context 'for a member role with admin_vulnerability true' do let(:member_role_abilities) { { read_vulnerability: true, admin_vulnerability: true } } - let(:allowed_abilities) { [:read_vulnerability, :admin_vulnerability] } + let(:allowed_abilities) do + [ + :admin_vulnerability, + :create_vulnerability_feedback, + :destroy_vulnerability_feedback, + :read_vulnerability, + :read_vulnerability_feedback, + :update_vulnerability_feedback + ] + end it_behaves_like 'custom roles abilities' end diff --git a/ee/spec/policies/vulnerabilities/feedback_policy_spec.rb b/ee/spec/policies/vulnerabilities/feedback_policy_spec.rb index c14ea10c3af1aa..c022b79580fc8d 100644 --- a/ee/spec/policies/vulnerabilities/feedback_policy_spec.rb +++ b/ee/spec/policies/vulnerabilities/feedback_policy_spec.rb @@ -99,6 +99,10 @@ end describe 'update_vulnerability_feedback' do + before do + stub_licensed_features(security_dashboard: true) + end + context 'when feedback type is issue' do let(:vulnerability_feedback) { Vulnerabilities::Feedback.new(project: project, feedback_type: :issue) } @@ -125,6 +129,10 @@ end describe 'destroy_vulnerability_feedback' do + before do + stub_licensed_features(security_dashboard: true) + end + context 'when feedback type is issue' do let(:vulnerability_feedback) { Vulnerabilities::Feedback.new(project: project, feedback_type: :issue) } diff --git a/ee/spec/presenters/merge_request_presenter_spec.rb b/ee/spec/presenters/merge_request_presenter_spec.rb index c45a3877bbb211..baa70b29bb4d9d 100644 --- a/ee/spec/presenters/merge_request_presenter_spec.rb +++ b/ee/spec/presenters/merge_request_presenter_spec.rb @@ -62,7 +62,11 @@ end end - describe 'create vulnerability feedback paths' do + describe 'create vulnerability feedback paths', feature_category: :vulnerability_management do + before do + stub_licensed_features(security_dashboard: true) + end + where(:create_feedback_path) do [ :create_vulnerability_feedback_issue_path, diff --git a/ee/spec/requests/api/vulnerability_findings_spec.rb b/ee/spec/requests/api/vulnerability_findings_spec.rb index dbcaaef6efa13b..dd570674afd4c1 100644 --- a/ee/spec/requests/api/vulnerability_findings_spec.rb +++ b/ee/spec/requests/api/vulnerability_findings_spec.rb @@ -58,7 +58,7 @@ context 'with an authorized user with proper permissions' do before do create(:vulnerability_statistic, project: project, pipeline: pipeline) - project.add_developer(user) + project.add_maintainer(user) end # Because fixture reports that power :ee_ci_job_artifact factory contain long report lists, diff --git a/ee/spec/requests/custom_roles/admin_vulnerability/request_spec.rb b/ee/spec/requests/custom_roles/admin_vulnerability/request_spec.rb index b300f7d477de2b..72c456ff6dfdb0 100644 --- a/ee/spec/requests/custom_roles/admin_vulnerability/request_spec.rb +++ b/ee/spec/requests/custom_roles/admin_vulnerability/request_spec.rb @@ -73,7 +73,7 @@ }]) end - pending "has access via a custom role" do + it "has access via a custom role" do post_graphql_mutation(graphql_mutation(:security_finding_dismiss, { uuid: security_finding.uuid, comment: "dismissal feedback", @@ -239,7 +239,7 @@ let_it_be(:vulnerability) { create(:vulnerability, :with_findings, project: project) } - pending "has access via a custom role" do + it "has access via a custom role" do post_graphql_mutation(graphql_mutation(:vulnerability_dismiss, { id: vulnerability.to_global_id.to_s, comment: "comment", diff --git a/ee/spec/serializers/merge_request_widget_entity_spec.rb b/ee/spec/serializers/merge_request_widget_entity_spec.rb index 9e9f0e41327969..fc9d27af1d5091 100644 --- a/ee/spec/serializers/merge_request_widget_entity_spec.rb +++ b/ee/spec/serializers/merge_request_widget_entity_spec.rb @@ -257,6 +257,7 @@ def create_all_artifacts describe '#can_read_vulnerability_feedback' do context 'when user has permissions to read vulnerability feedback' do before do + stub_licensed_features(security_dashboard: true) project.add_developer(user) end diff --git a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb index 90d86ba3bd631e..f98a4dde4b3865 100644 --- a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb +++ b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb @@ -136,7 +136,8 @@ context 'when allowed to destroy vulnerability feedback' do before do - project.add_developer(user) + stub_licensed_features(security_dashboard: true) + project.add_maintainer(user) end it 'contains destroy vulnerability feedback dismissal path' do diff --git a/ee/spec/services/ci/compare_security_reports_service_spec.rb b/ee/spec/services/ci/compare_security_reports_service_spec.rb index 3e01f9ee187bec..db5403a0df1103 100644 --- a/ee/spec/services/ci/compare_security_reports_service_spec.rb +++ b/ee/spec/services/ci/compare_security_reports_service_spec.rb @@ -29,7 +29,7 @@ def collect_ids(collection) describe '#execute DS' do before do - stub_licensed_features(dependency_scanning: true) + stub_licensed_features(security_dashboard: true, dependency_scanning: true) end let(:service) { described_class.new(project, current_user, report_type: 'dependency_scanning') } @@ -100,7 +100,7 @@ def collect_ids(collection) describe '#execute CS' do before do - stub_licensed_features(container_scanning: true) + stub_licensed_features(security_dashboard: true, container_scanning: true) end let(:service) { described_class.new(project, current_user, report_type: 'container_scanning') } @@ -149,7 +149,7 @@ def collect_ids(collection) describe '#execute DAST' do before do - stub_licensed_features(dast: true) + stub_licensed_features(security_dashboard: true, dast: true) end let(:service) { described_class.new(project, current_user, report_type: 'dast') } @@ -207,7 +207,7 @@ def collect_ids(collection) describe '#execute SAST' do before do - stub_licensed_features(sast: true) + stub_licensed_features(security_dashboard: true, sast: true) end let(:service) { described_class.new(project, current_user, report_type: 'sast') } @@ -257,7 +257,7 @@ def collect_ids(collection) describe '#execute SECRET DETECTION' do before do - stub_licensed_features(secret_detection: true) + stub_licensed_features(security_dashboard: true, secret_detection: true) end let(:service) { described_class.new(project, current_user, report_type: 'secret_detection') } diff --git a/ee/spec/services/ee/vulnerability_feedback_module/update_service_spec.rb b/ee/spec/services/ee/vulnerability_feedback_module/update_service_spec.rb index 44e239b8d645bb..ce5911b1fab0ec 100644 --- a/ee/spec/services/ee/vulnerability_feedback_module/update_service_spec.rb +++ b/ee/spec/services/ee/vulnerability_feedback_module/update_service_spec.rb @@ -11,7 +11,8 @@ let(:service) { described_class.new(project, user, comment: comment) } before do - group.add_developer(user) + stub_licensed_features(security_dashboard: true) + group.add_maintainer(user) end subject(:result) { service.execute(vuln_feedback) } @@ -75,7 +76,7 @@ let(:service) { described_class.new(project, second_user, vuln_feedback) } before do - group.add_developer(second_user) + group.add_maintainer(second_user) end it 'sets second user as the comment author' do diff --git a/ee/spec/services/vulnerabilities/destroy_dismissal_feedback_service_spec.rb b/ee/spec/services/vulnerabilities/destroy_dismissal_feedback_service_spec.rb index 8bd5540c63755d..c6dec27fe1ff0a 100644 --- a/ee/spec/services/vulnerabilities/destroy_dismissal_feedback_service_spec.rb +++ b/ee/spec/services/vulnerabilities/destroy_dismissal_feedback_service_spec.rb @@ -22,6 +22,10 @@ describe '#execute' do subject(:destroy_feedback) { described_class.new(user, vulnerability).execute } + before do + stub_licensed_features(security_dashboard: true) + end + context 'without necessary permissions' do it 'raises `Gitlab::Access::AccessDeniedError` error' do expect { destroy_feedback }.to raise_error(Gitlab::Access::AccessDeniedError) @@ -31,7 +35,7 @@ context 'with necessary permissions' do before do - project.add_developer(user) + project.add_maintainer(user) end it 'destroys the feedback records associated with the findings of the given vulnerability' do diff --git a/ee/spec/services/vulnerability_feedback/create_service_spec.rb b/ee/spec/services/vulnerability_feedback/create_service_spec.rb index c58b61c9cdafcc..290ce0f758656f 100644 --- a/ee/spec/services/vulnerability_feedback/create_service_spec.rb +++ b/ee/spec/services/vulnerability_feedback/create_service_spec.rb @@ -161,7 +161,7 @@ end it 'does not dismiss the existing vulnerability' do - expect { result }.not_to change { finding.vulnerability.reload.state }.from('detected') + expect { result }.to raise_error(Gitlab::Access::AccessDeniedError) end end -- GitLab From cf24ee915799a0a5995703a28ff75c8761ca2291 Mon Sep 17 00:00:00 2001 From: mo khan Date: Wed, 8 Nov 2023 10:45:10 -0700 Subject: [PATCH 2/4] Add request spec for Mutation.securityFindingDismiss --- .../security/finding/dismiss_spec.rb | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 ee/spec/requests/api/graphql/mutations/security/finding/dismiss_spec.rb diff --git a/ee/spec/requests/api/graphql/mutations/security/finding/dismiss_spec.rb b/ee/spec/requests/api/graphql/mutations/security/finding/dismiss_spec.rb new file mode 100644 index 00000000000000..70cb335ae4118c --- /dev/null +++ b/ee/spec/requests/api/graphql/mutations/security/finding/dismiss_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe "Mutation.securityFindingDismiss", feature_category: :vulnerability_management do + include GraphqlHelpers + + subject(:mutation) { graphql_mutation(:security_finding_dismiss, arguments) } + + let_it_be(:current_user) { create(:user) } + let_it_be(:project) { create(:project, :in_group) } + let_it_be(:group) { project.group } + let_it_be(:pipeline) { create(:ee_ci_pipeline, project: project) } + let_it_be(:build) { create(:ci_build, :success, project: project, pipeline: pipeline) } + let_it_be(:scan) { create(:security_scan, build: build) } + let_it_be(:security_finding) do + create(:security_finding, :with_finding_data, scan: scan, remediation_byte_offsets: [{ + "start_byte" => 0, + "end_byte" => 1 + }]) + end + + let(:arguments) do + { + uuid: security_finding.uuid, + comment: "dismissal feedback", + dismissal_reason: "USED_IN_TESTS" + } + end + + let(:mutation_response) { graphql_mutation_response(:security_finding_dismiss) } + + context "with a custom role" do + let!(:membership) { create(:project_member, :guest, user: current_user, source: project, member_role: role) } + + before do + stub_licensed_features(security_dashboard: true, custom_roles: true) + end + + context "with `admin_vulnerability` enabled" do + let(:role) { create(:member_role, :guest, :admin_vulnerability, namespace: group) } + + it "returns a successful response" do + post_graphql_mutation(mutation, current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response).to be_present + expect(mutation_response["securityFinding"]).to be_present + expect(mutation_response["errors"]).to be_empty + end + end + + context "with `admin_vulnerability` disabled" do + let(:role) { nil } + + it "returns an empty response" do + post_graphql_mutation(mutation, current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_mutation_response(:security_finding_dismiss)).to be_blank + end + end + end +end -- GitLab From 5b77bec54d493d3453d9847e38827b7a91997d79 Mon Sep 17 00:00:00 2001 From: mo khan Date: Fri, 10 Nov 2023 10:52:33 -0700 Subject: [PATCH 3/4] Remove request spec --- .../security/finding/dismiss_spec.rb | 64 ------------------- 1 file changed, 64 deletions(-) delete mode 100644 ee/spec/requests/api/graphql/mutations/security/finding/dismiss_spec.rb diff --git a/ee/spec/requests/api/graphql/mutations/security/finding/dismiss_spec.rb b/ee/spec/requests/api/graphql/mutations/security/finding/dismiss_spec.rb deleted file mode 100644 index 70cb335ae4118c..00000000000000 --- a/ee/spec/requests/api/graphql/mutations/security/finding/dismiss_spec.rb +++ /dev/null @@ -1,64 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" - -RSpec.describe "Mutation.securityFindingDismiss", feature_category: :vulnerability_management do - include GraphqlHelpers - - subject(:mutation) { graphql_mutation(:security_finding_dismiss, arguments) } - - let_it_be(:current_user) { create(:user) } - let_it_be(:project) { create(:project, :in_group) } - let_it_be(:group) { project.group } - let_it_be(:pipeline) { create(:ee_ci_pipeline, project: project) } - let_it_be(:build) { create(:ci_build, :success, project: project, pipeline: pipeline) } - let_it_be(:scan) { create(:security_scan, build: build) } - let_it_be(:security_finding) do - create(:security_finding, :with_finding_data, scan: scan, remediation_byte_offsets: [{ - "start_byte" => 0, - "end_byte" => 1 - }]) - end - - let(:arguments) do - { - uuid: security_finding.uuid, - comment: "dismissal feedback", - dismissal_reason: "USED_IN_TESTS" - } - end - - let(:mutation_response) { graphql_mutation_response(:security_finding_dismiss) } - - context "with a custom role" do - let!(:membership) { create(:project_member, :guest, user: current_user, source: project, member_role: role) } - - before do - stub_licensed_features(security_dashboard: true, custom_roles: true) - end - - context "with `admin_vulnerability` enabled" do - let(:role) { create(:member_role, :guest, :admin_vulnerability, namespace: group) } - - it "returns a successful response" do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(mutation_response).to be_present - expect(mutation_response["securityFinding"]).to be_present - expect(mutation_response["errors"]).to be_empty - end - end - - context "with `admin_vulnerability` disabled" do - let(:role) { nil } - - it "returns an empty response" do - post_graphql_mutation(mutation, current_user: current_user) - - expect(response).to have_gitlab_http_status(:success) - expect(graphql_mutation_response(:security_finding_dismiss)).to be_blank - end - end - end -end -- GitLab From 7606c1ebdd1b8e2f6aef84ee63c00dcddb6d1afc Mon Sep 17 00:00:00 2001 From: mo khan Date: Sat, 11 Nov 2023 10:32:05 -0700 Subject: [PATCH 4/4] Move developer context out of parameterized tests block --- ee/spec/policies/project_policy_spec.rb | 38 +++++++++++++------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 6e38d511f6f243..cf9af4506f4d6f 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -706,6 +706,26 @@ stub_licensed_features(security_dashboard: true) end + context 'with developer' do + let(:current_user) { developer } + + it { is_expected.to be_allowed(:read_vulnerability_feedback) } + it { is_expected.to be_disallowed(:create_vulnerability_feedback) } + it { is_expected.to be_disallowed(:update_vulnerability_feedback) } + it { is_expected.to be_disallowed(:destroy_vulnerability_feedback) } + + context "with `disable_developer_access_to_admin_vulnerability` disabled" do + before do + stub_feature_flags(disable_developer_access_to_admin_vulnerability: false) + end + + it { is_expected.to be_allowed(:read_vulnerability_feedback) } + it { is_expected.to be_allowed(:create_vulnerability_feedback) } + it { is_expected.to be_allowed(:update_vulnerability_feedback) } + it { is_expected.to be_allowed(:destroy_vulnerability_feedback) } + end + end + where(permission: %i[ read_vulnerability_feedback create_vulnerability_feedback @@ -738,24 +758,6 @@ it { is_expected.to be_allowed(permission) } end - context 'with developer' do - let(:current_user) { developer } - - if params[:permission] == :read_vulnerability_feedback - it { is_expected.to be_allowed(permission) } - else - it { is_expected.to be_disallowed(permission) } - end - - context "with `disable_developer_access_to_admin_vulnerability` disabled" do - before do - stub_feature_flags(disable_developer_access_to_admin_vulnerability: false) - end - - it { is_expected.to be_allowed(permission) } - end - end - context 'with reporter' do let(:current_user) { reporter } -- GitLab