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 0000000000000000000000000000000000000000..0ea2c4c8e41acc460616207147988479f99a7c4f --- /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 90895eb237adeb1922a56677417bb2be5db784e1..0628e82e592cd0e6961d14ac25209808639e6db9 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 eecd257b38ddab445b509935bc3da41092f3dcce..79a78df44ae434d45ec02d8df45fae5e028c5bf2 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 b9a5ee9c2b3e87358e706923e1326b6c532f781a..d8ed54c02485ed44348089a2d36b247e1c3a7851 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 57d82421b5d3c3ab8154f0cce8ae5d93f9169580..468e8ec9ef2c9c87cb34750168d693a6984088a1 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 2af745bd4d73aae46454b79ea567fadd756f0751..13f22dd01c560f10963eba5ac5777c71da0dc83e 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 6de00eea47469312b972476e7c2c6cd4c94dbed2..1511a2f6b492083dea70592dd828e9b191cb7fd3 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