Merge branch '34525-update-custom-dashboard' into 'master'

Update file content of an existing custom dashboard

Closes #34525

See merge request gitlab-org/gitlab!25024

app/controllers/projects/performance_monitoring/dashboards_controller.rb

@@ -22,6 +22,16 @@ module Projects
         end
       end
 
+      def update
+        result = ::Metrics::Dashboard::UpdateDashboardService.new(project, current_user, dashboard_params.merge(file_content_params)).execute
+
+        if result[:status] == :success
+          respond_update_success(result)
+        else
+          respond_error(result)
+        end
+      end
+
       private
 
       def respond_success(result)
@@ -43,6 +53,19 @@ module Projects
         flash[:notice] = message.html_safe
       end
 
+      def respond_update_success(result)
+        set_web_ide_link_update_notice(result.dig(:dashboard, :path))
+        respond_to do |format|
+          format.json { render status: result.delete(:http_status), json: result }
+        end
+      end
+
+      def set_web_ide_link_update_notice(new_dashboard_path)
+        web_ide_link_start = "<a href=\"#{ide_edit_path(project, redirect_safe_branch_name, new_dashboard_path)}\">"
+        message = _("Your dashboard has been updated. You can %{web_ide_link_start}edit it here%{web_ide_link_end}.") % { web_ide_link_start: web_ide_link_start, web_ide_link_end: "</a>" }
+        flash[:notice] = message.html_safe
+      end
+
       def validate_required_params!
         params.require(%i(branch file_name dashboard commit_message))
       end
@@ -54,6 +77,31 @@ module Projects
       def dashboard_params
         params.permit(%i(branch file_name dashboard commit_message)).to_h
       end
+
+      def file_content_params
+        params.permit(
+          file_content: [
+            :dashboard,
+            panel_groups: [
+              :group,
+              :priority,
+              panels: [
+                :type,
+                :title,
+                :y_label,
+                :weight,
+                metrics: [
+                  :id,
+                  :unit,
+                  :label,
+                  :query,
+                  :query_range
+                ]
+              ]
+            ]
+          ]
+        )
+      end
     end
   end
 end

app/services/metrics/dashboard/update_dashboard_service.rb

+# frozen_string_literal: true
+
+# Updates the content of a specified dashboard in .yml file inside `.gitlab/dashboards`
+module Metrics
+  module Dashboard
+    class UpdateDashboardService < ::BaseService
+      ALLOWED_FILE_TYPE = '.yml'
+      USER_DASHBOARDS_DIR = ::Metrics::Dashboard::ProjectDashboardService::DASHBOARD_ROOT
+
+      def execute
+        catch(:error) do
+          throw(:error, error(_(%q(You can't commit to this project)), :forbidden)) unless push_authorized?
+
+          result = ::Files::UpdateService.new(project, current_user, dashboard_attrs).execute
+          throw(:error, result.merge(http_status: :bad_request)) unless result[:status] == :success
+
+          success(result.merge(http_status: :created, dashboard: dashboard_details))
+        end
+      end
+
+      private
+
+      def dashboard_attrs
+        {
+          commit_message: params[:commit_message],
+          file_path: update_dashboard_path,
+          file_content: update_dashboard_content,
+          encoding: 'text',
+          branch_name: branch,
+          start_branch: repository.branch_exists?(branch) ? branch : project.default_branch
+        }
+      end
+
+      def dashboard_details
+        {
+          path: update_dashboard_path,
+          display_name: ::Metrics::Dashboard::ProjectDashboardService.name_for_path(update_dashboard_path),
+          default: false,
+          system_dashboard: false
+        }
+      end
+
+      def push_authorized?
+        Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch)
+      end
+
+      def branch
+        @branch ||= begin
+          throw(:error, error(_('There was an error updating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name?
+          throw(:error, error(_('There was an error updating the dashboard, branch named: %{branch} already exists.') % { branch: params[:branch] }, :bad_request)) unless new_or_default_branch?
+
+          params[:branch]
+        end
+      end
+
+      def new_or_default_branch?
+        !repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch]
+      end
+
+      def valid_branch_name?
+        Gitlab::GitRefValidator.validate(params[:branch])
+      end
+
+      def update_dashboard_path
+        File.join(USER_DASHBOARDS_DIR, file_name)
+      end
+
+      def target_file_type_valid?
+        File.extname(params[:file_name]) == ALLOWED_FILE_TYPE
+      end
+
+      def file_name
+        @file_name ||= begin
+          throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid?
+
+          File.basename(CGI.unescape(params[:file_name]))
+        end
+      end
+
+      def update_dashboard_content
+        ::PerformanceMonitoring::PrometheusDashboard.from_json(params[:file_content]).to_yaml
+      end
+
+      def repository
+        @repository ||= project.repository
+      end
+    end
+  end
+end

changelogs/unreleased/34525-update-custom-dashboard.yml

+---
+title: Update file content of an existing custom dashboard
+merge_request: 25024
+author:
+type: added

config/routes/project.rb

@@ -252,7 +252,11 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         end
 
         namespace :performance_monitoring do
-          resources :dashboards, only: [:create]
+          resources :dashboards, only: [:create] do
+            collection do
+              put '/:file_name', to: 'dashboards#update', constraints: { file_name: /.+\.yml/ }
+            end
+          end
         end
 
         namespace :error_tracking do

locale/gitlab.pot

@@ -19630,6 +19630,12 @@ msgstr ""
 msgid "There was an error trying to validate your query"
 msgstr ""
 
+msgid "There was an error updating the dashboard, branch name is invalid."
+msgstr ""
+
+msgid "There was an error updating the dashboard, branch named: %{branch} already exists."
+msgstr ""
+
 msgid "There was an error when reseting email token."
 msgstr ""
 
@@ -22689,6 +22695,9 @@ msgstr ""
 msgid "Your dashboard has been copied. You can %{web_ide_link_start}edit it here%{web_ide_link_end}."
 msgstr ""
 
+msgid "Your dashboard has been updated. You can %{web_ide_link_start}edit it here%{web_ide_link_end}."
+msgstr ""
+
 msgid "Your deployment services will be broken, you will need to manually fix the services after renaming."
 msgstr ""
 

spec/controllers/projects/performance_monitoring/dashboards_controller_spec.rb

@@ -129,4 +129,130 @@ describe Projects::PerformanceMonitoring::DashboardsController do
       end
     end
   end
+
+  describe 'PUT #update' do
+    context 'authenticated user' do
+      before do
+        sign_in(user)
+      end
+
+      let(:file_content) do
+        {
+          "dashboard" => "Dashboard Title",
+          "panel_groups" => [{
+            "group" => "Group Title",
+            "panels" => [{
+              "type" => "area-chart",
+              "title" => "Chart Title",
+              "y_label" => "Y-Axis",
+              "metrics" => [{
+                "id" => "metric_of_ages",
+                "unit" => "count",
+                "label" => "Metric of Ages",
+                "query_range" => "http_requests_total"
+              }]
+            }]
+          }]
+        }
+      end
+
+      let(:params) do
+        {
+          namespace_id: namespace,
+          project_id: project,
+          dashboard: dashboard,
+          file_name: file_name,
+          file_content: file_content,
+          commit_message: commit_message,
+          branch: branch_name,
+          format: :json
+        }
+      end
+
+      context 'project with repository feature' do
+        context 'with rights to push to the repository' do
+          before do
+            project.add_maintainer(user)
+          end
+
+          context 'valid parameters' do
+            context 'request format json' do
+              let(:update_dashboard_service_params) { params.except(:namespace_id, :project_id, :format) }
+
+              let(:update_dashboard_service_results) do
+                {
+                  status: :success,
+                  http_status: :created,
+                  dashboard: {
+                    path: ".gitlab/dashboards/custom_dashboard.yml",
+                    display_name: "custom_dashboard.yml",
+                    default: false,
+                    system_dashboard: false
+                  }
+                }
+              end
+
+              let(:update_dashboard_service) { instance_double(::Metrics::Dashboard::UpdateDashboardService, execute: update_dashboard_service_results) }
+
+              it 'returns path to new file' do
+                allow(controller).to receive(:repository).and_return(repository)
+                allow(repository).to receive(:find_branch).and_return(branch)
+                allow(::Metrics::Dashboard::UpdateDashboardService).to receive(:new).with(project, user, update_dashboard_service_params).and_return(update_dashboard_service)
+
+                put :update, params: params
+
+                expect(response).to have_gitlab_http_status :created
+                expect(response).to set_flash[:notice].to eq("Your dashboard has been updated. You can <a href=\"/-/ide/project/#{namespace.path}/#{project.name}/edit/#{branch_name}/-/.gitlab/dashboards/#{file_name}\">edit it here</a>.")
+                expect(json_response).to eq('status' => 'success', 'dashboard' => { 'default' => false, 'display_name' => "custom_dashboard.yml", 'path' => ".gitlab/dashboards/#{file_name}", 'system_dashboard' => false })
+              end
+
+              context 'UpdateDashboardService failure' do
+                it 'returns json with failure message' do
+                  allow(::Metrics::Dashboard::UpdateDashboardService).to receive(:new).and_return(double(execute: { status: :error, message: 'something went wrong', http_status: :bad_request }))
+
+                  put :update, params: params
+
+                  expect(response).to have_gitlab_http_status :bad_request
+                  expect(json_response).to eq('error' => 'something went wrong')
+                end
+              end
+            end
+          end
+
+          context 'missing branch' do
+            let(:branch_name) { nil }
+
+            it 'raises responds with :bad_request status code and error message' do
+              put :update, params: params
+
+              expect(response).to have_gitlab_http_status :bad_request
+              expect(json_response).to eq('error' => "Request parameter branch is missing.")
+            end
+          end
+        end
+
+        context 'without rights to push to repository' do
+          before do
+            project.add_guest(user)
+          end
+
+          it 'responds with :forbidden status code' do
+            put :update, params: params
+
+            expect(response).to have_gitlab_http_status :forbidden
+          end
+        end
+      end
+
+      context 'project without repository feature' do
+        let!(:project) { create(:project, name: 'dashboard-project', namespace: namespace) }
+
+        it 'responds with :not_found status code' do
+          put :update, params: params
+
+          expect(response).to have_gitlab_http_status :not_found
+        end
+      end
+    end
+  end
 end

spec/services/metrics/dashboard/update_dashboard_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Metrics::Dashboard::UpdateDashboardService, :use_clean_rails_memory_store_caching do
+  include MetricsDashboardHelpers
+
+  set(:user) { create(:user) }
+  set(:project) { create(:project, :repository) }
+  set(:environment) { create(:environment, project: project) }
+
+  describe '#execute' do
+    subject(:service_call) { described_class.new(project, user, params).execute }
+
+    let(:commit_message) { 'test' }
+    let(:branch) { 'dashboard_new_branch' }
+    let(:dashboard) { 'config/prometheus/common_metrics.yml' }
+    let(:file_name) { 'custom_dashboard.yml' }
+    let(:file_content_hash) { YAML.safe_load(File.read(dashboard)) }
+    let(:params) do
+      {
+        file_name: file_name,
+        file_content: file_content_hash,
+        commit_message: commit_message,
+        branch: branch
+      }
+    end
+
+    context 'user does not have push right to repository' do
+      it_behaves_like 'misconfigured dashboard service response', :forbidden, "You can't commit to this project"
+    end
+
+    context 'with rights to push to the repository' do
+      before do
+        project.add_maintainer(user)
+      end
+
+      context 'path traversal attack attempt' do
+        context 'with a yml extension' do
+          let(:file_name) { 'config/prometheus/../database.yml' }
+
+          it_behaves_like 'misconfigured dashboard service response', :bad_request, "A file with this name doesn't exist"
+        end
+
+        context 'without a yml extension' do
+          let(:file_name) { '../../..../etc/passwd' }
+
+          it_behaves_like 'misconfigured dashboard service response', :bad_request, "The file name should have a .yml extension"
+        end
+      end
+
+      context 'valid parameters' do
+        it_behaves_like 'valid dashboard update process'
+      end
+
+      context 'selected branch already exists' do
+        let(:branch) { 'existing_branch' }
+
+        before do
+          project.repository.add_branch(user, branch, 'master')
+        end
+
+        it_behaves_like 'misconfigured dashboard service response', :bad_request, "There was an error updating the dashboard, branch named: existing_branch already exists."
+      end
+
+      context 'Files::UpdateService success' do
+        before do
+          allow(::Files::UpdateService).to receive(:new).and_return(double(execute: { status: :success }))
+        end
+
+        it 'returns success', :aggregate_failures do
+          dashboard_details = {
+            path: '.gitlab/dashboards/custom_dashboard.yml',
+            display_name: 'custom_dashboard.yml',
+            default: false,
+            system_dashboard: false
+          }
+
+          expect(service_call[:status]).to be :success
+          expect(service_call[:http_status]).to be :created
+          expect(service_call[:dashboard]).to match dashboard_details
+        end
+
+        context 'with escaped characters in file name' do
+          let(:file_name) { "custom_dashboard%26copy.yml" }
+
+          it 'escapes the special characters', :aggregate_failures do
+            dashboard_details = {
+              path: '.gitlab/dashboards/custom_dashboard&copy.yml',
+              display_name: 'custom_dashboard&copy.yml',
+              default: false,
+              system_dashboard: false
+            }
+
+            expect(service_call[:status]).to be :success
+            expect(service_call[:http_status]).to be :created
+            expect(service_call[:dashboard]).to match dashboard_details
+          end
+        end
+      end
+
+      context 'Files::UpdateService fails' do
+        before do
+          allow(::Files::UpdateService).to receive(:new).and_return(double(execute: { status: :error }))
+        end
+
+        it 'returns error' do
+          expect(service_call[:status]).to be :error
+        end
+      end
+    end
+  end
+end

spec/support/shared_examples/services/metrics/dashboard_shared_examples.rb

@@ -85,3 +85,24 @@ RSpec.shared_examples 'valid dashboard cloning process' do |dashboard_template,
     end
   end
 end
+
+RSpec.shared_examples 'valid dashboard update process' do
+  let(:dashboard_attrs) do
+    {
+      commit_message: commit_message,
+      branch_name: branch,
+      start_branch: project.default_branch,
+      encoding: 'text',
+      file_path: ".gitlab/dashboards/#{file_name}",
+      file_content: ::PerformanceMonitoring::PrometheusDashboard.from_json(file_content_hash).to_yaml
+    }
+  end
+
+  it 'delegates commit creation to Files::UpdateService', :aggregate_failures do
+    service_instance = instance_double(::Files::UpdateService)
+    expect(::Files::UpdateService).to receive(:new).with(project, user, dashboard_attrs).and_return(service_instance)
+    expect(service_instance).to receive(:execute).and_return(status: :success)
+
+    service_call
+  end
+end