Merge branch 'refactor-instance-security-dashboard-permissions' into 'master'

Refactor instance security dashboard permissions

See merge request gitlab-org/gitlab!22740

ee/app/controllers/concerns/security_dashboards_permissions.rb

@@ -19,6 +19,6 @@ module SecurityDashboardsPermissions
   end
 
   def read_security_dashboard
-    "read_#{vulnerable.class.name.downcase}_security_dashboard".to_sym
+    "read_#{vulnerable.class.name.underscore}_security_dashboard".to_sym
   end
 end

ee/app/controllers/security/application_controller.rb

@@ -2,16 +2,21 @@
 
 module Security
   class ApplicationController < ::ApplicationController
-    before_action :authorize_read_security_dashboard!
+    include SecurityDashboardsPermissions
+
+    before_action :check_feature_enabled!
     before_action do
       push_frontend_feature_flag(:security_dashboard, default_enabled: true)
     end
 
-    private
+    protected
+
+    def check_feature_enabled!
+      render_404 unless Feature.enabled?(:security_dashboard, default_enabled: true)
+    end
 
-    def authorize_read_security_dashboard!
-      render_404 unless Feature.enabled?(:security_dashboard, default_enabled: true) &&
-        can?(current_user, :read_security_dashboard)
+    def vulnerable
+      @vulnerable ||= ApplicationInstance.new
     end
   end
 end

ee/app/controllers/security/vulnerability_findings_controller.rb

@@ -22,10 +22,6 @@ module Security
       end
     end
 
-    def vulnerable
-      @vulnerable ||= ApplicationInstance.new
-    end
-
     def valid_project_ids
       return security_dashboard_project_ids if request_project_ids.empty?
 

ee/app/helpers/ee/dashboard_helper.rb

@@ -55,10 +55,18 @@ module EE
           links << :operations
         end
 
-        if ::Feature.enabled?(:security_dashboard, default_enabled: true) && can?(current_user, :read_security_dashboard)
+        if security_dashboard_available?
           links << :security
         end
       end
     end
+
+    def security_dashboard_available?
+      app_instance = ApplicationInstance.new
+
+      ::Feature.enabled?(:security_dashboard, default_enabled: true) &&
+        app_instance.feature_available?(:security_dashboard) &&
+        can?(current_user, :read_application_instance_security_dashboard, app_instance)
+    end
   end
 end

ee/app/models/application_instance.rb

@@ -7,4 +7,8 @@ class ApplicationInstance
   def all_pipelines
     ::Ci::Pipeline.all
   end
+
+  def feature_available?(feature)
+    License.feature_available?(feature)
+  end
 end

ee/app/policies/application_instance_policy.rb

+# frozen_string_literal: true
+
+class ApplicationInstancePolicy < BasePolicy
+  rule { ~anonymous }.enable :read_application_instance_security_dashboard
+end

ee/app/policies/ee/global_policy.rb

@@ -9,16 +9,11 @@ module EE
         License.feature_available?(:operations_dashboard)
       end
 
-      condition(:security_dashboard_available) do
-        License.feature_available?(:security_dashboard)
-      end
-
       condition(:pages_size_limit_available) do
         License.feature_available?(:pages_size_limit)
       end
 
       rule { ~anonymous & operations_dashboard_available }.enable :read_operations_dashboard
-      rule { ~anonymous & security_dashboard_available }.enable :read_security_dashboard
 
       rule { admin }.policy do
         enable :read_licenses

ee/spec/helpers/ee/dashboard_helper_spec.rb

@@ -53,18 +53,31 @@ describe DashboardHelper, type: :helper do
     describe 'operations, environments and security' do
       using RSpec::Parameterized::TableSyntax
 
+      before do
+        allow(helper).to receive(:can?).and_return(false)
+      end
+
       where(:ability, :feature_flag, :nav_link) do
         :read_operations_dashboard                    | nil                     | :operations
         :read_operations_dashboard                    | :environments_dashboard | :environments
-        :read_security_dashboard   | :security_dashboard     | :security
+        :read_application_instance_security_dashboard | :security_dashboard     | :security
       end
 
       with_them do
         describe 'when the feature is enabled' do
           before do
             stub_feature_flags(feature_flag => true) unless feature_flag.nil?
-            allow(helper).to receive(:can?).and_return(false)
-            allow(helper).to receive(:can?).with(user, ability).and_return(true)
+          end
+
+          context 'and the feature is available on the license' do
+            context 'and the user is authenticated' do
+              before do
+                stub_resource_visibility(
+                  feature_flag,
+                  read_other_resources: true,
+                  read_security_dashboard: true,
+                  security_dashboard_available: true
+                )
               end
 
               it 'includes the nav link' do
@@ -72,6 +85,51 @@ describe DashboardHelper, type: :helper do
               end
             end
 
+            context 'and the user is not authenticated' do
+              let(:user) { nil }
+
+              before do
+                stub_resource_visibility(
+                  feature_flag,
+                  read_other_resources: false,
+                  read_security_dashboard: false,
+                  security_dashboard_available: true
+                )
+              end
+
+              it 'does not include the nav link' do
+                expect(helper.dashboard_nav_links).not_to include(nav_link)
+              end
+            end
+          end
+
+          context 'and the feature is not available on the license' do
+            before do
+              stub_resource_visibility(
+                feature_flag,
+                read_other_resources: false,
+                read_security_dashboard: true,
+                security_dashboard_available: false
+              )
+            end
+
+            it 'does not include the nav link' do
+              expect(helper.dashboard_nav_links).not_to include(nav_link)
+            end
+          end
+
+          def stub_resource_visibility(feature_flag, read_other_resources:, read_security_dashboard:, security_dashboard_available:)
+            if feature_flag == :security_dashboard
+              app_instance = double(ApplicationInstance, feature_available?: security_dashboard_available)
+              allow(ApplicationInstance).to receive(:new).and_return(app_instance)
+
+              allow(helper).to receive(:can?).with(user, ability, app_instance).and_return(read_security_dashboard)
+            else
+              allow(helper).to receive(:can?).with(user, ability).and_return(read_other_resources)
+            end
+          end
+        end
+
         describe 'when the feature is disabled' do
           before do
             stub_feature_flags(feature_flag => false) unless feature_flag.nil?

ee/spec/models/application_instance_spec.rb

@@ -16,4 +16,28 @@ describe ApplicationInstance do
       expect(::Ci::Pipeline).to have_received(:all)
     end
   end
+
+  describe '#feature_available?' do
+    subject { described_class.new.feature_available?(:security_dashboard) }
+
+    context "when the feature is available for the instance's license" do
+      before do
+        stub_licensed_features(security_dashboard: true)
+      end
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context "when the feature is not available for the instance's license" do
+      before do
+        stub_licensed_features(security_dashboard: false)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsy
+      end
+    end
+  end
 end

ee/spec/policies/application_instance_policy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApplicationInstancePolicy do
+  let(:current_user) { create(:user) }
+  let(:user) { create(:user) }
+
+  before do
+    stub_licensed_features(security_dashboard: true)
+  end
+
+  subject { described_class.new(current_user, [user]) }
+
+  describe 'read_application_instance_security_dashboard' do
+    context 'when the user is not logged in' do
+      let(:current_user) { nil }
+
+      it { is_expected.not_to be_allowed(:read_application_instance_security_dashboard) }
+    end
+
+    context 'when the user is logged in' do
+      it { is_expected.to be_allowed(:read_application_instance_security_dashboard) }
+    end
+  end
+end

ee/spec/policies/global_policy_spec.rb

@@ -60,32 +60,6 @@ describe GlobalPolicy do
     include_examples 'analytics policy', :view_productivity_analytics
   end
 
-  describe 'read_security_dashboard' do
-    context 'when the instance has an Ultimate license' do
-      before do
-        stub_licensed_features(security_dashboard: true)
-      end
-
-      context 'and the user is not logged in' do
-        let(:current_user) { nil }
-
-        it { is_expected.not_to be_allowed(:read_security_dashboard) }
-      end
-
-      context 'and the user is logged in' do
-        it { is_expected.to be_allowed(:read_security_dashboard) }
-      end
-    end
-
-    context 'when the instance does not have an Ultimate license' do
-      before do
-        stub_licensed_features(security_dashboard: false)
-      end
-
-      it { is_expected.not_to be_allowed(:read_security_dashboard) }
-    end
-  end
-
   describe 'update_max_pages_size' do
     context 'when feature is enabled' do
       before do

ee/spec/requests/security/vulnerability_findings_spec.rb

@@ -109,7 +109,7 @@ shared_examples 'instance security dashboard vulnerability findings endpoint' do
 end
 
 describe 'GET /-/security/vulnerability_findings' do
-  it_behaves_like 'instance security dashboard JSON endpoint' do
+  it_behaves_like 'security dashboard JSON endpoint' do
     let(:security_dashboard_request) do
       get security_vulnerability_findings_path, headers: { 'ACCEPT' => 'application/json' }
     end
@@ -234,7 +234,7 @@ describe 'GET /-/security/vulnerability_findings' do
 end
 
 describe 'GET /-/security/vulnerability_findings/summary' do
-  it_behaves_like 'instance security dashboard JSON endpoint' do
+  it_behaves_like 'security dashboard JSON endpoint' do
     let(:security_dashboard_request) do
       get summary_security_vulnerability_findings_path, headers: { 'ACCEPT' => 'application/json' }
     end
@@ -297,7 +297,7 @@ describe 'GET /-/security/vulnerability_findings/summary' do
 end
 
 describe 'GET /-/security/vulnerability_findings/history' do
-  it_behaves_like 'instance security dashboard JSON endpoint' do
+  it_behaves_like 'security dashboard JSON endpoint' do
     let(:security_dashboard_request) do
       get(
         history_security_vulnerability_findings_path,

ee/spec/support/shared_examples/requests/security/instance_security_dashboard_json_endpoint_shared_examples.rb → ee/spec/support/shared_examples/requests/security/security_dashboard_json_endpoint_shared_examples.rb

 # frozen_string_literal: true
 
-RSpec.shared_examples 'instance security dashboard JSON endpoint' do
+shared_examples 'security dashboard JSON endpoint' do
   context 'when the user is authenticated' do
     let(:security_application_controller_user) { create(:user) }