Merge branch 'rp-remove-read-environments-for-dashboard-apis' into 'master'

Do not check read_environments for metrics dashboard APIs

See merge request gitlab-org/gitlab!32422

app/controllers/projects/environments_controller.rb

@@ -10,7 +10,7 @@ class Projects::EnvironmentsController < Projects::ApplicationController
 
     push_frontend_feature_flag(:prometheus_computed_alerts)
   end
-  before_action :authorize_read_environment!
+  before_action :authorize_read_environment!, except: [:metrics, :additional_metrics, :metrics_dashboard, :metrics_redirect]
   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]

app/helpers/projects_helper.rb

@@ -410,7 +410,7 @@ module ProjectsHelper
       nav_tabs << :pipelines
     end
 
-    if can?(current_user, :read_environment, project) || can?(current_user, :read_cluster, project)
+    if can_view_operations_tab?(current_user, project)
       nav_tabs << :operations
     end
 
@@ -438,22 +438,29 @@ module ProjectsHelper
 
   def tab_ability_map
     {
-      environments:     :read_environment,
-      milestones:       :read_milestone,
-      snippets:         :read_snippet,
-      settings:         :admin_project,
-      builds:           :read_build,
-      clusters:         :read_cluster,
-      serverless:       :read_cluster,
-      error_tracking:   :read_sentry_issue,
-      alert_management: :read_alert_management_alert,
-      labels:           :read_label,
-      issues:           :read_issue,
-      project_members:  :read_project_member,
-      wiki:             :read_wiki
+      environments:       :read_environment,
+      metrics_dashboards: :metrics_dashboard,
+      milestones:         :read_milestone,
+      snippets:           :read_snippet,
+      settings:           :admin_project,
+      builds:             :read_build,
+      clusters:           :read_cluster,
+      serverless:         :read_cluster,
+      error_tracking:     :read_sentry_issue,
+      alert_management:   :read_alert_management_alert,
+      labels:             :read_label,
+      issues:             :read_issue,
+      project_members:    :read_project_member,
+      wiki:               :read_wiki
     }
   end
 
+  def can_view_operations_tab?(current_user, project)
+    [:read_environment, :read_cluster, :metrics_dashboard].any? do |ability|
+      can?(current_user, ability, project)
+    end
+  end
+
   def search_tab_ability_map
     @search_tab_ability_map ||= tab_ability_map.merge(
       blobs:          :download_code,

app/policies/project_policy.rb

@@ -278,7 +278,6 @@ class ProjectPolicy < BasePolicy
 
   rule { can?(:metrics_dashboard) }.policy do
     enable :read_prometheus
-    enable :read_environment
     enable :read_deployment
   end
 
@@ -429,27 +428,11 @@ class ProjectPolicy < BasePolicy
   rule { builds_disabled | repository_disabled }.policy do
     prevent(*create_read_update_admin_destroy(:build))
     prevent(*create_read_update_admin_destroy(:pipeline_schedule))
+    prevent(*create_read_update_admin_destroy(:environment))
     prevent(*create_read_update_admin_destroy(:cluster))
     prevent(*create_read_update_admin_destroy(:deployment))
   end
 
-  # Enabling `read_environment` specifically for the condition of `metrics_dashboard_allowed` is
-  # necessary due to the route for metrics dashboard requiring an environment id.
-  # This will be addressed in https://gitlab.com/gitlab-org/gitlab/-/issues/213833 when
-  # environments and metrics are decoupled and these rules will be removed.
-
-  rule { (builds_disabled | repository_disabled) & ~metrics_dashboard_allowed}.policy do
-    prevent(*create_read_update_admin_destroy(:environment))
-  end
-
-  rule { (builds_disabled | repository_disabled) & metrics_dashboard_allowed}.policy do
-    prevent :create_environment
-    prevent :update_environment
-    prevent :admin_environment
-    prevent :destroy_environment
-    enable :read_environment
-  end
-
   # There's two separate cases when builds_disabled is true:
   # 1. When internal CI is disabled - builds_disabled && internal_builds_disabled
   #   - We do not prevent the user from accessing Pipelines to allow them to access external CI

app/views/layouts/nav/sidebar/_project.html.haml

@@ -216,7 +216,7 @@
                   =  _('Operations')
             %li.divider.fly-out-top-item
 
-            - if project_nav_tab? :environments
+            - if project_nav_tab? :metrics_dashboards
               = nav_link(controller: :environments, action: [:metrics, :metrics_redirect]) do
                 = link_to metrics_project_environments_path(@project), title: _('Metrics'), class: 'shortcuts-metrics', data: { qa_selector: 'operations_metrics_link' } do
                   %span

spec/controllers/projects/environments_controller_spec.rb

@@ -354,6 +354,19 @@ describe Projects::EnvironmentsController do
       expect(response).to redirect_to(environment_metrics_path(environment))
     end
 
+    context 'with anonymous user and public dashboard visibility' do
+      let(:project) { create(:project, :public) }
+      let(:user) { create(:user) }
+
+      it 'redirects successfully' do
+        project.project_feature.update!(metrics_dashboard_access_level: ProjectFeature::ENABLED)
+
+        get :metrics_redirect, params: { namespace_id: project.namespace, project_id: project }
+
+        expect(response).to redirect_to(environment_metrics_path(environment))
+      end
+    end
+
     context 'when there are no environments' do
       let(:environment) { }
 
@@ -422,6 +435,19 @@ describe Projects::EnvironmentsController do
         get :metrics, params: environment_params
       end
     end
+
+    context 'with anonymous user and public dashboard visibility' do
+      let(:project) { create(:project, :public) }
+      let(:user) { create(:user) }
+
+      it 'returns success' do
+        project.project_feature.update!(metrics_dashboard_access_level: ProjectFeature::ENABLED)
+
+        get :metrics, params: environment_params
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+    end
   end
 
   describe 'GET #additional_metrics' do
@@ -497,6 +523,26 @@ describe Projects::EnvironmentsController do
         get :metrics, params: environment_params
       end
     end
+
+    context 'with anonymous user and public dashboard visibility' do
+      let(:project) { create(:project, :public) }
+      let(:user) { create(:user) }
+
+      it 'does not fail' do
+        allow(environment)
+          .to receive(:additional_metrics)
+          .and_return({
+            success: true,
+            data: {},
+            last_update: 42
+          })
+        project.project_feature.update!(metrics_dashboard_access_level: ProjectFeature::ENABLED)
+
+        additional_metrics(window_params)
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+    end
   end
 
   describe 'GET #metrics_dashboard' do
@@ -673,6 +719,17 @@ describe Projects::EnvironmentsController do
     it_behaves_like 'dashboard can be specified'
     it_behaves_like 'dashboard can be embedded'
 
+    context 'with anonymous user and public dashboard visibility' do
+      let(:project) { create(:project, :public) }
+      let(:user) { create(:user) }
+
+      before do
+        project.project_feature.update!(metrics_dashboard_access_level: ProjectFeature::ENABLED)
+      end
+
+      it_behaves_like 'the default dashboard'
+    end
+
     context 'permissions' do
       before do
         allow(controller).to receive(:can?).and_return true

spec/helpers/projects_helper_spec.rb

@@ -5,6 +5,9 @@ require 'spec_helper'
 describe ProjectsHelper do
   include ProjectForksHelper
 
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
   describe '#project_incident_management_setting' do
     let(:project) { create(:project) }
 
@@ -500,6 +503,23 @@ describe ProjectsHelper do
     end
   end
 
+  describe '#can_view_operations_tab?' do
+    before do
+      allow(helper).to receive(:current_user).and_return(user)
+    end
+
+    subject { helper.send(:can_view_operations_tab?, user, project) }
+
+    [:read_environment, :read_cluster, :metrics_dashboard].each do |ability|
+      it 'includes operations tab' do
+        allow(helper).to receive(:can?).and_return(false)
+        allow(helper).to receive(:can?).with(user, ability, project).and_return(true)
+
+        is_expected.to be(true)
+      end
+    end
+  end
+
   describe '#show_projects' do
     let(:projects) do
       create(:project)

spec/policies/project_policy_spec.rb

@@ -218,41 +218,16 @@ describe ProjectPolicy do
         project.project_feature.update(builds_access_level: ProjectFeature::DISABLED)
       end
 
-      context 'without metrics_dashboard_allowed' do
-        before do
-          project.project_feature.update(metrics_dashboard_access_level: ProjectFeature::DISABLED)
-        end
-
-        it 'disallows all permissions except pipeline when the feature is disabled' do
-          builds_permissions = [
-            :create_build, :read_build, :update_build, :admin_build, :destroy_build,
-            :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
-            :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment,
-            :create_cluster, :read_cluster, :update_cluster, :admin_cluster, :destroy_cluster,
-            :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment
-          ]
-
-          expect_disallowed(*builds_permissions)
-        end
-      end
-
-      context 'with metrics_dashboard_allowed' do
-        before do
-          project.project_feature.update(metrics_dashboard_access_level: ProjectFeature::ENABLED)
-        end
+      it 'disallows all permissions except pipeline when the feature is disabled' do
+        builds_permissions = [
+          :create_build, :read_build, :update_build, :admin_build, :destroy_build,
+          :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
+          :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment,
+          :create_cluster, :read_cluster, :update_cluster, :admin_cluster, :destroy_cluster,
+          :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment
+        ]
 
-        it 'disallows all permissions except pipeline and read_environment when the feature is disabled' do
-          builds_permissions = [
-            :create_build, :read_build, :update_build, :admin_build, :destroy_build,
-            :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
-            :create_environment, :update_environment, :admin_environment, :destroy_environment,
-            :create_cluster, :read_cluster, :update_cluster, :admin_cluster, :destroy_cluster,
-            :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment
-          ]
-
-          expect_disallowed(*builds_permissions)
-          expect_allowed(:read_environment)
-        end
+        expect_disallowed(*builds_permissions)
       end
     end
 
@@ -277,49 +252,20 @@ describe ProjectPolicy do
   context 'repository feature' do
     subject { described_class.new(owner, project) }
 
-    before do
+    it 'disallows all permissions when the feature is disabled' do
       project.project_feature.update(repository_access_level: ProjectFeature::DISABLED)
-    end
-
-    context 'without metrics_dashboard_allowed' do
-      before do
-        project.project_feature.update(metrics_dashboard_access_level: ProjectFeature::DISABLED)
-      end
 
-      it 'disallows all permissions when the feature is disabled' do
-        repository_permissions = [
-          :create_pipeline, :update_pipeline, :admin_pipeline, :destroy_pipeline,
-          :create_build, :read_build, :update_build, :admin_build, :destroy_build,
-          :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
-          :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment,
-          :create_cluster, :read_cluster, :update_cluster, :admin_cluster,
-          :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment,
-          :destroy_release
-        ]
-
-        expect_disallowed(*repository_permissions)
-      end
-    end
-
-    context 'with metrics_dashboard_allowed' do
-      before do
-        project.project_feature.update(metrics_dashboard_access_level: ProjectFeature::ENABLED)
-      end
-
-      it 'disallows all permissions when the feature is disabled' do
-        repository_permissions = [
-          :create_pipeline, :update_pipeline, :admin_pipeline, :destroy_pipeline,
-          :create_build, :read_build, :update_build, :admin_build, :destroy_build,
-          :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
-          :create_environment, :update_environment, :admin_environment, :destroy_environment,
-          :create_cluster, :read_cluster, :update_cluster, :admin_cluster,
-          :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment,
-          :destroy_release
-        ]
-
-        expect_disallowed(*repository_permissions)
-        expect_allowed(:read_environment)
-      end
+      repository_permissions = [
+        :create_pipeline, :update_pipeline, :admin_pipeline, :destroy_pipeline,
+        :create_build, :read_build, :update_build, :admin_build, :destroy_build,
+        :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule,
+        :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment,
+        :create_cluster, :read_cluster, :update_cluster, :admin_cluster,
+        :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment,
+        :destroy_release
+      ]
+
+      expect_disallowed(*repository_permissions)
     end
   end