Merge branch 'eb-clean-up-download-coverage' into 'master'

Add a specific policy for reading build group report results

See merge request gitlab-org/gitlab!32308

app/controllers/projects/ci/daily_build_group_report_results_controller.rb

@@ -7,7 +7,7 @@ class Projects::Ci::DailyBuildGroupReportResultsController < Projects::Applicati
   REPORT_WINDOW = 90.days
 
   before_action :validate_feature_flag!
-  before_action :authorize_download_code! # Share the same authorization rules as the graphs controller
+  before_action :authorize_read_build_report_results!
   before_action :validate_param_type!
 
   def index

app/controllers/projects/graphs_controller.rb

@@ -6,7 +6,7 @@ class Projects::GraphsController < Projects::ApplicationController
   # Authorize
   before_action :require_non_empty_project
   before_action :assign_ref_vars
-  before_action :authorize_download_code!
+  before_action :authorize_read_repository_graphs!
 
   def show
     respond_to do |format|
@@ -54,7 +54,8 @@ class Projects::GraphsController < Projects::ApplicationController
   end
 
   def get_daily_coverage_options
-    return unless Feature.enabled?(:ci_download_daily_code_coverage, default_enabled: true)
+    return unless Feature.enabled?(:ci_download_daily_code_coverage, @project, default_enabled: true)
+    return unless can?(current_user, :read_build_report_results, project)
 
     date_today = Date.current
     report_window = Projects::Ci::DailyBuildGroupReportResultsController::REPORT_WINDOW

app/finders/ci/daily_build_group_report_results_finder.rb

@@ -14,7 +14,7 @@ module Ci
     end
 
     def execute
-      return none unless can?(current_user, :download_code, project)
+      return none unless can?(current_user, :read_build_report_results, project)
 
       Ci::DailyBuildGroupReportResult.recent_results(
         {

app/policies/project_policy.rb

@@ -568,6 +568,14 @@ class ProjectPolicy < BasePolicy
 
   rule { build_service_proxy_enabled }.enable :build_service_proxy_enabled
 
+  rule { can?(:download_code) }.policy do
+    enable :read_repository_graphs
+  end
+
+  rule { can?(:read_build) & can?(:read_pipeline) }.policy do
+    enable :read_build_report_results
+  end
+
   private
 
   def team_member?

spec/controllers/projects/ci/daily_build_group_report_results_controller_spec.rb

@@ -9,21 +9,8 @@ RSpec.describe Projects::Ci::DailyBuildGroupReportResultsController do
     let(:param_type) { 'coverage' }
     let(:start_date) { '2019-12-10' }
     let(:end_date) { '2020-03-09' }
-
-    def create_daily_coverage(group_name, coverage, date)
-      create(
-        :ci_daily_build_group_report_result,
-        project: project,
-        ref_path: ref_path,
-        group_name: group_name,
-        data: { 'coverage' => coverage },
-        date: date
-      )
-    end
-
-    def csv_response
-      CSV.parse(response.body)
-    end
+    let(:allowed_to_read) { true }
+    let(:user) { create(:user) }
 
     before do
       create_daily_coverage('rspec', 79.0, '2020-03-09')
@@ -31,6 +18,11 @@ RSpec.describe Projects::Ci::DailyBuildGroupReportResultsController do
       create_daily_coverage('rspec', 67.0, '2019-12-09')
       create_daily_coverage('karma', 71.0, '2019-12-09')
 
+      sign_in(user)
+
+      allow(Ability).to receive(:allowed?).and_call_original
+      allow(Ability).to receive(:allowed?).with(user, :read_build_report_results, project).and_return(allowed_to_read)
+
       get :index, params: {
         namespace_id: project.namespace,
         project_id: project,
@@ -76,5 +68,28 @@ RSpec.describe Projects::Ci::DailyBuildGroupReportResultsController do
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
       end
     end
+
+    context 'when user is not allowed to read build report results' do
+      let(:allowed_to_read) { false }
+
+      it 'responds with 404 error' do
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+
+  def create_daily_coverage(group_name, coverage, date)
+    create(
+      :ci_daily_build_group_report_result,
+      project: project,
+      ref_path: ref_path,
+      group_name: group_name,
+      data: { 'coverage' => coverage },
+      date: date
+    )
+  end
+
+  def csv_response
+    CSV.parse(response.body)
   end
 end

spec/controllers/projects/graphs_controller_spec.rb

@@ -42,23 +42,37 @@ RSpec.describe Projects::GraphsController do
         expect(response).to render_template(:charts)
       end
 
-      it 'sets the daily coverage options' do
-        Timecop.freeze do
+      context 'when anonymous users can read build report results' do
+        it 'sets the daily coverage options' do
+          Timecop.freeze do
+            get(:charts, params: { namespace_id: project.namespace.path, project_id: project.path, id: 'master' })
+
+            expect(assigns[:daily_coverage_options]).to eq(
+              base_params: {
+                start_date: Time.current.to_date - 90.days,
+                end_date: Time.current.to_date,
+                ref_path: project.repository.expand_ref('master'),
+                param_type: 'coverage'
+              },
+              download_path: namespace_project_ci_daily_build_group_report_results_path(
+                namespace_id: project.namespace,
+                project_id: project,
+                format: :csv
+              )
+            )
+          end
+        end
+      end
+
+      context 'when anonymous users cannot read build report results' do
+        before do
+          project.update_column(:public_builds, false)
+
           get(:charts, params: { namespace_id: project.namespace.path, project_id: project.path, id: 'master' })
+        end
 
-          expect(assigns[:daily_coverage_options]).to eq(
-            base_params: {
-              start_date: Time.current.to_date - 90.days,
-              end_date: Time.current.to_date,
-              ref_path: project.repository.expand_ref('master'),
-              param_type: 'coverage'
-            },
-            download_path: namespace_project_ci_daily_build_group_report_results_path(
-              namespace_id: project.namespace,
-              project_id: project,
-              format: :csv
-            )
-          )
+        it 'does not set daily coverage options' do
+          expect(assigns[:daily_coverage_options]).to be_nil
         end
       end
     end

spec/finders/ci/daily_build_group_report_results_finder_spec.rb

@@ -8,17 +8,6 @@ describe Ci::DailyBuildGroupReportResultsFinder do
     let(:ref_path) { 'refs/heads/master' }
     let(:limit) { nil }
 
-    def create_daily_coverage(group_name, coverage, date)
-      create(
-        :ci_daily_build_group_report_result,
-        project: project,
-        ref_path: ref_path,
-        group_name: group_name,
-        data: { 'coverage' => coverage },
-        date: date
-      )
-    end
-
     let!(:rspec_coverage_1) { create_daily_coverage('rspec', 79.0, '2020-03-09') }
     let!(:karma_coverage_1) { create_daily_coverage('karma', 89.0, '2020-03-09') }
     let!(:rspec_coverage_2) { create_daily_coverage('rspec', 95.0, '2020-03-10') }
@@ -37,7 +26,7 @@ describe Ci::DailyBuildGroupReportResultsFinder do
       ).execute
     end
 
-    context 'when current user is allowed to download project code' do
+    context 'when current user is allowed to read build report results' do
       let(:current_user) { project.owner }
 
       it 'returns all matching results within the given date range' do
@@ -61,7 +50,7 @@ describe Ci::DailyBuildGroupReportResultsFinder do
       end
     end
 
-    context 'when current user is not allowed to download project code' do
+    context 'when current user is not allowed to read build report results' do
       let(:current_user) { create(:user) }
 
       it 'returns an empty result' do
@@ -69,4 +58,15 @@ describe Ci::DailyBuildGroupReportResultsFinder do
       end
     end
   end
+
+  def create_daily_coverage(group_name, coverage, date)
+    create(
+      :ci_daily_build_group_report_result,
+      project: project,
+      ref_path: ref_path,
+      group_name: group_name,
+      data: { 'coverage' => coverage },
+      date: date
+    )
+  end
 end

spec/policies/project_policy_spec.rb

@@ -833,4 +833,63 @@ describe ProjectPolicy do
       it { is_expected.to be_disallowed(:create_web_ide_terminal) }
     end
   end
+
+  describe 'read_repository_graphs' do
+    subject { described_class.new(guest, project) }
+
+    before do
+      allow(subject).to receive(:allowed?).with(:read_repository_graphs).and_call_original
+      allow(subject).to receive(:allowed?).with(:download_code).and_return(can_download_code)
+    end
+
+    context 'when user can download_code' do
+      let(:can_download_code) { true }
+
+      it { is_expected.to be_allowed(:read_repository_graphs) }
+    end
+
+    context 'when user cannot download_code' do
+      let(:can_download_code) { false }
+
+      it { is_expected.to be_disallowed(:read_repository_graphs) }
+    end
+  end
+
+  describe 'read_build_report_results' do
+    subject { described_class.new(guest, project) }
+
+    before do
+      allow(subject).to receive(:allowed?).with(:read_build_report_results).and_call_original
+      allow(subject).to receive(:allowed?).with(:read_build).and_return(can_read_build)
+      allow(subject).to receive(:allowed?).with(:read_pipeline).and_return(can_read_pipeline)
+    end
+
+    context 'when user can read_build and read_pipeline' do
+      let(:can_read_build) { true }
+      let(:can_read_pipeline) { true }
+
+      it { is_expected.to be_allowed(:read_build_report_results) }
+    end
+
+    context 'when user can read_build but cannot read_pipeline' do
+      let(:can_read_build) { true }
+      let(:can_read_pipeline) { false }
+
+      it { is_expected.to be_disallowed(:read_build_report_results) }
+    end
+
+    context 'when user cannot read_build but can read_pipeline' do
+      let(:can_read_build) { false }
+      let(:can_read_pipeline) { true }
+
+      it { is_expected.to be_disallowed(:read_build_report_results) }
+    end
+
+    context 'when user cannot read_build and cannot read_pipeline' do
+      let(:can_read_build) { false }
+      let(:can_read_pipeline) { false }
+
+      it { is_expected.to be_disallowed(:read_build_report_results) }
+    end
+  end
 end