Merge branch...

Merge branch 'security-respect-analytics-enabled-rule-for-project-level-analytics-features' into 'master'

Respect analytics_enabled policy rule

See merge request gitlab-org/security/gitlab!1214

app/controllers/projects/pipelines_controller.rb

@@ -9,6 +9,7 @@ class Projects::PipelinesController < Projects::ApplicationController
   before_action :set_pipeline_path, only: [:show]
   before_action :authorize_read_pipeline!
   before_action :authorize_read_build!, only: [:index]
+  before_action :authorize_read_analytics!, only: [:charts]
   before_action :authorize_create_pipeline!, only: [:new, :create, :config_variables]
   before_action :authorize_update_pipeline!, only: [:retry, :cancel]
   before_action do

app/policies/project_policy.rb

@@ -221,6 +221,7 @@ class ProjectPolicy < BasePolicy
     enable :read_pages_content
     enable :read_release
     enable :read_analytics
+    enable :read_insights
   end
 
   # These abilities are not allowed to admins that are not members of the project,
@@ -450,6 +451,9 @@ class ProjectPolicy < BasePolicy
 
   rule { analytics_disabled }.policy do
     prevent(:read_analytics)
+    prevent(:read_insights)
+    prevent(:read_cycle_analytics)
+    prevent(:read_repository_graphs)
   end
 
   rule { wiki_disabled }.policy do
@@ -523,6 +527,7 @@ class ProjectPolicy < BasePolicy
     enable :read_cycle_analytics
     enable :read_pages_content
     enable :read_analytics
+    enable :read_insights
 
     # NOTE: may be overridden by IssuePolicy
     enable :read_issue

changelogs/unreleased/security-respect-analytics-enabled-rule-for-project-level-analytics-featu.yml

+---
+title: Enforce the analytics enabled project setting for project-level analytics features
+merge_request:
+author:
+type: security

ee/app/controllers/projects/analytics/issues_analytics_controller.rb

@@ -29,10 +29,6 @@ class Projects::Analytics::IssuesAnalyticsController < Projects::ApplicationCont
 
   private
 
-  def authorize_read_issue_analytics!
-    render_404 unless project.feature_available?(:issues_analytics)
-  end
-
   def finder_type
     IssuesFinder
   end

ee/app/controllers/projects/insights_controller.rb

@@ -7,6 +7,7 @@ class Projects::InsightsController < Projects::ApplicationController
   helper_method :project_insights_config
 
   before_action :authorize_read_project!
+  before_action :authorize_read_insights!
 
   track_unique_visits :show, target_id: 'p_analytics_insights'
 

ee/app/policies/ee/project_policy.rb

@@ -151,6 +151,11 @@ module EE
         @subject.feature_available?(:code_review_analytics, @user)
       end
 
+      with_scope :subject
+      condition(:issue_analytics_enabled) do
+        @subject.feature_available?(:issues_analytics, @user)
+      end
+
       condition(:status_page_available) do
         @subject.feature_available?(:status_page, @user)
       end
@@ -185,6 +190,12 @@ module EE
         prevent :push_code
       end
 
+      rule { analytics_disabled }.policy do
+        prevent(:read_project_merge_request_analytics)
+        prevent(:read_code_review_analytics)
+        prevent(:read_issue_analytics)
+      end
+
       rule { feature_flags_related_issues_disabled | repository_disabled }.policy do
         prevent :admin_feature_flags_issue_links
       end
@@ -367,6 +378,8 @@ module EE
         prevent :modify_merge_request_committer_setting
       end
 
+      rule { issue_analytics_enabled }.enable :read_issue_analytics
+
       rule { can?(:read_merge_request) & code_review_analytics_enabled }.enable :read_code_review_analytics
 
       rule { reporter & project_activity_analytics_available }

ee/spec/policies/project_policy_spec.rb

@@ -1748,4 +1748,80 @@ RSpec.describe ProjectPolicy do
       end
     end
   end
+
+  describe 'read_analytics' do
+    context 'with various analytics features' do
+      let_it_be(:project_with_analytics_disabled) { create(:project, :analytics_disabled) }
+      let_it_be(:project_with_analytics_private) { create(:project, :analytics_private) }
+      let_it_be(:project_with_analytics_enabled) { create(:project, :analytics_enabled) }
+
+      before do
+        stub_licensed_features(issues_analytics: true, code_review_analytics: true, project_merge_request_analytics: true)
+
+        project_with_analytics_disabled.add_developer(developer)
+        project_with_analytics_private.add_developer(developer)
+        project_with_analytics_enabled.add_developer(developer)
+      end
+
+      context 'when analytics is enabled for the project' do
+        let(:project) { project_with_analytics_disabled }
+
+        context 'for guest user' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:read_project_merge_request_analytics) }
+          it { is_expected.to be_disallowed(:read_code_review_analytics) }
+          it { is_expected.to be_disallowed(:read_issue_analytics) }
+        end
+
+        context 'for developer' do
+          let(:current_user) { developer }
+
+          it { is_expected.to be_disallowed(:read_project_merge_request_analytics) }
+          it { is_expected.to be_disallowed(:read_code_review_analytics) }
+          it { is_expected.to be_disallowed(:read_issue_analytics) }
+        end
+      end
+
+      context 'when analytics is private for the project' do
+        let(:project) { project_with_analytics_private }
+
+        context 'for guest user' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:read_project_merge_request_analytics) }
+          it { is_expected.to be_disallowed(:read_code_review_analytics) }
+          it { is_expected.to be_disallowed(:read_issue_analytics) }
+        end
+
+        context 'for developer' do
+          let(:current_user) { developer }
+
+          it { is_expected.to be_allowed(:read_project_merge_request_analytics) }
+          it { is_expected.to be_allowed(:read_code_review_analytics) }
+          it { is_expected.to be_allowed(:read_issue_analytics) }
+        end
+      end
+
+      context 'when analytics is enabled for the project' do
+        let(:project) { project_with_analytics_private }
+
+        context 'for guest user' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:read_project_merge_request_analytics) }
+          it { is_expected.to be_disallowed(:read_code_review_analytics) }
+          it { is_expected.to be_disallowed(:read_issue_analytics) }
+        end
+
+        context 'for developer' do
+          let(:current_user) { developer }
+
+          it { is_expected.to be_allowed(:read_project_merge_request_analytics) }
+          it { is_expected.to be_allowed(:read_code_review_analytics) }
+          it { is_expected.to be_allowed(:read_issue_analytics) }
+        end
+      end
+    end
+  end
 end

spec/factories/projects.rb

@@ -28,6 +28,7 @@ FactoryBot.define do
       forking_access_level { ProjectFeature::ENABLED }
       merge_requests_access_level { ProjectFeature::ENABLED }
       repository_access_level { ProjectFeature::ENABLED }
+      analytics_access_level { ProjectFeature::ENABLED }
       pages_access_level do
         visibility_level == Gitlab::VisibilityLevel::PUBLIC ? ProjectFeature::ENABLED : ProjectFeature::PRIVATE
       end
@@ -63,7 +64,8 @@ FactoryBot.define do
         repository_access_level: evaluator.repository_access_level,
         pages_access_level: evaluator.pages_access_level,
         metrics_dashboard_access_level: evaluator.metrics_dashboard_access_level,
-        operations_access_level: evaluator.operations_access_level
+        operations_access_level: evaluator.operations_access_level,
+        analytics_access_level: evaluator.analytics_access_level
       }
 
       project.build_project_feature(hash)
@@ -335,6 +337,9 @@ FactoryBot.define do
     trait(:operations_enabled)           { operations_access_level { ProjectFeature::ENABLED } }
     trait(:operations_disabled)          { operations_access_level { ProjectFeature::DISABLED } }
     trait(:operations_private)           { operations_access_level { ProjectFeature::PRIVATE } }
+    trait(:analytics_enabled)           { analytics_access_level { ProjectFeature::ENABLED } }
+    trait(:analytics_disabled)          { analytics_access_level { ProjectFeature::DISABLED } }
+    trait(:analytics_private)           { analytics_access_level { ProjectFeature::PRIVATE } }
 
     trait :auto_devops do
       association :auto_devops, factory: :project_auto_devops

spec/policies/project_policy_spec.rb

@@ -1057,6 +1057,78 @@ RSpec.describe ProjectPolicy do
       it { is_expected.to be_allowed(:read_analytics) }
     end
 
+    context 'with various analytics features' do
+      let_it_be(:project_with_analytics_disabled) { create(:project, :analytics_disabled) }
+      let_it_be(:project_with_analytics_private) { create(:project, :analytics_private) }
+      let_it_be(:project_with_analytics_enabled) { create(:project, :analytics_enabled) }
+
+      before do
+        project_with_analytics_disabled.add_developer(developer)
+        project_with_analytics_private.add_developer(developer)
+        project_with_analytics_enabled.add_developer(developer)
+      end
+
+      context 'when analytics is enabled for the project' do
+        let(:project) { project_with_analytics_disabled }
+
+        context 'for guest user' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:read_cycle_analytics) }
+          it { is_expected.to be_disallowed(:read_insights) }
+          it { is_expected.to be_disallowed(:read_repository_graphs) }
+        end
+
+        context 'for developer' do
+          let(:current_user) { developer }
+
+          it { is_expected.to be_disallowed(:read_cycle_analytics) }
+          it { is_expected.to be_disallowed(:read_insights) }
+          it { is_expected.to be_disallowed(:read_repository_graphs) }
+        end
+      end
+
+      context 'when analytics is private for the project' do
+        let(:project) { project_with_analytics_private }
+
+        context 'for guest user' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:read_cycle_analytics) }
+          it { is_expected.to be_disallowed(:read_insights) }
+          it { is_expected.to be_disallowed(:read_repository_graphs) }
+        end
+
+        context 'for developer' do
+          let(:current_user) { developer }
+
+          it { is_expected.to be_allowed(:read_cycle_analytics) }
+          it { is_expected.to be_allowed(:read_insights) }
+          it { is_expected.to be_allowed(:read_repository_graphs) }
+        end
+      end
+
+      context 'when analytics is enabled for the project' do
+        let(:project) { project_with_analytics_private }
+
+        context 'for guest user' do
+          let(:current_user) { guest }
+
+          it { is_expected.to be_disallowed(:read_cycle_analytics) }
+          it { is_expected.to be_disallowed(:read_insights) }
+          it { is_expected.to be_disallowed(:read_repository_graphs) }
+        end
+
+        context 'for developer' do
+          let(:current_user) { developer }
+
+          it { is_expected.to be_allowed(:read_cycle_analytics) }
+          it { is_expected.to be_allowed(:read_insights) }
+          it { is_expected.to be_allowed(:read_repository_graphs) }
+        end
+      end
+    end
+
     context 'project member' do
       let(:project) { private_project }
 

spec/requests/api/projects_spec.rb

@@ -886,6 +886,7 @@ RSpec.describe API::Projects do
         merge_method: 'ff'
       }).tap do |attrs|
         attrs[:operations_access_level] = 'disabled'
+        attrs[:analytics_access_level] = 'disabled'
       end
 
       post api('/projects', user), params: project