Merge branch '29925-gitlab-shell-hooks-can-no-longer-send-absolute-paths-to-gitlab-ce' into 'master'

Generate and handle a gl_repository param to pass around components

Closes #29925

See merge request !10992

7 files changed, 171 insertions, 47 deletions

diff --git a/spec/lib/gitlab/gl_repository_spec.rb b/spec/lib/gitlab/gl_repository_spec.rb
new file mode 100644
index 00000000000..ac3558ab386
--- /dev/null
+++ b/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
diff --git a/spec/lib/gitlab/repo_path_spec.rb b/spec/lib/gitlab/repo_path_spec.rb
index 0fb5d7646f2..f94c9c2e315 100644
--- a/spec/lib/gitlab/repo_path_spec.rb
+++ b/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({
diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb
index b703e9808a8..beb1791a429 100644
--- a/spec/lib/gitlab/workhorse_spec.rb
+++ b/spec/lib/gitlab/workhorse_spec.rb
@@ -181,10 +181,23 @@ describe Gitlab::Workhorse, lib: true do
     let(:user) { create(:user) }
     let(:repo_path) { repository.path_to_repo }
     let(:action) { 'info_refs' }
+    let(:params) do
+      { GL_ID: "user-#{user.id}", GL_REPOSITORY: "project-#{project.id}", RepoPath: repo_path }
+    end
+
+    subject { described_class.git_http_ok(repository, false, user, action) }
+
+    it { expect(subject).to include(params) }
 
-    subject { described_class.git_http_ok(repository, user, action) }
+    context 'when is_wiki' do
+      let(:params) do
+        { GL_ID: "user-#{user.id}", GL_REPOSITORY: "wiki-#{project.id}", RepoPath: repo_path }
+      end
+
+      subject { described_class.git_http_ok(repository, true, user, action) }
 
-    it { expect(subject).to include({ GL_ID: "user-#{user.id}", RepoPath: repo_path }) }
+      it { expect(subject).to include(params) }
+    end
 
     context 'when Gitaly is enabled' do
       let(:gitaly_params) do
diff --git a/spec/requests/api/helpers/internal_helpers_spec.rb b/spec/requests/api/helpers/internal_helpers_spec.rb
deleted file mode 100644
index db716b340f1..00000000000
--- a/spec/requests/api/helpers/internal_helpers_spec.rb
+++ /dev/null
@@ -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
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 429f1a4e375..2ceb4648ece 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/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)
diff --git a/spec/support/matchers/gitlab_git_matchers.rb b/spec/support/matchers/gitlab_git_matchers.rb
new file mode 100644
index 00000000000..c840cd4bf2d
--- /dev/null
+++ b/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
diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb
index 5ab3c4a0e34..0260416dbe2 100644
--- a/spec/workers/post_receive_spec.rb
+++ b/spec/workers/post_receive_spec.rb
@@ -5,6 +5,7 @@ describe PostReceive do
   let(:wrongly_encoded_changes) { changes.encode("ISO-8859-1").force_encoding("UTF-8") }
   let(:base64_changes) { Base64.encode64(wrongly_encoded_changes) }
   let(:project) { create(:project, :repository) }
+  let(:project_identifier) { "project-#{project.id}" }
   let(:key) { create(:key, user: project.owner) }
   let(:key_id) { key.shell_id }
 
@@ -14,6 +15,26 @@ describe PostReceive do
     end
   end
 
+  context 'with a non-existing project' do
+    let(:project_identifier) { "project-123456789" }
+    let(:error_message) do
+      "Triggered hook for non-existing project with identifier \"#{project_identifier}\""
+    end
+
+    it "returns false and logs an error" do
+      expect(Gitlab::GitLogger).to receive(:error).with("POST-RECEIVE: #{error_message}")
+      expect(described_class.new.perform(project_identifier, key_id, base64_changes)).to be(false)
+    end
+  end
+
+  context "with an absolute path as the project identifier" do
+    it "searches the project by full path" do
+      expect(Project).to receive(:find_by_full_path).with(project.full_path).and_call_original
+
+      described_class.new.perform(pwd(project), key_id, base64_changes)
+    end
+  end
+
   describe "#process_project_changes" do
     before do
       allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner)
@@ -25,7 +46,7 @@ describe PostReceive do
       it "calls GitTagPushService" do
         expect_any_instance_of(GitPushService).to receive(:execute).and_return(true)
         expect_any_instance_of(GitTagPushService).not_to receive(:execute)
-        described_class.new.perform(pwd(project), key_id, base64_changes)
+        described_class.new.perform(project_identifier, key_id, base64_changes)
       end
     end
 
@@ -35,7 +56,7 @@ describe PostReceive do
       it "calls GitTagPushService" do
         expect_any_instance_of(GitPushService).not_to receive(:execute)
         expect_any_instance_of(GitTagPushService).to receive(:execute).and_return(true)
-        described_class.new.perform(pwd(project), key_id, base64_changes)
+        described_class.new.perform(project_identifier, key_id, base64_changes)
       end
     end
 
@@ -45,12 +66,12 @@ describe PostReceive do
       it "does not call any of the services" do
         expect_any_instance_of(GitPushService).not_to receive(:execute)
         expect_any_instance_of(GitTagPushService).not_to receive(:execute)
-        described_class.new.perform(pwd(project), key_id, base64_changes)
+        described_class.new.perform(project_identifier, key_id, base64_changes)
       end
     end
 
     context "gitlab-ci.yml" do
-      subject { described_class.new.perform(pwd(project), key_id, base64_changes) }
+      subject { described_class.new.perform(project_identifier, key_id, base64_changes) }
 
       context "creates a Ci::Pipeline for every change" do
         before do
@@ -74,8 +95,8 @@ describe PostReceive do
 
   context "webhook" do
     it "fetches the correct project" do
-      expect(Project).to receive(:find_by_full_path).with(project.path_with_namespace).and_return(project)
-      described_class.new.perform(pwd(project), key_id, base64_changes)
+      expect(Project).to receive(:find_by).with(id: project.id.to_s)
+      described_class.new.perform(project_identifier, key_id, base64_changes)
     end
 
     it "does not run if the author is not in the project" do
@@ -85,22 +106,22 @@ describe PostReceive do
 
       expect(project).not_to receive(:execute_hooks)
 
-      expect(described_class.new.perform(pwd(project), key_id, base64_changes)).to be_falsey
+      expect(described_class.new.perform(project_identifier, key_id, base64_changes)).to be_falsey
     end
 
     it "asks the project to trigger all hooks" do
-      allow(Project).to receive(:find_by_full_path).and_return(project)
+      allow(Project).to receive(:find_by).and_return(project)
       expect(project).to receive(:execute_hooks).twice
       expect(project).to receive(:execute_services).twice
 
-      described_class.new.perform(pwd(project), key_id, base64_changes)
+      described_class.new.perform(project_identifier, key_id, base64_changes)
     end
 
     it "enqueues a UpdateMergeRequestsWorker job" do
-      allow(Project).to receive(:find_by_full_path).and_return(project)
+      allow(Project).to receive(:find_by).and_return(project)
       expect(UpdateMergeRequestsWorker).to receive(:perform_async).with(project.id, project.owner.id, any_args)
 
-      described_class.new.perform(pwd(project), key_id, base64_changes)
+      described_class.new.perform(project_identifier, key_id, base64_changes)
     end
   end