Hide Contribution Analytics from non-group members

because non-group members should not be allowed to
view activities or stats from group members.

Render promotions page if promotions are on and
the user does not have access to the feature.

ee/app/controllers/groups/contribution_analytics_controller.rb

@@ -3,6 +3,7 @@
 class Groups::ContributionAnalyticsController < Groups::ApplicationController
   before_action :group
   before_action :check_contribution_analytics_available!
+  before_action :authorize_read_contribution_analytics!
 
   layout 'group'
 
@@ -27,6 +28,28 @@ class Groups::ContributionAnalyticsController < Groups::ApplicationController
   end
 
   def check_contribution_analytics_available!
-    render_404 unless @group.feature_available?(:contribution_analytics) || LicenseHelper.show_promotions?(current_user)
+    return if group_has_access_to_feature?
+
+    show_promotions? ? render_promotion : render_404
+  end
+
+  def authorize_read_contribution_analytics!
+    render_403 unless user_has_access_to_feature?
+  end
+
+  def render_promotion
+    render 'shared/promotions/_promote_contribution_analytics'
+  end
+
+  def show_promotions?
+    LicenseHelper.show_promotions?(current_user)
+  end
+
+  def group_has_access_to_feature?
+    @group.feature_available?(:contribution_analytics)
+  end
+
+  def user_has_access_to_feature?
+    can?(current_user, :read_group_contribution_analytics, @group)
   end
 end

ee/app/policies/ee/group_policy.rb

@@ -75,7 +75,7 @@ module EE
       rule { can?(:read_cluster) & cluster_deployments_available }
         .enable :read_cluster_environments
 
-      rule { can?(:read_group) & contribution_analytics_available }
+      rule { has_access & contribution_analytics_available }
         .enable :read_group_contribution_analytics
 
       rule { reporter & cycle_analytics_available }.policy do

ee/changelogs/unreleased/security-ag-contribution-analytics.yml

+---
+title: Don't show Contribution Analytics to users who are not group members
+merge_request:
+author:
+type: security

ee/spec/controllers/groups/contribution_analytics_controller_spec.rb

@@ -6,6 +6,7 @@ describe Groups::ContributionAnalyticsController do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
+  let(:guest_user) { create(:user) }
   let(:group) { create(:group) }
   let(:project) { create(:project, :repository, group: group) }
   let(:issue) { create(:issue, project: project) }
@@ -30,128 +31,198 @@ describe Groups::ContributionAnalyticsController do
     group.add_owner(user)
     group.add_user(user2, GroupMember::DEVELOPER)
     group.add_user(user3, GroupMember::MAINTAINER)
-    sign_in(user)
-
-    create_event(user, project, issue, Event::CLOSED)
-    create_event(user2, project, issue, Event::CLOSED)
-    create_event(user2, project, merge_request, Event::CREATED)
-    create_event(user3, project, merge_request, Event::CREATED)
-    create_push_event(user, project)
-    create_push_event(user3, project)
   end
 
-  it 'returns 404 when feature is not available and we dont show promotions' do
-    stub_licensed_features(contribution_analytics: false)
+  describe '#authorize_read_contribution_analytics!' do
+    before do
+      group.add_user(guest_user, GroupMember::GUEST)
+      sign_in(guest_user)
+    end
 
-    get :show, params: { group_id: group.path }
+    context 'when user has access to the group' do
+      let(:request) { get :show, params: { group_id: group.path } }
 
-    expect(response).to have_gitlab_http_status(:not_found)
-  end
+      context 'when feature is available to the group' do
+        before do
+          allow(License).to receive(:feature_available?).and_call_original
+          allow(License).to receive(:feature_available?)
+            .with(:contribution_analytics)
+            .and_return(true)
 
-  context 'unlicensed but we show promotions' do
-    before do
-      allow(License).to receive(:current).and_return(nil)
-      allow(LicenseHelper).to receive(:show_promotions?).and_return(true)
-      stub_application_setting(check_namespace_plan: false)
-    end
+          allow(Ability).to receive(:allowed?).and_call_original
+          allow(Ability).to receive(:allowed?)
+            .with(guest_user, :read_group_contribution_analytics, group)
+            .and_return(user_has_access_to_feature)
+        end
 
-    it 'returns page when feature is not available and we show promotions' do
-      stub_licensed_features(contribution_analytics: false)
+        context 'when user has access to the feature' do
+          let(:user_has_access_to_feature) { true }
 
-      get :show, params: { group_id: group.path }
+          it 'renders 200' do
+            request
 
-      expect(response).to have_gitlab_http_status(:ok)
+            expect(response).to have_gitlab_http_status(:ok)
+          end
+        end
+
+        context 'when user does not have access to the feature' do
+          let(:user_has_access_to_feature) { false }
+
+          it 'renders 403' do
+            request
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
+        end
+      end
     end
-  end
 
-  it 'sets instance variables properly', :aggregate_failures do
-    get :show, params: { group_id: group.path }
+    describe '#check_contribution_analytics_available!' do
+      before do
+        group.add_user(guest_user, GroupMember::GUEST)
+        sign_in(guest_user)
+      end
 
-    expect(response).to have_gitlab_http_status(:ok)
+      context 'when feature is not available to the group' do
+        let(:request) { get :show, params: { group_id: group.path } }
 
-    expect(assigns[:data_collector].users).to match_array([user, user2, user3])
-    expect(assigns[:data_collector].total_events_by_author_count.values.sum).to eq(6)
-    stats = assigns[:data_collector].group_member_contributions_table_data
+        before do
+          allow(License).to receive(:feature_available?).and_call_original
+          allow(License).to receive(:feature_available?)
+            .with(:contribution_analytics)
+            .and_return(false)
 
-    # NOTE: The array ordering matters! The view references them all by index
-    expect(stats[:merge_requests_created][:data]).to eq([0, 1, 1])
-    expect(stats[:issues_closed][:data]).to eq([1, 1, 0])
-    expect(stats[:push][:data]).to eq([1, 0, 1])
-  end
+          allow(LicenseHelper).to receive(:show_promotions?)
+            .and_return(show_promotions)
+        end
 
-  it "returns member contributions JSON when format is JSON" do
-    get :show, params: { group_id: group.path }, format: :json
-
-    expect(json_response.length).to eq(3)
-
-    first_user = json_response.at(0)
-    expect(first_user["username"]).to eq(user.username)
-    expect(first_user["user_web_url"]).to eq("/#{user.username}")
-    expect(first_user["fullname"]).to eq(user.name)
-    expect(first_user["push"]).to eq(1)
-    expect(first_user["issues_created"]).to eq(0)
-    expect(first_user["issues_closed"]).to eq(1)
-    expect(first_user["merge_requests_created"]).to eq(0)
-    expect(first_user["merge_requests_merged"]).to eq(0)
-    expect(first_user["total_events"]).to eq(2)
-  end
+        context 'when promotions are on' do
+          let(:show_promotions) { true }
+
+          it 'renders promotions page' do
+            request
 
-  it "includes projects in subgroups" do
-    subgroup = create(:group, parent: group)
-    subproject = create(:project, :repository, group: subgroup)
+            expect(response).to render_template(
+              'shared/promotions/_promote_contribution_analytics')
+          end
+        end
 
-    create_event(user, subproject, issue, Event::CLOSED)
-    create_push_event(user, subproject)
+        context 'when promotions are not on' do
+          let(:show_promotions) { false }
 
-    get :show, params: { group_id: group.path }, format: :json
+          it 'renders 404' do
+            request
 
-    first_user = json_response.first
-    expect(first_user["issues_closed"]).to eq(2)
-    expect(first_user["push"]).to eq(2)
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
+        end
+      end
+    end
   end
 
-  it "excludes projects outside of the group" do
-    empty_group = create(:group)
-    other_project = create(:project, :repository)
+  describe 'with contributions' do
+    before do
+      sign_in(user)
 
-    create_event(user, other_project, issue, Event::CLOSED)
-    create_push_event(user, other_project)
+      create_event(user, project, issue, Event::CLOSED)
+      create_event(user2, project, issue, Event::CLOSED)
+      create_event(user2, project, merge_request, Event::CREATED)
+      create_event(user3, project, merge_request, Event::CREATED)
+      create_push_event(user, project)
+      create_push_event(user3, project)
+    end
 
-    get :show, params: { group_id: empty_group.path }, format: :json
+    it 'sets instance variables properly', :aggregate_failures do
+      get :show, params: { group_id: group.path }
 
-    expect(json_response).to be_empty
-  end
+      expect(response).to have_gitlab_http_status(:ok)
+
+      expect(assigns[:data_collector].users).to match_array([user, user2, user3])
+      expect(assigns[:data_collector].total_events_by_author_count.values.sum).to eq(6)
+      stats = assigns[:data_collector].group_member_contributions_table_data
 
-  it 'does not cause N+1 queries when the format is JSON' do
-    control_count = ActiveRecord::QueryRecorder.new do
+      # NOTE: The array ordering matters! The view references them all by index
+      expect(stats[:merge_requests_created][:data]).to eq([0, 1, 1])
+      expect(stats[:issues_closed][:data]).to eq([1, 1, 0])
+      expect(stats[:push][:data]).to eq([1, 0, 1])
+    end
+
+    it "returns member contributions JSON when format is JSON" do
       get :show, params: { group_id: group.path }, format: :json
+
+      expect(json_response.length).to eq(3)
+
+      first_user = json_response.at(0)
+      expect(first_user["username"]).to eq(user.username)
+      expect(first_user["user_web_url"]).to eq("/#{user.username}")
+      expect(first_user["fullname"]).to eq(user.name)
+      expect(first_user["push"]).to eq(1)
+      expect(first_user["issues_created"]).to eq(0)
+      expect(first_user["issues_closed"]).to eq(1)
+      expect(first_user["merge_requests_created"]).to eq(0)
+      expect(first_user["merge_requests_merged"]).to eq(0)
+      expect(first_user["total_events"]).to eq(2)
     end
 
-    controller.instance_variable_set(:@group, nil)
-    user4 = create(:user)
-    group.add_user(user4, GroupMember::DEVELOPER)
+    it "includes projects in subgroups" do
+      subgroup = create(:group, parent: group)
+      subproject = create(:project, :repository, group: subgroup)
 
-    expect { get :show, params: { group_id: group.path }, format: :json }
-      .not_to exceed_query_limit(control_count)
-  end
+      create_event(user, subproject, issue, Event::CLOSED)
+      create_push_event(user, subproject)
 
-  describe 'with views' do
-    render_views
+      get :show, params: { group_id: group.path }, format: :json
 
-    it 'avoids a N+1 query in #show' do
-      # Warm the cache
-      get :show, params: { group_id: group.path }
+      first_user = json_response.first
+      expect(first_user["issues_closed"]).to eq(2)
+      expect(first_user["push"]).to eq(2)
+    end
 
-      control_queries = ActiveRecord::QueryRecorder.new { get :show, params: { group_id: group.path } }
-      create_push_event(user, project)
+    it "excludes projects outside of the group" do
+      empty_group = create(:group)
+      other_project = create(:project, :repository)
+
+      empty_group.add_reporter(user)
+
+      create_event(user, other_project, issue, Event::CLOSED)
+      create_push_event(user, other_project)
+
+      get :show, params: { group_id: empty_group.path }, format: :json
 
-      expect { get :show, params: { group_id: group.path } }.not_to exceed_query_limit(control_queries)
+      expect(json_response).to be_empty
     end
-  end
 
-  describe 'GET #index' do
-    subject { get :show, params: { group_id: group.to_param } }
+    it 'does not cause N+1 queries when the format is JSON' do
+      control_count = ActiveRecord::QueryRecorder.new do
+        get :show, params: { group_id: group.path }, format: :json
+      end
+
+      controller.instance_variable_set(:@group, nil)
+      user4 = create(:user)
+      group.add_user(user4, GroupMember::DEVELOPER)
+
+      expect { get :show, params: { group_id: group.path }, format: :json }
+        .not_to exceed_query_limit(control_count)
+    end
+
+    describe 'with views' do
+      render_views
+
+      it 'avoids a N+1 query in #show' do
+        # Warm the cache
+        get :show, params: { group_id: group.path }
 
-    it_behaves_like 'disabled when using an external authorization service'
+        control_queries = ActiveRecord::QueryRecorder.new { get :show, params: { group_id: group.path } }
+        create_push_event(user, project)
+
+        expect { get :show, params: { group_id: group.path } }.not_to exceed_query_limit(control_queries)
+      end
+    end
+
+    describe 'GET #index' do
+      subject { get :show, params: { group_id: group.to_param } }
+
+      it_behaves_like 'disabled when using an external authorization service'
+    end
   end
 end

ee/spec/helpers/ee/groups_helper_spec.rb

@@ -3,14 +3,15 @@
 require 'spec_helper'
 
 describe GroupsHelper do
-  let(:user) { create(:user, group_view: :security_dashboard) }
+  let(:owner) { create(:user, group_view: :security_dashboard) }
+  let(:current_user) { owner }
   let(:group) { create(:group, :private) }
 
   before do
-    allow(helper).to receive(:current_user) { user }
+    allow(helper).to receive(:current_user) { current_user }
     helper.instance_variable_set(:@group, group)
 
-    group.add_owner(user)
+    group.add_owner(owner)
   end
 
   describe '#group_epics_count' do
@@ -49,6 +50,21 @@ describe GroupsHelper do
 
       expect(helper.group_sidebar_links).not_to include(:contribution_analytics, :epics)
     end
+
+    context 'when contribution analytics is available' do
+      before do
+        stub_licensed_features(contribution_analytics: true)
+      end
+
+      context 'signed in user is a project member but not a member of the group' do
+        let(:current_user) { create(:user) }
+        let(:private_project) { create(:project, :private, group: group)}
+
+        it 'hides Contribution Analytics' do
+          expect(helper.group_sidebar_links).not_to include(:contribution_analytics)
+        end
+      end
+    end
   end
 
   describe '#permanent_deletion_date' do
@@ -107,10 +123,10 @@ describe GroupsHelper do
 
     with_them do
       it 'returns the expected value' do
-        allow(helper).to receive(:current_user) { user? ? user : nil }
+        allow(helper).to receive(:current_user) { user? ? owner : nil }
         allow(::Gitlab).to receive(:com?) { gitlab_com? }
-        allow(user).to receive(:ab_feature_enabled?) { ab_feature_enabled? }
-        allow(user).to receive(:created_at) { created_at }
+        allow(owner).to receive(:ab_feature_enabled?) { ab_feature_enabled? }
+        allow(owner).to receive(:created_at) { created_at }
         allow(::Feature).to receive(:enabled?).with(:discover_security) { discover_security_feature_enabled? }
         allow(group).to receive(:feature_available?) { security_dashboard_feature_available? }
         allow(helper).to receive(:can?) { can_admin_group? }

ee/spec/policies/group_policy_spec.rb

@@ -48,7 +48,34 @@ describe GroupPolicy do
       stub_licensed_features(contribution_analytics: true)
     end
 
-    it { is_expected.to be_allowed(:read_group_contribution_analytics) }
+    context 'when signed in user is a member of the group' do
+      it { is_expected.to be_allowed(:read_group_contribution_analytics) }
+    end
+
+    describe 'when user is not a member of the group' do
+      let(:current_user) { non_group_member }
+      let(:private_group) { create(:group, :private) }
+
+      subject { described_class.new(non_group_member, private_group) }
+
+      context 'when user is not invited to any of the group projects' do
+        it do
+          is_expected.not_to be_allowed(:read_group_contribution_analytics)
+        end
+      end
+
+      context 'when user is invited to a group project, but not to the group' do
+        let(:private_project) { create(:project, :private, group: private_group) }
+
+        before do
+          private_project.add_guest(non_group_member)
+        end
+
+        it do
+          is_expected.not_to be_allowed(:read_group_contribution_analytics)
+        end
+      end
+    end
   end
 
   context 'when contribution analytics is not available' do

ee/spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb

@@ -11,23 +11,50 @@ describe 'layouts/nav/sidebar/_group' do
   let(:user) { create(:user) }
 
   describe 'contribution analytics tab' do
-    it 'is not visible when there is no valid license and we dont show promotions' do
-      stub_licensed_features(contribution_analytics: false)
+    let!(:current_user) { create(:user) }
 
-      render
+    before do
+      group.add_guest(current_user)
 
-      expect(rendered).not_to have_text 'Contribution Analytics'
+      allow(view).to receive(:current_user).and_return(current_user)
     end
 
-    context 'no license installed' do
-      let!(:cuser) { create(:admin) }
+    context 'contribution analytics feature is available' do
+      before do
+        stub_licensed_features(contribution_analytics: true)
+      end
+
+      it 'is visible' do
+        render
 
+        expect(rendered).to have_text 'Contribution Analytics'
+      end
+    end
+
+    context 'contribution analytics feature is not available' do
+      before do
+        stub_licensed_features(contribution_analytics: false)
+      end
+
+      context 'we do not show promotions' do
+        before do
+          allow(LicenseHelper).to receive(:show_promotions?).and_return(false)
+        end
+
+        it 'is not visible' do
+          render
+
+          expect(rendered).not_to have_text 'Contribution Analytics'
+        end
+      end
+    end
+
+    context 'no license installed' do
       before do
         allow(License).to receive(:current).and_return(nil)
         stub_application_setting(check_namespace_plan: false)
 
         allow(view).to receive(:can?) { |*args| Ability.allowed?(*args) }
-        allow(view).to receive(:current_user).and_return(cuser)
       end
 
       it 'is visible when there is no valid license but we show promotions' do

spec/support/shared_contexts/policies/group_policy_shared_context.rb

@@ -7,6 +7,7 @@ RSpec.shared_context 'GroupPolicy context' do
   let_it_be(:maintainer) { create(:user) }
   let_it_be(:owner) { create(:user) }
   let_it_be(:admin) { create(:admin) }
+  let_it_be(:non_group_member) { create(:user) }
   let_it_be(:group, refind: true) { create(:group, :private, :owner_subgroup_creation_only) }
 
   let(:guest_permissions) do