Merge branch 'security-ag-cycle-analytics-guest-permissions' into 'master'

Prevent guests from seeing commits for cycle analytics

See merge request gitlab/gitlabhq!3519

6 files changed, 64 insertions, 6 deletions

diff --git a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js
index cd67ba5fab8..1074ce0e744 100644
--- a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js
+++ b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js
@@ -58,10 +58,16 @@ export default () => {
         service: this.createCycleAnalyticsService(cycleAnalyticsEl.dataset.requestPath),
       };
     },
+    defaultNumberOfSummaryItems: 3,
     computed: {
       currentStage() {
         return this.store.currentActiveStage();
       },
+      summaryTableColumnClass() {
+        return this.state.summary.length === this.$options.defaultNumberOfSummaryItems
+          ? 'col-sm-3'
+          : 'col-sm-4';
+      },
     },
     created() {
       // Conditional check placed here to prevent this method from being called on the
diff --git a/app/views/projects/cycle_analytics/show.html.haml b/app/views/projects/cycle_analytics/show.html.haml
index 7fedd1ab785..1691af9dfdd 100644
--- a/app/views/projects/cycle_analytics/show.html.haml
+++ b/app/views/projects/cycle_analytics/show.html.haml
@@ -13,10 +13,10 @@
       .content-block
         .container-fluid
           .row
-            .col-sm-3.col-12.column{ "v-for" => "item in state.summary" }
+            .col-12.column{ "v-for" => "item in state.summary", ":class" => "summaryTableColumnClass" }
               %h3.header {{ item.value }}
               %p.text {{ item.title }}
-            .col-sm-3.col-12.column
+            .col-12.column{ ":class" => "summaryTableColumnClass" }
               .dropdown.inline.js-ca-dropdown
                 %button.dropdown-menu-toggle{ "data-toggle" => "dropdown", :type => "button" }
                   %span.dropdown-label {{ n__('Last %d day', 'Last %d days', 30) }}
diff --git a/changelogs/unreleased/security-ag-cycle-analytics-guest-permissions.yml b/changelogs/unreleased/security-ag-cycle-analytics-guest-permissions.yml
new file mode 100644
index 00000000000..c7a3b8923cd
--- /dev/null
+++ b/changelogs/unreleased/security-ag-cycle-analytics-guest-permissions.yml
@@ -0,0 +1,5 @@
+---
+title: Hide commit counts from guest users in Cycle Analytics.
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/cycle_analytics/stage_summary.rb b/lib/gitlab/cycle_analytics/stage_summary.rb
index ea440c441b7..9c75d4bb455 100644
--- a/lib/gitlab/cycle_analytics/stage_summary.rb
+++ b/lib/gitlab/cycle_analytics/stage_summary.rb
@@ -11,13 +11,29 @@ module Gitlab
       end
 
       def data
-        [serialize(Summary::Issue.new(project: @project, from: @from, to: @to, current_user: @current_user)),
-         serialize(Summary::Commit.new(project: @project, from: @from, to: @to)),
-         serialize(Summary::Deploy.new(project: @project, from: @from, to: @to))]
+        summary = [issue_stats]
+        summary << commit_stats if user_has_sufficient_access?
+        summary << deploy_stats
       end
 
       private
 
+      def issue_stats
+        serialize(Summary::Issue.new(project: @project, from: @from, to: @to, current_user: @current_user))
+      end
+
+      def commit_stats
+        serialize(Summary::Commit.new(project: @project, from: @from, to: @to))
+      end
+
+      def deploy_stats
+        serialize(Summary::Deploy.new(project: @project, from: @from, to: @to))
+      end
+
+      def user_has_sufficient_access?
+        @project.team.member?(@current_user, Gitlab::Access::REPORTER)
+      end
+
       def serialize(summary_object)
         AnalyticsSummarySerializer.new.represent(summary_object)
       end
diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb
index 0fc4841ee0e..e9751aa2e72 100644
--- a/spec/features/cycle_analytics_spec.rb
+++ b/spec/features/cycle_analytics_spec.rb
@@ -112,6 +112,10 @@ describe 'Cycle Analytics', :js do
       wait_for_requests
     end
 
+    it 'does not show the commit stats' do
+      expect(page).to have_no_selector(:xpath, commits_counter_selector)
+    end
+
     it 'needs permissions to see restricted stages' do
       expect(find('.stage-events')).to have_content(issue.title)
 
@@ -127,8 +131,12 @@ describe 'Cycle Analytics', :js do
     find(:xpath, "//p[contains(text(),'New Issue')]/preceding-sibling::h3")
   end
 
+  def commits_counter_selector
+    "//p[contains(text(),'Commits')]/preceding-sibling::h3"
+  end
+
   def commits_counter
-    find(:xpath, "//p[contains(text(),'Commits')]/preceding-sibling::h3")
+    find(:xpath, commits_counter_selector)
   end
 
   def deploys_counter
diff --git a/spec/lib/gitlab/cycle_analytics/stage_summary_spec.rb b/spec/lib/gitlab/cycle_analytics/stage_summary_spec.rb
index 8f9dac6d281..94edef20296 100644
--- a/spec/lib/gitlab/cycle_analytics/stage_summary_spec.rb
+++ b/spec/lib/gitlab/cycle_analytics/stage_summary_spec.rb
@@ -6,6 +6,11 @@ describe Gitlab::CycleAnalytics::StageSummary do
   let(:project) { create(:project, :repository) }
   let(:options) { { from: 1.day.ago, current_user: user } }
   let(:user) { create(:user, :admin) }
+
+  before do
+    project.add_maintainer(user)
+  end
+
   let(:stage_summary) { described_class.new(project, options).data }
 
   describe "#new_issues" do
@@ -86,6 +91,24 @@ describe Gitlab::CycleAnalytics::StageSummary do
         expect(subject).to eq(2)
       end
     end
+
+    context 'when a guest user is signed in' do
+      let(:guest_user) { create(:user) }
+
+      before do
+        project.add_guest(guest_user)
+        options.merge!({ current_user: guest_user })
+      end
+
+      it 'does not include commit stats' do
+        data = described_class.new(project, options).data
+        expect(includes_commits?(data)).to be_falsy
+      end
+
+      def includes_commits?(data)
+        data.any? { |h| h["title"] == 'Commits' }
+      end
+    end
   end
 
   describe "#deploys" do