Refactor project transfer service. Add security check when create or transfer project into group

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

3 files changed, 63 insertions, 60 deletions

diff --git a/app/services/project_transfer_service.rb b/app/services/project_transfer_service.rb
deleted file mode 100644
index 7055ef32aee..00000000000
--- a/app/services/project_transfer_service.rb
+++ /dev/null
@@ -1,46 +0,0 @@
-# ProjectTransferService class
-#
-# Used for transfer project to another namespace
-#
-class ProjectTransferService
-  include Gitlab::ShellAdapter
-
-  class TransferError < StandardError; end
-
-  attr_accessor :project
-
-  def transfer(project, new_namespace)
-    Project.transaction do
-      old_path = project.path_with_namespace
-      new_path = File.join(new_namespace.try(:path) || '', project.path)
-
-      if Project.where(path: project.path, namespace_id: new_namespace.try(:id)).present?
-        raise TransferError.new("Project with same path in target namespace already exists")
-      end
-
-      # Remove old satellite
-      project.satellite.destroy
-
-      # Apply new namespace id
-      project.namespace = new_namespace
-      project.save!
-
-      # Move main repository
-      unless gitlab_shell.mv_repository(old_path, new_path)
-        raise TransferError.new('Cannot move project')
-      end
-
-      # Move wiki repo also if present
-      gitlab_shell.mv_repository("#{old_path}.wiki", "#{new_path}.wiki")
-
-      # Create a new satellite (reload project from DB)
-      Project.find(project.id).ensure_satellite_exists
-
-      # clear project cached events
-      project.reset_events_cache
-
-      true
-    end
-  end
-end
-
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index 11b65256502..7d322f25e44 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -97,7 +97,7 @@ module Projects
 
     def allowed_namespace?(user, namespace_id)
       namespace = Namespace.find_by(id: namespace_id)
-      current_user.can?(:manage_namespace, namespace)
+      current_user.can?(:create_projects, namespace)
     end
   end
 end
diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb
index f8e27f6f1c6..d115e92a105 100644
--- a/app/services/projects/transfer_service.rb
+++ b/app/services/projects/transfer_service.rb
@@ -1,22 +1,71 @@
+# Projects::TransferService class
+#
+# Used for transfer project to another namespace
+#
+# Ex.
+#   # Move projects to namespace with ID 17 by user
+#   Projects::TransferService.new(project, user, namespace_id: 17).execute
+#
 module Projects
   class TransferService < BaseService
-    def execute(role = :default)
-      namespace_id = params[:project].delete(:namespace_id)
-      allowed_transfer = can?(current_user, :change_namespace, project) || role == :admin
-
-      if allowed_transfer && namespace_id.present?
-        if namespace_id.to_i != project.namespace_id
-          # Transfer to someone namespace
-          namespace = Namespace.find(namespace_id)
-          project.transfer(namespace)
-        end
-      end
+    include Gitlab::ShellAdapter
+    class TransferError < StandardError; end
+
+    def execute
+      namespace_id = params.delete(:namespace_id)
+      namespace = Namespace.find_by(id: namespace_id)
 
-    rescue ProjectTransferService::TransferError => ex
+      if allowed_transfer?(current_user, project, namespace)
+        transfer(project, namespace)
+      else
+        project.errors.add(:namespace, 'is invalid')
+        false
+      end
+    rescue Projects::TransferService::TransferError => ex
       project.reload
       project.errors.add(:namespace_id, ex.message)
       false
     end
+
+    def transfer(project, new_namespace)
+      Project.transaction do
+        old_path = project.path_with_namespace
+        new_path = File.join(new_namespace.try(:path) || '', project.path)
+
+        if Project.where(path: project.path, namespace_id: new_namespace.try(:id)).present?
+          raise TransferError.new("Project with same path in target namespace already exists")
+        end
+
+        # Remove old satellite
+        project.satellite.destroy
+
+        # Apply new namespace id
+        project.namespace = new_namespace
+        project.save!
+
+        # Move main repository
+        unless gitlab_shell.mv_repository(old_path, new_path)
+          raise TransferError.new('Cannot move project')
+        end
+
+        # Move wiki repo also if present
+        gitlab_shell.mv_repository("#{old_path}.wiki", "#{new_path}.wiki")
+
+        # Create a new satellite (reload project from DB)
+        Project.find(project.id).ensure_satellite_exists
+
+        # clear project cached events
+        project.reset_events_cache
+
+        true
+      end
+    end
+
+    def allowed_transfer?(current_user, project, namespace)
+      namespace &&
+        can?(current_user, :change_namespace, project) &&
+        namespace.id != project.namespace_id &&
+        current_user.can?(:create_projects, namespace)
+    end
   end
 end
-