Merge branch '207871-git-access-project' into 'master'

Extract GitAccessProject

See merge request gitlab-org/gitlab!33690

app/controllers/repositories/git_http_controller.rb

@@ -9,7 +9,7 @@ module Repositories
 
     rescue_from Gitlab::GitAccess::ForbiddenError, with: :render_403_with_exception
     rescue_from Gitlab::GitAccess::NotFoundError, with: :render_404_with_exception
-    rescue_from Gitlab::GitAccess::ProjectCreationError, with: :render_422_with_exception
+    rescue_from Gitlab::GitAccessProject::CreationError, with: :render_422_with_exception
     rescue_from Gitlab::GitAccess::TimeoutError, with: :render_503_with_exception
 
     # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull)

lib/gitlab/git_access.rb

@@ -8,7 +8,6 @@ module Gitlab
 
     ForbiddenError = Class.new(StandardError)
     NotFoundError = Class.new(StandardError)
-    ProjectCreationError = Class.new(StandardError)
     TimeoutError = Class.new(StandardError)
     ProjectMovedError = Class.new(NotFoundError)
 
@@ -75,7 +74,7 @@ module Gitlab
 
       check_db_accessibility!(cmd)
       check_namespace!
-      check_project!(changes, cmd)
+      check_project!(cmd)
       check_repository_existence!
 
       case cmd
@@ -112,8 +111,7 @@ module Gitlab
 
     private
 
-    def check_project!(changes, cmd)
-      ensure_project_on_push!(cmd, changes)
+    def check_project!(_cmd)
       check_project_accessibility!
       add_project_moved_message!
     end
@@ -230,32 +228,6 @@ module Gitlab
       end
     end
 
-    def ensure_project_on_push!(cmd, changes)
-      return if project || deploy_key?
-      return unless receive_pack?(cmd) && changes == ANY && authentication_abilities.include?(:push_code)
-
-      namespace = Namespace.find_by_full_path(namespace_path)
-
-      return unless user&.can?(:create_projects, namespace)
-
-      project_params = {
-        path: repository_path,
-        namespace_id: namespace.id,
-        visibility_level: Gitlab::VisibilityLevel::PRIVATE
-      }
-
-      project = Projects::CreateService.new(user, project_params).execute
-
-      unless project.saved?
-        raise ProjectCreationError, "Could not create project: #{project.errors.full_messages.join(', ')}"
-      end
-
-      @project = project
-      user_access.project = @project
-
-      Checks::ProjectCreated.new(repository, user, protocol).add_message
-    end
-
     def check_repository_existence!
       unless repository.exists?
         raise NotFoundError, ERROR_MESSAGES[:no_repo]

lib/gitlab/git_access_project.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class GitAccessProject < GitAccess
+    extend ::Gitlab::Utils::Override
+
+    CreationError = Class.new(StandardError)
+
+    private
+
+    override :check_project!
+    def check_project!(cmd)
+      ensure_project_on_push!(cmd)
+
+      super
+    end
+
+    def ensure_project_on_push!(cmd)
+      return if project || deploy_key?
+      return unless receive_pack?(cmd) && changes == ANY && authentication_abilities.include?(:push_code)
+
+      namespace = Namespace.find_by_full_path(namespace_path)
+
+      return unless user&.can?(:create_projects, namespace)
+
+      project_params = {
+        path: repository_path,
+        namespace_id: namespace.id,
+        visibility_level: Gitlab::VisibilityLevel::PRIVATE
+      }
+
+      project = Projects::CreateService.new(user, project_params).execute
+
+      unless project.saved?
+        raise CreationError, "Could not create project: #{project.errors.full_messages.join(', ')}"
+      end
+
+      @project = project
+      user_access.project = @project
+
+      Checks::ProjectCreated.new(repository, user, protocol).add_message
+    end
+  end
+end

lib/gitlab/git_access_snippet.rb

@@ -47,11 +47,10 @@ module Gitlab
     end
 
     override :check_project!
-    def check_project!(cmd, changes)
+    def check_project!(cmd)
       return unless snippet.is_a?(ProjectSnippet)
 
-      check_project_accessibility!
-      add_project_moved_message!
+      super
     end
 
     override :check_push_access!

lib/gitlab/gl_repository.rb

@@ -6,7 +6,7 @@ module Gitlab
 
     PROJECT = RepoType.new(
       name: :project,
-      access_checker_class: Gitlab::GitAccess,
+      access_checker_class: Gitlab::GitAccessProject,
       repository_resolver: -> (project) { project&.repository }
     ).freeze
     WIKI = RepoType.new(

spec/lib/gitlab/git_access_project_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::GitAccessProject do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository) }
+  let(:actor) { user }
+  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(:changes) { Gitlab::GitAccess::ANY }
+  let(:push_access_check) { access.check('git-receive-pack', changes) }
+  let(:pull_access_check) { access.check('git-upload-pack', changes) }
+
+  describe '#check_project_accessibility!' do
+    context 'when the project is nil' do
+      let(:project) { nil }
+      let(:project_path) { "new-project" }
+
+      context 'when user is allowed to create project in namespace' do
+        let(:namespace_path) { user.namespace.path }
+        let(:access) do
+          described_class.new(actor, nil,
+            protocol, authentication_abilities: authentication_abilities,
+            repository_path: project_path, namespace_path: namespace_path)
+        end
+
+        it 'blocks pull access with "not found"' do
+          expect { pull_access_check }.to raise_not_found
+        end
+
+        it 'allows push access' do
+          expect { push_access_check }.not_to raise_error
+        end
+      end
+
+      context 'when user is not allowed to create project in namespace' do
+        let(:user2) { create(:user) }
+        let(:namespace_path) { user2.namespace.path }
+        let(:access) do
+          described_class.new(actor, nil,
+            protocol, authentication_abilities: authentication_abilities,
+            repository_path: project_path, namespace_path: namespace_path)
+        end
+
+        it 'blocks push and pull with "not found"' do
+          aggregate_failures do
+            expect { pull_access_check }.to raise_not_found
+            expect { push_access_check }.to raise_not_found
+          end
+        end
+      end
+    end
+  end
+
+  describe '#ensure_project_on_push!' do
+    let(:access) do
+      described_class.new(actor, project,
+        protocol, authentication_abilities: authentication_abilities,
+        repository_path: project_path, namespace_path: namespace_path)
+    end
+
+    before do
+      allow(access).to receive(:changes).and_return(changes)
+    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) { Gitlab::GitAccess::ANY }
+
+          context 'when authentication abilities include push code' do
+            let(:authentication_abilities) { [:push_code] }
+
+            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_on_push!, cmd) }
+                  .to change { Project.count }.by(1)
+                  .and change { Project.where(namespace: user.namespace, name: project_path).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_on_push!, cmd) }.not_to change { Project.count }
+              end
+            end
+          end
+
+          context 'when authentication abilities do not include push code' do
+            let(:authentication_abilities) { [] }
+
+            context 'when user can create project in namespace' do
+              let(:namespace_path) { user.namespace.path }
+
+              it 'does not create a new project' do
+                expect { access.send(:ensure_project_on_push!, cmd) }.not_to change { Project.count }
+              end
+            end
+          end
+        end
+
+        context 'when check contains actual changes' do
+          let(:changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" }
+
+          it 'does not create a new project' do
+            expect { access.send(:ensure_project_on_push!, cmd) }.not_to change { Project.count }
+          end
+        end
+      end
+
+      context 'when project exists' do
+        let(:changes) { Gitlab::GitAccess::ANY }
+        let!(:project) { create(:project) }
+
+        it 'does not create a new project' do
+          expect { access.send(:ensure_project_on_push!, cmd) }.not_to change { Project.count }
+        end
+      end
+
+      context 'when deploy key is used' do
+        let(:key) { create(:deploy_key, user: user) }
+        let(:actor) { key }
+        let(:project_path) { "nonexistent" }
+        let(:project) { nil }
+        let(:namespace_path) { user.namespace.path }
+        let(:changes) { Gitlab::GitAccess::ANY }
+
+        it 'does not create a new project' do
+          expect { access.send(:ensure_project_on_push!, cmd) }.not_to change { Project.count }
+        end
+      end
+    end
+
+    context 'when pull' do
+      let(:cmd) { 'git-upload-pack' }
+      let(:changes) { Gitlab::GitAccess::ANY }
+
+      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_on_push!, cmd) }.not_to change { Project.count }
+        end
+      end
+    end
+  end
+
+  def raise_not_found
+    raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found])
+  end
+end

spec/lib/gitlab/git_access_spec.rb

@@ -228,40 +228,12 @@ describe Gitlab::GitAccess do
     context 'when the project is nil' do
       let(:project) { nil }
       let(:project_path) { "new-project" }
+      let(:namespace_path) { user.namespace.path }
 
-      context 'when user is allowed to create project in namespace' do
-        let(:namespace_path) { user.namespace.path }
-        let(:access) do
-          described_class.new(actor, nil,
-            protocol, authentication_abilities: authentication_abilities,
-                      repository_path: project_path, namespace_path: namespace_path,
-                      redirected_path: redirected_path)
-        end
-
-        it 'blocks pull access with "not found"' do
+      it 'blocks push and pull with "not found"' do
+        aggregate_failures do
           expect { pull_access_check }.to raise_not_found
-        end
-
-        it 'allows push access' do
-          expect { push_access_check }.not_to raise_error
-        end
-      end
-
-      context 'when user is not allowed to create project in namespace' do
-        let(:user2) { create(:user) }
-        let(:namespace_path) { user2.namespace.path }
-        let(:access) do
-          described_class.new(actor, nil,
-            protocol, authentication_abilities: authentication_abilities,
-                      repository_path: project_path, namespace_path: namespace_path,
-                      redirected_path: redirected_path)
-        end
-
-        it 'blocks push and pull with "not found"' do
-          aggregate_failures do
-            expect { pull_access_check }.to raise_not_found
-            expect { push_access_check }.to raise_not_found
-          end
+          expect { push_access_check }.to raise_not_found
         end
       end
     end
@@ -443,106 +415,6 @@ describe Gitlab::GitAccess do
     end
   end
 
-  describe '#ensure_project_on_push!' do
-    let(:access) do
-      described_class.new(actor, project,
-        protocol, authentication_abilities: authentication_abilities,
-                  repository_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) { Gitlab::GitAccess::ANY }
-
-          context 'when authentication abilities include push code' do
-            let(:authentication_abilities) { [:push_code] }
-
-            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_on_push!, 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_on_push!, cmd, changes) }.not_to change { Project.count }
-              end
-            end
-          end
-
-          context 'when authentication abilities do not include push code' do
-            let(:authentication_abilities) { [] }
-
-            context 'when user can create project in namespace' do
-              let(:namespace_path) { user.namespace.path }
-
-              it 'does not create a new project' do
-                expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count }
-              end
-            end
-          end
-        end
-
-        context 'when check contains actual changes' do
-          let(:changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" }
-
-          it 'does not create a new project' do
-            expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count }
-          end
-        end
-      end
-
-      context 'when project exists' do
-        let(:changes) { Gitlab::GitAccess::ANY }
-        let!(:project) { create(:project) }
-
-        it 'does not create a new project' do
-          expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count }
-        end
-      end
-
-      context 'when deploy key is used' do
-        let(:key) { create(:deploy_key, user: user) }
-        let(:actor) { key }
-        let(:project_path) { "nonexistent" }
-        let(:project) { nil }
-        let(:namespace_path) { user.namespace.path }
-        let(:changes) { Gitlab::GitAccess::ANY }
-
-        it 'does not create a new project' do
-          expect { access.send(:ensure_project_on_push!, cmd, changes) }.not_to change { Project.count }
-        end
-      end
-    end
-
-    context 'when pull' do
-      let(:cmd) { 'git-upload-pack' }
-      let(:changes) { Gitlab::GitAccess::ANY }
-
-      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_on_push!, cmd, changes) }.not_to change { Project.count }
-        end
-      end
-    end
-  end
-
   describe '#check_download_access!' do
     it 'allows maintainers to pull' do
       project.add_maintainer(user)