Clean up metrics dashboard policy

Clean up metrics dashboard policy, adjust frontend variable naming and
add more specs.

app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue

@@ -128,7 +128,7 @@ export default {
       wikiAccessLevel: 20,
       snippetsAccessLevel: 20,
       pagesAccessLevel: 20,
-      metricsAccessLevel: featureAccessLevel.PROJECT_MEMBERS,
+      metricsDashboardAccessLevel: featureAccessLevel.PROJECT_MEMBERS,
       containerRegistryEnabled: true,
       lfsEnabled: true,
       requestAccessEnabled: true,
@@ -216,7 +216,7 @@ export default {
         this.buildsAccessLevel = Math.min(10, this.buildsAccessLevel);
         this.wikiAccessLevel = Math.min(10, this.wikiAccessLevel);
         this.snippetsAccessLevel = Math.min(10, this.snippetsAccessLevel);
-        this.metricsAccessLevel = Math.min(10, this.metricsAccessLevel);
+        this.metricsDashboardAccessLevel = Math.min(10, this.metricsDashboardAccessLevel);
         if (this.pagesAccessLevel === 20) {
           // When from Internal->Private narrow access for only members
           this.pagesAccessLevel = 10;
@@ -231,7 +231,7 @@ export default {
         if (this.wikiAccessLevel > 0) this.wikiAccessLevel = 20;
         if (this.snippetsAccessLevel > 0) this.snippetsAccessLevel = 20;
         if (this.pagesAccessLevel === 10) this.pagesAccessLevel = 20;
-        if (this.metricsAccessLevel === 10) this.metricsAccessLevel = 20;
+        if (this.metricsDashboardAccessLevel === 10) this.metricsDashboardAccessLevel = 20;
         this.highlightChanges();
       }
     },
@@ -492,7 +492,7 @@ export default {
         <div class="project-feature-controls">
           <div class="select-wrapper">
             <select
-              v-model="metricsAccessLevel"
+              v-model="metricsDashboardAccessLevel"
               :disabled="metricsOptionsDropdownEnabled"
               name="project[project_feature_attributes][metrics_dashboard_access_level]"
               class="form-control project-repo-select select-control"

app/controllers/projects/environments_controller.rb

@@ -4,7 +4,14 @@ class Projects::EnvironmentsController < Projects::ApplicationController
   include MetricsDashboard
 
   layout 'project'
-  before_action :authorize_read_environment!, except: [:metrics, :additional_metrics, :metrics_dashboard]
+
+  before_action only: [:metrics, :additional_metrics, :metrics_dashboard] do
+    authorize_metrics_dashboard!
+
+    push_frontend_feature_flag(:prometheus_computed_alerts)
+    push_frontend_feature_flag(:metrics_dashboard_annotations, project)
+  end
+  before_action :authorize_read_environment!
   before_action :authorize_create_environment!, only: [:new, :create]
   before_action :authorize_stop_environment!, only: [:stop]
   before_action :authorize_update_environment!, only: [:edit, :update, :cancel_auto_stop]
@@ -12,12 +19,6 @@ class Projects::EnvironmentsController < Projects::ApplicationController
   before_action :environment, only: [:show, :edit, :update, :stop, :terminal, :terminal_websocket_authorize, :metrics, :cancel_auto_stop]
   before_action :verify_api_request!, only: :terminal_websocket_authorize
   before_action :expire_etag_cache, only: [:index], unless: -> { request.format.json? }
-  before_action only: [:metrics, :additional_metrics, :metrics_dashboard] do
-    authorize_metrics_dashboard!
-
-    push_frontend_feature_flag(:prometheus_computed_alerts)
-    push_frontend_feature_flag(:metrics_dashboard_annotations, project)
-  end
   after_action :expire_etag_cache, only: [:cancel_auto_stop]
 
   def index

app/helpers/projects_helper.rb

@@ -590,7 +590,7 @@ module ProjectsHelper
       containerRegistryEnabled: !!project.container_registry_enabled,
       lfsEnabled: !!project.lfs_enabled,
       emailsDisabled: project.emails_disabled?,
-      metricsAccessLevel: feature.metrics_dashboard_access_level
+      metricsDashboardAccessLevel: feature.metrics_dashboard_access_level
     }
   end
 

app/models/project.rb

@@ -2398,15 +2398,6 @@ class Project < ApplicationRecord
   def after_wiki_activity
     touch(:last_activity_at, :last_repository_updated_at)
   end
-  
-  def metrics_dashboard_allowed?(user)
-    project_feature.metrics_dashboard_access_level = 10 if private? # private projects should never have an access level above private
-
-    return true if public? && metrics_dashboard_access_level >= 20
-    return false unless user
-
-    feature_available?(:metrics_dashboard, user) ? true : false
-  end
 
   private
 

app/models/project_feature.rb

@@ -23,7 +23,7 @@ class ProjectFeature < ApplicationRecord
   PUBLIC   = 30
 
   FEATURES = %i(issues forking merge_requests wiki snippets builds repository pages metrics_dashboard).freeze
-  PRIVATE_FEATURES_MIN_ACCESS_LEVEL = { merge_requests: Gitlab::Access::REPORTER }.freeze
+  PRIVATE_FEATURES_MIN_ACCESS_LEVEL = { merge_requests: Gitlab::Access::REPORTER, metrics_dashboard: Gitlab::Access::REPORTER }.freeze
   PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT = { repository: Gitlab::Access::REPORTER }.freeze
   STRING_OPTIONS = HashWithIndifferentAccess.new({
     'disabled' => DISABLED,

app/policies/project_policy.rb

@@ -90,7 +90,7 @@ class ProjectPolicy < BasePolicy
 
   with_scope :subject
   condition(:metrics_dashboard_allowed) do
-    @subject.metrics_dashboard_allowed?(@user)
+    feature_available?(:metrics_dashboard)
   end
 
   with_scope :global
@@ -233,6 +233,7 @@ class ProjectPolicy < BasePolicy
     enable :read_prometheus
     enable :read_metrics_dashboard_annotation
     enable :read_alert_management_alerts
+    enable :metrics_dashboard
   end
 
   # We define `:public_user_access` separately because there are cases in gitlab-ee
@@ -255,8 +256,11 @@ class ProjectPolicy < BasePolicy
     enable :fork_project
   end
 
-  rule { metrics_dashboard_allowed }.enable do
-    enable :metrics_dashboard
+  rule { metrics_dashboard_disabled }.policy do
+    prevent(:metrics_dashboard)
+  end
+
+  rule { can?(:metrics_dashboard) }.policy do
     enable :read_prometheus
     enable :read_environment
     enable :read_deployment
@@ -340,6 +344,14 @@ class ProjectPolicy < BasePolicy
     enable :admin_terraform_state
   end
 
+  rule { public_project & metrics_dashboard_allowed }.policy do
+    enable :metrics_dashboard
+  end
+
+  rule { internal_access & metrics_dashboard_allowed }.policy do
+    enable :metrics_dashboard
+  end
+
   rule { (mirror_available & can?(:admin_project)) | admin }.enable :admin_remote_mirror
   rule { can?(:push_code) }.enable :admin_tag
 

app/services/metrics/dashboard/base_service.rb

@@ -40,7 +40,9 @@ module Metrics
       # Determines whether users should be able to view
       # dashboards at all.
       def allowed?
-        project.metrics_dashboard_allowed?(current_user)
+        return false unless params[:environment]
+
+        project&.feature_available?(:metrics_dashboard, current_user)
       end
 
       # Returns a new dashboard Hash, supplemented with DB info

changelogs/unreleased/rc-add_metrics_dashboard_policy.yml

+---
+title: Add ability to change metrics dashboard visibility
+merge_request: 29634
+author:
+type: added

doc/user/project/settings/index.md

@@ -61,6 +61,7 @@ Use the switches to enable or disable the following features:
 | **Wiki**                          | ✓                         | Enables a separate system for [documentation](../wiki/)                                                                                                                                        |
 | **Snippets**                      | ✓                         | Enables [sharing of code and text](../../snippets.md)                                                                                                                                          |
 | **Pages**                         | ✓                         | Allows you to [publish static websites](../pages/)                                                                                                                                             |
+| **Metrics Dashboard**             | ✓                         | Control access to [metrics dashboard](../integrations/prometheus.md)
 
 Some features depend on others:
 
@@ -80,13 +81,15 @@ Some features depend on others:
 
 - If you disable **Repository** functionality, GitLab also disables the following
   features for your project:
-
   - **Merge Requests**
   - **Pipelines**
   - **Container Registry**
   - **Git Large File Storage**
   - **Packages**
 
+- Metrics dashboard access is dependent on reading both project environments and deployments.
+  This means anyone with access to metrics dashboard will also have access to environments and deployments.
+
 #### Disabling email notifications
 
 Project owners can disable all [email notifications](../../profile/notifications.md#gitlab-notification-emails)

spec/controllers/projects/environments_controller_spec.rb

@@ -410,6 +410,18 @@ describe Projects::EnvironmentsController do
         expect(json_response['last_update']).to eq(42)
       end
     end
+
+    context 'permissions' do
+      before do
+        allow(controller).to receive(:can?).and_return true
+      end
+
+      it 'checks :metrics_dashboard ability' do
+        expect(controller).to receive(:can?).with(anything, :metrics_dashboard, anything)
+
+        get :metrics, params: environment_params
+      end
+    end
   end
 
   describe 'GET #additional_metrics' do
@@ -473,6 +485,18 @@ describe Projects::EnvironmentsController do
           .to raise_error(ActionController::ParameterMissing)
       end
     end
+
+    context 'permissions' do
+      before do
+        allow(controller).to receive(:can?).and_return true
+      end
+
+      it 'checks :metrics_dashboard ability' do
+        expect(controller).to receive(:can?).with(anything, :metrics_dashboard, anything)
+
+        get :metrics, params: environment_params
+      end
+    end
   end
 
   describe 'GET #metrics_dashboard' do
@@ -648,6 +672,18 @@ describe Projects::EnvironmentsController do
     it_behaves_like 'the default dashboard'
     it_behaves_like 'dashboard can be specified'
     it_behaves_like 'dashboard can be embedded'
+
+    context 'permissions' do
+      before do
+        allow(controller).to receive(:can?).and_return true
+      end
+
+      it 'checks :metrics_dashboard ability' do
+        expect(controller).to receive(:can?).with(anything, :metrics_dashboard, anything)
+
+        get :metrics, params: environment_params
+      end
+    end
   end
 
   describe 'GET #search' do

spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js

@@ -489,7 +489,7 @@ describe('Settings Panel', () => {
         .find('[name="project[project_feature_attributes][metrics_dashboard_access_level]"]')
         .setValue(visibilityOptions.PUBLIC);
 
-      expect(wrapper.vm.metricsAccessLevel).toBe(visibilityOptions.PUBLIC);
+      expect(wrapper.vm.metricsDashboardAccessLevel).toBe(visibilityOptions.PUBLIC);
     });
 
     it('should contain help text', () => {

spec/models/project_spec.rb

@@ -5944,145 +5944,6 @@ describe Project do
     end
   end
 
-  describe '#metrics_dashboard_allowed?' do
-    context 'project is public' do
-      let(:project) { create(:project, :public) }
-
-      context 'metrics_dashboard_access_level is set to private' do
-        before do
-          project.metrics_dashboard_access_level = 'private'
-        end
-
-        it 'does not allow anonymous access' do
-          user = nil
-
-          expect(project.metrics_dashboard_allowed?(user)).to be false
-        end
-
-        it 'allows logged in users' do
-          user = create(:user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be false
-        end
-
-        it 'allows project members' do
-          user = create(:user)
-          project.add_developer(user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be true
-        end
-      end
-
-      context 'metrics_dashboard_access_level is set to enabled' do
-        before do
-          project.metrics_dashboard_access_level = 'enabled'
-        end
-
-        it 'allows annoymous access' do
-          user = nil
-
-          expect(project.metrics_dashboard_allowed?(user)).to be true
-        end
-
-        it 'allows logged in users' do
-          user = create(:user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be true
-        end
-
-        it 'allows project members' do
-          user = create(:user)
-          project.add_developer(user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be true
-        end
-      end
-    end
-
-    context 'project is internal' do
-      let(:project) { create(:project, :internal) }
-
-      context 'metrics_dashboard_access_level is set to private' do
-        before do
-          project.metrics_dashboard_access_level = 'private'
-        end
-
-        it 'does not allow anonymous access' do
-          user = nil
-
-          expect(project.metrics_dashboard_allowed?(user)).to be false
-        end
-
-        it 'allows logged in users' do
-          user = create(:user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be false
-        end
-
-        it 'allows project members' do
-          user = create(:user)
-          project.add_developer(user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be true
-        end
-      end
-
-      context 'metrics_dashboard_access_level is set to enabled' do
-        before do
-          project.metrics_dashboard_access_level = 'enabled'
-        end
-
-        it 'does not allow anonymous access' do
-          user = nil
-
-          expect(project.metrics_dashboard_allowed?(user)).to be false
-        end
-
-        it 'allows logged in users' do
-          user = create(:user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be true
-        end
-
-        it 'allows project members' do
-          user = create(:user)
-          project.add_developer(user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be true
-        end
-      end
-    end
-
-    context 'project is private' do
-      let(:project) { create(:project, :private) }
-
-      context 'metrics_dashboard_access_level is set to private' do
-        before do
-          project.metrics_dashboard_access_level = 'private'
-        end
-
-        it 'does not allow anonymous access' do
-          user = nil
-
-          expect(project.metrics_dashboard_allowed?(user)).to be false
-        end
-
-        it 'does not allow logged in users' do
-          user = create(:user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be false
-        end
-
-        it 'allows project members' do
-          user = create(:user)
-          project.add_developer(user)
-
-          expect(project.metrics_dashboard_allowed?(user)).to be true
-        end
-      end
-    end
-  end
-
   def finish_job(export_job)
     export_job.start
     export_job.finish

spec/policies/project_policy_spec.rb

@@ -29,6 +29,7 @@ describe ProjectPolicy do
       admin_issue admin_label admin_list read_commit_status read_build
       read_container_image read_pipeline read_environment read_deployment
       read_merge_request download_wiki_code read_sentry_issue read_metrics_dashboard_annotation
+      metrics_dashboard
     ]
   end
 
@@ -485,4 +486,190 @@ describe ProjectPolicy do
       it { is_expected.to be_disallowed(:read_prometheus_alerts) }
     end
   end
+
+  describe 'metrics_dashboard feature' do
+    subject { described_class.new(current_user, project) }
+
+    context 'public project' do
+      let(:project) { create(:project, :public) }
+
+      context 'feature private' do
+        context 'with reporter' do
+          let(:current_user) { reporter }
+
+          it { is_expected.to be_allowed(:metrics_dashboard) }
+          it { is_expected.to be_allowed(:read_prometheus) }
+          it { is_expected.to be_allowed(:read_deployment) }
+        end
+
+        context 'with guest' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:metrics_dashboard) }
+        end
+
+        context 'with anonymous' do
+          let(:current_user) { nil }
+
+          it { is_expected.to be_disallowed(:metrics_dashboard) }
+        end
+      end
+
+      context 'feature enabled' do
+        before do
+          project.project_feature.update(metrics_dashboard_access_level: ProjectFeature::ENABLED)
+        end
+
+        context 'with reporter' do
+          let(:current_user) { reporter }
+
+          it { is_expected.to be_allowed(:metrics_dashboard) }
+          it { is_expected.to be_allowed(:read_prometheus) }
+          it { is_expected.to be_allowed(:read_deployment) }
+        end
+
+        context 'with guest' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_allowed(:metrics_dashboard) }
+          it { is_expected.to be_allowed(:read_prometheus) }
+          it { is_expected.to be_allowed(:read_deployment) }
+        end
+
+        context 'with anonymous' do
+          let(:current_user) { nil }
+
+          it { is_expected.to be_allowed(:metrics_dashboard) }
+          it { is_expected.to be_allowed(:read_prometheus) }
+          it { is_expected.to be_allowed(:read_deployment) }
+        end
+      end
+    end
+
+    context 'internal project' do
+      let(:project) { create(:project, :internal) }
+
+      context 'feature private' do
+        context 'with reporter' do
+          let(:current_user) { reporter }
+
+          it { is_expected.to be_allowed(:metrics_dashboard) }
+          it { is_expected.to be_allowed(:read_prometheus) }
+          it { is_expected.to be_allowed(:read_deployment) }
+        end
+
+        context 'with guest' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:metrics_dashboard) }
+        end
+
+        context 'with anonymous' do
+          let(:current_user) { nil }
+
+          it { is_expected.to be_disallowed(:metrics_dashboard)}
+        end
+      end
+
+      context 'feature enabled' do
+        before do
+          project.project_feature.update(metrics_dashboard_access_level: ProjectFeature::ENABLED)
+        end
+
+        context 'with reporter' do
+          let(:current_user) { reporter }
+
+          it { is_expected.to be_allowed(:metrics_dashboard) }
+          it { is_expected.to be_allowed(:read_prometheus) }
+          it { is_expected.to be_allowed(:read_deployment) }
+        end
+
+        context 'with guest' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_allowed(:metrics_dashboard) }
+          it { is_expected.to be_allowed(:read_prometheus) }
+          it { is_expected.to be_allowed(:read_deployment) }
+        end
+
+        context 'with anonymous' do
+          let(:current_user) { nil }
+
+          it { is_expected.to be_disallowed(:metrics_dashboard) }
+        end
+      end
+    end
+
+    context 'private project' do
+      let(:project) { create(:project, :private) }
+
+      context 'feature private' do
+        context 'with reporter' do
+          let(:current_user) { reporter }
+
+          it { is_expected.to be_allowed(:metrics_dashboard) }
+          it { is_expected.to be_allowed(:read_prometheus) }
+          it { is_expected.to be_allowed(:read_deployment) }
+        end
+
+        context 'with guest' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:metrics_dashboard) }
+        end
+
+        context 'with anonymous' do
+          let(:current_user) { nil }
+
+          it { is_expected.to be_disallowed(:metrics_dashboard) }
+        end
+      end
+
+      context 'feature enabled' do
+        context 'with reporter' do
+          let(:current_user) { reporter }
+
+          it { is_expected.to be_allowed(:metrics_dashboard) }
+          it { is_expected.to be_allowed(:read_prometheus) }
+          it { is_expected.to be_allowed(:read_deployment) }
+        end
+
+        context 'with guest' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:metrics_dashboard) }
+        end
+
+        context 'with anonymous' do
+          let(:current_user) { nil }
+
+          it { is_expected.to be_disallowed(:metrics_dashboard) }
+        end
+      end
+    end
+
+    context 'feature disabled' do
+      before do
+        project.project_feature.update(metrics_dashboard_access_level: ProjectFeature::DISABLED)
+      end
+
+      context 'with reporter' do
+        let(:current_user) { reporter }
+
+        it { is_expected.to be_disallowed(:metrics_dashboard) }
+      end
+
+      context 'with guest' do
+        let(:current_user) { guest }
+
+        it { is_expected.to be_disallowed(:metrics_dashboard) }
+      end
+
+      context 'with anonymous' do
+        let(:current_user) { nil }
+
+        it { is_expected.to be_disallowed(:metrics_dashboard) }
+      end
+    end
+  end
 end