Do not check read_environments for metrics dashboard

- Do not check the read_environments permission for metrics dashboard
APIs: metrics_redirect, metrics, additional_metrics, metrics_dashboard.

- These APIs only needs to read the name, slug and state of one
environment.

- Access to these APIs is controlled by the metrics_dashboard
feature/permission.

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/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

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/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