Move CI access logic into GitAccess

7 files changed, 198 insertions, 57 deletions

diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb
index ae91e02488a..2b6afaa6233 100644
--- a/app/controllers/concerns/lfs_request.rb
+++ b/app/controllers/concerns/lfs_request.rb
@@ -106,4 +106,8 @@ module LfsRequest
   def objects
     @objects ||= (params[:objects] || []).to_a
   end
+
+  def has_authentication_ability?(capability)
+    (authentication_abilities || []).include?(capability)
+  end
 end
diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 44b0853e3e9..7f3205a8001 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -128,28 +128,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     @authentication_result = Gitlab::Auth.find_for_git_client(
       login, password, project: project, ip: request.ip)
 
-    return false unless @authentication_result.success?
-
-    if download_request?
-      authentication_has_download_access?
-    else
-      authentication_has_upload_access?
-    end
+    @authentication_result.success?
   end
 
   def ci?
     authentication_result.ci?(project)
   end
-
-  def authentication_has_download_access?
-    has_authentication_ability?(:download_code) || has_authentication_ability?(:build_download_code)
-  end
-
-  def authentication_has_upload_access?
-    has_authentication_ability?(:push_code)
-  end
-
-  def has_authentication_ability?(capability)
-    (authentication_abilities || []).include?(capability)
-  end
 end
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index e7b498599f2..2c2766cf623 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -67,20 +67,24 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   end
 
   def render_denied
-    if user && can?(user, :read_project, project)
-      render plain: access_check.message, status: :forbidden
+    if access_check.message == Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found]
+      render plain: access_check.message, status: :not_found
     else
-      # Do not leak information about project existence
-      render_not_found
+      render plain: access_check.message, status: :forbidden
     end
   end
 
   def upload_pack_allowed?
-    access_check.allowed? || ci?
+    access_check.allowed?
   end
 
   def access
-    @access ||= access_klass.new(user, project, 'http', authentication_abilities: authentication_abilities)
+    @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities)
+  end
+
+  def access_actor
+    return user if user
+    return :ci if ci?
   end
 
   def access_check
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 1d052ac9b33..1ffac5385c2 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -63,6 +63,11 @@ module Gitlab
       authentication_abilities.include?(:build_download_code) && user_access.can_do_action?(:build_download_code)
     end
 
+    # Allow generic CI (build without a user) for backwards compatibility
+    def ci_can_download_code?
+      authentication_abilities.include?(:build_download_code) && ci?
+    end
+
     def protocol_allowed?
       Gitlab::ProtocolAccess.allowed?(protocol)
     end
@@ -115,6 +120,7 @@ module Gitlab
       return if deploy_key?
 
       passed = user_can_download_code? ||
+        ci_can_download_code? ||
         build_can_download_code? ||
         guest_can_download_code?
 
@@ -184,11 +190,17 @@ module Gitlab
       actor.is_a?(DeployKey)
     end
 
+    def ci?
+      actor == :ci
+    end
+
     def can_read_project?
-      if deploy_key
+      if deploy_key?
         deploy_key.has_access_to?(project)
       elsif user
         user.can?(:read_project, project)
+      elsif ci?
+        true # allow CI (build without a user) for backwards compatibility
       end || Guest.can?(:read_project, project)
     end
 
@@ -213,10 +225,12 @@ module Gitlab
         case actor
         when User
           actor
-        when DeployKey
-          nil
         when Key
           actor.user
+        when DeployKey
+          nil
+        when :ci
+          nil
         end
     end
 
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 1033fef9684..0efe15856fc 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::GitAccess, lib: true do
+  let(:pull_access_check) { access.check('git-upload-pack', '_any') }
+  let(:push_access_check) { access.check('git-receive-pack', '_any') }
   let(:access) { Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: authentication_abilities) }
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
@@ -51,7 +53,123 @@ describe Gitlab::GitAccess, lib: true do
     end
   end
 
-  describe '#check with commands disabled' do
+  describe '#check_project_accessibility!' do
+    context 'when the project exists' do
+      context 'when actor exists' do
+        context 'when actor is a DeployKey' do
+          let(:deploy_key) { create(:deploy_key, user: user, can_push: true) }
+          let(:actor) { deploy_key }
+
+          context 'when the DeployKey has access to the project' do
+            before { deploy_key.projects << project }
+
+            it 'allows pull access' do
+              expect(pull_access_check.allowed?).to be_truthy
+            end
+
+            it 'allows push access' do
+              expect(push_access_check.allowed?).to be_truthy
+            end
+          end
+
+          context 'when the Deploykey does not have access to the project' do
+            it 'blocks pulls with "not found"' do
+              expect(pull_access_check.allowed?).to be_falsey
+              expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+            end
+
+            it 'blocks pushes with "not found"' do
+              expect(push_access_check.allowed?).to be_falsey
+              expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+            end
+          end
+        end
+
+        context 'when actor is a User' do
+          context 'when the User can read the project' do
+            before { project.team << [user, :master] }
+
+            it 'allows pull access' do
+              expect(pull_access_check.allowed?).to be_truthy
+            end
+
+            it 'allows push access' do
+              expect(push_access_check.allowed?).to be_truthy
+            end
+          end
+
+          context 'when the User cannot read the project' do
+            it 'blocks pulls with "not found"' do
+              expect(pull_access_check.allowed?).to be_falsey
+              expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+            end
+
+            it 'blocks pushes with "not found"' do
+              expect(push_access_check.allowed?).to be_falsey
+              expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+            end
+          end
+        end
+
+        # For backwards compatibility
+        context 'when actor is :ci' do
+          let(:actor) { :ci }
+          let(:authentication_abilities) { build_authentication_abilities }
+
+          it 'allows pull access' do
+            expect(pull_access_check.allowed?).to be_truthy
+          end
+
+          it 'does not block pushes with "not found"' do
+            expect(push_access_check.allowed?).to be_falsey
+            expect(push_access_check.message).to eq('You are not allowed to upload code for this project.')
+          end
+        end
+      end
+
+      context 'when actor is nil' do
+        let(:actor) { nil }
+
+        context 'when guests can read the project' do
+          let(:project) { create(:project, :repository, :public) }
+
+          it 'allows pull access' do
+            expect(pull_access_check.allowed?).to be_truthy
+          end
+
+          it 'does not block pushes with "not found"' do
+            expect(push_access_check.allowed?).to be_falsey
+            expect(push_access_check.message).to eq('You are not allowed to upload code for this project.')
+          end
+        end
+
+        context 'when guests cannot read the project' do
+          it 'blocks pulls with "not found"' do
+            expect(pull_access_check.allowed?).to be_falsey
+            expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+          end
+
+          it 'blocks pushes with "not found"' do
+            expect(push_access_check.allowed?).to be_falsey
+            expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+          end
+        end
+      end
+    end
+
+    context 'when the project is nil' do
+      let(:project) { nil }
+
+      it 'blocks any command with "not found"' do
+        expect(pull_access_check.allowed?).to be_falsey
+        expect(pull_access_check.message).to eq('The project you were looking for could not be found.')
+        expect(push_access_check.allowed?).to be_falsey
+        expect(push_access_check.message).to eq('The project you were looking for could not be found.')
+      end
+    end
+  end
+
+  describe '#check_command_disabled!' do
     before { project.team << [user, :master] }
 
     context 'over http' do
@@ -219,6 +337,14 @@ describe Gitlab::GitAccess, lib: true do
           end
         end
       end
+
+      describe 'generic CI (build without a user)' do
+        let(:actor) { :ci }
+
+        context 'pull code' do
+          it { expect(subject).to be_allowed }
+        end
+      end
     end
   end
 
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 080e2f12cd7..ab7c56fcdf0 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -489,29 +489,41 @@ describe 'Git HTTP requests', lib: true do
         end
 
         context "when a gitlab ci token is provided" do
+          let(:project) { create(:project, :repository) }
           let(:build) { create(:ci_build, :running) }
-          let(:project) { build.project }
           let(:other_project) { create(:empty_project) }
 
+          before do
+            build.update!(project: project) # can't associate it on factory create
+          end
+
           context 'when build created by system is authenticated' do
             let(:path) { "#{project.path_with_namespace}.git" }
             let(:env) { { user: 'gitlab-ci-token', password: build.token } }
 
             it_behaves_like 'pulls are allowed'
 
-            # TODO Verify this is desired behavior
-            it "rejects pushes with 401 Unauthorized (no project existence information leak)" do
+            # A non-401 here is not an information leak since the system is
+            # "authenticated" as CI using the correct token. It does not have
+            # push access, so pushes should be rejected as forbidden, and giving
+            # a reason is fine.
+            #
+            # We know for sure it is not an information leak since pulls using
+            # the build token must be allowed.
+            it "rejects pushes with 403 Forbidden" do
               push_get(path, env)
 
-              expect(response).to have_http_status(:unauthorized)
+              expect(response).to have_http_status(:forbidden)
+              expect(response.body).to eq(git_access_error(:upload))
             end
 
-            # TODO Verify this is desired behavior. Should be 403 Forbidden?
+            # We are "authenticated" as CI using a valid token here. But we are
+            # not authorized to see any other project, so return "not found".
             it "rejects pulls for other project with 404 Not Found" do
               clone_get("#{other_project.path_with_namespace}.git", env)
 
               expect(response).to have_http_status(:not_found)
-              expect(response.body).to eq('TODO: What should this be?')
+              expect(response.body).to eq(git_access_error(:project_not_found))
             end
           end
 
@@ -522,31 +534,27 @@ describe 'Git HTTP requests', lib: true do
             end
 
             shared_examples 'can download code only' do
-              it 'downloads get status 200' do
-                allow_any_instance_of(Repository).
-                  to receive(:exists?).and_return(true)
-
-                clone_get "#{project.path_with_namespace}.git",
-                  user: 'gitlab-ci-token', password: build.token
+              let(:path) { "#{project.path_with_namespace}.git" }
+              let(:env) { { user: 'gitlab-ci-token', password: build.token } }
 
-                expect(response).to have_http_status(:ok)
-                expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE)
-              end
-
-              it 'downloads from non-existing repository and gets 403' do
-                allow_any_instance_of(Repository).
-                  to receive(:exists?).and_return(false)
+              it_behaves_like 'pulls are allowed'
 
-                clone_get "#{project.path_with_namespace}.git",
-                  user: 'gitlab-ci-token', password: build.token
+              context 'when the repo does not exist' do
+                let(:project) { create(:empty_project) }
+                
+                it 'rejects pulls with 403 Forbidden' do
+                  clone_get path, env
 
-                expect(response).to have_http_status(:forbidden)
+                  expect(response).to have_http_status(:forbidden)
+                  expect(response.body).to eq(git_access_error(:no_repo))
+                end
               end
 
-              it 'uploads get status 403' do
-                push_get "#{project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
+              it 'rejects pushes with 403 Forbidden' do
+                push_get path, env
 
-                expect(response).to have_http_status(:unauthorized)
+                expect(response).to have_http_status(:forbidden)
+                expect(response.body).to eq(git_access_error(:upload))
               end
             end
 
diff --git a/spec/support/git_http_helpers.rb b/spec/support/git_http_helpers.rb
index d3d51560a9d..b8289e6c5f1 100644
--- a/spec/support/git_http_helpers.rb
+++ b/spec/support/git_http_helpers.rb
@@ -52,14 +52,17 @@ module GitHttpHelpers
   end
 
   def git_access_error(error_key)
-    Gitlab::GitAccess::ERROR_MESSAGES[error_key]
+    message = Gitlab::GitAccess::ERROR_MESSAGES[error_key]
+    message || raise("GitAccess error message key '#{error_key}' not found")
   end
 
   def git_access_wiki_error(error_key)
-    Gitlab::GitAccessWiki::ERROR_MESSAGES[error_key]
+    message = Gitlab::GitAccessWiki::ERROR_MESSAGES[error_key]
+    message || raise("GitAccessWiki error message key '#{error_key}' not found")
   end
 
   def change_access_error(error_key)
-    Gitlab::Checks::ChangeAccess::ERROR_MESSAGES[error_key]
+    message = Gitlab::Checks::ChangeAccess::ERROR_MESSAGES[error_key]
+    message || raise("ChangeAccess error message key '#{error_key}' not found")
   end
 end