Merge branch 'jy-enable-security-dashboard-custom-user' into 'master'

Refactor: custom user can see project security dashboard

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129075



Merged-by: Jarka Košanová <jarka@gitlab.com>
Approved-by: mo khan <mo@mokhan.ca>
Approved-by: Jarka Košanová <jarka@gitlab.com>
Reviewed-by: mo khan <mo@mokhan.ca>
Co-authored-by: Jessie Young <jessieyoung@gitlab.com>

app/policies/project_policy.rb

@@ -862,7 +862,11 @@ class ProjectPolicy < BasePolicy
     enable :set_pipeline_variables
   end
-  rule { ~security_and_compliance_disabled & can?(:developer_access) }.policy do
+  rule { security_and_compliance_disabled }.policy do
+    prevent :access_security_and_compliance
+  end
+
+  rule { can?(:developer_access) }.policy do
     enable :access_security_and_compliance
   end

ee/app/policies/ee/project_policy.rb

@@ -400,14 +400,19 @@ module ProjectPolicy
         enable :admin_vulnerability_external_issue_link
       end
-      rule { can?(:read_security_resource) & ~security_and_compliance_disabled }.policy do
+      rule { can?(:read_security_resource) }.policy do
         enable :read_vulnerability
       end
-      rule { can?(:read_security_resource) & ~security_and_compliance_disabled & can?(:developer_access) }.policy do
+      rule { can?(:read_security_resource) & can?(:developer_access) }.policy do
         enable :admin_vulnerability
       end
+      rule { security_and_compliance_disabled }.policy do
+        prevent :admin_vulnerability
+        prevent :read_vulnerability
+      end
+
       rule { security_bot & auto_fix_enabled }.policy do
         enable :push_code
         enable :create_merge_request_from
@@ -470,7 +475,7 @@ module ProjectPolicy
         enable :read_project_runners
       end
-      rule { ~security_and_compliance_disabled & auditor }.policy do
+      rule { auditor }.policy do
         enable :access_security_and_compliance
       end

ee/spec/controllers/projects/security/vulnerabilities_controller_spec.rb

@@ -15,24 +15,12 @@
     sign_in(user)
   end
-  shared_examples 'security and compliance disabled' do
-    before do
-      project.project_feature.update!(security_and_compliance_access_level: Featurable::DISABLED)
-    end
-
-    it { is_expected.to have_gitlab_http_status(:not_found) }
-
-    it_behaves_like "doesn't track govern usage event", 'users_visiting_security_vulnerabilities' do
-      let(:request) { subject }
-    end
-  end
-
   describe 'GET #new' do
     subject(:request_new_vulnerability_page) do
       get :new, params: { namespace_id: project.namespace, project_id: project }
     end
-    it_behaves_like 'security and compliance disabled'
+    it_behaves_like 'security and compliance feature'
     it 'checks if the user can create a vulnerability' do
       allow(controller).to receive(:can?).and_call_original
@@ -63,11 +51,11 @@
   describe 'GET #show' do
     let_it_be(:pipeline) { create(:ci_pipeline, sha: project.commit.id, project: project, user: user) }
-    let_it_be(:vulnerability) { create(:vulnerability, project: project) }
+    let_it_be(:vulnerability) { create(:vulnerability, :with_finding, project: project) }
     subject(:show_vulnerability) { get :show, params: { namespace_id: project.namespace, project_id: project, id: vulnerability.id } }
-    it_behaves_like 'security and compliance disabled'
+    it_behaves_like 'security and compliance feature'
     context "when there's an attached pipeline" do
       let_it_be(:finding) { create(:vulnerabilities_finding, :with_pipeline, vulnerability: vulnerability) }
@@ -114,7 +102,7 @@
     subject(:show_vulnerability_discussion_list) { get :discussions, params: { namespace_id: project.namespace, project_id: project, id: vulnerability } }
-    it_behaves_like 'security and compliance disabled'
+    it_behaves_like 'security and compliance feature'
     it 'renders discussions' do
       show_vulnerability_discussion_list

ee/spec/controllers/projects/security/vulnerability_report_controller_spec.rb

@@ -12,18 +12,6 @@
     stub_licensed_features(security_dashboard: true)
   end
-  shared_examples 'security and compliance disabled' do
-    before do
-      project.project_feature.update!(security_and_compliance_access_level: Featurable::DISABLED)
-    end
-
-    it { is_expected.to have_gitlab_http_status(:not_found) }
-
-    it_behaves_like "doesn't track govern usage event", 'users_visiting_security_vulnerability_report' do
-      let(:request) { subject }
-    end
-  end
-
   describe 'GET #index' do
     let(:pipeline) { create(:ci_pipeline, sha: project.commit.id, project: project, user: user) }
@@ -34,7 +22,7 @@
       get :index, params: { namespace_id: project.namespace, project_id: project }
     end
-    it_behaves_like 'security and compliance disabled'
+    it_behaves_like 'security and compliance feature'
     context 'when project has no vulnerabilities' do
       it 'renders empty state' do

ee/spec/graphql/types/vulnerability_type_spec.rb

@@ -70,13 +70,13 @@
   end
   RSpec.shared_examples "N+1 queries" do |single_query_count, multiple_queries_count|
-    it 'avoids N+1 database queries' do
+    it 'avoids N+1 database queries', :request_store do
       GitlabSchema.execute(query, context: { current_user: user })
       control_count = ActiveRecord::QueryRecorder.new { GitlabSchema.execute(query, context: { current_user: user }) }.count
       expect(control_count).to eq(single_query_count)
-      create_list(:vulnerability, 3, :with_state_transition, :with_remediation, project: project)
+      _create_three_vulnerabilities = create_list(:vulnerability, 3, :with_state_transition, :with_remediation, project: project)
       expect { GitlabSchema.execute(query, context: { current_user: user }) }.not_to exceed_query_limit(multiple_queries_count)
     end
@@ -112,8 +112,9 @@
     end
     context 'N+1 queries' do
-      single_query_count = 15
-      multiple_queries_count = single_query_count + (3 * 3)
+      single_query_count = 10
+      number_of_vulnerabilities = 3
+      multiple_queries_count = single_query_count + (number_of_vulnerabilities * 3)
       it_behaves_like "N+1 queries", single_query_count, multiple_queries_count
     end
@@ -133,8 +134,9 @@
     let(:query_field) { 'hasRemediations' }
     context 'N+1 queries' do
-      single_query_count = 15
-      multiple_queries_count = single_query_count + (3 * 2)
+      single_query_count = 10
+      number_of_vulnerabilities = 3
+      multiple_queries_count = single_query_count + (number_of_vulnerabilities * 2)
       it_behaves_like "N+1 queries", single_query_count, multiple_queries_count
     end
@@ -144,8 +146,9 @@
     let(:query_field) { 'hasSolutions' }
     context 'N+1 queries' do
-      single_query_count = 15
-      multiple_queries_count = single_query_count + (3 * 3)
+      single_query_count = 10
+      number_of_vulnerabilities = 3
+      multiple_queries_count = single_query_count + (number_of_vulnerabilities * 3)
       it_behaves_like "N+1 queries", single_query_count, multiple_queries_count
     end

ee/spec/policies/project_policy_spec.rb

@@ -650,25 +650,50 @@
   end
   describe 'access_security_and_compliance' do
-    context 'when the user is auditor' do
-      let(:current_user) { create(:user, :auditor) }
-
+    shared_examples 'correct access to security and compliance' do
       before do
         project.project_feature.update!(security_and_compliance_access_level: access_level)
       end
-      context 'when the "Security and Compliance" is not enabled' do
+      context 'when "Security and Compliance" is disabled' do
         let(:access_level) { Featurable::DISABLED }
         it { is_expected.to be_disallowed(:access_security_and_compliance) }
+        it { is_expected.to be_disallowed(:admin_vulnerability) }
+        it { is_expected.to be_disallowed(:read_vulnerability) }
       end
-      context 'when the "Security and Compliance" is enabled' do
+      context 'when "Security and Compliance" is enabled' do
         let(:access_level) { Featurable::PRIVATE }
         it { is_expected.to be_allowed(:access_security_and_compliance) }
       end
     end
+
+    context 'when the user is developer' do
+      let(:current_user) { developer }
+
+      it_behaves_like 'correct access to security and compliance'
+    end
+
+    context 'when the user has a custom role that enables read_vulnerability' do
+      let(:current_user) { guest }
+      let_it_be(:project) { create(:project, :in_group) }
+
+      before do
+        stub_licensed_features(custom_roles: true)
+        project_member = create(:project_member, :guest, user: current_user, source: project)
+        create(:member_role, :guest, read_vulnerability: true, members: [project_member], namespace: project.group)
+      end
+
+      it_behaves_like 'correct access to security and compliance'
+    end
+
+    context 'when the user is auditor' do
+      let(:current_user) { create(:user, :auditor) }
+
+      it_behaves_like 'correct access to security and compliance'
+    end
   end
   describe 'vulnerability feedback permissions' do
@@ -758,7 +783,6 @@
         let(:current_user) { developer }
         include_context 'when security dashboard feature is not available'
-
         it { is_expected.to be_disallowed(:admin_vulnerability) }
         it { is_expected.to be_disallowed(:read_vulnerability) }
         it { is_expected.to be_disallowed(:create_vulnerability_export) }

ee/spec/requests/custom_roles/read_vulnerability/request_spec.rb

@@ -51,7 +51,7 @@
         get project_security_dashboard_index_path(project)
         expect(response).to have_gitlab_http_status(:ok)
-        expect(response.body).to have_css('div#js-project-security-dashboard[data-has-vulnerabilities="true"]')
+        expect(response).to render_template(:index)
       end
     end
   end
@@ -62,7 +62,7 @@
         get project_security_vulnerability_report_index_path(project)
         expect(response).to have_gitlab_http_status(:ok)
-        expect(response.body).to have_css('div#js-security-report-app[data-has-vulnerabilities="true"]')
+        expect(response).to render_template(:index)
       end
     end
   end

ee/spec/support/shared_examples/controllers/projects/security_and_compliance_feature_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'security and compliance feature' do
+  context 'when security and compliance disabled' do
+    before do
+      project.project_feature.update!(security_and_compliance_access_level: Featurable::DISABLED)
+    end
+
+    context 'when user has role that enables sufficient access' do
+      before do
+        group.add_developer(user)
+      end
+
+      it { is_expected.to have_gitlab_http_status(:not_found) }
+    end
+
+    context 'when user does not have role that enables sufficient access' do
+      before do
+        group.add_guest(user)
+      end
+
+      it { is_expected.to have_gitlab_http_status(:not_found) }
+    end
+
+    it_behaves_like "doesn't track govern usage event", 'users_visiting_security_vulnerabilities' do
+      let(:request) { subject }
+    end
+  end
+
+  context 'when security and compliance enabled' do
+    before do
+      project.project_feature.update!(security_and_compliance_access_level: Featurable::ENABLED)
+    end
+
+    context 'when user has role that enables sufficient access' do
+      before do
+        group.add_developer(user)
+      end
+
+      it { is_expected.not_to have_gitlab_http_status(:not_found) }
+    end
+
+    context 'when user does not have role that enables sufficient access' do
+      before do
+        group.add_guest(user)
+      end
+
+      it { is_expected.to have_gitlab_http_status(:not_found) }
+    end
+  end
+end