`Gitlab::Checks::ChangeAccess` looks for user permissions that don't exist
Summary
- In
Gitlab::Checks::ChangeAccess
, we check for the:force_push_code_to_protected_branches
and:remove_protected_branches
permissions. These symbols are not defined anywhere else in the codebase, except in the spec for this module.
if forced_push? && user_access.cannot_do_action?(:force_push_code_to_protected_branches)
return "You are not allowed to force push code to a protected branch on this project."
elsif Gitlab::Git.blank_ref?(@newrev) && user_access.cannot_do_action?(:remove_protected_branches)
return "You are not allowed to delete protected branches from this project."
end
-
After investigating, it looks like the
:force_push_code_to_protected_branches
and:remove_protected_branches
permissions are always disallowed, regardless of the user's level of access. Because of this, we don't need to define them in the ability/policy files, which act as a whitelist, and only require a list of permissions that should be allowed. -
I created this issue to make sure everyone is on the same page here, and to decide if the status quo is alright, or if we need to change things up.
Potential Changes
-
Don't check
user.can?
for permissions that don't exist in the policy file. This would make things less confusing, but would also make it harder to change the behavior of these always-disallowed permissions. For example, right now, if we need to allow admin to delete protected branches (for example), we just need to add:remove_protected_branches
to the relevant ability/policy file, with no other changes required. -
List these permissions in the policy/ability files using the
cannot!
declaration. After taking a quick look at the code, it seems like this is used to blacklist permissions that were previously whitelisted, but I don't see any reason why we can't use it for our purposes here.
Links
-
Gitlab::Checks::ChangeAccess
: https://gitlab.com/gitlab-org/gitlab-ee/blob/master/lib/gitlab/checks/change_access.rb#L30-34
/cc @DouweM