Cop/UserAdmin: Migrate remaining locations where policies are not used to determine admin status

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

Close this issue

In !25056 (merged) we found some calls that determine if a user is an admin just by looking at the boolean flag in the db record, instead of relying on the policies. This is a similar issue as we solved in !18530 (merged). The problem is that admin mode will not be enforced in those parts of the codebase.

@ifarkas proposed exploring the possibility of overriding User#admin? to use policies, but we determined later that it was too risky since it's an internal rails method.

The current approach is to slowly migrate the remaining locations to use policies and add a rubocop to warn / forbid using it in the future.

We've found the following situations where this is not enforced:

lib/gitlab/graphql/authorize/authorize_field_service.rb in allowed_access(current_user, object), see original discussion
app/models/issue_collection.rb in updatable_by_user(user), see original discussion
app/models/group.rb in max_member_access_for_user(user), see original discussion and temporarilly skipped spec in ee/spec/models/ee/event_spec.rb and spec/models/member_spec.rb

There's also rubocop and haml-lints where this is currently ignored and need to be reviewed. See !55693 (merged) for more details:

The following discussion from !25056 (merged) should be addressed:

@dlouzan started a discussion: (+6 comments)
I disabled the mocking as we will do in #31511 (closed) for some relevant specs related to this functionality.

But there's some locations where I've found the policies are not used, unsure if this is on purpose, an oversight, or just organic growth of the codebase over time.

As an example, taking spec/requests/api/graphql/group_query_spec.rb:111 spec I will explicitly disable automatic admin mode in the GraphQL controller. The spec checks if an admin has access to all private groups via lib/gitlab/graphql/authorize/authorize_field_service.rb, but actually this will not check the abilities, just if the user is an admin via User#admin?:
```
pry(#<Gitlab::Graphql::Authorize::AuthorizeFieldService>)> GroupPolicy.new(current_user, object).debug(ability)
- [0] enable when public_group ((@user1 : Group/1))
- [0] enable when logged_in_viewable ((@user1 : Group/1))
- [0] enable when admin ((@user1 : Group/1))
- [0] enable when auditor ((@user1 : Group/1))
- [14] prevent when needs_new_sso_session ((@user1 : Group/1))
+ [16] enable when guest ((@user1 : Group/1))
- [28] prevent when all?(ip_enforcement_prevents_access, ~owner) ((@user1 : Group/1))
```
Notice above that this is enabled by the :guest condition, tested in Group#max_member_access_for_user(user) via a call to user.admin?. The condition :admin above (that would be shielded by admin mode) is correctly rejected, but the permissions are in the end enabled by :guest.

So in cases like this, though we're making sure that the GraphQL queries are also going through the admin mode code path, the specs in the background are not actually testing that.

On the other hand, I have specs like spec/requests/api/graphql/mutations/snippets/mark_as_spam_spec.rb:54 that actually rely on the policies to determine if a user is an admin. If I disable the GraphQL automatic bypass session for admin mode, this is correctly prevented:
```
pry(#<Mutations::Snippets::MarkAsSpam>)> PersonalSnippetPolicy.new(context[:current_user], snippet).debug(:admin_snippet)
- [0] enable when admin ((@user1 : PersonalSnippet/1))
- [16] enable when is_author ((@user1 : PersonalSnippet/1))
=> #<DeclarativePolicy::Runner::State:0x00007fd8edbfb418 @enabled=false, @prevented=true>
```
Long story short, I really hope this is one of the last steps before we remove the :do_not_mock_admin_mode tag in specs 🔥

/cc @ifarkas @reprazent

Edited Jul 04, 2025 by 🤖 GitLab Bot 🤖