diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-06-24 10:37:04 +0530 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2016-07-13 13:24:56 +0530 |
commit | 60245bbe228014a9f689adafd64b571883ef6eb3 (patch) | |
tree | 2af0d7785fb1e9c1f168b01d704e788d42d89503 /lib/gitlab/git_access.rb | |
parent | 495db09653bafb0371e5d5a5f12d5bc33cdb584b (diff) | |
download | gitlab-ce-60245bbe228014a9f689adafd64b571883ef6eb3.tar.gz |
Refactor `Gitlab::GitAccess`
1. Don't use case statements for dispatch anymore. This leads to a lot
of duplication, and makes the logic harder to follow.
2. Remove duplicated logic.
- For example, the `can_push_to_branch?` exists, but we also have a
different way of checking the same condition within `change_access_check`.
- This kind of duplication is removed, and the `can_push_to_branch?`
method is used in both places.
3. Move checks returning true/false to `UserAccess`.
- All public methods in `GitAccess` now return an instance of
`GitAccessStatus`. Previously, some methods would return
true/false as well, which was confusing.
- It makes sense for these kinds of checks to be at the level of a
user, so the `UserAccess` class was repurposed for this. The prior
`UserAccess.allowed?` classmethod is converted into an instance
method.
- All external uses of these checks have been migrated to use the
`UserAccess` class
4. Move the "change_access_check" into a separate class.
- Create the `GitAccess::ChangeAccessCheck` class to run these
checks, which are quite substantial.
- `ChangeAccessCheck` returns an instance of `GitAccessStatus` as
well.
5. Break out the boolean logic in `ChangeAccessCheck` into `if/else`
chains - this seems more readable.
6. I can understand that this might look like overkill for !4892, but I
think this is a good opportunity to clean it up.
- http://martinfowler.com/bliki/OpportunisticRefactoring.html
Diffstat (limited to 'lib/gitlab/git_access.rb')
-rw-r--r-- | lib/gitlab/git_access.rb | 159 |
1 files changed, 30 insertions, 129 deletions
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index feaf845209e..cfb48ac2382 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -1,62 +1,17 @@ +# Check a user's access to perform a git action. All public methods in this +# class return an instance of `GitlabAccessStatus` module Gitlab class GitAccess DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive } PUSH_COMMANDS = %w{ git-receive-pack } - attr_reader :actor, :project, :protocol + attr_reader :actor, :project, :protocol, :user_access def initialize(actor, project, protocol) @actor = actor @project = project @protocol = protocol - end - - def user - return @user if defined?(@user) - - @user = - case actor - when User - actor - when DeployKey - nil - when Key - actor.user - end - end - - def deploy_key - actor if actor.is_a?(DeployKey) - end - - def can_push_to_branch?(ref) - return false unless user - - if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref) - user.can?(:push_code_to_protected_branches, project) - else - user.can?(:push_code, project) - end - end - - def can_merge_to_branch?(ref) - return false unless user - - if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref) - user.can?(:push_code_to_protected_branches, project) - else - user.can?(:push_code, project) - end - end - - def can_read_project? - if user - user.can?(:read_project, project) - elsif deploy_key - deploy_key.projects.include?(project) - else - false - end + @user_access = UserAccess.new(user, project: project) end def check(cmd, changes = nil) @@ -66,11 +21,11 @@ module Gitlab return build_status_object(false, "No user or key was provided.") end - if user && !user_allowed? + if user && !user_access.allowed? return build_status_object(false, "Your account has been blocked.") end - unless project && can_read_project? + 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 @@ -105,7 +60,7 @@ module Gitlab end def user_download_access_check - unless user.can?(:download_code, project) + unless user_access.can_do_action?(:download_code) return build_status_object(false, "You are not allowed to download code from this project.") end @@ -135,103 +90,49 @@ module Gitlab build_status_object(true) end - def can_user_do_action?(action) - @permission_cache ||= {} - @permission_cache[action] ||= user.can?(action, project) - end - def change_access_check(change) - oldrev, newrev, ref = change.split(' ') - - action = - if project.protected_branch?(branch_name(ref)) - protected_branch_action(oldrev, newrev, branch_name(ref)) - elsif (tag_ref = tag_name(ref)) && protected_tag?(tag_ref) - # Prevent any changes to existing git tag unless user has permissions - :admin_project - else - :push_code - end - - unless can_user_do_action?(action) - status = - case action - when :force_push_code_to_protected_branches - build_status_object(false, "You are not allowed to force push code to a protected branch on this project.") - when :remove_protected_branches - build_status_object(false, "You are not allowed to deleted protected branches from this project.") - when :push_code_to_protected_branches - build_status_object(false, "You are not allowed to push code to protected branches on this project.") - when :admin_project - build_status_object(false, "You are not allowed to change existing tags on this project.") - else # :push_code - build_status_object(false, "You are not allowed to push code to this project.") - end - return status - end - - build_status_object(true) - end - - def forced_push?(oldrev, newrev) - Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev) + ChangeAccessCheck.new(change, user_access: user_access, project: project).exec end - def protocol_allowed? Gitlab::ProtocolAccess.allowed?(protocol) 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) - :force_push_code_to_protected_branches - elsif Gitlab::Git.blank_ref?(newrev) - # and we dont allow remove of protected branch - :remove_protected_branches - elsif matching_merge_request?(newrev, branch_name) && project.developers_can_merge_to_protected_branch?(branch_name) - :push_code - elsif project.developers_can_push_to_protected_branch?(branch_name) - :push_code - else - :push_code_to_protected_branches - end - end - - def protected_tag?(tag_name) - project.repository.tag_exists?(tag_name) + def matching_merge_request?(newrev, branch_name) + Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? end - def user_allowed? - Gitlab::UserAccess.allowed?(user) + def deploy_key + actor if actor.is_a?(DeployKey) end - def branch_name(ref) - ref = ref.to_s - if Gitlab::Git.branch_ref?(ref) - Gitlab::Git.ref_name(ref) - else - nil - end - end - def tag_name(ref) - ref = ref.to_s - if Gitlab::Git.tag_ref?(ref) - Gitlab::Git.ref_name(ref) + def deploy_key_can_read_project? + if deploy_key + deploy_key.projects.include?(project) else - nil + false end end protected + def user + return @user if defined?(@user) + + @user = + case actor + when User + actor + when DeployKey + nil + when Key + actor.user + end + end + def build_status_object(status, message = '') GitAccessStatus.new(status, message) end |