Don't run single change checks when changes are unknown
When the `changes` passed to `GitAccess` are the literal string `_any`, which indicates that this is a pre-authorization check, we now check whether the user can push to any branch in the project in question, instead of running the per-change check with `oldrev` `_any`, `newrev` `nil`, and `ref` `nil`.
This commit is contained in:
		
							parent
							
								
									551e84c2cb
								
							
						
					
					
						commit
						3a0ae96c0e
					
				|  | @ -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 | ||||
|  |  | |||
|  | @ -28,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 | ||||
|  | @ -269,18 +270,27 @@ module Gitlab | |||
|       # Deploy keys with write access can push anything | ||||
|       return if deploy_key? | ||||
| 
 | ||||
|       # 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 | ||||
|       if changes == ANY | ||||
|         can_push = user_access.can_do_action?(:push_code) || | ||||
|           project.any_branch_allows_collaboration?(user_access.user) | ||||
| 
 | ||||
|       # 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 | ||||
|         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 | ||||
| 
 | ||||
|         # 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) | ||||
|         # 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 | ||||
| 
 | ||||
|  | @ -354,7 +364,7 @@ module Gitlab | |||
|     protected | ||||
| 
 | ||||
|     def changes_list | ||||
|       @changes_list ||= Gitlab::ChangesList.new(changes) | ||||
|       @changes_list ||= Gitlab::ChangesList.new(changes == ANY ? [] : changes) | ||||
|     end | ||||
| 
 | ||||
|     def user | ||||
|  |  | |||
|  | @ -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 | ||||
|  |  | |||
|  | @ -736,7 +736,8 @@ describe Gitlab::GitAccess do | |||
|     end | ||||
| 
 | ||||
|     let(:changes) do | ||||
|       { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow", | ||||
|       { any: Gitlab::GitAccess::ANY, | ||||
|         push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow", | ||||
|         push_master: '6f6d7e7ed 570e7b2ab refs/heads/master', | ||||
|         push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/feature', | ||||
|         push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\ | ||||
|  | @ -798,6 +799,7 @@ describe Gitlab::GitAccess do | |||
| 
 | ||||
|     permissions_matrix = { | ||||
|       admin: { | ||||
|         any: true, | ||||
|         push_new_branch: true, | ||||
|         push_master: true, | ||||
|         push_protected_branch: true, | ||||
|  | @ -809,6 +811,7 @@ describe Gitlab::GitAccess do | |||
|       }, | ||||
| 
 | ||||
|       maintainer: { | ||||
|         any: true, | ||||
|         push_new_branch: true, | ||||
|         push_master: true, | ||||
|         push_protected_branch: true, | ||||
|  | @ -820,6 +823,7 @@ describe Gitlab::GitAccess do | |||
|       }, | ||||
| 
 | ||||
|       developer: { | ||||
|         any: true, | ||||
|         push_new_branch: true, | ||||
|         push_master: true, | ||||
|         push_protected_branch: false, | ||||
|  | @ -831,6 +835,7 @@ describe Gitlab::GitAccess do | |||
|       }, | ||||
| 
 | ||||
|       reporter: { | ||||
|         any: false, | ||||
|         push_new_branch: false, | ||||
|         push_master: false, | ||||
|         push_protected_branch: false, | ||||
|  | @ -842,6 +847,7 @@ describe Gitlab::GitAccess do | |||
|       }, | ||||
| 
 | ||||
|       guest: { | ||||
|         any: false, | ||||
|         push_new_branch: false, | ||||
|         push_master: false, | ||||
|         push_protected_branch: false, | ||||
|  |  | |||
		Loading…
	
		Reference in New Issue