Generate and handle a gl_repository param to pass around components

This new param allows us to share project information between components
that don't share or don't have access to the same filesystem
mountpoints, for example between Gitaly and Rails or between Rails and
Gitlab-Shell hooks. The previous parameters are still supported, but if
found, gl_repository is prefered. The old parameters should be deprecated
once all components support the new format.

changelogs/unreleased/29925-gitlab-shell-hooks-can-no-longer-send-absolute-paths-to-gitlab-ce.yml

@@ -0,0 +1,4 @@
+---
+title: Generate and handle a gl_repository param to pass around components
+merge_request: 10992
+author:

lib/api/helpers/internal_helpers.rb

@@ -1,48 +1,14 @@
 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, storages = Gitlab.config.repositories.storages.values)
-        project_path = project_path.sub(/\.git\z/, '')
-
-        storages.each do |storage|
-          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_by_full_path(project_path)
+        set_project unless defined?(@wiki)
+        @wiki
       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_by_full_path(project_path)
-        end
+        set_project unless defined?(@project)
+        @project
       end
 
       def ssh_authentication_abilities
@@ -66,6 +32,16 @@ module API
 
         ::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action])
       end
+
+      private
+
+      def set_project
+        if params[:gl_repository]
+          @project, @wiki = Gitlab::GlRepository.parse(params[:gl_repository])
+        else
+          @project, @wiki = Gitlab::RepoPath.parse(params[:project])
+        end
+      end
     end
   end
 end

lib/api/internal.rb

@@ -42,6 +42,10 @@ module API
         if access_status.status
           log_user_activity(actor)
 
+          # Project id to pass between components that don't share/don't have
+          # access to the same filesystem mounts
+          response[:gl_repository] = "#{wiki? ? 'wiki' : 'project'}-#{project.id}"
+
           # Return the repository full path so that gitlab-shell has it when
           # handling ssh commands
           response[:repository_path] =
@@ -134,11 +138,9 @@ module API
 
         return unless Gitlab::GitalyClient.enabled?
 
-        relative_path = Gitlab::RepoPath.strip_storage_path(params[:repo_path])
-        project = Project.find_by_full_path(relative_path.sub(/\.(git|wiki)\z/, ''))
-
         begin
-          Gitlab::GitalyClient::Notifications.new(project.repository).post_receive
+          repository = wiki? ? project.wiki.repository : project.repository
+          Gitlab::GitalyClient::Notifications.new(repository.raw_repository).post_receive
         rescue GRPC::Unavailable => e
           render_api_error!(e, 500)
         end

lib/gitlab/gl_repository.rb

@@ -0,0 +1,16 @@
+module Gitlab
+  module GlRepository
+    def self.parse(gl_repository)
+      match_data = /\A(project|wiki)-([1-9][0-9]*)\z/.match(gl_repository)
+      unless match_data
+        raise ArgumentError, "Invalid GL Repository \"#{gl_repository}\""
+      end
+
+      type, id = match_data.captures
+      project = Project.find_by(id: id)
+      wiki = type == 'wiki'
+
+      [project, wiki]
+    end
+  end
+end

lib/gitlab/repo_path.rb

@@ -2,18 +2,29 @@ module Gitlab
   module RepoPath
     NotFoundError = Class.new(StandardError)
 
-    def self.strip_storage_path(repo_path)
-      result = nil
-
-      Gitlab.config.repositories.storages.values.each do |params|
-        storage_path = params['path']
-        if repo_path.start_with?(storage_path)
-          result = repo_path.sub(storage_path, '')
-          break
-        end
+    def self.parse(repo_path)
+      project_path = strip_storage_path(repo_path.sub(/\.git\z/, ''), fail_on_not_found: false)
+      project = Project.find_by_full_path(project_path)
+      if project_path.end_with?('.wiki') && !project
+        project = Project.find_by_full_path(project_path.chomp('.wiki'))
+        wiki = true
+      else
+        wiki = false
       end
 
-      if result.nil?
+      [project, wiki]
+    end
+
+    def self.strip_storage_path(repo_path, fail_on_not_found: true)
+      result = repo_path
+
+      storage = Gitlab.config.repositories.storages.values.find do |params|
+        repo_path.start_with?(params['path'])
+      end
+
+      if storage
+        result = result.sub(storage['path'], '')
+      elsif fail_on_not_found
         raise NotFoundError.new("No known storage path matches #{repo_path.inspect}")
       end
 

spec/lib/gitlab/gl_repository_spec.rb

@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe ::Gitlab::GlRepository do
+  describe '.parse' do
+    set(:project) { create(:project) }
+
+    it 'parses a project gl_repository' do
+      expect(described_class.parse("project-#{project.id}")).to eq([project, false])
+    end
+
+    it 'parses a wiki gl_repository' do
+      expect(described_class.parse("wiki-#{project.id}")).to eq([project, true])
+    end
+
+    it 'throws an argument error on an invalid gl_repository' do
+      expect { described_class.parse("badformat-#{project.id}") }.to raise_error(ArgumentError)
+    end
+  end
+end

spec/lib/gitlab/repo_path_spec.rb

@@ -1,6 +1,30 @@
 require 'spec_helper'
 
 describe ::Gitlab::RepoPath do
+  describe '.parse' do
+    set(:project) { create(:project) }
+
+    it 'parses a full repository path' do
+      expect(described_class.parse(project.repository.path)).to eq([project, false])
+    end
+
+    it 'parses a full wiki path' do
+      expect(described_class.parse(project.wiki.repository.path)).to eq([project, true])
+    end
+
+    it 'parses a relative repository path' do
+      expect(described_class.parse(project.full_path + '.git')).to eq([project, false])
+    end
+
+    it 'parses a relative wiki path' do
+      expect(described_class.parse(project.full_path + '.wiki.git')).to eq([project, true])
+    end
+
+    it 'parses a relative path starting with /' do
+      expect(described_class.parse('/' + project.full_path + '.git')).to eq([project, false])
+    end
+  end
+
   describe '.strip_storage_path' do
     before do
       allow(Gitlab.config.repositories).to receive(:storages).and_return({

spec/requests/api/helpers/internal_helpers_spec.rb

@@ -1,32 +0,0 @@
-require 'spec_helper'
-
-describe ::API::Helpers::InternalHelpers do
-  include described_class
-
-  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, [{ 'path' => storage_path }]) }
-
-            it { is_expected.to eq(expected) }
-          end
-        end
-      end
-    end
-  end
-end

spec/requests/api/internal_spec.rb

@@ -180,6 +180,7 @@ describe API::Internal do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo)
+          expect(json_response["gl_repository"]).to eq("wiki-#{project.id}")
           expect(user).not_to have_an_activity_record
         end
       end
@@ -191,6 +192,7 @@ describe API::Internal do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo)
+          expect(json_response["gl_repository"]).to eq("wiki-#{project.id}")
           expect(user).to have_an_activity_record
         end
       end
@@ -202,6 +204,7 @@ describe API::Internal do
           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)
+          expect(json_response["gl_repository"]).to eq("project-#{project.id}")
           expect(user).to have_an_activity_record
         end
       end
@@ -213,6 +216,7 @@ describe API::Internal do
           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)
+          expect(json_response["gl_repository"]).to eq("project-#{project.id}")
           expect(user).not_to have_an_activity_record
         end
 
@@ -223,6 +227,7 @@ describe API::Internal do
             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)
+            expect(json_response["gl_repository"]).to eq("project-#{project.id}")
           end
         end
 
@@ -233,6 +238,7 @@ describe API::Internal do
             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)
+            expect(json_response["gl_repository"]).to eq("project-#{project.id}")
           end
         end
       end
@@ -444,18 +450,39 @@ describe API::Internal do
 
       expect(json_response).to eq([])
     end
+
+    context 'with a gl_repository parameter' do
+      let(:gl_repository) { "project-#{project.id}" }
+
+      it 'returns link to create new merge request' do
+        get api("/internal/merge_request_urls?gl_repository=#{gl_repository}&changes=#{changes}"), secret_token: secret_token
+
+        expect(json_response).to match [{
+          "branch_name" => "new_branch",
+          "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
+          "new_merge_request" => true
+        }]
+      end
+    end
   end
 
   describe 'POST /notify_post_receive' do
     let(:valid_params) do
-      { repo_path: project.repository.path, secret_token: secret_token }
+      { project: project.repository.path, secret_token: secret_token }
+    end
+
+    let(:valid_wiki_params) do
+      { project: project.wiki.repository.path, secret_token: secret_token }
     end
 
     before do
       allow(Gitlab.config.gitaly).to receive(:enabled).and_return(true)
     end
 
-    it "calls the Gitaly client if it's enabled" do
+    it "calls the Gitaly client with the project's repository" do
+      expect(Gitlab::GitalyClient::Notifications).
+        to receive(:new).with(gitlab_git_repository_with(path: project.repository.path)).
+        and_call_original
       expect_any_instance_of(Gitlab::GitalyClient::Notifications).
         to receive(:post_receive)
 
@@ -464,6 +491,18 @@ describe API::Internal do
       expect(response).to have_http_status(200)
     end
 
+    it "calls the Gitaly client with the wiki's repository if it's a wiki" do
+      expect(Gitlab::GitalyClient::Notifications).
+        to receive(:new).with(gitlab_git_repository_with(path: project.wiki.repository.path)).
+        and_call_original
+      expect_any_instance_of(Gitlab::GitalyClient::Notifications).
+        to receive(:post_receive)
+
+      post api("/internal/notify_post_receive"), valid_wiki_params
+
+      expect(response).to have_http_status(200)
+    end
+
     it "returns 500 if the gitaly call fails" do
       expect_any_instance_of(Gitlab::GitalyClient::Notifications).
         to receive(:post_receive).and_raise(GRPC::Unavailable)
@@ -472,6 +511,40 @@ describe API::Internal do
 
       expect(response).to have_http_status(500)
     end
+
+    context 'with a gl_repository parameter' do
+      let(:valid_params) do
+        { gl_repository: "project-#{project.id}", secret_token: secret_token }
+      end
+
+      let(:valid_wiki_params) do
+        { gl_repository: "wiki-#{project.id}", secret_token: secret_token }
+      end
+
+      it "calls the Gitaly client with the project's repository" do
+        expect(Gitlab::GitalyClient::Notifications).
+          to receive(:new).with(gitlab_git_repository_with(path: project.repository.path)).
+          and_call_original
+        expect_any_instance_of(Gitlab::GitalyClient::Notifications).
+          to receive(:post_receive)
+
+        post api("/internal/notify_post_receive"), valid_params
+
+        expect(response).to have_http_status(200)
+      end
+
+      it "calls the Gitaly client with the wiki's repository if it's a wiki" do
+        expect(Gitlab::GitalyClient::Notifications).
+          to receive(:new).with(gitlab_git_repository_with(path: project.wiki.repository.path)).
+          and_call_original
+        expect_any_instance_of(Gitlab::GitalyClient::Notifications).
+          to receive(:post_receive)
+
+        post api("/internal/notify_post_receive"), valid_wiki_params
+
+        expect(response).to have_http_status(200)
+      end
+    end
   end
 
   def project_with_repo_path(path)

spec/support/matchers/gitlab_git_matchers.rb

@@ -0,0 +1,6 @@
+RSpec::Matchers.define :gitlab_git_repository_with do |values|
+  match do |actual|
+    actual.is_a?(Gitlab::Git::Repository) &&
+      values.all? { |k, v| actual.send(k) == v }
+  end
+end