Merge branch '207869-refactor-repository-routing' into 'master'

Generalize repository routing

See merge request gitlab-org/gitlab!45872

app/controllers/repositories/git_http_client_controller.rb

@@ -87,8 +87,12 @@ module Repositories
       @project
     end
 
+    def repository_path
+      @repository_path ||= params[:repository_path]
+    end
+
     def parse_repo_path
-      @container, @project, @repo_type, @redirected_path = Gitlab::RepoPath.parse("#{params[:namespace_id]}/#{params[:repository_id]}")
+      @container, @project, @repo_type, @redirected_path = Gitlab::RepoPath.parse(repository_path)
     end
 
     def render_missing_personal_access_token

app/controllers/repositories/git_http_controller.rb

@@ -90,7 +90,6 @@ module Repositories
     def access
       @access ||= access_klass.new(access_actor, container, 'http',
         authentication_abilities: authentication_abilities,
-        namespace_path: params[:namespace_id],
         repository_path: repository_path,
         redirected_path: redirected_path,
         auth_result_type: auth_result_type)
@@ -113,10 +112,6 @@ module Repositories
       @access_klass ||= repo_type.access_checker_class
     end
 
-    def repository_path
-      @repository_path ||= params[:repository_id].sub(/\.git$/, '')
-    end
-
     def log_user_activity
       Users::ActivityService.new(user).execute
     end

config/routes/git_http.rb

-concern :gitactionable do
-  scope(controller: :git_http) do
-    get '/info/refs', action: :info_refs
-    post '/git-upload-pack', action: :git_upload_pack
-    post '/git-receive-pack', action: :git_receive_pack
-  end
-end
-
-concern :lfsable do
-  # Git LFS API (metadata)
-  scope(path: 'info/lfs/objects', controller: :lfs_api) do
-    post :batch
-    post '/', action: :deprecated
-    get '/*oid', action: :deprecated
-  end
-
-  scope(path: 'info/lfs') do
-    resources :lfs_locks, controller: :lfs_locks_api, path: 'locks' do
-      post :unlock, on: :member
-      post :verify, on: :collection
-    end
-  end
-
-  # GitLab LFS object storage
-  scope(path: 'gitlab-lfs/objects/*oid', controller: :lfs_storage, constraints: { oid: /[a-f0-9]{64}/ }) do
-    get '/', action: :download
-
-    scope constraints: { size: /[0-9]+/ } do
-      put '/*size/authorize', action: :upload_authorize
-      put '/*size', action: :upload_finalize
-    end
-  end
-end
-
-# Git route for personal and project snippets
-scope(path: ':namespace_id/:repository_id',
-      format: nil,
-      constraints: { namespace_id: Gitlab::PathRegex.personal_and_project_snippets_path_regex, repository_id: /\d+\.git/ },
-      module: :repositories) do
-  concerns :gitactionable
-end
-
-scope(path: '*namespace_id/:repository_id',
-      format: nil,
-      constraints: { namespace_id: Gitlab::PathRegex.full_namespace_route_regex }) do
-  scope(constraints: { repository_id: Gitlab::PathRegex.project_git_route_regex }) do
+scope(path: '*repository_path', format: false) do
+  constraints(repository_path: Gitlab::PathRegex.repository_git_route_regex) do
     scope(module: :repositories) do
-      concerns :gitactionable
-      concerns :lfsable
+      # Git HTTP API
+      scope(controller: :git_http) do
+        get '/info/refs', action: :info_refs
+        post '/git-upload-pack', action: :git_upload_pack
+        post '/git-receive-pack', action: :git_receive_pack
+      end
+
+      # NOTE: LFS routes are exposed on all repository types, but we still check for
+      # LFS availability on the repository container in LfsRequest#require_lfs_enabled!
+
+      # Git LFS API (metadata)
+      scope(path: 'info/lfs/objects', controller: :lfs_api) do
+        post :batch
+        post '/', action: :deprecated
+        get '/*oid', action: :deprecated
+      end
+
+      scope(path: 'info/lfs') do
+        resources :lfs_locks, controller: :lfs_locks_api, path: 'locks' do
+          post :unlock, on: :member
+          post :verify, on: :collection
+        end
+      end
+
+      # GitLab LFS object storage
+      scope(path: 'gitlab-lfs/objects/*oid', controller: :lfs_storage, constraints: { oid: /[a-f0-9]{64}/ }) do
+        get '/', action: :download
+
+        constraints(size: /[0-9]+/) do
+          put '/*size/authorize', action: :upload_authorize
+          put '/*size', action: :upload_finalize
+        end
+      end
     end
   end
 
   # Redirect /group/project.wiki.git to the project wiki
-  scope(format: true, constraints: { repository_id: Gitlab::PathRegex.project_wiki_git_route_regex, format: :git }) do
+  constraints(repository_path: Gitlab::PathRegex.repository_wiki_git_route_regex) do
     wiki_redirect = redirect do |params, request|
-      project_id = params[:repository_id].delete_suffix('.wiki')
-      path = [params[:namespace_id], project_id, 'wikis'].join('/')
+      container_path = params[:repository_path].delete_suffix('.wiki.git')
+      path = File.join(container_path, '-', 'wikis')
       path << "?#{request.query_string}" unless request.query_string.blank?
       path
     end
@@ -63,22 +50,14 @@ scope(path: '*namespace_id/:repository_id',
   end
 
   # Redirect /group/project/info/refs to /group/project.git/info/refs
-  scope(constraints: { repository_id: Gitlab::PathRegex.project_route_regex }) do
-    # Allow /info/refs, /info/refs?service=git-upload-pack, and
-    # /info/refs?service=git-receive-pack, but nothing else.
-    #
-    git_http_handshake = lambda do |request|
-      ::Constraints::ProjectUrlConstrainer.new.matches?(request, existence_check: false) &&
-        (request.query_string.blank? ||
-         request.query_string.match(/\Aservice=git-(upload|receive)-pack\z/))
-    end
-
+  # This allows cloning a repository without the trailing `.git`
+  constraints(repository_path: Gitlab::PathRegex.repository_route_regex) do
     ref_redirect = redirect do |params, request|
-      path = "#{params[:namespace_id]}/#{params[:repository_id]}.git/info/refs"
+      path = "#{params[:repository_path]}.git/info/refs"
       path << "?#{request.query_string}" unless request.query_string.blank?
       path
     end
 
-    get '/info/refs', constraints: git_http_handshake, to: ref_redirect
+    get '/info/refs', constraints: ::Constraints::RepositoryRedirectUrlConstrainer.new, to: ref_redirect
   end
 end

ee/app/controllers/ee/repositories/git_http_controller.rb

@@ -75,11 +75,7 @@ module EE
       end
 
       def jwt_scope_valid?
-        decoded_authorization[:scope] == repository_full_path
-      end
-
-      def repository_full_path
-        File.join(params[:namespace_id], repository_path)
+        decoded_authorization[:scope] == repository_path.delete_suffix('.git')
       end
 
       def decoded_authorization

ee/spec/lib/gitlab/git_access_spec.rb

@@ -11,8 +11,7 @@ RSpec.describe Gitlab::GitAccess do
   let(:actor) { user }
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
-  let(:namespace_path) { nil }
-  let(:project_path) { nil }
+  let(:repository_path) { "#{project.full_path}.git" }
   let(:protocol) { 'web' }
   let(:authentication_abilities) { %i[read_project download_code push_code] }
   let(:redirected_path) { nil }
@@ -408,8 +407,6 @@ RSpec.describe Gitlab::GitAccess do
   end
 
   describe '#check_push_access!' do
-    let(:project_path) { project.path }
-    let(:namespace_path) { project&.namespace&.path }
     let(:protocol) { 'ssh' }
     let(:unprotected_branch) { 'unprotected_branch' }
 
@@ -771,8 +768,7 @@ RSpec.describe Gitlab::GitAccess do
       project,
       protocol,
       authentication_abilities: authentication_abilities,
-      namespace_path: namespace_path,
-      repository_path: project_path,
+      repository_path: repository_path,
       redirected_path: redirected_path
     )
   end

ee/spec/requests/repositories/git_http_controller_spec.rb

@@ -97,16 +97,23 @@ RSpec.describe Repositories::GitHttpController, type: :request do
         it_behaves_like 'triggers Geo'
       end
 
-      context 'with personal snippet' do
+      context 'with a project wiki' do
+        let_it_be(:wiki) { create(:project_wiki, :empty_repo, project: project) }
+        let_it_be(:path) { "#{wiki.full_path}.git" }
+
+        it_behaves_like 'triggers Geo'
+      end
+
+      context 'with a personal snippet' do
         let_it_be(:snippet) { create(:personal_snippet, :repository, author: user) }
-        let(:path) { "snippets/#{snippet.id}.git" }
+        let_it_be(:path) { "snippets/#{snippet.id}.git" }
 
         it_behaves_like 'triggers Geo'
       end
 
-      context 'with project snippet' do
+      context 'with a project snippet' do
         let_it_be(:snippet) { create(:project_snippet, :repository, author: user, project: project) }
-        let(:path) { "#{project.full_path}/snippets/#{snippet.id}.git" }
+        let_it_be(:path) { "#{project.full_path}/snippets/#{snippet.id}.git" }
 
         it_behaves_like 'triggers Geo'
       end

ee/spec/routing/git_http_routing_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'EE git_http routing' do
+  describe 'Geo routing' do
+    it_behaves_like 'git repository routes' do
+      let(:path) { '/-/push_from_secondary/node/gitlab-org/gitlab-test.git' }
+      let(:container_path) { '/gitlab-org/gitlab-test' }
+      let(:params) { { geo_node_id: 'node', repository_path: 'gitlab-org/gitlab-test.git' } }
+    end
+  end
+end

lib/api/helpers/internal_helpers.rb

@@ -31,8 +31,7 @@ module API
       def access_checker_for(actor, protocol)
         access_checker_klass.new(actor.key_or_user, container, protocol,
           authentication_abilities: ssh_authentication_abilities,
-          namespace_path: namespace_path,
-          repository_path: project_path,
+          repository_path: repository_path,
           redirected_path: redirected_path)
       end
 
@@ -71,18 +70,22 @@ module API
         false
       end
 
-      def project_path
-        project&.path || project_path_match[:project_path]
-      end
-
-      def namespace_path
-        project&.namespace&.full_path || project_path_match[:namespace_path]
-      end
-
       private
 
-      def project_path_match
-        @project_path_match ||= params[:project].match(Gitlab::PathRegex.full_project_git_path_regex) || {}
+      def repository_path
+        if container
+          "#{container.full_path}.git"
+        elsif params[:project]
+          # When the project doesn't exist, we still need to pass on the path
+          # to support auto-creation in `GitAccessProject`.
+          #
+          # For consistency with the Git HTTP controllers, we normalize the path
+          # to remove a leading slash and ensure a trailing `.git`.
+          #
+          # NOTE: For GitLab Shell, `params[:project]` is the full repository path
+          # from the SSH command, with an optional trailing `.git`.
+          "#{params[:project].delete_prefix('/').delete_suffix('.git')}.git"
+        end
       end
 
       # rubocop:disable Gitlab/ModuleWithInstanceVariables
@@ -96,7 +99,7 @@ module API
       end
       # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
-      # Project id to pass between components that don't share/don't have
+      # Repository id to pass between components that don't share/don't have
       # access to the same filesystem mounts
       def gl_repository
         repo_type.identifier_for_container(container)
@@ -106,8 +109,9 @@ module API
         repository.full_path
       end
 
-      # Return the repository depending on whether we want the wiki or the
-      # regular repository
+      # Return the repository for the detected type and container
+      #
+      # @returns [Repository]
       def repository
         @repository ||= repo_type.repository_for(container)
       end

lib/constraints/project_url_constrainer.rb

@@ -4,7 +4,7 @@ module Constraints
   class ProjectUrlConstrainer
     def matches?(request, existence_check: true)
       namespace_path = request.params[:namespace_id]
-      project_path = request.params[:project_id] || request.params[:id] || request.params[:repository_id]
+      project_path = request.params[:project_id] || request.params[:id]
       full_path = [namespace_path, project_path].join('/')
 
       return false unless ProjectPathValidator.valid_path?(full_path)

lib/constraints/repository_redirect_url_constrainer.rb

+# frozen_string_literal: true
+
+module Constraints
+  class RepositoryRedirectUrlConstrainer
+    def matches?(request)
+      path = request.params[:repository_path].delete_suffix('.git')
+      query = request.query_string
+
+      git_request?(query) && container_path?(path)
+    end
+
+    # Allow /info/refs, /info/refs?service=git-upload-pack, and
+    # /info/refs?service=git-receive-pack, but nothing else.
+    def git_request?(query)
+      query.blank? ||
+        query == 'service=git-upload-pack' ||
+        query == 'service=git-receive-pack'
+    end
+
+    # Check if the path matches any known repository containers.
+    # These also cover wikis, since a `.wiki` suffix is valid in project/group paths too.
+    def container_path?(path)
+      NamespacePathValidator.valid_path?(path) ||
+        ProjectPathValidator.valid_path?(path) ||
+        path =~ Gitlab::PathRegex.full_snippets_repository_path_regex
+    end
+  end
+end

lib/gitlab/git_access.rb

@@ -43,7 +43,7 @@ module Gitlab
     ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS
 
     attr_reader :actor, :protocol, :authentication_abilities,
-                :namespace_path, :redirected_path, :auth_result_type,
+                :repository_path, :redirected_path, :auth_result_type,
                 :cmd, :changes
     attr_accessor :container
 
@@ -57,21 +57,16 @@ module Gitlab
       raise ArgumentError, "No error message defined for #{key}"
     end
 
-    def initialize(actor, container, protocol, authentication_abilities:, namespace_path: nil, repository_path: nil, redirected_path: nil, auth_result_type: nil)
+    def initialize(actor, container, protocol, authentication_abilities:, repository_path: nil, redirected_path: nil, auth_result_type: nil)
       @actor     = actor
       @container = container
       @protocol  = protocol
       @authentication_abilities = Array(authentication_abilities)
-      @namespace_path = namespace_path
       @repository_path = repository_path
       @redirected_path = redirected_path
       @auth_result_type = auth_result_type
     end
 
-    def repository_path
-      @repository_path ||= project&.path
-    end
-
     def check(cmd, changes)
       @changes = changes
       @cmd = cmd

lib/gitlab/git_access_project.rb

@@ -35,7 +35,19 @@ module Gitlab
     end
 
     def namespace
-      @namespace ||= Namespace.find_by_full_path(namespace_path)
+      strong_memoize(:namespace) { Namespace.find_by_full_path(namespace_path) }
+    end
+
+    def namespace_path
+      strong_memoize(:namespace_path) { repository_path_match[:namespace_path] }
+    end
+
+    def project_path
+      strong_memoize(:project_path) { repository_path_match[:project_path] }
+    end
+
+    def repository_path_match
+      strong_memoize(:repository_path_match) { repository_path.match(Gitlab::PathRegex.full_project_git_path_regex) || {} }
     end
 
     def ensure_project_on_push!
@@ -44,7 +56,7 @@ module Gitlab
       return unless user&.can?(:create_projects, namespace)
 
       project_params = {
-        path: repository_path,
+        path: project_path,
         namespace_id: namespace.id,
         visibility_level: Gitlab::VisibilityLevel::PRIVATE
       }

lib/gitlab/path_regex.rb

@@ -180,12 +180,16 @@ module Gitlab
       end
     end
 
-    def project_git_route_regex
-      @project_git_route_regex ||= /#{project_route_regex}\.git/.freeze
+    def repository_route_regex
+      @repository_route_regex ||= /#{full_namespace_route_regex}|#{personal_snippet_repository_path_regex}/.freeze
     end
 
-    def project_wiki_git_route_regex
-      @project_wiki_git_route_regex ||= /#{PATH_REGEX_STR}\.wiki/.freeze
+    def repository_git_route_regex
+      @repository_git_route_regex ||= /#{repository_route_regex}\.git/.freeze
+    end
+
+    def repository_wiki_git_route_regex
+      @repository_wiki_git_route_regex ||= /#{full_namespace_route_regex}\.wiki\.git/.freeze
     end
 
     def full_namespace_path_regex
@@ -250,10 +254,6 @@ module Gitlab
       %r{\A(#{personal_snippet_repository_path_regex}|#{project_snippet_repository_path_regex})\z}
     end
 
-    def personal_and_project_snippets_path_regex
-      %r{#{personal_snippet_path_regex}|#{project_snippet_path_regex}}
-    end
-
     def container_image_regex
       @container_image_regex ||= %r{([\w\.-]+\/){0,1}[\w\.-]+}.freeze
     end

lib/gitlab/repo_path.rb

@@ -5,7 +5,7 @@ module Gitlab
     NotFoundError = Class.new(StandardError)
 
     def self.parse(path)
-      repo_path = path.sub(/\.git\z/, '').sub(%r{\A/}, '')
+      repo_path = path.delete_prefix('/').delete_suffix('.git')
       redirected_path = nil
 
       # Detect the repo type based on the path, the first one tried is the project

spec/controllers/repositories/git_http_controller_spec.rb

@@ -9,28 +9,20 @@ RSpec.describe Repositories::GitHttpController do
   let_it_be(:personal_snippet) { create(:personal_snippet, :public, :repository) }
   let_it_be(:project_snippet) { create(:project_snippet, :public, :repository, project: project) }
 
-  let(:namespace_id) { project.namespace.to_param }
-  let(:repository_id) { project.path + '.git' }
-  let(:container_params) do
-    {
-      namespace_id: namespace_id,
-      repository_id: repository_id
-    }
-  end
-
-  let(:params) { container_params }
+  shared_examples Repositories::GitHttpController do
+    let(:repository_path) { "#{container.full_path}.git" }
+    let(:params) { { repository_path: repository_path } }
 
-  describe 'HEAD #info_refs' do
-    it 'returns 403' do
-      head :info_refs, params: params
+    describe 'HEAD #info_refs' do
+      it 'returns 403' do
+        head :info_refs, params: params
 
-      expect(response).to have_gitlab_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
     end
-  end
 
-  shared_examples 'info_refs behavior' do
     describe 'GET #info_refs' do
-      let(:params) { container_params.merge(service: 'git-upload-pack') }
+      let(:params) { super().merge(service: 'git-upload-pack') }
 
       it 'returns 401 for unauthenticated requests to public repositories when http protocol is disabled' do
         stub_application_setting(enabled_git_access_protocol: 'ssh')
@@ -43,6 +35,26 @@ RSpec.describe Repositories::GitHttpController do
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
 
+      it 'calls the right access checker class with the right object' do
+        allow(controller).to receive(:verify_workhorse_api!).and_return(true)
+
+        access_double = double
+        options = {
+          authentication_abilities: [:download_code],
+          repository_path: repository_path,
+          redirected_path: nil,
+          auth_result_type: :none
+        }
+
+        expect(access_checker_class).to receive(:new)
+          .with(nil, container, 'http', hash_including(options))
+          .and_return(access_double)
+
+        allow(access_double).to receive(:check).and_return(false)
+
+        get :info_refs, params: params
+      end
+
       context 'with authorized user' do
         before do
           request.headers.merge! auth_env(user.username, user.password, nil)
@@ -97,125 +109,89 @@ RSpec.describe Repositories::GitHttpController do
         end
       end
     end
-  end
 
-  shared_examples 'git_upload_pack behavior' do |expected|
     describe 'POST #git_upload_pack' do
       before do
-        allow(controller).to receive(:authenticate_user).and_return(true)
         allow(controller).to receive(:verify_workhorse_api!).and_return(true)
-        allow(controller).to receive(:access_check).and_return(nil)
       end
 
-      def send_request
+      it 'returns 200' do
         post :git_upload_pack, params: params
+
+        expect(response).to have_gitlab_http_status(:ok)
       end
+    end
+  end
+
+  context 'when repository container is a project' do
+    it_behaves_like Repositories::GitHttpController do
+      let(:container) { project }
+      let(:user) { project.owner }
+      let(:access_checker_class) { Gitlab::GitAccess }
 
-      context 'on a read-only instance' do
+      describe 'POST #git_upload_pack' do
         before do
-          allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+          allow(controller).to receive(:verify_workhorse_api!).and_return(true)
         end
 
-        it 'does not update project statistics' do
-          expect(ProjectDailyStatisticsWorker).not_to receive(:perform_async)
-
-          send_request
+        def send_request
+          post :git_upload_pack, params: params
         end
-      end
 
-      if expected
-        context 'when project_statistics_sync feature flag is disabled' do
+        context 'on a read-only instance' do
           before do
-            stub_feature_flags(project_statistics_sync: false)
+            allow(Gitlab::Database).to receive(:read_only?).and_return(true)
           end
 
-          it 'updates project statistics async' do
-            expect(ProjectDailyStatisticsWorker).to receive(:perform_async)
+          it 'does not update project statistics' do
+            expect(ProjectDailyStatisticsWorker).not_to receive(:perform_async)
 
             send_request
           end
         end
 
-        it 'updates project statistics sync' do
-          expect { send_request }.to change {
-            Projects::DailyStatisticsFinder.new(project).total_fetch_count
-          }.from(0).to(1)
-        end
-      else
         context 'when project_statistics_sync feature flag is disabled' do
           before do
             stub_feature_flags(project_statistics_sync: false)
           end
 
-          it 'does not update project statistics' do
-            expect(ProjectDailyStatisticsWorker).not_to receive(:perform_async)
+          it 'updates project statistics async for projects' do
+            expect(ProjectDailyStatisticsWorker).to receive(:perform_async)
 
             send_request
           end
         end
 
-        it 'does not update project statistics' do
-          expect { send_request }.not_to change {
-            Projects::DailyStatisticsFinder.new(project).total_fetch_count
-          }.from(0)
+        it 'updates project statistics sync for projects' do
+          expect { send_request }.to change {
+            Projects::DailyStatisticsFinder.new(container).total_fetch_count
+          }.from(0).to(1)
         end
       end
     end
   end
 
-  shared_examples 'access checker class' do
-    let(:params) { container_params.merge(service: 'git-upload-pack') }
-
-    it 'calls the right access class checker with the right object' do
-      allow(controller).to receive(:verify_workhorse_api!).and_return(true)
-
-      access_double = double
-      expect(expected_class).to receive(:new).with(anything, expected_object, 'http', anything).and_return(access_double)
-      allow(access_double).to receive(:check).and_return(false)
-
-      get :info_refs, params: params
-    end
-  end
-
-  context 'when repository container is a project' do
-    it_behaves_like 'info_refs behavior' do
+  context 'when repository container is a project wiki' do
+    it_behaves_like Repositories::GitHttpController do
+      let(:container) { create(:project_wiki, :empty_repo, project: project) }
       let(:user) { project.owner }
-    end
-
-    it_behaves_like 'git_upload_pack behavior', true
-    it_behaves_like 'access checker class' do
-      let(:expected_class) { Gitlab::GitAccess }
-      let(:expected_object) { project }
+      let(:access_checker_class) { Gitlab::GitAccessWiki }
     end
   end
 
   context 'when repository container is a personal snippet' do
-    let(:namespace_id) { 'snippets' }
-    let(:repository_id) { personal_snippet.to_param + '.git' }
-
-    it_behaves_like 'info_refs behavior' do
+    it_behaves_like Repositories::GitHttpController do
+      let(:container) { personal_snippet }
       let(:user) { personal_snippet.author }
-    end
-
-    it_behaves_like 'git_upload_pack behavior', false
-    it_behaves_like 'access checker class' do
-      let(:expected_class) { Gitlab::GitAccessSnippet }
-      let(:expected_object) { personal_snippet }
+      let(:access_checker_class) { Gitlab::GitAccessSnippet }
     end
   end
 
   context 'when repository container is a project snippet' do
-    let(:namespace_id) { project.full_path + '/snippets' }
-    let(:repository_id) { project_snippet.to_param + '.git' }
-
-    it_behaves_like 'info_refs behavior' do
+    it_behaves_like Repositories::GitHttpController do
+      let(:container) { project_snippet }
       let(:user) { project_snippet.author }
-    end
-
-    it_behaves_like 'git_upload_pack behavior', false
-    it_behaves_like 'access checker class' do
-      let(:expected_class) { Gitlab::GitAccessSnippet }
-      let(:expected_object) { project_snippet }
+      let(:access_checker_class) { Gitlab::GitAccessSnippet }
     end
   end
 end

spec/controllers/repositories/lfs_storage_controller_spec.rb

@@ -23,8 +23,7 @@ RSpec.describe Repositories::LfsStorageController do
 
     let(:params) do
       {
-        namespace_id: project.namespace.path,
-        repository_id: "#{project.path}.git",
+        repository_path: "#{project.full_path}.git",
         oid: '6b9765d3888aaec789e8c309eb05b05c3a87895d6ad70d2264bd7270fff665ac',
         size: '6725030'
       }

spec/factories/sequences.rb

@@ -13,5 +13,6 @@ FactoryBot.define do
   sequence(:past_time) { |n| 4.hours.ago + (2 * n).seconds }
   sequence(:iid)
   sequence(:sha) { |n| Digest::SHA1.hexdigest("commit-like-#{n}") }
+  sequence(:oid) { |n| Digest::SHA2.hexdigest("oid-like-#{n}") }
   sequence(:variable) { |n| "var#{n}" }
 end

spec/lib/gitlab/git_access_project_spec.rb

@@ -9,6 +9,7 @@ RSpec.describe Gitlab::GitAccessProject do
   let(:actor) { user }
   let(:project_path) { project.path }
   let(:namespace_path) { project&.namespace&.path }
+  let(:repository_path) { "#{namespace_path}/#{project_path}.git" }
   let(:protocol) { 'ssh' }
   let(:authentication_abilities) { %i[read_project download_code push_code] }
   let(:changes) { Gitlab::GitAccess::ANY }
@@ -17,7 +18,7 @@ RSpec.describe Gitlab::GitAccessProject do
   let(:access) do
     described_class.new(actor, container, protocol,
                         authentication_abilities: authentication_abilities,
-                        repository_path: project_path, namespace_path: namespace_path)
+                        repository_path: repository_path)
   end
 
   describe '#check_namespace!' do
@@ -103,6 +104,20 @@ RSpec.describe Gitlab::GitAccessProject do
               end
             end
 
+            context 'when namespace is blank' do
+              let(:repository_path) { 'project.git' }
+
+              it_behaves_like 'no project is created' do
+                let(:raise_specific_error) { raise_namespace_not_found }
+              end
+            end
+
+            context 'when namespace does not exist' do
+              let(:namespace_path) { 'unknown' }
+
+              it_behaves_like 'no project is created'
+            end
+
             context 'when user cannot create project in namespace' do
               let(:user2) { create(:user) }
               let(:namespace_path) { user2.namespace.path }

spec/lib/gitlab/git_access_spec.rb

@@ -10,8 +10,7 @@ RSpec.describe Gitlab::GitAccess do
 
   let(:actor) { user }
   let(:project) { create(:project, :repository) }
-  let(:project_path) { project&.path }
-  let(:namespace_path) { project&.namespace&.path }
+  let(:repository_path) { "#{project.full_path}.git" }
   let(:protocol) { 'ssh' }
   let(:authentication_abilities) { %i[read_project download_code push_code] }
   let(:redirected_path) { nil }
@@ -210,10 +209,9 @@ RSpec.describe Gitlab::GitAccess do
       end
     end
 
-    context 'when the project is nil' do
+    context 'when the project does not exist' do
       let(:project) { nil }
-      let(:project_path) { "new-project" }
-      let(:namespace_path) { user.namespace.path }
+      let(:repository_path) { "#{user.namespace.path}/new-project.git" }
 
       it 'blocks push and pull with "not found"' do
         aggregate_failures do
@@ -452,9 +450,8 @@ RSpec.describe Gitlab::GitAccess do
 
       context 'when project is public' do
         let(:public_project) { create(:project, :public, :repository) }
-        let(:project_path) { public_project.path }
-        let(:namespace_path) { public_project.namespace.path }
-        let(:access) { access_class.new(nil, public_project, 'web', authentication_abilities: [:download_code], repository_path: project_path, namespace_path: namespace_path) }
+        let(:repository_path) { "#{public_project.full_path}.git" }
+        let(:access) { access_class.new(nil, public_project, 'web', authentication_abilities: [:download_code], repository_path: repository_path) }
 
         context 'when repository is enabled' do
           it 'give access to download code' do
@@ -1169,7 +1166,7 @@ RSpec.describe Gitlab::GitAccess do
   def access
     access_class.new(actor, project, protocol,
                         authentication_abilities: authentication_abilities,
-                        namespace_path: namespace_path, repository_path: project_path,
+                        repository_path: repository_path,
                         redirected_path: redirected_path, auth_result_type: auth_result_type)
   end
 

spec/lib/gitlab/path_regex_spec.rb

@@ -433,37 +433,85 @@ RSpec.describe Gitlab::PathRegex do
     it { is_expected.not_to match('gitlab.git') }
   end
 
-  shared_examples 'invalid snippet routes' do
-    it { is_expected.not_to match('gitlab-org/gitlab/snippets/1.git') }
-    it { is_expected.not_to match('snippets/1.git') }
-    it { is_expected.not_to match('gitlab-org/gitlab/snippets/') }
-    it { is_expected.not_to match('/gitlab-org/gitlab/snippets/1') }
-    it { is_expected.not_to match('gitlab-org/gitlab/snippets/foo') }
-    it { is_expected.not_to match('root/snippets/1') }
-    it { is_expected.not_to match('/snippets/1') }
-    it { is_expected.not_to match('snippets/') }
-    it { is_expected.not_to match('snippets/foo') }
-  end
+  context 'repository routes' do
+    # Paths that match a known container
+    let_it_be(:container_paths) do
+      [
+        'gitlab-org',
+        'gitlab-org/gitlab-test',
+        'gitlab-org/gitlab-test/snippets/1',
+        'gitlab-org/gitlab-test/snippets/foo', # ambiguous, we allow creating a sub-group called 'snippets'
+        'snippets/1'
+      ]
+    end
+
+    # Paths that never match a container
+    let_it_be(:invalid_paths) do
+      [
+        'gitlab/',
+        '/gitlab',
+        'gitlab/foo/',
+        '?gitlab',
+        'git lab',
+        '/snippets/1',
+        'snippets/foo',
+        'gitlab-org/gitlab/snippets/'
+      ]
+    end
+
+    let_it_be(:git_paths) { container_paths.map { |path| path + '.git' } }
+    let_it_be(:snippet_paths) { container_paths.grep(%r{snippets/\d}) }
+    let_it_be(:wiki_git_paths) { (container_paths - snippet_paths).map { |path| path + '.wiki.git' } }
+    let_it_be(:invalid_git_paths) { invalid_paths.map { |path| path + '.git' } }
+
+    def expect_route_match(paths)
+      paths.each { |path| is_expected.to match(path) }
+    end
+
+    def expect_no_route_match(paths)
+      paths.each { |path| is_expected.not_to match(path) }
+    end
+
+    describe '.repository_route_regex' do
+      subject { %r{\A#{described_class.repository_route_regex}\z} }
+
+      it 'matches the expected paths' do
+        expect_route_match(container_paths)
+        expect_no_route_match(invalid_paths + git_paths)
+      end
+    end
 
-  describe '.full_snippets_repository_path_regex' do
-    subject { described_class.full_snippets_repository_path_regex }
+    describe '.repository_git_route_regex' do
+      subject { %r{\A#{described_class.repository_git_route_regex}\z} }
 
-    it { is_expected.to match('gitlab-org/gitlab/snippets/1') }
-    it { is_expected.to match('snippets/1') }
+      it 'matches the expected paths' do
+        expect_route_match(git_paths + wiki_git_paths)
+        expect_no_route_match(container_paths + invalid_paths + invalid_git_paths)
+      end
+    end
 
-    it_behaves_like 'invalid snippet routes'
-  end
+    describe '.repository_wiki_git_route_regex' do
+      subject { %r{\A#{described_class.repository_wiki_git_route_regex}\z} }
 
-  describe '.personal_and_project_snippets_path_regex' do
-    subject { %r{\A#{described_class.personal_and_project_snippets_path_regex}\z} }
+      it 'matches the expected paths' do
+        expect_route_match(wiki_git_paths)
+        expect_no_route_match(git_paths + invalid_git_paths)
+      end
 
-    it { is_expected.to match('gitlab-org/gitlab/snippets') }
-    it { is_expected.to match('snippets') }
+      it { is_expected.not_to match('snippets/1.wiki.git') }
+    end
 
-    it { is_expected.not_to match('gitlab-org/gitlab/snippets/1') }
-    it { is_expected.not_to match('snippets/1') }
+    describe '.full_snippets_repository_path_regex' do
+      subject { described_class.full_snippets_repository_path_regex }
 
-    it_behaves_like 'invalid snippet routes'
+      it 'matches the expected paths' do
+        expect_route_match(snippet_paths)
+        expect_no_route_match(container_paths - snippet_paths + git_paths + invalid_paths)
+      end
+
+      it { is_expected.not_to match('root/snippets/1') }
+      it { is_expected.not_to match('gitlab-org/gitlab-test/snippets/foo') }
+    end
   end
 
   describe '.container_image_regex' do

spec/requests/api/internal/base_spec.rb

@@ -722,8 +722,7 @@ RSpec.describe API::Internal::Base do
           'ssh',
           {
             authentication_abilities: [:read_project, :download_code, :push_code],
-            namespace_path: project.namespace.path,
-            repository_path: project.path,
+            repository_path: "#{project.full_path}.git",
             redirected_path: nil
           }
         ).and_return(access_checker)

spec/routing/git_http_routing_spec.rb

@@ -3,22 +3,60 @@
 require 'spec_helper'
 
 RSpec.describe 'git_http routing' do
-  include RSpec::Rails::RequestExampleGroup
+  describe 'code repositories' do
+    it_behaves_like 'git repository routes' do
+      let(:path) { '/gitlab-org/gitlab-test.git' }
+    end
+  end
+
+  describe 'wiki repositories' do
+    context 'in project' do
+      let(:path) { '/gitlab-org/gitlab-test.wiki.git' }
+
+      it_behaves_like 'git repository routes'
+
+      describe 'redirects', type: :request do
+        let(:web_path) { '/gitlab-org/gitlab-test/-/wikis' }
+
+        it 'redirects namespace/project.wiki.git to the project wiki' do
+          expect(get(path)).to redirect_to(web_path)
+        end
 
-  describe 'wiki.git routing', 'routing' do
-    let(:wiki_path)  { '/gitlab/gitlabhq/wikis' }
+        it 'preserves query parameters' do
+          expect(get("#{path}?foo=bar&baz=qux")).to redirect_to("#{web_path}?foo=bar&baz=qux")
+        end
 
-    it 'redirects namespace/project.wiki.git to the project wiki' do
-      expect(get('/gitlab/gitlabhq.wiki.git')).to redirect_to(wiki_path)
+        it 'only redirects when the format is .git' do
+          expect(get(path.delete_suffix('.git'))).not_to redirect_to(web_path)
+          expect(get(path.delete_suffix('.git') + '.json')).not_to redirect_to(web_path)
+        end
+      end
     end
 
-    it 'preserves query parameters' do
-      expect(get('/gitlab/gitlabhq.wiki.git?foo=bar&baz=qux')).to redirect_to("#{wiki_path}?foo=bar&baz=qux")
+    context 'in toplevel group' do
+      it_behaves_like 'git repository routes' do
+        let(:path) { '/gitlab-org.wiki.git' }
+      end
+    end
+
+    context 'in child group' do
+      it_behaves_like 'git repository routes' do
+        let(:path) { '/gitlab-org/child.wiki.git' }
+      end
+    end
+  end
+
+  describe 'snippet repositories' do
+    context 'personal snippet' do
+      it_behaves_like 'git repository routes' do
+        let(:path) { '/snippets/123.git' }
+      end
     end
 
-    it 'only redirects when the format is .git' do
-      expect(get('/gitlab/gitlabhq.wiki')).not_to redirect_to(wiki_path)
-      expect(get('/gitlab/gitlabhq.wiki.json')).not_to redirect_to(wiki_path)
+    context 'project snippet' do
+      it_behaves_like 'git repository routes' do
+        let(:path) { '/gitlab-org/gitlab-test/snippets/123.git' }
+      end
     end
   end
 end

spec/routing/routing_spec.rb

@@ -54,10 +54,6 @@ RSpec.describe "Mounted Apps", "routing" do
   it "to API" do
     expect(get("/api/issues")).to be_routable
   end
-
-  it "to Grack" do
-    expect(get("/gitlab/gitlabhq.git")).to be_routable
-  end
 end
 
 #     snippets GET    /snippets(.:format)          snippets#index

spec/support/helpers/git_http_helpers.rb

@@ -5,45 +5,45 @@ require_relative 'workhorse_helpers'
 module GitHttpHelpers
   include WorkhorseHelpers
 
-  def clone_get(project, **options)
-    get "/#{project}/info/refs", params: { service: 'git-upload-pack' }, headers: auth_env(*options.values_at(:user, :password, :spnego_request_token))
+  def clone_get(repository_path, **options)
+    get "/#{repository_path}/info/refs", params: { service: 'git-upload-pack' }, headers: auth_env(*options.values_at(:user, :password, :spnego_request_token))
   end
 
-  def clone_post(project, **options)
-    post "/#{project}/git-upload-pack", headers: auth_env(*options.values_at(:user, :password, :spnego_request_token))
+  def clone_post(repository_path, **options)
+    post "/#{repository_path}/git-upload-pack", headers: auth_env(*options.values_at(:user, :password, :spnego_request_token))
   end
 
-  def push_get(project, **options)
-    get "/#{project}/info/refs", params: { service: 'git-receive-pack' }, headers: auth_env(*options.values_at(:user, :password, :spnego_request_token))
+  def push_get(repository_path, **options)
+    get "/#{repository_path}/info/refs", params: { service: 'git-receive-pack' }, headers: auth_env(*options.values_at(:user, :password, :spnego_request_token))
   end
 
-  def push_post(project, **options)
-    post "/#{project}/git-receive-pack", headers: auth_env(*options.values_at(:user, :password, :spnego_request_token))
+  def push_post(repository_path, **options)
+    post "/#{repository_path}/git-receive-pack", headers: auth_env(*options.values_at(:user, :password, :spnego_request_token))
   end
 
-  def download(project, user: nil, password: nil, spnego_request_token: nil)
+  def download(repository_path, user: nil, password: nil, spnego_request_token: nil)
     args = { user: user, password: password, spnego_request_token: spnego_request_token }
 
-    clone_get(project, **args)
+    clone_get(repository_path, **args)
     yield response
 
-    clone_post(project, **args)
+    clone_post(repository_path, **args)
     yield response
   end
 
-  def upload(project, user: nil, password: nil, spnego_request_token: nil)
+  def upload(repository_path, user: nil, password: nil, spnego_request_token: nil)
     args = { user: user, password: password, spnego_request_token: spnego_request_token }
 
-    push_get(project, **args)
+    push_get(repository_path, **args)
     yield response
 
-    push_post(project, **args)
+    push_post(repository_path, **args)
     yield response
   end
 
-  def download_or_upload(project, **args, &block)
-    download(project, **args, &block)
-    upload(project, **args, &block)
+  def download_or_upload(repository_path, **args, &block)
+    download(repository_path, **args, &block)
+    upload(repository_path, **args, &block)
   end
 
   def auth_env(user, password, spnego_request_token)

spec/support/shared_examples/routing/git_http_routing_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'git repository routes' do
+  let(:params) { { repository_path: path.delete_prefix('/') } }
+  let(:container_path) { path.delete_suffix('.git') }
+
+  it 'routes Git endpoints' do
+    expect(get("#{path}/info/refs")).to route_to('repositories/git_http#info_refs', **params)
+    expect(post("#{path}/git-upload-pack")).to route_to('repositories/git_http#git_upload_pack', **params)
+    expect(post("#{path}/git-receive-pack")).to route_to('repositories/git_http#git_receive_pack', **params)
+  end
+
+  context 'requests without .git format' do
+    it 'redirects requests to /info/refs', type: :request do
+      expect(get("#{container_path}/info/refs")).to redirect_to("#{container_path}.git/info/refs")
+      expect(get("#{container_path}/info/refs?service=git-upload-pack")).to redirect_to("#{container_path}.git/info/refs?service=git-upload-pack")
+      expect(get("#{container_path}/info/refs?service=git-receive-pack")).to redirect_to("#{container_path}.git/info/refs?service=git-receive-pack")
+    end
+
+    it 'does not redirect other requests' do
+      expect(post("#{container_path}/git-upload-pack")).not_to be_routable
+    end
+  end
+
+  it 'routes LFS endpoints' do
+    oid = generate(:oid)
+
+    expect(post("#{path}/info/lfs/objects/batch")).to route_to('repositories/lfs_api#batch', **params)
+    expect(post("#{path}/info/lfs/objects")).to route_to('repositories/lfs_api#deprecated', **params)
+    expect(get("#{path}/info/lfs/objects/#{oid}")).to route_to('repositories/lfs_api#deprecated', oid: oid, **params)
+
+    expect(post("#{path}/info/lfs/locks/123/unlock")).to route_to('repositories/lfs_locks_api#unlock', id: '123', **params)
+    expect(post("#{path}/info/lfs/locks/verify")).to route_to('repositories/lfs_locks_api#verify', **params)
+
+    expect(get("#{path}/gitlab-lfs/objects/#{oid}")).to route_to('repositories/lfs_storage#download', oid: oid, **params)
+    expect(put("#{path}/gitlab-lfs/objects/#{oid}/456/authorize")).to route_to('repositories/lfs_storage#upload_authorize', oid: oid, size: '456', **params)
+    expect(put("#{path}/gitlab-lfs/objects/#{oid}/456")).to route_to('repositories/lfs_storage#upload_finalize', oid: oid, size: '456', **params)
+
+    expect(put("#{path}/gitlab-lfs/objects/foo")).not_to be_routable
+    expect(put("#{path}/gitlab-lfs/objects/#{oid}/foo")).not_to be_routable
+    expect(put("#{path}/gitlab-lfs/objects/#{oid}/foo/authorize")).not_to be_routable
+  end
+end