Merge branch 'fix-unathorized-cloning' into 'security'

Ensure external users are not able to clone disabled repositories.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/23788

See merge request !2017

Signed-off-by: Rémy Coutable <remy@rymai.me>

11 files changed, 187 insertions, 47 deletions

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 383e184d796..3f41916e6d3 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -21,10 +21,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   def authenticate_user
     @authentication_result = Gitlab::Auth::Result.new
 
-    if project && project.public? && download_request?
-      return # Allow access
-    end
-
     if allow_basic_auth? && basic_auth_provided?
       login, password = user_name_and_password(request)
 
@@ -41,6 +37,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
         send_final_spnego_response
         return # Allow access
       end
+    elsif project && download_request? && Guest.can?(:download_code, project)
+      @authentication_result = Gitlab::Auth::Result.new(nil, project, :none, [:download_code])
+
+      return # Allow access
     end
 
     send_challenges
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index 662d38b10a5..13caeb42d40 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -78,11 +78,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   def upload_pack_allowed?
     return false unless Gitlab.config.gitlab_shell.upload_pack
 
-    if user
-      access_check.allowed?
-    else
-      ci? || project.public?
-    end
+    access_check.allowed? || ci?
   end
 
   def access
diff --git a/app/helpers/lfs_helper.rb b/app/helpers/lfs_helper.rb
index 95b60aeab5f..d3966ba1f10 100644
--- a/app/helpers/lfs_helper.rb
+++ b/app/helpers/lfs_helper.rb
@@ -1,6 +1,6 @@
 module LfsHelper
   include Gitlab::Routing.url_helpers
-  
+
   def require_lfs_enabled!
     return if Gitlab.config.lfs.enabled
 
@@ -27,7 +27,7 @@ module LfsHelper
   def lfs_download_access?
     return false unless project.lfs_enabled?
 
-    project.public? || ci? || lfs_deploy_token? || user_can_download_code? || build_can_download_code?
+    ci? || lfs_deploy_token? || user_can_download_code? || build_can_download_code?
   end
 
   def user_can_download_code?
diff --git a/app/models/guest.rb b/app/models/guest.rb
new file mode 100644
index 00000000000..01285ca1264
--- /dev/null
+++ b/app/models/guest.rb
@@ -0,0 +1,7 @@
+class Guest
+  class << self
+    def can?(action, subject)
+      Ability.allowed?(nil, action, subject)
+    end
+  end
+end
diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb
index 8ea88da8a53..3fc1c70be75 100644
--- a/app/services/auth/container_registry_authentication_service.rb
+++ b/app/services/auth/container_registry_authentication_service.rb
@@ -76,7 +76,7 @@ module Auth
 
       case requested_action
       when 'pull'
-        requested_project.public? || build_can_pull?(requested_project) || user_can_pull?(requested_project)
+        build_can_pull?(requested_project) || user_can_pull?(requested_project)
       when 'push'
         build_can_push?(requested_project) || user_can_push?(requested_project)
       else
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 799794c0171..bcbf6455998 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -2,8 +2,18 @@
 # class return an instance of `GitlabAccessStatus`
 module Gitlab
   class GitAccess
+    UnauthorizedError = Class.new(StandardError)
+
+    ERROR_MESSAGES = {
+      upload: 'You are not allowed to upload code for this project.',
+      download: 'You are not allowed to download code from this project.',
+      deploy_key: 'Deploy keys are not allowed to push code.',
+      no_repo: 'A repository for this project does not exist yet.'
+    }
+
     DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }
     PUSH_COMMANDS = %w{ git-receive-pack }
+    ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS
 
     attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
 
@@ -16,56 +26,43 @@ module Gitlab
     end
 
     def check(cmd, changes)
-      return build_status_object(false, "Git access over #{protocol.upcase} is not allowed") unless protocol_allowed?
-
-      unless actor
-        return build_status_object(false, "No user or key was provided.")
-      end
-
-      if user && !user_access.allowed?
-        return build_status_object(false, "Your account has been blocked.")
-      end
-
-      unless project && (user_access.can_read_project? || deploy_key_can_read_project?)
-        return build_status_object(false, 'The project you were looking for could not be found.')
-      end
+      check_protocol!
+      check_active_user!
+      check_project_accessibility!
+      check_command_existence!(cmd)
 
       case cmd
       when *DOWNLOAD_COMMANDS
         download_access_check
       when *PUSH_COMMANDS
         push_access_check(changes)
-      else
-        build_status_object(false, "The command you're trying to execute is not allowed.")
       end
+
+      build_status_object(true)
+    rescue UnauthorizedError => ex
+      build_status_object(false, ex.message)
     end
 
     def download_access_check
       if user
         user_download_access_check
-      elsif deploy_key
-        build_status_object(true)
-      else
-        raise 'Wrong actor'
+      elsif deploy_key.nil? && !Guest.can?(:download_code, project)
+        raise UnauthorizedError, ERROR_MESSAGES[:download]
       end
     end
 
     def push_access_check(changes)
       if user
         user_push_access_check(changes)
-      elsif deploy_key
-        build_status_object(false, "Deploy keys are not allowed to push code.")
       else
-        raise 'Wrong actor'
+        raise UnauthorizedError, ERROR_MESSAGES[deploy_key ? :deploy_key : :upload]
       end
     end
 
     def user_download_access_check
       unless user_can_download_code? || build_can_download_code?
-        return build_status_object(false, "You are not allowed to download code from this project.")
+        raise UnauthorizedError, ERROR_MESSAGES[:download]
       end
-
-      build_status_object(true)
     end
 
     def user_can_download_code?
@@ -78,15 +75,15 @@ module Gitlab
 
     def user_push_access_check(changes)
       unless authentication_abilities.include?(:push_code)
-        return build_status_object(false, "You are not allowed to upload code for this project.")
+        raise UnauthorizedError, ERROR_MESSAGES[:upload]
       end
 
       if changes.blank?
-        return build_status_object(true)
+        return # Allow access.
       end
 
       unless project.repository.exists?
-        return build_status_object(false, "A repository for this project does not exist yet.")
+        raise UnauthorizedError, ERROR_MESSAGES[:no_repo]
       end
 
       changes_list = Gitlab::ChangesList.new(changes)
@@ -96,11 +93,9 @@ module Gitlab
         status = change_access_check(change)
         unless status.allowed?
           # If user does not have access to make at least one change - cancel all push
-          return status
+          raise UnauthorizedError, status.message
         end
       end
-
-      build_status_object(true)
     end
 
     def change_access_check(change)
@@ -113,6 +108,30 @@ module Gitlab
 
     private
 
+    def check_protocol!
+      unless protocol_allowed?
+        raise UnauthorizedError, "Git access over #{protocol.upcase} is not allowed"
+      end
+    end
+
+    def check_active_user!
+      if user && !user_access.allowed?
+        raise UnauthorizedError, "Your account has been blocked."
+      end
+    end
+
+    def check_project_accessibility!
+      if project.blank? || !can_read_project?
+        raise UnauthorizedError, 'The project you were looking for could not be found.'
+      end
+    end
+
+    def check_command_existence!(cmd)
+      unless ALL_COMMANDS.include?(cmd)
+        raise UnauthorizedError, "The command you're trying to execute is not allowed."
+      end
+    end
+
     def matching_merge_request?(newrev, branch_name)
       Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
     end
@@ -130,6 +149,16 @@ module Gitlab
       end
     end
 
+    def can_read_project?
+      if user
+        user_access.can_read_project?
+      elsif deploy_key
+        deploy_key_can_read_project?
+      else
+        Guest.can?(:read_project, project)
+      end
+    end
+
     protected
 
     def user
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index dd4a86b1e31..bfd88a254f1 100644
--- a/spec/factories/projects.rb
+++ b/spec/factories/projects.rb
@@ -49,13 +49,17 @@ FactoryGirl.define do
     end
 
     after(:create) do |project, evaluator|
+      # Builds and MRs can't have higher visibility level than repository access level.
+      builds_access_level = [evaluator.builds_access_level, evaluator.repository_access_level].min
+      merge_requests_access_level = [evaluator.merge_requests_access_level, evaluator.repository_access_level].min
+
       project.project_feature.
-        update_attributes(
+        update_attributes!(
           wiki_access_level: evaluator.wiki_access_level,
-          builds_access_level: evaluator.builds_access_level,
+          builds_access_level: builds_access_level,
           snippets_access_level: evaluator.snippets_access_level,
           issues_access_level: evaluator.issues_access_level,
-          merge_requests_access_level: evaluator.merge_requests_access_level,
+          merge_requests_access_level: merge_requests_access_level,
           repository_access_level: evaluator.repository_access_level
         )
     end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 62aa212f1f6..f1d0a190002 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -66,6 +66,7 @@ describe Gitlab::GitAccess, lib: true do
 
       context 'pull code' do
         it { expect(subject.allowed?).to be_falsey }
+        it { expect(subject.message).to match(/You are not allowed to download code/) }
       end
     end
 
@@ -77,6 +78,7 @@ describe Gitlab::GitAccess, lib: true do
 
       context 'pull code' do
         it { expect(subject.allowed?).to be_falsey }
+        it { expect(subject.message).to match(/Your account has been blocked/) }
       end
     end
 
@@ -84,6 +86,29 @@ describe Gitlab::GitAccess, lib: true do
       context 'pull code' do
         it { expect(subject.allowed?).to be_falsey }
       end
+
+      context 'when project is public' do
+        let(:public_project) { create(:project, :public) }
+        let(:guest_access) { Gitlab::GitAccess.new(nil, public_project, 'web', authentication_abilities: []) }
+        subject { guest_access.check('git-upload-pack', '_any') }
+
+        context 'when repository is enabled' do
+          it 'give access to download code' do
+            public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::ENABLED)
+
+            expect(subject.allowed?).to be_truthy
+          end
+        end
+
+        context 'when repository is disabled' do
+          it 'does not give access to download code' do
+            public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED)
+
+            expect(subject.allowed?).to be_falsey
+            expect(subject.message).to match(/You are not allowed to download code/)
+          end
+        end
+      end
     end
 
     describe 'deploy key permissions' do
diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb
index 576cda595bb..576aa5c366f 100644
--- a/spec/lib/gitlab/git_access_wiki_spec.rb
+++ b/spec/lib/gitlab/git_access_wiki_spec.rb
@@ -18,7 +18,7 @@ describe Gitlab::GitAccessWiki, lib: true do
       project.team << [user, :developer]
     end
 
-    subject { access.push_access_check(changes) }
+    subject { access.check('git-receive-pack', changes) }
 
     it { expect(subject.allowed?).to be_truthy }
   end
diff --git a/spec/models/guest_spec.rb b/spec/models/guest_spec.rb
new file mode 100644
index 00000000000..d79f929f7a1
--- /dev/null
+++ b/spec/models/guest_spec.rb
@@ -0,0 +1,47 @@
+require 'spec_helper'
+
+describe Guest, lib: true do
+  let(:public_project) { create(:project, :public) }
+  let(:private_project) { create(:project, :private) }
+  let(:internal_project) { create(:project, :internal) }
+
+  describe '.can_pull?' do
+    context 'when project is private' do
+      it 'does not allow to pull the repo' do
+        expect(Guest.can?(:download_code, private_project)).to eq(false)
+      end
+    end
+
+    context 'when project is internal' do
+      it 'does not allow to pull the repo' do
+        expect(Guest.can?(:download_code, internal_project)).to eq(false)
+      end
+    end
+
+    context 'when project is public' do
+      context 'when repository is disabled' do
+        it 'does not allow to pull the repo' do
+          public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED)
+
+          expect(Guest.can?(:download_code, public_project)).to eq(false)
+        end
+      end
+
+      context 'when repository is accessible only by team members' do
+        it 'does not allow to pull the repo' do
+          public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::PRIVATE)
+
+          expect(Guest.can?(:download_code, public_project)).to eq(false)
+        end
+      end
+
+      context 'when repository is enabled' do
+        it 'allows to pull the repo' do
+          public_project.project_feature.update_attribute(:repository_access_level, ProjectFeature::ENABLED)
+
+          expect(Guest.can?(:download_code, public_project)).to eq(true)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 27f0fd22ae6..f1728d61def 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -115,6 +115,38 @@ describe 'Git HTTP requests', lib: true do
             end.to raise_error(JWT::DecodeError)
           end
         end
+
+        context 'when the repo is public' do
+          context 'but the repo is disabled' do
+            it 'does not allow to clone the repo' do
+              project = create(:project, :public, repository_access_level: ProjectFeature::DISABLED)
+
+              download("#{project.path_with_namespace}.git", {}) do |response|
+                expect(response).to have_http_status(:unauthorized)
+              end
+            end
+          end
+
+          context 'but the repo is enabled' do
+            it 'allows to clone the repo' do
+              project = create(:project, :public, repository_access_level: ProjectFeature::ENABLED)
+
+              download("#{project.path_with_namespace}.git", {}) do |response|
+                expect(response).to have_http_status(:ok)
+              end
+            end
+          end
+
+          context 'but only project members are allowed' do
+            it 'does not allow to clone the repo' do
+              project = create(:project, :public, repository_access_level: ProjectFeature::PRIVATE)
+
+              download("#{project.path_with_namespace}.git", {}) do |response|
+                expect(response).to have_http_status(:unauthorized)
+              end
+            end
+          end
+        end
       end
 
       context "when the project is private" do