Cop/UserAdmin: Migrate remaining locations where policies are not used to determine admin status
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
inallowed_access(current_user, object)
, see original discussion -
app/models/issue_collection.rb
inupdatable_by_user(user)
, see original discussion -
app/models/group.rb
inmax_member_access_for_user(user)
, see original discussion and temporarilly skipped spec inee/spec/models/ee/event_spec.rb
andspec/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:
-
app/views/dashboard/projects/_zero_authorized_projects.html.haml
-
app/views/doorkeeper/authorizations/new.html.haml
-
app/views/layouts/nav/_dashboard.html.haml
-
app/views/projects/_bitbucket_import_modal.html.haml
-
app/views/projects/_gitlab_import_modal.html.haml
-
app/views/projects/snippets/index.html.haml
-
app/views/users/show.html.haml
-
ee/app/views/namespaces/_shared_runners_minutes_setting.html.haml
-
ee/app/views/shared/_old_repository_size_limit_setting.html.haml
-
ee/app/views/shared/_repository_size_limit_setting.html.haml
-
ee/app/views/shared/projects/_removed.html.haml
-
ee/app/views/shared/promotions/_promotion_link_project.html.haml
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 vialib/gitlab/graphql/authorize/authorize_field_service.rb
, but actually this will not check the abilities, just if the user is an admin viaUser#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 inGroup#max_member_access_for_user(user)
via a call touser.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