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

Ensure external users are not able to clone disabled repositories.

EE MR for gitlab/gitlabhq!2017

See merge request !506
Signed-off-by: Rémy Coutable <remy@rymai.me>

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

app/controllers/projects/git_http_controller.rb

@@ -82,11 +82,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

app/helpers/lfs_helper.rb

@@ -31,7 +31,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?

app/models/guest.rb

+class Guest
+  class << self
+    def can?(action, subject)
+      Ability.allowed?(nil, action, subject)
+    end
+  end
+end

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

lib/gitlab/git_access.rb

@@ -3,10 +3,19 @@
 module Gitlab
   class GitAccess
     include PathLocksHelper
+    UnauthorizedError = Class.new(StandardError)
 
     DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive }
     PUSH_COMMANDS = %w{ git-receive-pack }
     GIT_ANNEX_COMMANDS = %w{ git-annex-shell }
+    ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS + GIT_ANNEX_COMMANDS
+
+    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.'
+    }
 
     attr_reader :actor, :project, :protocol, :user_access, :authentication_abilities
 
@@ -19,23 +28,12 @@ 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
+      check_protocol!
+      check_active_user!
+      check_project_accessibility!
+      check_command_existence!(cmd)
 
-      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? || geo_node_key)
-        return build_status_object(false, 'The project you were looking for could not be found.')
-      end
-
-      if Gitlab::Geo.secondary? && !Gitlab::Geo.license_allows?
-        return build_status_object(false, 'Your current license does not have GitLab Geo add-on enabled.')
-      end
+      check_geo_license!
 
       case cmd
       when *DOWNLOAD_COMMANDS
@@ -44,47 +42,43 @@ module Gitlab
         push_access_check(changes)
       when *GIT_ANNEX_COMMANDS
         git_annex_access_check(project, 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 || geo_node_key
-        build_status_object(true)
-      else
-        raise 'Wrong actor'
+      elsif deploy_key.nil? && geo_node_key.nil? && !Guest.can?(:download_code, project)
+        raise UnauthorizedError, ERROR_MESSAGES[:download]
       end
     end
 
     def push_access_check(changes)
       if project.repository_read_only?
-        return build_status_object(false, 'The repository is temporarily read-only. Please try again later.')
+        raise UnauthorizedError, 'The repository is temporarily read-only. Please try again later.'
       end
 
       if Gitlab::Geo.secondary?
-        return build_status_object(false, "You can't push code on a secondary GitLab Geo node.")
+        raise UnauthorizedError, "You can't push code on a secondary GitLab Geo node."
       end
 
-      return build_status_object(true) if git_annex_branch_sync?(changes)
+      return if git_annex_branch_sync?(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?
@@ -97,20 +91,24 @@ 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
 
-      return build_status_object(false, "A repository for this project does not exist yet.") unless project.repository.exists?
+      unless project.repository.exists?
+        raise UnauthorizedError, ERROR_MESSAGES[:no_repo]
+      end
 
-      return build_status_object(false, Gitlab::RepositorySizeError.new(project).push_error) if project.above_size_limit?
+      if project.above_size_limit?
+        raise UnauthorizedError, Gitlab::RepositorySizeError.new(project).push_error
+      end
 
       if ::License.block_changes?
         message = ::LicenseHelper.license_message(signed_in: true, is_admin: (user && user.is_admin?))
-        return build_status_object(false, message)
+        raise UnauthorizedError, message
       end
 
       changes_list = Gitlab::ChangesList.new(changes)
@@ -122,7 +120,7 @@ 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
 
         if project.size_limit_enabled?
@@ -131,10 +129,8 @@ module Gitlab
       end
 
       if project.changes_will_exceed_size_limit?(push_size_in_bytes.to_mb)
-        return build_status_object(false, Gitlab::RepositorySizeError.new(project).new_changes_error)
+        raise UnauthorizedError, Gitlab::RepositorySizeError.new(project).new_changes_error
       end
-
-      build_status_object(true)
     end
 
     def change_access_check(change)
@@ -145,12 +141,42 @@ module Gitlab
       Gitlab::ProtocolAccess.allowed?(protocol)
     end
 
+    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 check_geo_license!
+      if Gitlab::Geo.secondary? && !Gitlab::Geo.license_allows?
+        raise UnauthorizedError, 'Your current license does not have GitLab Geo add-on enabled.'
+      end
+    end
+
     def matching_merge_request?(newrev, branch_name)
       Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
     end
 
-    private
-
     def protected_branch_action(oldrev, newrev, branch_name)
       # we dont allow force push to protected branch
       if forced_push?(oldrev, newrev)
@@ -188,6 +214,18 @@ module Gitlab
       end
     end
 
+    def can_read_project?
+      if user
+        user_access.can_read_project?
+      elsif deploy_key
+        deploy_key_can_read_project?
+      elsif geo_node_key
+        true
+      else
+        Guest.can?(:read_project, project)
+      end
+    end
+
     protected
 
     def user
@@ -211,24 +249,22 @@ module Gitlab
     end
 
     def git_annex_access_check(project, changes)
-      return build_status_object(false, "git-annex is disabled") unless Gitlab.config.gitlab_shell.git_annex_enabled
+      raise UnauthorizedError, "git-annex is disabled" unless Gitlab.config.gitlab_shell.git_annex_enabled
 
       unless user && user_access.allowed?
-        return build_status_object(false, "You don't have access")
+        raise UnauthorizedError, "You don't have access"
       end
 
       unless project.repository.exists?
-        return build_status_object(false, "Repository does not exist")
+        raise UnauthorizedError, "Repository does not exist"
       end
 
       if Gitlab::Geo.enabled? && Gitlab::Geo.secondary?
-        return build_status_object(false, "You can't use git-annex with a secondary GitLab Geo node.")
+        raise UnauthorizedError, "You can't use git-annex with a secondary GitLab Geo node."
       end
 
-      if user.can?(:push_code, project)
-        build_status_object(true)
-      else
-        build_status_object(false, "You don't have permission")
+      unless user.can?(:push_code, project)
+        raise UnauthorizedError, "You don't have permission"
       end
     end
 

spec/factories/projects.rb

@@ -70,13 +70,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

spec/lib/gitlab/git_access_wiki_spec.rb

@@ -20,7 +20,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 }
 

spec/models/guest_spec.rb

+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

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 Kerberos token is provided" do