Merge branch 'security-ag-contribution-analytics-2nd-attempt' into 'master'

Hide Contribution Analytics from non-group members

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

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