Preload aliased boundaries for granular token authorization

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab:jmrp-granular-preload-aliased-boundary into master

What does this MR do and why?

The granular token boundary preloader looks up each directive's boundary method with reflect_on_association, so a method that aliases an association is skipped. MergeRequestType declares boundary: :project, and MergeRequest defines alias_method :project, :target_project, so the project namespaces of merge request nodes were never preloaded. The review note expected them to be skipped earlier, at the respond_to?(:reflect_on_association) guard, but Authorize::FieldExtension preloads a list's nodes as records, before MergeRequestPresenter wraps them.

Each was then loaded in its own query where the token check reads it and the list's resolver does not preload it: a legacy token's root namespace check, or a fine-grained token scoped to selected groups and projects or to personal projects. A group's merge requests are spared (their resolver preloads project_namespace); an issue's related merge requests are not.

This MR looks the association up by the method's original name (UnboundMethod#original_name), so boundary: :project preloads target_project and its project_namespace; any other method is still skipped. Resolving the alias in the preloader covers any aliased boundary and keeps the directive on the model's public name; the note's boundary: :target_project would fix this type alone.

The issue's other case, Ci::RunnerType's boundary: :owner, picks one of several associations, which the preloader cannot derive. The preload: option the issue suggests adds to the directive's API, so I left it for a follow-up once agreed there.

No migration or feature flag: only what is preloaded changes, not what a token may read. One query by primary key now loads the project namespaces, instead of one per project.

References

Related to #605200

Screenshots or screen recordings

Not applicable: no UI change.

How to set up and validate locally

  1. Have merge requests in several projects and a personal access token without fine-grained permissions.

  2. In a Rails console:

    token = PersonalAccessToken.find(<token id>)
merge_requests = MergeRequest.where(id: [<merge request ids>]).preload(:target_project).to_a
ActiveRecord::Base.logger = Logger.new($stdout)
Gitlab::Graphql::Authz::BoundaryExtractors::Preloader.preload_boundaries(Types::MergeRequestType, merge_requests, { access_token: token })

  3. On master the log shows one namespaces query by id per project; on this branch, one for all of them, besides one more that both make.

What these calls and a GraphQL query issued, on master and on this branch (Ruby 3.3.11)

I ran the calls of step 2 under ActiveRecord::QueryRecorder, with a legacy token: for merge requests in one and then four projects, master issued 2 and 5 namespaces queries, this branch 2 and 2. spec/lib/gitlab/graphql/authz/boundary_extractors/preloader_spec.rb covers the same path.

Through the GraphQL API, an issue's relatedMergeRequests with merge requests in one and then four projects, queried with a legacy token, issued 13 and 16 namespaces queries on master and 13 and 13 on this branch, where one "namespaces"."id" IN (...) query replaces the lookups by id of each project.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Specs, with the fix and with master's preloader (Ruby 3.3.11)

bundle exec rspec spec/lib/gitlab/graphql/authz/boundary_extractors/preloader_spec.rb, exit status 0 on this branch:

26 examples, 0 failures

The same spec with master's preloader.rb, exit status 1: the three new examples of "when a project boundary is an alias of an association" fail, the query count one with

Expected 3 queries, but got 6

Query Diff:
-----------
SELECT "namespaces"."id", "namespaces"."name", ...
-- (expected: 0, got: 1)
   WHERE "namespaces"."type" = 'Project' AND "namespaces"."id" = 131 LIMIT 1
-- (expected: 0, got: 1)
   WHERE "namespaces"."type" = 'Project' AND "namespaces"."id" = 132 LIMIT 1
-- (expected: 0, got: 1)
   WHERE "namespaces"."type" = 'Project' AND "namespaces"."id" = 133 LIMIT 1

26 examples, 3 failures

RuboCop on both files, the commit message linter and Danger's commit and title linters pass. Left to the pipeline: Ruby 3.4 and the FOSS run.

Merge request reports