Adds proposal for 26388 refactor26388-refactor-proposal

13 files changed, 184 insertions, 141 deletions

diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 90a9079fab3..3d3a444b718 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -11,7 +11,6 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
   def info_refs
     log_user_activity if upload_pack?
-    create_new_project if receive_pack? && project.blank?
 
     render_ok
   end
@@ -48,10 +47,6 @@ 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)
@@ -70,7 +65,10 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
 
   def access
-    @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: namespace)
+    @access ||= access_klass.new(access_actor, project,
+      'http', authentication_abilities: authentication_abilities,
+              namespace_path: params[:namespace_id], project_path: project_path,
+              redirected_path: redirected_path)
   end
 
   def access_actor
@@ -82,14 +80,15 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     # Use the magic string '_any' to indicate we do not know what the
     # changes are. This is also what gitlab-shell does.
     access.check(git_command, '_any')
+    @project ||= access.project
   end
 
   def access_klass
     @access_klass ||= wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
   end
 
-  def namespace
-    @namespace ||= Namespace.find_by_full_path(params[:namespace_id])
+  def project_path
+    @project_path ||= params[:project_id].sub(/\.git$/, '')
   end
 
   def log_user_activity
diff --git a/app/services/projects/create_from_push_service.rb b/app/services/projects/create_from_push_service.rb
index 949a0bfdc09..c38fdf04cc5 100644
--- a/app/services/projects/create_from_push_service.rb
+++ b/app/services/projects/create_from_push_service.rb
@@ -27,10 +27,9 @@ module Projects
 
     def project_params
       {
-        description: "",
-        path: project_path.gsub(/\.git$/, ''),
+        path: project_path,
         namespace_id: namespace&.id,
-        visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s
+        visibility_level: Gitlab::VisibilityLevel::PRIVATE
       }
     end
   end
diff --git a/doc/gitlab-basics/create-project.md b/doc/gitlab-basics/create-project.md
index d491d676884..9218cfc8bbf 100644
--- a/doc/gitlab-basics/create-project.md
+++ b/doc/gitlab-basics/create-project.md
@@ -39,18 +39,18 @@
 
 When you create a new repo locally, instead of going to GitLab to manually
 create a new project and then push the repo, you can directly push it to
-GitLab to create the new project, all without leaving your terminal. That
-will automatically create a new project under a GitLab namespace that you have access to 
-with its visibility set to private by default (you can later change it).
+GitLab to create the new project, all without leaving your terminal. If you have access to that
+namespace, we will automatically create a new project under that GitLab namespace with its 
+visibility set to private by default (you can later change it in the UI).
 
 This can be done by using either SSH or HTTP:
 
 ```
 ## Git push using SSH
-git push git@gitlab.com:namespace/nonexistent-project.git branch_name
+git push git@gitlab.example.com:namespace/nonexistent-project.git
 
 ## Git push using HTTP
-git push https://gitlab.com/namespace/nonexistent-project.git branch_name
+git push https://gitlab.example.com/namespace/nonexistent-project.git
 ```
 
 Once the push finishes successfully, a remote message will indicate
@@ -61,10 +61,10 @@ remote:
 remote: The private project namespace/nonexistent-project was created.
 remote:
 remote: To configure the remote, run:
-remote:   git remote add origin https://gitlab.com/namespace/nonexistent-project.git
+remote:   git remote add origin https://gitlab.example.com/namespace/nonexistent-project.git
 remote:
 remote: To view the project, visit:
-remote:   https://gitlab.com/namespace/nonexistent-project
+remote:   https://gitlab.example.com/namespace/nonexistent-project
 remote:
 ```
 
diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb
index bff245fe9a2..fdb441375eb 100644
--- a/lib/api/helpers/internal_helpers.rb
+++ b/lib/api/helpers/internal_helpers.rb
@@ -66,16 +66,18 @@ module API
         false
       end
 
-      def project_namespace
-        strong_memoize(:project_namespace) do
-          project&.namespace || Namespace.find_by_full_path(project_match[:namespace_path])
-        end
+      def project_path
+        project&.path || project_path_match[:project_path]
+      end
+
+      def namespace_path
+        project&.namespace&.path || project_path_match[:namespace_path]
       end
 
       private
 
-      def project_match
-        @project_match ||= params[:project].match(Gitlab::PathRegex.full_project_git_path_regex) || {}
+      def project_path_match
+        @project_path_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 ed6d022df97..9285fb90cdc 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -42,23 +42,18 @@ module API
           end
 
         access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
-        access_checker = access_checker_klass
-          .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path, target_namespace: project_namespace)
+        access_checker = access_checker_klass.new(actor, project,
+          protocol, authentication_abilities: ssh_authentication_abilities,
+                    namespace_path: namespace_path, project_path: project_path,
+                    redirected_path: redirected_path)
 
         begin
           access_checker.check(params[:action], params[:changes])
+          @project ||= access_checker.project
         rescue Gitlab::GitAccess::UnauthorizedError, Gitlab::GitAccess::NotFoundError => e
           return { status: false, message: e.message }
         end
 
-        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
-
         log_user_activity(actor)
 
         {
diff --git a/lib/gitlab/checks/base_project.rb b/lib/gitlab/checks/post_push_message.rb
index dd6c007b356..473c0385b34 100644
--- a/lib/gitlab/checks/base_project.rb
+++ b/lib/gitlab/checks/post_push_message.rb
@@ -1,6 +1,6 @@
 module Gitlab
   module Checks
-    class BaseProject
+    class PostPushMessage
       def initialize(project, user, protocol)
         @project = project
         @user = user
diff --git a/lib/gitlab/checks/project_created.rb b/lib/gitlab/checks/project_created.rb
index bd1e204bc81..ac24ff13b2a 100644
--- a/lib/gitlab/checks/project_created.rb
+++ b/lib/gitlab/checks/project_created.rb
@@ -1,6 +1,6 @@
 module Gitlab
   module Checks
-    class ProjectCreated < BaseProject
+    class ProjectCreated < PostPushMessage
       PROJECT_CREATED = "project_created".freeze
 
       def message
diff --git a/lib/gitlab/checks/project_moved.rb b/lib/gitlab/checks/project_moved.rb
index eca59e88e24..216d2d6acc3 100644
--- a/lib/gitlab/checks/project_moved.rb
+++ b/lib/gitlab/checks/project_moved.rb
@@ -1,6 +1,6 @@
 module Gitlab
   module Checks
-    class ProjectMoved < BaseProject
+    class ProjectMoved < PostPushMessage
       REDIRECT_NAMESPACE = "redirect_namespace".freeze
 
       def initialize(project, user, protocol, redirected_path)
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 84299dd5790..4615d67ce90 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -28,32 +28,37 @@ module Gitlab
     PUSH_COMMANDS = %w{ git-receive-pack }.freeze
     ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS
 
-    attr_reader :actor, :project, :protocol, :authentication_abilities, :redirected_path, :target_namespace
+    attr_reader :actor, :project, :protocol, :authentication_abilities, :namespace_path, :project_path, :redirected_path
 
-    def initialize(actor, project, protocol, authentication_abilities:, redirected_path: nil, target_namespace: nil)
+    def initialize(actor, project, protocol, authentication_abilities:, namespace_path: nil, project_path: nil, redirected_path: nil)
       @actor    = actor
       @project  = project
       @protocol = protocol
-      @redirected_path = redirected_path
       @authentication_abilities = authentication_abilities
-      @target_namespace = target_namespace
+      @namespace_path = namespace_path
+      @project_path = project_path
+      @redirected_path = redirected_path
     end
 
     def check(cmd, changes)
       check_protocol!
       check_valid_actor!
       check_active_user!
-      check_project_accessibility!(cmd)
-      check_project_moved!
       check_command_disabled!(cmd)
       check_command_existence!(cmd)
-      check_repository_existence!(cmd)
+      check_db_accessibility!(cmd)
+
+      ensure_project!(cmd, changes)
+
+      check_project_accessibility!
+      check_project_moved!
+      check_repository_existence!
 
       case cmd
       when *DOWNLOAD_COMMANDS
         check_download_access!
       when *PUSH_COMMANDS
-        check_push_access!(cmd, changes)
+        check_push_access!(changes)
       end
 
       true
@@ -99,8 +104,8 @@ module Gitlab
       end
     end
 
-    def check_project_accessibility!(cmd)
-      unless can_create_project_in_namespace?(cmd) || can_read_project?
+    def check_project_accessibility!
+      if project.blank? || !can_read_project?
         raise NotFoundError, ERROR_MESSAGES[:project_not_found]
       end
     end
@@ -143,8 +148,23 @@ module Gitlab
       end
     end
 
-    def check_repository_existence!(cmd)
-      unless can_create_project_in_namespace?(cmd) || project.repository.exists?
+    def check_db_accessibility!(cmd)
+      return unless receive_pack?(cmd)
+
+      if Gitlab::Database.read_only?
+        raise UnauthorizedError, push_to_read_only_message
+      end
+    end
+
+    def ensure_project!(cmd, changes)
+      if can_create_project_in_namespace?(cmd, changes)
+        @project = ::Projects::CreateFromPushService.new(user, project_path, namespace, protocol).execute
+        user_access.project = @project
+      end
+    end
+
+    def check_repository_existence!
+      unless project.repository.exists?
         raise UnauthorizedError, ERROR_MESSAGES[:no_repo]
       end
     end
@@ -161,13 +181,7 @@ module Gitlab
       end
     end
 
-    def check_push_access!(cmd, changes)
-      if Gitlab::Database.read_only?
-        raise UnauthorizedError, push_to_read_only_message
-      end
-
-      return if can_create_project_in_namespace?(cmd)
-
+    def check_push_access!(changes)
       if project.repository_read_only?
         raise UnauthorizedError, ERROR_MESSAGES[:read_only]
       end
@@ -180,8 +194,6 @@ module Gitlab
         raise UnauthorizedError, ERROR_MESSAGES[:upload]
       end
 
-      return if changes.blank? # Allow access.
-
       check_change_access!(changes)
     end
 
@@ -198,6 +210,8 @@ module Gitlab
     end
 
     def check_change_access!(changes)
+      return if changes.blank? # Allow access.
+
       changes_list = Gitlab::ChangesList.new(changes)
 
       # Iterate over all changes to find if user allowed all of them to be applied
@@ -240,11 +254,11 @@ module Gitlab
       end || Guest.can?(:read_project, project)
     end
 
-    def can_create_project_in_namespace?(cmd)
+    def can_create_project_in_namespace?(cmd, changes)
       strong_memoize(:can_create_project_in_namespace) do
-        return false unless push?(cmd) && target_namespace && project.blank?
+        return false unless !project && receive_pack?(cmd) && changes == '_any'
 
-        user.can?(:create_projects, target_namespace)
+        user&.can?(:create_projects, namespace)
       end
     end
 
@@ -260,10 +274,6 @@ 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
@@ -288,6 +298,12 @@ module Gitlab
         end
     end
 
+    def namespace
+      return unless namespace_path
+
+      @namespace ||= Namespace.find_by_full_path(namespace_path)
+    end
+
     def user_access
       @user_access ||= if ci?
                          CiAccess.new
diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb
index c6a594d38d1..111c9f20049 100644
--- a/lib/gitlab/path_regex.rb
+++ b/lib/gitlab/path_regex.rb
@@ -188,7 +188,7 @@ module Gitlab
     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
+      @full_project_git_path_regex ||= /\A\/?(?<namespace_path>#{full_namespace_route_regex})\/(?<project_path>#{project_route_regex})\.git\z/.freeze
     end
 
     def full_namespace_format_regex
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index f357488ac61..15eb1c41213 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -6,7 +6,8 @@ module Gitlab
       [user&.id, project&.id]
     end
 
-    attr_reader :user, :project
+    attr_reader :user
+    attr_accessor :project
 
     def initialize(user, project: nil)
       @user = user
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 6bfac67ddae..b36f4bd271b 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -5,11 +5,19 @@ describe Gitlab::GitAccess do
 
   let(:actor) { user }
   let(:project) { create(:project, :repository) }
+  let(:project_path) { project.path }
+  let(:namespace_path) { project&.namespace&.path }
   let(:protocol) { 'ssh' }
   let(:authentication_abilities) { %i[read_project download_code push_code] }
   let(:redirected_path) { nil }
 
-  let(:access) { described_class.new(actor, project, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path) }
+  let(:access) do
+    described_class.new(actor, project,
+      protocol, authentication_abilities: authentication_abilities,
+                namespace_path: namespace_path, project_path: project_path,
+                redirected_path: redirected_path)
+  end
+
   let(:push_access_check) { access.check('git-receive-pack', '_any') }
   let(:pull_access_check) { access.check('git-upload-pack', '_any') }
 
@@ -145,6 +153,7 @@ describe Gitlab::GitAccess do
 
     context 'when the project is nil' do
       let(:project) { nil }
+      let(:project_path) { "new-project" }
 
       it 'blocks push and pull with "not found"' do
         aggregate_failures do
@@ -154,7 +163,13 @@ describe Gitlab::GitAccess do
       end
 
       context 'when user is allowed to create project in namespace' do
-        let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user.namespace) }
+        let(:namespace_path) { user.namespace.path }
+        let(:access) do
+          described_class.new(actor, nil,
+            protocol, authentication_abilities: authentication_abilities,
+                      project_path: project_path, namespace_path: namespace_path,
+                      redirected_path: redirected_path)
+        end
 
         it 'blocks pull access with "not found"' do
           expect { pull_access_check }.to raise_not_found
@@ -167,7 +182,13 @@ describe Gitlab::GitAccess do
 
       context 'when user is not allowed to create project in namespace' do
         let(:user2) { create(:user) }
-        let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user2.namespace) }
+        let(:namespace_path) { user2.namespace.path }
+        let(:access) do
+          described_class.new(actor, nil,
+            protocol, authentication_abilities: authentication_abilities,
+                      project_path: project_path, namespace_path: namespace_path,
+                      redirected_path: redirected_path)
+        end
 
         it 'blocks push and pull with "not found"' do
           aggregate_failures do
@@ -335,34 +356,83 @@ describe Gitlab::GitAccess do
     end
   end
 
-  describe '#check_namespace_accessibility!' do
-    context 'when project exists' do
-      context 'when user can pull or push' do
-        before do
-          project.add_master(user)
+  describe '#check_db_accessibility!' do
+    context 'when in a read-only GitLab instance' do
+      before do
+        create(:protected_branch, name: 'feature', project: project)
+        allow(Gitlab::Database).to receive(:read_only?) { true }
+      end
+
+      it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:cannot_push_to_read_only]) }
+    end
+  end
+
+  describe '#ensure_project!' do
+    let(:access) do
+      described_class.new(actor, project,
+        protocol, authentication_abilities: authentication_abilities,
+                  project_path: project_path, namespace_path: namespace_path,
+                  redirected_path: redirected_path)
+    end
+
+    context 'when push' do
+      let(:cmd) { 'git-receive-pack' }
+
+      context 'when project does not exist' do
+        let(:project_path) { "nonexistent" }
+        let(:project) { nil }
+
+        context 'when changes is _any' do
+          let(:changes) { '_any' }
+
+          context 'when user can create project in namespace' do
+            let(:namespace_path) { user.namespace.path }
+
+            it 'creates a new project' do
+              expect { access.send(:ensure_project!, cmd, changes) }.to change { Project.count }.by(1)
+            end
+          end
+
+          context 'when user cannot create project in namespace' do
+            let(:user2) { create(:user) }
+            let(:namespace_path) { user2.namespace.path }
+
+            it 'does not create a new project' do
+              expect { access.send(:ensure_project!, cmd, changes) }.not_to change { Project.count }
+            end
+          end
         end
 
-        it 'does not block pull or push' do
-          aggregate_failures do
-            expect { push_access_check }.not_to raise_error
-            expect { pull_access_check }.not_to raise_error
+        context 'when check contains actual changes' do
+          let(:changes) { URI.escape("#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch") }
+
+          it 'does not create a new project' do
+            expect { access.send(:ensure_project!, cmd, changes) }.not_to change { Project.count }
           end
         end
       end
+
+      context 'when project exists' do
+        let(:changes) { '_any' }
+        let!(:project) { create(:project) }
+
+        it 'does not create a new project' do
+          expect { access.send(:ensure_project!, cmd, changes) }.not_to change { Project.count }
+        end
+      end
     end
 
-    context 'when project does not exist' do
-      context 'when namespace exists' do
-        context 'when user is unable to push to namespace' do
-          let(:user2) { create(:user) }
-          let(:access) { described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path, target_namespace: user2.namespace) }
+    context 'when pull' do
+      let(:cmd) { 'git-upload-pack' }
+      let(:changes) { '_any' }
 
-          it 'blocks push and pull' do
-            aggregate_failures do
-              expect { push_access_check }.to raise_not_found
-              expect { pull_access_check }.to raise_not_found
-            end
-          end
+      context 'when project does not exist' do
+        let(:project_path) { "new-project" }
+        let(:namespace_path) { user.namespace.path }
+        let(:project) { nil }
+
+        it 'does not create a new project' do
+          expect { access.send(:ensure_project!, cmd, changes) }.not_to change { Project.count }
         end
       end
     end
@@ -395,7 +465,9 @@ describe Gitlab::GitAccess do
 
       context 'when project is public' do
         let(:public_project) { create(:project, :public, :repository) }
-        let(:access) { described_class.new(nil, public_project, 'web', authentication_abilities: []) }
+        let(:project_path) { public_project.path }
+        let(:namespace_path) { public_project.namespace.path }
+        let(:access) { described_class.new(nil, public_project, 'web', authentication_abilities: [], project_path: project_path, namespace_path: namespace_path) }
 
         context 'when repository is enabled' do
           it 'give access to download code' do
@@ -558,17 +630,15 @@ describe Gitlab::GitAccess do
           end
 
           aggregate_failures do
-            Gitlab::GitAccess::ALL_COMMANDS.each do |cmd|
-              matrix.each do |action, allowed|
-                check = -> { access.send(:check_push_access!, cmd, changes[action]) }
-
-                if allowed
-                  expect(&check).not_to raise_error,
-                                        -> { "expected #{action} to be allowed" }
-                else
-                  expect(&check).to raise_error(Gitlab::GitAccess::UnauthorizedError),
-                                    -> { "expected #{action} to be disallowed" }
-                end
+            matrix.each do |action, allowed|
+              check = -> { access.send(:check_push_access!, changes[action]) }
+
+              if allowed
+                expect(&check).not_to raise_error,
+                  -> { "expected #{action} to be allowed" }
+              else
+                expect(&check).to raise_error(Gitlab::GitAccess::UnauthorizedError),
+                  -> { "expected #{action} to be disallowed" }
               end
             end
           end
@@ -697,19 +767,6 @@ describe Gitlab::GitAccess do
                                                             admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
       end
     end
-
-    context "when in a read-only GitLab instance" do
-      before do
-        create(:protected_branch, name: 'feature', project: project)
-        allow(Gitlab::Database).to receive(:read_only?) { true }
-      end
-
-      # Only check admin; if an admin can't do it, other roles can't either
-      matrix = permissions_matrix[:admin].dup
-      matrix.each { |key, _| matrix[key] = false }
-
-      run_permission_checks(admin: matrix)
-    end
   end
 
   describe 'build authentication abilities' do
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 5ef180348aa..ea6b0a71849 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -386,32 +386,6 @@ describe API::Internal do
             expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
             expect(json_response["gl_repository"]).to eq("project-#{project.id}")
           end
-
-          context 'when project does not exist' do
-            it 'creates a new project' do
-              path = "#{user.namespace.path}/notexist.git"
-
-              expect do
-                push_with_path(key, path)
-              end.to change { Project.count }.by(1)
-
-              expect(response).to have_gitlab_http_status(200)
-              expect(json_response["status"]).to be_truthy
-              expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(path)
-            end
-
-            it 'handles project creation failure' do
-              path = "#{user.namespace.path}/new.git"
-
-              expect do
-                push_with_path(key, path)
-              end.not_to change { Project.count }
-
-              expect(response).to have_gitlab_http_status(200)
-              expect(json_response["status"]).to be_falsey
-              expect(json_response["message"]).to eq("Could not create project: Path new is a reserved name")
-            end
-          end
         end
       end
     end