Extract permission validation and re-use for creation

In order to safely expose management_project_id on create we need to
apply the same validations as we do on update.

app/services/clusters/create_service.rb

@@ -23,6 +23,8 @@ module Clusters
         cluster.errors.add(:base, _('Instance does not support multiple Kubernetes clusters'))
       end
 
+      validate_management_project_permissions(cluster)
+
       return cluster if cluster.errors.present?
 
       cluster.tap do |cluster|
@@ -57,6 +59,11 @@ module Clusters
     def can_create_cluster?
       clusterable.clusters.empty?
     end
+
+    def validate_management_project_permissions(cluster)
+      Clusters::Management::ValidatePermissionsService.new(current_user)
+        .execute(cluster, params[:management_project_id])
+    end
   end
 end
 

app/services/clusters/management/validate_permissions_service.rb

+# frozen_string_literal: true
+
+module Clusters
+  module Management
+    class ValidatePermissionsService
+      attr_reader :current_user
+
+      def initialize(user = nil)
+        @current_user = user
+      end
+
+      def execute(cluster, management_project_id)
+        if management_project_id.present?
+          management_project = management_project_scope(cluster).find_by_id(management_project_id)
+
+          unless management_project
+            cluster.errors.add(:management_project_id, _('Project does not exist or you don\'t have permission to perform this action'))
+
+            return false
+          end
+
+          unless can_admin_pipeline_for_project?(management_project)
+            # Use same message as not found to prevent enumeration
+            cluster.errors.add(:management_project_id, _('Project does not exist or you don\'t have permission to perform this action'))
+
+            return false
+          end
+        end
+
+        true
+      end
+
+      private
+
+      def can_admin_pipeline_for_project?(project)
+        Ability.allowed?(current_user, :admin_pipeline, project)
+      end
+
+      def management_project_scope(cluster)
+        return ::Project.all if cluster.instance_type?
+
+        group =
+          if cluster.group_type?
+            cluster.first_group
+          elsif cluster.project_type?
+            cluster.first_project&.namespace
+          end
+
+        # Prevent users from selecting nested projects until
+        # https://gitlab.com/gitlab-org/gitlab/issues/34650 is resolved
+        include_subgroups = cluster.group_type?
+
+        ::GroupProjectsFinder.new(group: group, current_user: current_user, options: { only_owned: true, include_subgroups: include_subgroups }).execute
+      end
+    end
+  end
+end

app/services/clusters/update_service.rb

@@ -18,46 +18,9 @@ module Clusters
 
     private
 
-    def can_admin_pipeline_for_project?(project)
-      Ability.allowed?(current_user, :admin_pipeline, project)
-    end
-
     def validate_params(cluster)
-      if params[:management_project_id].present?
-        management_project = management_project_scope(cluster).find_by_id(params[:management_project_id])
-
-        unless management_project
-          cluster.errors.add(:management_project_id, _('Project does not exist or you don\'t have permission to perform this action'))
-
-          return false
-        end
-
-        unless can_admin_pipeline_for_project?(management_project)
-          # Use same message as not found to prevent enumeration
-          cluster.errors.add(:management_project_id, _('Project does not exist or you don\'t have permission to perform this action'))
-
-          return false
-        end
-      end
-
-      true
-    end
-
-    def management_project_scope(cluster)
-      return ::Project.all if cluster.instance_type?
-
-      group =
-        if cluster.group_type?
-          cluster.first_group
-        elsif cluster.project_type?
-          cluster.first_project&.namespace
-        end
-
-      # Prevent users from selecting nested projects until
-      # https://gitlab.com/gitlab-org/gitlab/issues/34650 is resolved
-      include_subgroups = cluster.group_type?
-
-      ::GroupProjectsFinder.new(group: group, current_user: current_user, options: { only_owned: true, include_subgroups: include_subgroups }).execute
+      ::Clusters::Management::ValidatePermissionsService.new(current_user)
+        .execute(cluster, params[:management_project_id])
     end
   end
 end

spec/requests/api/group_clusters_spec.rb

@@ -206,6 +206,7 @@ describe API::GroupClusters do
           expect(cluster_result.name).to eq('test-cluster')
           expect(cluster_result.domain).to eq('domain.example.com')
           expect(cluster_result.managed).to be_falsy
+          expect(cluster_result.management_project_id).to eq management_project_id
           expect(platform_kubernetes.rbac?).to be_truthy
           expect(platform_kubernetes.api_url).to eq(api_url)
           expect(platform_kubernetes.token).to eq('sample-token')
@@ -237,6 +238,18 @@ describe API::GroupClusters do
         end
       end
 
+      context 'current user does not have access to management_project_id' do
+        let(:management_project_id) { create(:project).id }
+
+        it 'responds with 400' do
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        it 'returns validation errors' do
+          expect(json_response['message']['management_project_id'].first).to match('don\'t have permission')
+        end
+      end
+
       context 'with invalid params' do
         let(:api_url) { 'invalid_api_url' }
 

spec/requests/api/project_clusters_spec.rb

@@ -153,6 +153,10 @@ describe API::ProjectClusters do
     let(:management_project) { create(:project, namespace: project.namespace) }
     let(:management_project_id) { management_project.id }
 
+    before do
+      management_project.add_maintainer(current_user)
+    end
+
     let(:platform_kubernetes_attributes) do
       {
         api_url: api_url,
@@ -197,6 +201,7 @@ describe API::ProjectClusters do
           expect(cluster_result.name).to eq('test-cluster')
           expect(cluster_result.domain).to eq('domain.example.com')
           expect(cluster_result.managed).to be_falsy
+          expect(cluster_result.management_project_id).to eq management_project_id
           expect(platform_kubernetes.rbac?).to be_truthy
           expect(platform_kubernetes.api_url).to eq(api_url)
           expect(platform_kubernetes.namespace).to eq(namespace)
@@ -230,6 +235,18 @@ describe API::ProjectClusters do
         end
       end
 
+      context 'current user does not have access to management_project_id' do
+        let(:management_project_id) { create(:project).id }
+
+        it 'responds with 400' do
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        it 'returns validation errors' do
+          expect(json_response['message']['management_project_id'].first).to match('don\'t have permission')
+        end
+      end
+
       context 'with invalid params' do
         let(:namespace) { 'invalid_namespace' }
 

spec/services/clusters/create_service_spec.rb

@@ -59,4 +59,92 @@ describe Clusters::CreateService do
       end
     end
   end
+
+  context 'when params includes :management_project_id' do
+    subject(:cluster) { described_class.new(user, params).execute(access_token: access_token) }
+
+    let(:params) do
+      {
+        name: 'test-cluster',
+        provider_type: :gcp,
+        provider_gcp_attributes: {
+          gcp_project_id: 'gcp-project',
+          zone: 'us-central1-a',
+          num_nodes: 1,
+          machine_type: 'machine_type-a',
+          legacy_abac: 'true'
+        },
+        clusterable: clusterable,
+        management_project_id: management_project_id
+      }
+    end
+
+    let(:clusterable) { project }
+    let(:management_project_id) { management_project.id }
+    let(:management_project_namespace) { project.namespace }
+    let(:management_project) { create(:project, namespace: management_project_namespace) }
+
+    context 'management_project is non-existent' do
+      let(:management_project_id) { 0 }
+
+      it 'does not persist the cluster' do
+        expect(cluster).not_to be_persisted
+
+        expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+      end
+    end
+
+    shared_examples 'setting a management project' do
+      context 'when user is authorized to adminster manangement_project' do
+        before do
+          management_project.add_maintainer(user)
+        end
+
+        it 'persists the cluster' do
+          expect(cluster).to be_persisted
+
+          expect(cluster.management_project).to eq(management_project)
+        end
+      end
+
+      context 'when user is not authorized to adminster manangement_project' do
+        it 'does not persist the cluster' do
+          expect(cluster).not_to be_persisted
+
+          expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+        end
+      end
+    end
+
+    shared_examples 'setting a management project outside of scope' do
+      context 'when manangement_project is outside of the namespace scope' do
+        let(:management_project_namespace) { create(:group) }
+
+        it 'does not persist the cluster' do
+          expect(cluster).not_to be_persisted
+
+          expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+        end
+      end
+    end
+
+    context 'project cluster' do
+      include_examples 'setting a management project'
+      include_examples 'setting a management project outside of scope'
+    end
+
+    context 'group cluster' do
+      let(:management_project_namespace) { create(:group) }
+      let(:clusterable) { management_project_namespace }
+
+      include_examples 'setting a management project'
+      include_examples 'setting a management project outside of scope'
+    end
+
+    context 'instance cluster' do
+      let(:clusterable) { Clusters::Instance.new }
+
+      include_examples 'setting a management project'
+    end
+  end
 end

spec/services/clusters/management/validate_permissions_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Clusters::Management::ValidatePermissionsService do
+  describe '#execute' do
+    subject { described_class.new(user).execute(cluster, management_project_id) }
+
+    let(:cluster) { build(:cluster, :project, projects: [create(:project)]) }
+    let(:user) { create(:user) }
+
+    context 'when management_project_id is nil' do
+      let(:management_project_id) { nil }
+
+      it { is_expected.to be true }
+    end
+
+    context 'when management_project_id is not nil' do
+      let(:management_project_id) { management_project.id }
+      let(:management_project_namespace) { create(:group) }
+      let(:management_project) { create(:project, namespace: management_project_namespace) }
+
+      context 'when management_project does not exist' do
+        let(:management_project_id) { 0 }
+
+        it 'adds errors to the cluster and returns false' do
+          is_expected.to eq false
+
+          expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+        end
+      end
+
+      shared_examples 'management project is in scope' do
+        context 'when user is authorized to administer manangement_project' do
+          before do
+            management_project.add_maintainer(user)
+          end
+
+          it 'adds no error and returns true' do
+            is_expected.to eq true
+
+            expect(cluster.errors).to be_empty
+          end
+        end
+
+        context 'when user is not authorized to adminster manangement_project' do
+          it 'adds an error and returns false' do
+            is_expected.to eq false
+
+            expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+          end
+        end
+      end
+
+      shared_examples 'management project is out of scope' do
+        context 'when manangement_project is outside of the namespace scope' do
+          let(:management_project_namespace) { create(:group) }
+
+          it 'adds an error and returns false' do
+            is_expected.to eq false
+
+            expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+          end
+        end
+      end
+
+      context 'project cluster' do
+        let(:cluster) { build(:cluster, :project, projects: [create(:project, namespace: management_project_namespace)]) }
+
+        include_examples 'management project is in scope'
+        include_examples 'management project is out of scope'
+      end
+
+      context 'group cluster' do
+        let(:cluster) { build(:cluster, :group, groups: [management_project_namespace]) }
+
+        include_examples 'management project is in scope'
+        include_examples 'management project is out of scope'
+      end
+
+      context 'instance cluster' do
+        let(:cluster) { build(:cluster, :instance) }
+
+        include_examples 'management project is in scope'
+      end
+    end
+  end
+end