Move new project on push logic to a service

15 files changed, 186 insertions, 144 deletions

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 5660a9027d5..90a9079fab3 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -5,21 +5,13 @@ class Projects::GitHttpController < Projects::GitHttpClientController
 
   rescue_from Gitlab::GitAccess::UnauthorizedError, with: :render_403
   rescue_from Gitlab::GitAccess::NotFoundError, with: :render_404
+  rescue_from Gitlab::GitAccess::ProjectCreationError, with: :render_422
 
   # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull)
   # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
   def info_refs
     log_user_activity if upload_pack?
-
-    if user && project.blank? && receive_pack?
-      @project = ::Projects::CreateService.new(user, project_params).execute
-
-      if @project.saved?
-        Gitlab::Checks::NewProject.new(user, @project, 'http').add_new_project_message
-      else
-        raise Gitlab::GitAccess::NotFoundError, 'Could not create project'
-      end
-    end
+    create_new_project if receive_pack? && project.blank?
 
     render_ok
   end
@@ -31,8 +23,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController
 
   # POST /foo/bar.git/git-receive-pack" (git push)
   def git_receive_pack
-    raise Gitlab::GitAccess::NotFoundError, 'Could not create project' unless project
-
     render_ok
   end
 
@@ -58,6 +48,10 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     end
   end
 
+  def create_new_project
+    @project = ::Projects::CreateFromPushService.new(user, params[:project_id], namespace, 'http').execute
+  end
+
   def render_ok
     set_workhorse_internal_api_content_type
     render json: Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name)
@@ -71,6 +65,10 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     render plain: exception.message, status: :not_found
   end
 
+  def render_422(exception)
+    render plain: exception.message, status: :unprocessable_entity
+  end
+
   def access
     @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: namespace)
   end
@@ -90,17 +88,8 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     @access_klass ||= wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
   end
 
-  def project_params
-    {
-        description: "",
-        path: Project.parse_project_id(params[:project_id]),
-        namespace_id: namespace&.id,
-        visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s
-    }
-  end
-
   def namespace
-    @namespace ||= Namespace.find_by_path_or_name(params[:namespace_id])
+    @namespace ||= Namespace.find_by_full_path(params[:namespace_id])
   end
 
   def log_user_activity
diff --git a/app/models/project.rb b/app/models/project.rb
index a4d83b2a79a..12d5f28f5ea 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -468,10 +468,6 @@ class Project < ActiveRecord::Base
     def group_ids
       joins(:namespace).where(namespaces: { type: 'Group' }).select(:namespace_id)
     end
-
-    def parse_project_id(project_id)
-      project_id.gsub("\.git", '')
-    end
   end
 
   # returns all ancestor-groups upto but excluding the given namespace
diff --git a/app/services/projects/create_from_push_service.rb b/app/services/projects/create_from_push_service.rb
new file mode 100644
index 00000000000..58b3e091438
--- /dev/null
+++ b/app/services/projects/create_from_push_service.rb
@@ -0,0 +1,37 @@
+module Projects
+  class CreateFromPushService < BaseService
+    attr_reader :user, :project_path, :namespace, :protocol
+
+    def initialize(user, project_path, namespace, protocol)
+      @user = user
+      @project_path = project_path
+      @namespace = namespace
+      @protocol = protocol
+    end
+
+    def execute
+      return unless user
+
+      project = Projects::CreateService.new(user, project_params).execute
+
+      if project.saved?
+        Gitlab::Checks::ProjectCreated.new(user, project, protocol).add_project_created_message
+      else
+        raise Gitlab::GitAccess::ProjectCreationError, "Could not create project: #{project.errors.full_messages.join(', ')}"
+      end
+
+      project
+    end
+
+    private
+
+    def project_params
+      {
+        description: "",
+        path: project_path.gsub(/\.git$/, ''),
+        namespace_id: namespace&.id,
+        visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s
+      }
+    end
+  end
+end
diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb
index fd568c5ef30..2340e962918 100644
--- a/lib/api/helpers/internal_helpers.rb
+++ b/lib/api/helpers/internal_helpers.rb
@@ -29,10 +29,6 @@ module API
         {}
       end
 
-      def receive_pack?
-        params[:action] == 'git-receive-pack'
-      end
-
       def fix_git_env_repository_paths(env, repository_path)
         if obj_dir_relative = env['GIT_OBJECT_DIRECTORY_RELATIVE'].presence
           env['GIT_OBJECT_DIRECTORY'] = File.join(repository_path, obj_dir_relative)
@@ -51,6 +47,10 @@ module API
         ::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action])
       end
 
+      def receive_pack?
+        params[:action] == 'git-receive-pack'
+      end
+
       def merge_request_urls
         ::MergeRequests::GetUrlsService.new(project).execute(params[:changes])
       end
@@ -64,29 +64,14 @@ module API
         false
       end
 
-      def project_params
-        {
-          description: "",
-          path: Project.parse_project_id(project_match[:project_id]),
-          namespace_id: project_namespace&.id,
-          visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s
-        }
+      def project_namespace
+        @project_namespace ||= project&.namespace || Namespace.find_by_full_path(project_match[:namespace_path])
       end
 
       private
 
-      def project_path_regex
-        @project_regex ||= /\A(?<namespace_id>#{Gitlab::PathRegex.full_namespace_route_regex})\/(?<project_id>#{Gitlab::PathRegex.project_git_route_regex})\z/.freeze
-      end
-
       def project_match
-        @project_match ||= params[:project].match(project_path_regex)
-      end
-
-      def project_namespace
-        return unless project_match
-
-        @project_namespace ||= Namespace.find_by_path_or_name(project_match[:namespace_id])
+        @project_match ||= params[:project].match(Gitlab::PathRegex.full_project_git_path_regex)
       end
 
       # rubocop:disable Gitlab/ModuleWithInstanceVariables
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index b7475af2044..841a34eb67f 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -51,13 +51,11 @@ module API
           return { status: false, message: e.message }
         end
 
-        if user && project.blank? && receive_pack?
-          @project = ::Projects::CreateService.new(user, project_params).execute
-
-          if @project.saved?
-            Gitlab::Checks::NewProject.new(user, @project, protocol).add_new_project_message
-          else
-            return { status: false, message: "Could not create project" }
+        if receive_pack? && project.blank?
+          begin
+            @project = ::Projects::CreateFromPushService.new(user, project_match[:project_path], project_namespace, protocol).execute
+          rescue Gitlab::GitAccess::ProjectCreationError => e
+            return { status: false, message: e.message }
           end
         end
 
@@ -218,10 +216,10 @@ module API
         # key could be used
         if user
           redirect_message = Gitlab::Checks::ProjectMoved.fetch_redirect_message(user.id, project.id)
-          new_project_message = Gitlab::Checks::NewProject.fetch_new_project_message(user.id, project.id)
+          project_created_message = Gitlab::Checks::ProjectCreated.fetch_project_created_message(user.id, project.id)
 
           output[:redirected_message] = redirect_message if redirect_message
-          output[:new_project_message] = new_project_message if new_project_message
+          output[:project_created_message] = project_created_message if project_created_message
         end
 
         output
diff --git a/lib/gitlab/checks/new_project.rb b/lib/gitlab/checks/project_created.rb
index 488c5c03c32..f05e8b4a7e8 100644
--- a/lib/gitlab/checks/new_project.rb
+++ b/lib/gitlab/checks/project_created.rb
@@ -1,7 +1,7 @@
 module Gitlab
   module Checks
-    class NewProject
-      NEW_PROJECT = "new_project".freeze
+    class ProjectCreated
+      PROJECT_CREATED = "project_created".freeze
 
       def initialize(user, project, protocol)
         @user = user
@@ -9,26 +9,26 @@ module Gitlab
         @protocol = protocol
       end
 
-      def self.fetch_new_project_message(user_id, project_id)
-        new_project_key = new_project_message_key(user_id, project_id)
+      def self.fetch_project_created_message(user_id, project_id)
+        project_created_key = project_created_message_key(user_id, project_id)
 
         Gitlab::Redis::SharedState.with do |redis|
-          message = redis.get(new_project_key)
-          redis.del(new_project_key)
+          message = redis.get(project_created_key)
+          redis.del(project_created_key)
           message
         end
       end
 
-      def add_new_project_message
+      def add_project_created_message
         return unless user.present? && project.present?
 
         Gitlab::Redis::SharedState.with do |redis|
-          key = self.class.new_project_message_key(user.id, project.id)
-          redis.setex(key, 5.minutes, new_project_message)
+          key = self.class.project_created_message_key(user.id, project.id)
+          redis.setex(key, 5.minutes, project_created_message)
         end
       end
 
-      def new_project_message
+      def project_created_message
         <<~MESSAGE.strip_heredoc
 
         The private project #{project.full_path} was created.
@@ -46,8 +46,8 @@ module Gitlab
 
       attr_reader :project, :user, :protocol
 
-      def self.new_project_message_key(user_id, project_id)
-        "#{NEW_PROJECT}:#{user_id}:#{project_id}"
+      def self.project_created_message_key(user_id, project_id)
+        "#{PROJECT_CREATED}:#{user_id}:#{project_id}"
       end
 
       def project_url
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 38649a4fdef..32a2395a26b 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -4,6 +4,7 @@ module Gitlab
   class GitAccess
     UnauthorizedError = Class.new(StandardError)
     NotFoundError = Class.new(StandardError)
+    ProjectCreationError = Class.new(StandardError)
     ProjectMovedError = Class.new(NotFoundError)
 
     ERROR_MESSAGES = {
@@ -13,13 +14,13 @@ module Gitlab
         'This deploy key does not have write access to this project.',
       no_repo: 'A repository for this project does not exist yet.',
       project_not_found: 'The project you were looking for could not be found.',
-      namespace_not_found: 'The namespace you were looking for could not be found.',
       account_blocked: 'Your account has been blocked.',
       command_not_allowed: "The command you're trying to execute is not allowed.",
       upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.',
       receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.',
       read_only: 'The repository is temporarily read-only. Please try again later.',
-      cannot_push_to_read_only: "You can't push code to a read-only GitLab instance."
+      cannot_push_to_read_only: "You can't push code to a read-only GitLab instance.",
+      namespace_not_found: 'The namespace you were looking for could not be found.'
     }.freeze
 
     DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }.freeze
@@ -52,7 +53,7 @@ module Gitlab
         check_download_access!
       when *PUSH_COMMANDS
         check_push_access!(cmd, changes)
-        check_namespace_accessibility!(cmd)
+        check_namespace_accessibility!
       end
 
       true
@@ -148,7 +149,7 @@ module Gitlab
       end
     end
 
-    def check_namespace_accessibility!(cmd)
+    def check_namespace_accessibility!
       return unless project.blank?
 
       unless target_namespace
@@ -248,7 +249,7 @@ module Gitlab
     end
 
     def can_create_project_in_namespace?(cmd)
-      return false unless PUSH_COMMANDS.include?(cmd) && target_namespace
+      return false unless push?(cmd) && target_namespace
 
       user.can?(:create_projects, target_namespace)
     end
@@ -265,6 +266,10 @@ module Gitlab
       command == 'git-receive-pack'
     end
 
+    def push?(cmd)
+      PUSH_COMMANDS.include?(cmd)
+    end
+
     def upload_pack_disabled_over_http?
       !Gitlab.config.gitlab_shell.upload_pack
     end
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index f679b5e8ed6..1c9477e84b2 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -25,10 +25,6 @@ module Gitlab
       true
     end
 
-    def check_repository_creation!(cmd)
-      # Method not used in wiki
-    end
-
     def push_to_read_only_message
       ERROR_MESSAGES[:read_only]
     end
diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb
index 7e5dfd33502..4a2db11a978 100644
--- a/lib/gitlab/path_regex.rb
+++ b/lib/gitlab/path_regex.rb
@@ -187,6 +187,10 @@ module Gitlab
       @full_project_path_regex ||= %r{\A#{full_namespace_route_regex}/#{project_route_regex}/\z}
     end
 
+    def full_project_git_path_regex
+      @full_project_git_path_regex ||= /\A(\/|)(?<namespace_path>#{full_namespace_route_regex})\/(?<project_path>#{project_git_route_regex})\z/.freeze
+    end
+
     def full_namespace_format_regex
       @namespace_format_regex ||= /A#{FULL_NAMESPACE_FORMAT_REGEX}\z/.freeze
     end
diff --git a/spec/lib/gitlab/checks/new_project_spec.rb b/spec/lib/gitlab/checks/new_project_spec.rb
deleted file mode 100644
index c696e02e41a..00000000000
--- a/spec/lib/gitlab/checks/new_project_spec.rb
+++ /dev/null
@@ -1,46 +0,0 @@
-require 'rails_helper'
-
-describe Gitlab::Checks::NewProject, :clean_gitlab_redis_shared_state do
-  let(:user) { create(:user) }
-  let(:project) { create(:project) }
-
-  describe '.fetch_new_project_message' do
-    context 'with a new project message queue' do
-      let(:new_project) { described_class.new(user, project, 'http') }
-
-      before do
-        new_project.add_new_project_message
-      end
-
-      it 'returns new project message' do
-        expect(described_class.fetch_new_project_message(user.id, project.id)).to eq(new_project.new_project_message)
-      end
-
-      it 'deletes the new project message from redis' do
-        expect(Gitlab::Redis::SharedState.with { |redis| redis.get("new_project:#{user.id}:#{project.id}") }).not_to be_nil
-        described_class.fetch_new_project_message(user.id, project.id)
-        expect(Gitlab::Redis::SharedState.with { |redis| redis.get("new_project:#{user.id}:#{project.id}") }).to be_nil
-      end
-    end
-
-    context 'with no new project message queue' do
-      it 'returns nil' do
-        expect(described_class.fetch_new_project_message(1, 2)).to be_nil
-      end
-    end
-  end
-
-  describe '#add_new_project_message' do
-    it 'queues a new project message' do
-      new_project = described_class.new(user, project, 'http')
-
-      expect(new_project.add_new_project_message).to eq('OK')
-    end
-
-    it 'handles anonymous push' do
-      new_project = described_class.new(user, nil, 'http')
-
-      expect(new_project.add_new_project_message).to be_nil
-    end
-  end
-end
diff --git a/spec/lib/gitlab/checks/project_created_spec.rb b/spec/lib/gitlab/checks/project_created_spec.rb
new file mode 100644
index 00000000000..c79798bc84d
--- /dev/null
+++ b/spec/lib/gitlab/checks/project_created_spec.rb
@@ -0,0 +1,46 @@
+require 'rails_helper'
+
+describe Gitlab::Checks::ProjectCreated, :clean_gitlab_redis_shared_state do
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+
+  describe '.fetch_project_created_message' do
+    context 'with a project created message queue' do
+      let(:project_created) { described_class.new(user, project, 'http') }
+
+      before do
+        project_created.add_project_created_message
+      end
+
+      it 'returns project created message' do
+        expect(described_class.fetch_project_created_message(user.id, project.id)).to eq(project_created.project_created_message)
+      end
+
+      it 'deletes the project created message from redis' do
+        expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).not_to be_nil
+        described_class.fetch_project_created_message(user.id, project.id)
+        expect(Gitlab::Redis::SharedState.with { |redis| redis.get("project_created:#{user.id}:#{project.id}") }).to be_nil
+      end
+    end
+
+    context 'with no project created message queue' do
+      it 'returns nil' do
+        expect(described_class.fetch_project_created_message(1, 2)).to be_nil
+      end
+    end
+  end
+
+  describe '#add_project_created_message' do
+    it 'queues a project created message' do
+      project_created = described_class.new(user, project, 'http')
+
+      expect(project_created.add_project_created_message).to eq('OK')
+    end
+
+    it 'handles anonymous push' do
+      project_created = described_class.new(user, nil, 'http')
+
+      expect(project_created.add_project_created_message).to be_nil
+    end
+  end
+end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 3c98c95e301..e6ad35867c0 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -335,7 +335,7 @@ describe Gitlab::GitAccess do
     end
   end
 
-  describe '#check_namespace_accessibility!' do
+  describe '#check_namespace_existence!' do
     context 'when project exists' do
       context 'when user can pull or push' do
         before do
@@ -838,18 +838,11 @@ describe Gitlab::GitAccess do
   end
 
   def raise_not_found
-    raise_error(Gitlab::GitAccess::NotFoundError,
-                Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found])
+    raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found])
   end
 
   def raise_namespace_not_found
-    raise_error(Gitlab::GitAccess::NotFoundError,
-                Gitlab::GitAccess::ERROR_MESSAGES[:namespace_not_found])
-  end
-
-  def raise_project_create
-    raise_error(Gitlab::GitAccess::NotFoundError,
-                Gitlab::GitAccess::ERROR_MESSAGES[:create])
+    raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:namespace_not_found])
   end
 
   def build_authentication_abilities
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index a502a798368..da940571bc1 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1079,12 +1079,6 @@ describe Project do
     end
   end
 
-  describe '.parse_project_id' do
-    it 'removes .git from the project_id' do
-      expect(described_class.parse_project_id('new-project.git')).to eq('new-project')
-    end
-  end
-
   context 'repository storage by default' do
     let(:project) { create(:project) }
 
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 5f6790312e4..1e8197653ce 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -366,6 +366,17 @@ describe API::Internal do
           end
         end
 
+        context 'project as /namespace/project' do
+          it do
+            push(key, project_with_repo_path('/' + project.full_path))
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response["status"]).to be_truthy
+            expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
+            expect(json_response["gl_repository"]).to eq("project-#{project.id}")
+          end
+        end
+
         context 'project as namespace/project' do
           it do
             push(key, project_with_repo_path(project.full_path))
@@ -823,14 +834,14 @@ describe API::Internal do
 
     context 'with new project data' do
       it 'returns new project message on the response' do
-        new_project = Gitlab::Checks::NewProject.new(user, project, 'http')
-        new_project.add_new_project_message
+        project_created = Gitlab::Checks::ProjectCreated.new(user, project, 'http')
+        project_created.add_project_created_message
 
         post api("/internal/post_receive"), valid_params
 
         expect(response).to have_gitlab_http_status(200)
-        expect(json_response["new_project_message"]).to be_present
-        expect(json_response["new_project_message"]).to eq(new_project.new_project_message)
+        expect(json_response["project_created_message"]).to be_present
+        expect(json_response["project_created_message"]).to eq(project_created.project_created_message)
       end
     end
 
diff --git a/spec/services/projects/create_from_push_service_spec.rb b/spec/services/projects/create_from_push_service_spec.rb
new file mode 100644
index 00000000000..8e45697cd85
--- /dev/null
+++ b/spec/services/projects/create_from_push_service_spec.rb
@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+describe Projects::CreateFromPushService do
+  let(:user) { create(:user) }
+  let(:project_path) { "nonexist" }
+  let(:namespace) { user&.namespace }
+  let(:protocol) { 'http' }
+
+  subject { described_class.new(user, project_path, namespace, protocol) }
+
+  it 'creates project' do
+    expect_any_instance_of(Projects::CreateService).to receive(:execute).and_call_original
+
+    expect { subject.execute }.to change { Project.count }.by(1)
+  end
+
+  it 'raises project creation error when project creation fails' do
+    allow_any_instance_of(Project).to receive(:saved?).and_return(false)
+
+    expect { subject.execute }.to raise_error(Gitlab::GitAccess::ProjectCreationError)
+  end
+
+  context 'when user is nil' do
+    let(:user) { nil }
+
+    subject { described_class.new(user, project_path, namespace, cmd, protocol) }
+
+    it 'returns nil' do
+      expect_any_instance_of(Projects::CreateService).not_to receive(:execute)
+
+      expect(subject.execute).to be_nil
+    end
+  end
+end