Merge branch 'dm-git-access-any-ce' into 'master'

[CE] Don't run checks for changed refs when specific changes are unknown

See merge request gitlab-org/gitlab-ce!23990

6 files changed, 42 insertions, 27 deletions

diff --git a/lib/gitlab/checks/base_checker.rb b/lib/gitlab/checks/base_checker.rb
index 7fbcf6a4ff4..09b17b5b76b 100644
--- a/lib/gitlab/checks/base_checker.rb
+++ b/lib/gitlab/checks/base_checker.rb
@@ -18,12 +18,16 @@ module Gitlab
 
       private
 
+      def creation?
+        Gitlab::Git.blank_ref?(oldrev)
+      end
+
       def deletion?
         Gitlab::Git.blank_ref?(newrev)
       end
 
       def update?
-        !Gitlab::Git.blank_ref?(oldrev) && !deletion?
+        !creation? && !deletion?
       end
 
       def updated_from_web?
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index 7778d3068cc..8a57a3a6d9a 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -10,7 +10,7 @@ module Gitlab
       attr_reader(*ATTRIBUTES)
 
       def initialize(
-        change, user_access:, project:, skip_authorization: false,
+        change, user_access:, project:,
         skip_lfs_integrity_check: false, protocol:, logger:
       )
         @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@@ -18,7 +18,6 @@ module Gitlab
         @tag_name = Gitlab::Git.tag_name(@ref)
         @user_access = user_access
         @project = project
-        @skip_authorization = skip_authorization
         @skip_lfs_integrity_check = skip_lfs_integrity_check
         @protocol = protocol
 
@@ -27,8 +26,6 @@ module Gitlab
       end
 
       def exec
-        return true if skip_authorization
-
         ref_level_checks
         # Check of commits should happen as the last step
         # given they're expensive in terms of performance
diff --git a/lib/gitlab/checks/diff_check.rb b/lib/gitlab/checks/diff_check.rb
index 63da9a3d6b5..ea0d8c85a66 100644
--- a/lib/gitlab/checks/diff_check.rb
+++ b/lib/gitlab/checks/diff_check.rb
@@ -11,7 +11,7 @@ module Gitlab
       }.freeze
 
       def validate!
-        return if deletion? || newrev.nil?
+        return if deletion?
         return unless should_run_diff_validations?
         return if commits.empty?
 
diff --git a/lib/gitlab/checks/push_check.rb b/lib/gitlab/checks/push_check.rb
index f3a52f09868..91f8d0bdbc8 100644
--- a/lib/gitlab/checks/push_check.rb
+++ b/lib/gitlab/checks/push_check.rb
@@ -6,7 +6,7 @@ module Gitlab
       def validate!
         logger.log_timed("Checking if you are allowed to push...") do
           unless can_push?
-            raise GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.'
+            raise GitAccess::UnauthorizedError, GitAccess::ERROR_MESSAGES[:push_code]
           end
         end
       end
@@ -15,7 +15,7 @@ module Gitlab
 
       def can_push?
         user_access.can_do_action?(:push_code) ||
-          user_access.can_push_to_branch?(branch_name)
+          project.branch_allows_collaboration?(user_access.user, branch_name)
       end
     end
   end
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 802fa65dd63..010bd0e520c 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -12,6 +12,10 @@ module Gitlab
     TimeoutError = Class.new(StandardError)
     ProjectMovedError = Class.new(NotFoundError)
 
+    # Use the magic string '_any' to indicate we do not know what the
+    # changes are. This is also what gitlab-shell does.
+    ANY = '_any'
+
     ERROR_MESSAGES = {
       upload: 'You are not allowed to upload code for this project.',
       download: 'You are not allowed to download code from this project.',
@@ -24,7 +28,8 @@ module Gitlab
       upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.',
       receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.',
       read_only: 'The repository is temporarily read-only. Please try again later.',
-      cannot_push_to_read_only: "You can't push code to a read-only GitLab instance."
+      cannot_push_to_read_only: "You can't push code to a read-only GitLab instance.",
+      push_code: 'You are not allowed to push code to this project.'
     }.freeze
 
     INTERNAL_TIMEOUT = 50.seconds.freeze
@@ -199,7 +204,7 @@ module Gitlab
 
     def ensure_project_on_push!(cmd, changes)
       return if project || deploy_key?
-      return unless receive_pack?(cmd) && changes == '_any' && authentication_abilities.include?(:push_code)
+      return unless receive_pack?(cmd) && changes == ANY && authentication_abilities.include?(:push_code)
 
       namespace = Namespace.find_by_full_path(namespace_path)
 
@@ -256,24 +261,34 @@ module Gitlab
         raise UnauthorizedError, ERROR_MESSAGES[:upload]
       end
 
-      return if changes.blank? # Allow access this is needed for EE.
-
       check_change_access!
     end
 
     def check_change_access!
-      # If there are worktrees with a HEAD pointing to a non-existent object,
-      # calls to `git rev-list --all` will fail in git 2.15+. This should also
-      # clear stale lock files.
-      project.repository.clean_stale_repository_files
-
-      # Iterate over all changes to find if user allowed all of them to be applied
-      changes_list.each.with_index do |change, index|
-        first_change = index == 0
-
-        # If user does not have access to make at least one change, cancel all
-        # push by allowing the exception to bubble up
-        check_single_change_access(change, skip_lfs_integrity_check: !first_change)
+      # Deploy keys with write access can push anything
+      return if deploy_key?
+
+      if changes == ANY
+        can_push = user_access.can_do_action?(:push_code) ||
+          project.any_branch_allows_collaboration?(user_access.user)
+
+        unless can_push
+          raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code]
+        end
+      else
+        # If there are worktrees with a HEAD pointing to a non-existent object,
+        # calls to `git rev-list --all` will fail in git 2.15+. This should also
+        # clear stale lock files.
+        project.repository.clean_stale_repository_files
+
+        # Iterate over all changes to find if user allowed all of them to be applied
+        changes_list.each.with_index do |change, index|
+          first_change = index == 0
+
+          # If user does not have access to make at least one change, cancel all
+          # push by allowing the exception to bubble up
+          check_single_change_access(change, skip_lfs_integrity_check: !first_change)
+        end
       end
     end
 
@@ -282,7 +297,6 @@ module Gitlab
         change,
         user_access: user_access,
         project: project,
-        skip_authorization: deploy_key?,
         skip_lfs_integrity_check: skip_lfs_integrity_check,
         protocol: protocol,
         logger: logger
@@ -348,7 +362,7 @@ module Gitlab
     protected
 
     def changes_list
-      @changes_list ||= Gitlab::ChangesList.new(changes)
+      @changes_list ||= Gitlab::ChangesList.new(changes == ANY ? [] : changes)
     end
 
     def user
diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb
index 3f24001e4ee..0af91957fa8 100644
--- a/lib/gitlab/git_access_wiki.rb
+++ b/lib/gitlab/git_access_wiki.rb
@@ -15,7 +15,7 @@ module Gitlab
       authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_wiki_code)
     end
 
-    def check_single_change_access(change, _options = {})
+    def check_change_access!
       unless user_access.can_do_action?(:create_wiki)
         raise UnauthorizedError, ERROR_MESSAGES[:write_to_wiki]
       end