Merge branch '33876-ensure-proper-access-level-check-on-pa' into 'master'

Ensure proper access level to productivity analytics

Closes #34050 and #33876

See merge request gitlab-org/gitlab!18662

changelogs/unreleased/33876-ensure-proper-access-level-check-on-pa.yml

+---
+title: Allow to view productivity analytics page without a license
+merge_request: 33876
+author:
+type: fixed

ee/app/controllers/analytics/application_controller.rb

@@ -21,8 +21,7 @@ class Analytics::ApplicationController < ApplicationController
 
   def check_feature_availability!(feature)
     return render_403 unless ::License.feature_available?(feature)
-    return unless @group
-    return render_403 unless @group.root_ancestor.feature_available?(feature)
+    return render_403 unless @group && @group.root_ancestor.feature_available?(feature)
   end
 
   def load_group

ee/app/controllers/analytics/productivity_analytics_controller.rb

@@ -9,7 +9,8 @@ class Analytics::ProductivityAnalyticsController < Analytics::ApplicationControl
   before_action :load_project
   before_action -> {
     check_feature_availability!(:productivity_analytics)
-  }
+  }, if: -> { request.format.json? }
+
   before_action -> {
     authorize_view_by_action!(:view_productivity_analytics)
   }

ee/spec/controllers/analytics/productivity_analytics_controller_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe Analytics::ProductivityAnalyticsController do
   let(:current_user) { create(:user) }
+  let(:group) { create :group }
 
   before do
     sign_in(current_user) if current_user
@@ -12,8 +13,6 @@ describe Analytics::ProductivityAnalyticsController do
   end
 
   describe 'usage counter' do
-    let(:group) { create :group }
-
     before do
       group.add_owner(current_user)
     end
@@ -29,7 +28,7 @@ describe Analytics::ProductivityAnalyticsController do
     it "doesn't increment the usage counter when JSON request is sent" do
       expect(Gitlab::UsageDataCounters::ProductivityAnalyticsCounter).not_to receive(:count).with(:views)
 
-      get :show, format: :json
+      get :show, format: :json, params: { group_id: group }
 
       expect(response).to be_successful
     end
@@ -38,14 +37,6 @@ describe Analytics::ProductivityAnalyticsController do
   describe 'GET show' do
     subject { get :show }
 
-    it 'checks for premium license' do
-      stub_licensed_features(productivity_analytics: false)
-
-      subject
-
-      expect(response).to have_gitlab_http_status(403)
-    end
-
     it 'authorizes for ability to view analytics' do
       expect(Ability).to receive(:allowed?).with(current_user, :view_productivity_analytics, :global).and_return(false)
 
@@ -54,7 +45,9 @@ describe Analytics::ProductivityAnalyticsController do
       expect(response).to have_gitlab_http_status(403)
     end
 
-    it 'renders show template' do
+    it 'renders show template regardless of license' do
+      stub_licensed_features(productivity_analytics: false)
+
       subject
 
       expect(response).to be_successful
@@ -86,25 +79,49 @@ describe Analytics::ProductivityAnalyticsController do
               .and_return(analytics_mock)
     end
 
+    it 'checks for premium license' do
+      stub_licensed_features(productivity_analytics: false)
+
+      subject
+
+      expect(response).to have_gitlab_http_status(403)
+    end
+
+    context 'without group_id specified' do
+      it 'returns 403' do
+        subject
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
     context 'with non-existing group_id' do
       let(:params) { { group_id: 'SOMETHING_THAT_DOES_NOT_EXIST' } }
 
       it 'renders 404' do
         subject
+
         expect(response).to have_gitlab_http_status(404)
       end
     end
 
     context 'with non-existing project_id' do
-      let(:group) { create :group }
-      let(:params) { { group_id: group.full_path, project_id: 'SOMETHING_THAT_DOES_NOT_EXIST' } }
+      let(:params) { { group_id: group, project_id: 'SOMETHING_THAT_DOES_NOT_EXIST' } }
 
       it 'renders 404' do
         subject
+
         expect(response).to have_gitlab_http_status(404)
       end
     end
 
+    context 'with group specified' do
+      let(:params) { { group_id: group } }
+
+      before do
+        group.add_owner(current_user)
+      end
+
       context 'for list of MRs' do
         let!(:merge_request ) { create :merge_request, :merged}
 
@@ -137,7 +154,8 @@ describe Analytics::ProductivityAnalyticsController do
       end
 
       context 'for scatterplot charts' do
-      let(:params) { { chart_type: 'scatterplot', metric_type: 'commits_count' } }
+        let(:params) { super().merge({ chart_type: 'scatterplot', metric_type: 'commits_count' }) }
+
         it 'renders whatever analytics returns for scatterplot' do
           allow(analytics_mock).to receive(:scatterplot_data).with(type: 'commits_count').and_return('scatterplot_data')
 
@@ -148,7 +166,8 @@ describe Analytics::ProductivityAnalyticsController do
       end
 
       context 'for histogram charts' do
-      let(:params) { { chart_type: 'histogram', metric_type: 'commits_count' } }
+        let(:params) { super().merge({ chart_type: 'histogram', metric_type: 'commits_count' }) }
+
         it 'renders whatever analytics returns for histogram' do
           allow(analytics_mock).to receive(:histogram_data).with(type: 'commits_count').and_return('histogram_data')
 
@@ -158,4 +177,5 @@ describe Analytics::ProductivityAnalyticsController do
         end
       end
     end
+  end
 end