Merge branch 'security-nfriend-hide-project-ci-cd-analytics-for-guests' into 'master'

Hide project-level CI/CD Analytics page for Guest users

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

app/controllers/projects/pipelines_controller.rb

@@ -9,7 +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, :show]
-  before_action :authorize_read_analytics!, only: [:charts]
+  before_action :authorize_read_ci_cd_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/graphql/resolvers/project_pipeline_statistics_resolver.rb

@@ -2,8 +2,12 @@
 
 module Resolvers
   class ProjectPipelineStatisticsResolver < BaseResolver
+    include Gitlab::Graphql::Authorize::AuthorizeResource
     type Types::Ci::AnalyticsType, null: true
 
+    authorizes_object!
+    authorize :read_ci_cd_analytics
+
     def resolve
       weekly_stats = Gitlab::Ci::Charts::WeekChart.new(object)
       monthly_stats = Gitlab::Ci::Charts::MonthChart.new(object)

app/policies/project_policy.rb

@@ -284,6 +284,7 @@ class ProjectPolicy < BasePolicy
     enable :read_confidential_issues
     enable :read_package
     enable :read_product_analytics
+    enable :read_ci_cd_analytics
   end
 
   # We define `:public_user_access` separately because there are cases in gitlab-ee
@@ -484,6 +485,7 @@ class ProjectPolicy < BasePolicy
     prevent(:read_insights)
     prevent(:read_cycle_analytics)
     prevent(:read_repository_graphs)
+    prevent(:read_ci_cd_analytics)
   end
 
   rule { wiki_disabled }.policy do
@@ -559,6 +561,7 @@ class ProjectPolicy < BasePolicy
     enable :read_cycle_analytics
     enable :read_pages_content
     enable :read_analytics
+    enable :read_ci_cd_analytics
     enable :read_insights
 
     # NOTE: may be overridden by IssuePolicy

lib/sidebars/projects/menus/analytics_menu.rb

@@ -46,6 +46,7 @@ module Sidebars
         def ci_cd_analytics_menu_item
           if !context.project.feature_available?(:builds, context.current_user) ||
             !can?(context.current_user, :read_build, context.project) ||
+            !can?(context.current_user, :read_ci_cd_analytics, context.project) ||
             context.project.empty_repo?
             return ::Sidebars::NilMenuItem.new(item_id: :ci_cd_analytics)
           end

spec/graphql/resolvers/project_pipeline_statistics_resolver_spec.rb

@@ -5,14 +5,24 @@ require 'spec_helper'
 RSpec.describe Resolvers::ProjectPipelineStatisticsResolver do
   include GraphqlHelpers
 
-  let_it_be(:project) { create(:project) }
+  let_it_be(:project) { create(:project, :private) }
+  let_it_be(:guest) { create(:user) }
+  let_it_be(:reporter) { create(:user) }
+
+  let(:current_user) { reporter }
+
+  before_all do
+    project.add_guest(guest)
+    project.add_reporter(reporter)
+  end
 
   specify do
     expect(described_class).to have_nullable_graphql_type(::Types::Ci::AnalyticsType)
   end
 
   def resolve_statistics(project, args)
-    resolve(described_class, obj: project, args: args)
+    ctx = { current_user: current_user }
+    resolve(described_class, obj: project, args: args, ctx: ctx)
   end
 
   describe '#resolve' do
@@ -32,5 +42,15 @@ RSpec.describe Resolvers::ProjectPipelineStatisticsResolver do
         :pipeline_times_values
       )
     end
+
+    context 'when the user does not have access to the CI/CD analytics data' do
+      let(:current_user) { guest }
+
+      it 'returns nil' do
+        result = resolve_statistics(project, {})
+
+        expect(result).to be_nil
+      end
+    end
   end
 end

spec/lib/sidebars/projects/menus/analytics_menu_spec.rb

@@ -4,15 +4,19 @@ require 'spec_helper'
 
 RSpec.describe Sidebars::Projects::Menus::AnalyticsMenu do
   let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:guest) do
+    create(:user).tap { |u| project.add_guest(u) }
+  end
 
-  let(:user) { project.owner }
-  let(:context) { Sidebars::Projects::Context.new(current_user: user, container: project, current_ref: project.repository.root_ref) }
+  let(:owner) { project.owner }
+  let(:current_user) { owner }
+  let(:context) { Sidebars::Projects::Context.new(current_user: current_user, container: project, current_ref: project.repository.root_ref) }
 
   subject { described_class.new(context) }
 
   describe '#render?' do
     context 'whe user cannot read analytics' do
-      let(:user) { nil }
+      let(:current_user) { nil }
 
       it 'returns false' do
         expect(subject.render?).to be false
@@ -79,7 +83,7 @@ RSpec.describe Sidebars::Projects::Menus::AnalyticsMenu do
       end
 
       describe 'when the user does not have access' do
-        let(:user) { nil }
+        let(:current_user) { guest }
 
         specify { is_expected.to be_nil }
       end
@@ -99,7 +103,7 @@ RSpec.describe Sidebars::Projects::Menus::AnalyticsMenu do
       end
 
       describe 'when the user does not have access' do
-        let(:user) { nil }
+        let(:current_user) { nil }
 
         specify { is_expected.to be_nil }
       end
@@ -111,7 +115,7 @@ RSpec.describe Sidebars::Projects::Menus::AnalyticsMenu do
       specify { is_expected.not_to be_nil }
 
       describe 'when the user does not have access' do
-        let(:user) { nil }
+        let(:current_user) { nil }
 
         specify { is_expected.to be_nil }
       end

spec/policies/project_policy_spec.rb

@@ -1131,12 +1131,20 @@ RSpec.describe ProjectPolicy do
       let_it_be(:project_with_analytics_enabled) { create(:project, :analytics_enabled) }
 
       before do
+        project_with_analytics_disabled.add_guest(guest)
+        project_with_analytics_private.add_guest(guest)
+        project_with_analytics_enabled.add_guest(guest)
+
+        project_with_analytics_disabled.add_reporter(reporter)
+        project_with_analytics_private.add_reporter(reporter)
+        project_with_analytics_enabled.add_reporter(reporter)
+
         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
+      context 'when analytics is disabled for the project' do
         let(:project) { project_with_analytics_disabled }
 
         context 'for guest user' do
@@ -1145,6 +1153,16 @@ RSpec.describe ProjectPolicy do
           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) }
+          it { is_expected.to be_disallowed(:read_ci_cd_analytics) }
+        end
+
+        context 'for reporter user' do
+          let(:current_user) { reporter }
+
+          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) }
+          it { is_expected.to be_disallowed(:read_ci_cd_analytics) }
         end
 
         context 'for developer' do
@@ -1153,6 +1171,7 @@ RSpec.describe ProjectPolicy do
           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) }
+          it { is_expected.to be_disallowed(:read_ci_cd_analytics) }
         end
       end
 
@@ -1162,9 +1181,19 @@ RSpec.describe ProjectPolicy do
         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_allowed(:read_cycle_analytics) }
+          it { is_expected.to be_allowed(:read_insights) }
           it { is_expected.to be_disallowed(:read_repository_graphs) }
+          it { is_expected.to be_disallowed(:read_ci_cd_analytics) }
+        end
+
+        context 'for reporter user' do
+          let(:current_user) { reporter }
+
+          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) }
+          it { is_expected.to be_allowed(:read_ci_cd_analytics) }
         end
 
         context 'for developer' do
@@ -1173,18 +1202,29 @@ RSpec.describe ProjectPolicy do
           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) }
+          it { is_expected.to be_allowed(:read_ci_cd_analytics) }
         end
       end
 
       context 'when analytics is enabled for the project' do
-        let(:project) { project_with_analytics_private }
+        let(:project) { project_with_analytics_enabled }
 
         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_allowed(:read_cycle_analytics) }
+          it { is_expected.to be_allowed(:read_insights) }
           it { is_expected.to be_disallowed(:read_repository_graphs) }
+          it { is_expected.to be_disallowed(:read_ci_cd_analytics) }
+        end
+
+        context 'for reporter user' do
+          let(:current_user) { reporter }
+
+          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) }
+          it { is_expected.to be_allowed(:read_ci_cd_analytics) }
         end
 
         context 'for developer' do
@@ -1193,6 +1233,7 @@ RSpec.describe ProjectPolicy do
           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) }
+          it { is_expected.to be_allowed(:read_ci_cd_analytics) }
         end
       end
     end