Merge branch 'add-metrics-dashboard-permission-check' into 'master'

Add permission check to metrics dashboards

See merge request gitlab-org/gitlab-ce!30017

7 files changed, 48 insertions, 8 deletions

diff --git a/changelogs/unreleased/add-metrics-dashboard-permission-check.yml b/changelogs/unreleased/add-metrics-dashboard-permission-check.yml
new file mode 100644
index 00000000000..0ea2c4c8e41
--- /dev/null
+++ b/changelogs/unreleased/add-metrics-dashboard-permission-check.yml
@@ -0,0 +1,5 @@
+---
+title: Add permission check to metrics dashboards endpoint
+merge_request: 30017
+author:
+type: added
diff --git a/lib/gitlab/metrics/dashboard/base_service.rb b/lib/gitlab/metrics/dashboard/base_service.rb
index 90895eb237a..0628e82e592 100644
--- a/lib/gitlab/metrics/dashboard/base_service.rb
+++ b/lib/gitlab/metrics/dashboard/base_service.rb
@@ -10,6 +10,8 @@ module Gitlab
         NOT_FOUND_ERROR = Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError
 
         def get_dashboard
+          return error('Insufficient permissions.', :unauthorized) unless allowed?
+
           success(dashboard: process_dashboard)
         rescue NOT_FOUND_ERROR
           error("#{dashboard_path} could not be found.", :not_found)
@@ -30,6 +32,12 @@ module Gitlab
 
         private
 
+        # Determines whether users should be able to view
+        # dashboards at all.
+        def allowed?
+          Ability.allowed?(current_user, :read_environment, project)
+        end
+
         # Returns a new dashboard Hash, supplemented with DB info
         def process_dashboard
           Gitlab::Metrics::Dashboard::Processor
diff --git a/spec/lib/gitlab/metrics/dashboard/dynamic_dashboard_service_spec.rb b/spec/lib/gitlab/metrics/dashboard/dynamic_dashboard_service_spec.rb
index eecd257b38d..79a78df44ae 100644
--- a/spec/lib/gitlab/metrics/dashboard/dynamic_dashboard_service_spec.rb
+++ b/spec/lib/gitlab/metrics/dashboard/dynamic_dashboard_service_spec.rb
@@ -6,13 +6,19 @@ describe Gitlab::Metrics::Dashboard::DynamicDashboardService, :use_clean_rails_m
   include MetricsDashboardHelpers
 
   set(:project) { build(:project) }
+  set(:user) { create(:user) }
   set(:environment) { create(:environment, project: project) }
 
+  before do
+    project.add_maintainer(user)
+  end
+
   describe '#get_dashboard' do
-    let(:service_params) { [project, nil, { environment: environment, dashboard_path: nil }] }
+    let(:service_params) { [project, user, { environment: environment, dashboard_path: nil }] }
     let(:service_call) { described_class.new(*service_params).get_dashboard }
 
     it_behaves_like 'valid embedded dashboard service response'
+    it_behaves_like 'raises error for users with insufficient permissions'
 
     it 'caches the unprocessed dashboard for subsequent calls' do
       expect(YAML).to receive(:safe_load).once.and_call_original
diff --git a/spec/lib/gitlab/metrics/dashboard/finder_spec.rb b/spec/lib/gitlab/metrics/dashboard/finder_spec.rb
index b9a5ee9c2b3..d8ed54c0248 100644
--- a/spec/lib/gitlab/metrics/dashboard/finder_spec.rb
+++ b/spec/lib/gitlab/metrics/dashboard/finder_spec.rb
@@ -6,12 +6,17 @@ describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store_cachi
   include MetricsDashboardHelpers
 
   set(:project) { build(:project) }
+  set(:user) { create(:user) }
   set(:environment) { create(:environment, project: project) }
   let(:system_dashboard_path) { Gitlab::Metrics::Dashboard::SystemDashboardService::SYSTEM_DASHBOARD_PATH}
 
+  before do
+    project.add_maintainer(user)
+  end
+
   describe '.find' do
     let(:dashboard_path) { '.gitlab/dashboards/test.yml' }
-    let(:service_call) { described_class.find(project, nil, environment, dashboard_path: dashboard_path) }
+    let(:service_call) { described_class.find(project, user, environment, dashboard_path: dashboard_path) }
 
     it_behaves_like 'misconfigured dashboard service response', :not_found
 
@@ -41,13 +46,13 @@ describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store_cachi
     end
 
     context 'when no dashboard is specified' do
-      let(:service_call) { described_class.find(project, nil, environment) }
+      let(:service_call) { described_class.find(project, user, environment) }
 
       it_behaves_like 'valid dashboard service response'
     end
 
     context 'when the dashboard is expected to be embedded' do
-      let(:service_call) { described_class.find(project, nil, environment, dashboard_path: nil, embedded: true) }
+      let(:service_call) { described_class.find(project, user, environment, dashboard_path: nil, embedded: true) }
 
       it_behaves_like 'valid embedded dashboard service response'
     end
diff --git a/spec/lib/gitlab/metrics/dashboard/project_dashboard_service_spec.rb b/spec/lib/gitlab/metrics/dashboard/project_dashboard_service_spec.rb
index 57d82421b5d..468e8ec9ef2 100644
--- a/spec/lib/gitlab/metrics/dashboard/project_dashboard_service_spec.rb
+++ b/spec/lib/gitlab/metrics/dashboard/project_dashboard_service_spec.rb
@@ -5,8 +5,8 @@ require 'rails_helper'
 describe Gitlab::Metrics::Dashboard::ProjectDashboardService, :use_clean_rails_memory_store_caching do
   include MetricsDashboardHelpers
 
-  set(:user) { build(:user) }
-  set(:project) { build(:project) }
+  set(:user) { create(:user) }
+  set(:project) { create(:project) }
   set(:environment) { create(:environment, project: project) }
 
   before do
@@ -22,6 +22,8 @@ describe Gitlab::Metrics::Dashboard::ProjectDashboardService, :use_clean_rails_m
       it_behaves_like 'misconfigured dashboard service response', :not_found
     end
 
+    it_behaves_like 'raises error for users with insufficient permissions'
+
     context 'when the dashboard exists' do
       let(:project) { project_with_dashboard(dashboard_path) }
 
diff --git a/spec/lib/gitlab/metrics/dashboard/system_dashboard_service_spec.rb b/spec/lib/gitlab/metrics/dashboard/system_dashboard_service_spec.rb
index 2af745bd4d7..13f22dd01c5 100644
--- a/spec/lib/gitlab/metrics/dashboard/system_dashboard_service_spec.rb
+++ b/spec/lib/gitlab/metrics/dashboard/system_dashboard_service_spec.rb
@@ -5,15 +5,21 @@ require 'spec_helper'
 describe Gitlab::Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memory_store_caching do
   include MetricsDashboardHelpers
 
-  set(:project) { build(:project) }
+  set(:user) { create(:user) }
+  set(:project) { create(:project) }
   set(:environment) { create(:environment, project: project) }
 
+  before do
+    project.add_maintainer(user)
+  end
+
   describe 'get_dashboard' do
     let(:dashboard_path) { described_class::SYSTEM_DASHBOARD_PATH }
-    let(:service_params) { [project, nil, { environment: environment, dashboard_path: dashboard_path }] }
+    let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
     let(:service_call) { described_class.new(*service_params).get_dashboard }
 
     it_behaves_like 'valid dashboard service response'
+    it_behaves_like 'raises error for users with insufficient permissions'
 
     it 'caches the unprocessed dashboard for subsequent calls' do
       expect(YAML).to receive(:safe_load).once.and_call_original
diff --git a/spec/support/helpers/metrics_dashboard_helpers.rb b/spec/support/helpers/metrics_dashboard_helpers.rb
index 6de00eea474..1511a2f6b49 100644
--- a/spec/support/helpers/metrics_dashboard_helpers.rb
+++ b/spec/support/helpers/metrics_dashboard_helpers.rb
@@ -50,4 +50,12 @@ module MetricsDashboardHelpers
 
     it_behaves_like 'valid dashboard service response for schema'
   end
+
+  shared_examples_for 'raises error for users with insufficient permissions' do
+    context 'when the user does not have sufficient access' do
+      let(:user) { build(:user) }
+
+      it_behaves_like 'misconfigured dashboard service response', :unauthorized
+    end
+  end
 end