Merge branch '24496-fix-internal-api-project-lookup' into 'master'

Fix POST /internal/allowed to cope with gitlab-shell v4.0.0 project paths

Closes #24496

See merge request !7480

7 files changed, 126 insertions, 61 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index a035768a6c1..146424d2b1c 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -15,16 +15,6 @@ class Repository
     Gitlab.config.repositories.storages
   end
 
-  def self.remove_storage_from_path(repo_path)
-    storages.find do |_, storage_path|
-      if repo_path.start_with?(storage_path)
-        return repo_path.sub(storage_path, '')
-      end
-    end
-
-    repo_path
-  end
-
   def initialize(path_with_namespace, project)
     @path_with_namespace = path_with_namespace
     @project = project
diff --git a/changelogs/unreleased/24496-fix-internal-api-project-lookup.yml b/changelogs/unreleased/24496-fix-internal-api-project-lookup.yml
new file mode 100644
index 00000000000..a95295c00f3
--- /dev/null
+++ b/changelogs/unreleased/24496-fix-internal-api-project-lookup.yml
@@ -0,0 +1,4 @@
+---
+title: Fix POST /internal/allowed to cope with gitlab-shell v4.0.0 project paths
+merge_request: 7480
+author:
diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb
new file mode 100644
index 00000000000..eb223c1101d
--- /dev/null
+++ b/lib/api/helpers/internal_helpers.rb
@@ -0,0 +1,57 @@
+module API
+  module Helpers
+    module InternalHelpers
+      # Project paths may be any of the following:
+      #   * /repository/storage/path/namespace/project
+      #   * /namespace/project
+      #   * namespace/project
+      #
+      # In addition, they may have a '.git' extension and multiple namespaces
+      #
+      # Transform all these cases to 'namespace/project'
+      def clean_project_path(project_path, storage_paths = Repository.storages.values)
+        project_path = project_path.sub(/\.git\z/, '')
+
+        storage_paths.each do |storage_path|
+          storage_path = File.expand_path(storage_path)
+
+          if project_path.start_with?(storage_path)
+            project_path = project_path.sub(storage_path, '')
+            break
+          end
+        end
+
+        project_path.sub(/\A\//, '')
+      end
+
+      def project_path
+        @project_path ||= clean_project_path(params[:project])
+      end
+
+      def wiki?
+        @wiki ||= project_path.end_with?('.wiki') &&
+          !Project.find_with_namespace(project_path)
+      end
+
+      def project
+        @project ||= begin
+          # Check for *.wiki repositories.
+          # Strip out the .wiki from the pathname before finding the
+          # project. This applies the correct project permissions to
+          # the wiki repository as well.
+          project_path.chomp!('.wiki') if wiki?
+
+          Project.find_with_namespace(project_path)
+        end
+      end
+
+      def ssh_authentication_abilities
+        [
+          :read_project,
+          :download_code,
+          :push_code
+        ]
+      end
+    end
+  end
+end
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index ccf181402f9..7087ce11401 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -3,6 +3,8 @@ module API
   class Internal < Grape::API
     before { authenticate_by_gitlab_shell_token! }
 
+    helpers ::API::Helpers::InternalHelpers
+
     namespace 'internal' do
       # Check if git command is allowed to project
       #
@@ -14,42 +16,6 @@ module API
       #   ref - branch name
       #   forced_push - forced_push
       #   protocol - Git access protocol being used, e.g. HTTP or SSH
-      #
-
-      helpers do
-        def project_path
-          @project_path ||= begin
-            project_path = params[:project].sub(/\.git\z/, '')
-            Repository.remove_storage_from_path(project_path)
-          end
-        end
-
-        def wiki?
-          @wiki ||= project_path.end_with?('.wiki') &&
-            !Project.find_with_namespace(project_path)
-        end
-
-        def project
-          @project ||= begin
-            # Check for *.wiki repositories.
-            # Strip out the .wiki from the pathname before finding the
-            # project. This applies the correct project permissions to
-            # the wiki repository as well.
-            project_path.chomp!('.wiki') if wiki?
-
-            Project.find_with_namespace(project_path)
-          end
-        end
-
-        def ssh_authentication_abilities
-          [
-            :read_project,
-            :download_code,
-            :push_code
-          ]
-        end
-      end
-
       post "/allowed" do
         status 200
 
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 3bd5741f2b7..2470d504c68 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1556,14 +1556,4 @@ describe Repository, models: true do
       end.to raise_error(Repository::CommitError)
     end
   end
-
-  describe '#remove_storage_from_path' do
-    let(:storage_path) { project.repository_storage_path }
-    let(:project_path) { project.path_with_namespace }
-    let(:full_path) { File.join(storage_path, project_path) }
-
-    it { expect(Repository.remove_storage_from_path(full_path)).to eq(project_path) }
-    it { expect(Repository.remove_storage_from_path(project_path)).to eq(project_path) }
-    it { expect(Repository.remove_storage_from_path(storage_path)).to eq('') }
-  end
 end
diff --git a/spec/requests/api/api_internal_helpers_spec.rb b/spec/requests/api/api_internal_helpers_spec.rb
new file mode 100644
index 00000000000..be4bc39ada2
--- /dev/null
+++ b/spec/requests/api/api_internal_helpers_spec.rb
@@ -0,0 +1,32 @@
+require 'spec_helper'
+
+describe ::API::Helpers::InternalHelpers do
+  include ::API::Helpers::InternalHelpers
+
+  describe '.clean_project_path' do
+    project = 'namespace/project'
+    namespaced = File.join('namespace2', project)
+
+    {
+      File.join(Dir.pwd, project)    => project,
+      File.join(Dir.pwd, namespaced) => namespaced,
+      project                        => project,
+      namespaced                     => namespaced,
+      project + '.git'               => project,
+      namespaced + '.git'            => namespaced,
+      "/" + project                  => project,
+      "/" + namespaced               => namespaced,
+    }.each do |project_path, expected|
+      context project_path do
+        # Relative and absolute storage paths, with and without trailing /
+        ['.', './', Dir.pwd, Dir.pwd + '/'].each do |storage_path|
+          context "storage path is #{storage_path}" do
+            subject { clean_project_path(project_path, [storage_path]) }
+
+            it { is_expected.to eq(expected) }
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index f0f590b0331..8f1a1f9e827 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -191,6 +191,26 @@ describe API::API, api: true  do
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
         end
+
+        context 'project as /namespace/project' do
+          it do
+            pull(key, project_with_repo_path('/' + project.path_with_namespace))
+
+            expect(response).to have_http_status(200)
+            expect(json_response["status"]).to be_truthy
+            expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
+          end
+        end
+
+        context 'project as namespace/project' do
+          it do
+            pull(key, project_with_repo_path(project.path_with_namespace))
+
+            expect(response).to have_http_status(200)
+            expect(json_response["status"]).to be_truthy
+            expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
+          end
+        end
       end
     end
 
@@ -299,7 +319,7 @@ describe API::API, api: true  do
 
     context 'project does not exist' do
       it do
-        pull(key, OpenStruct.new(path_with_namespace: 'gitlab/notexists'))
+        pull(key, project_with_repo_path('gitlab/notexist'))
 
         expect(response).to have_http_status(200)
         expect(json_response["status"]).to be_falsey
@@ -392,11 +412,17 @@ describe API::API, api: true  do
     end
   end
 
+  def project_with_repo_path(path)
+    double().tap do |fake_project|
+      allow(fake_project).to receive_message_chain('repository.path_to_repo' => path)
+    end
+  end
+
   def pull(key, project, protocol = 'ssh')
     post(
       api("/internal/allowed"),
       key_id: key.id,
-      project: project.path_with_namespace,
+      project: project.repository.path_to_repo,
       action: 'git-upload-pack',
       secret_token: secret_token,
       protocol: protocol
@@ -408,7 +434,7 @@ describe API::API, api: true  do
       api("/internal/allowed"),
       changes: 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master',
       key_id: key.id,
-      project: project.path_with_namespace,
+      project: project.repository.path_to_repo,
       action: 'git-receive-pack',
       secret_token: secret_token,
       protocol: protocol
@@ -420,7 +446,7 @@ describe API::API, api: true  do
       api("/internal/allowed"),
       ref: 'master',
       key_id: key.id,
-      project: project.path_with_namespace,
+      project: project.repository.path_to_repo,
       action: 'git-upload-archive',
       secret_token: secret_token,
       protocol: 'ssh'
@@ -432,7 +458,7 @@ describe API::API, api: true  do
       api("/internal/lfs_authenticate"),
       key_id: key_id,
       secret_token: secret_token,
-      project: project.path_with_namespace
+      project: project.repository.path_to_repo
     )
   end
 end