Fix milestone Advanced Search for Planner role via merge_requests feature path

Ravi Kumarrequested to merge
duo/fix/538211-milestone-search-planner-role into master

What does this MR do and why?

Fixes milestone visibility in Advanced Search for Guest and Planner members on projects where issues_access_level is DISABLED and merge_requests_access_level is ENABLED. These members previously could not search milestones despite having legitimate milestone access.

Root cause

Search::Elastic::MilestoneQueryBuilder authorizes milestone search with either issues or merge_requests access. The minimum access level per feature comes from ProjectFeature::PRIVATE_FEATURES_MIN_ACCESS_LEVEL: GUEST for issues, REPORTER for merge_requests. The GUEST minimum for issues is why the issues path already works whenever issues are enabled.

The Elasticsearch membership filters apply the REPORTER minimum to both the ENABLED and PRIVATE feature access levels. ProjectPolicy#access_allowed_to? does not treat those two the same: ENABLED returns true for anyone with project access, while PRIVATE requires team_access_level >= ProjectFeature.required_minimum_access_level(feature).

Four points explain why the REPORTER minimum is wrong for ENABLED and right for PRIVATE:

  1. read_milestone lives in config/authz/roles/guest.yml. Planner has inherits_from: guest, so both Guest and Planner hold read_milestone. Planner does not inherit Reporter permissions.
  2. ProjectPolicy revokes read_milestone only through rule { issues_disabled & merge_requests_disabled } -- both features must be inaccessible.
  3. When merge_requests_access_level is ENABLED, access_allowed_to? returns true, so merge_requests_disabled is false and read_milestone survives for a Guest or Planner member even with issues disabled. The REPORTER minimum was wrong here, and only here.
  4. When merge_requests_access_level is PRIVATE and issues are disabled, access_allowed_to? is false for both roles, both features count as disabled, and ProjectPolicy genuinely prevents read_milestone. Advanced Search must keep returning nothing for those members, so keeping REPORTER for PRIVATE is required for correctness rather than a conservative choice.

Fix

A new relaxed_feature_access option carries a per-feature min_access_level and the list of project feature access levels it applies to. MilestoneQueryBuilder sets merge_requests: { min_access_level: Gitlab::Access::GUEST, feature_access_levels: [::ProjectFeature::ENABLED] }.

feature_access_rules converts the requested features, plus any matching relaxed_feature_access entry, into a flat list of FeatureAccessRule values, so relaxed access is no longer a separate code path appended after the main loop -- it is just another rule. Each FeatureAccessRule holds the feature, the <feature>_access_level values it accepts (ProjectFeature states), the member role required on public and internal projects, the member role required on private projects (Gitlab::Access values), and any project ids reached via a custom role; relaxed rules carry no custom-role ids, since they are authorised purely by membership level.

Each rule resolves to an id list, and group_rules_by_ids keys the emitted clauses on that id list, so rules resolving to the same list share a single clause, with their per-feature predicates OR-ed inside it via a nested bool.should. This is what stops an id list being serialised into the query body more than once. Because grouping is keyed on the id list's contents, rules whose lists differ -- for example when a custom role grants one feature but not another -- stay in separate clauses automatically.

The four filter builders now take feature_rules: in place of a single feature:. Given exactly one rule they emit a flat terms filter identical to the previous output, which is why every caller that passes a single feature produces a byte-identical query. Id lists are built with + rather than by mutating the shared per-access-level buckets, so an already-built clause cannot be widened after the fact.

This MR also deletes build_access_contexts and consolidate_access_permissions. Neither had a caller anywhere in the repository, including specs, before this branch.

Risk and cost

The change is additive in effect: where rules share an id list the relaxed feature predicate is OR-ed into an existing clause, which distributes to exactly the query you would get by adding a separate should clause. PRIVATE_FEATURES_MIN_ACCESS_LEVEL is unchanged, so merge-request feature checks elsewhere in the app are untouched.

MilestoneQueryBuilder is the only place in the codebase that sets relaxed_feature_access, and the only caller that passes more than one feature. A single rule still emits a flat terms filter, so every other search scope produces a byte-identical query, including the by_search_level_and_membership JSON fixtures.

Cost: clauses are keyed on the resolved id list, so rules that resolve to the same list share one clause with their feature predicates OR-ed inside it. For milestones the relaxed rule resolves to the same GUEST list as issues, so it adds a feature predicate rather than a second copy of the id list -- the query body carries the same number of project id lists as before this MR, and fewer when a user's GUEST and REPORTER lists are equal.

Both membership paths look ids up once per distinct access level rather than once per rule. Measured on group level milestone search, this is 19 queries against 25 on master.

Tests

permission_table_for_milestone_access gains a block for :private | :disabled | :enabled, which was absent from the table -- the reason no existing spec guarded this bug. The Guest row expects 1.

The same table gains its first Planner rows: :private | :disabled | :enabled expects 1, and :private | :disabled | :private expects 0.

ee/spec/lib/search/elastic/milestone_query_builder_spec.rb covers the relaxed clause being emitted for Guest and Planner members, omitted for a non-member, emitted through group ancestry, and emitted for an external user who is a Guest member of an internal project. The last case exercises the build_public_and_internal_project_filters branch, since add_visibility_level_filter restricts the membership-free public/internal clause to PUBLIC visibility for external users, leaving the relaxed membership clause as the only way to reach an INTERNAL project.

The same file also asserts the emitted clause shape. One example asserts the query body includes the shared project id list only once -- that the relaxed rule reuses the issues clause's id list instead of repeating it -- which fails against the pre-fix code with a count of 2. A second context covers a custom role that grants the issues ability on a different project: it stubs Search::Concerns::FeatureCustomAbilityMap::FEATURE_TO_ABILITY_MAP and Authz::Project#permitted, then asserts that project appears in the issues clause but not in the relaxed merge_requests clause, using a public project so the relaxed clause isn't skipped by the empty-id-list guard; it fails against 686eca9f8622 with expected [479, 480] not to include 480.

Documentation

doc/user/permissions.md had a shared footnote on the Search milestones and Search confidential issues and comments rows stating that the Planner role cannot use advanced search for milestones or for comments on confidential issues, linking to epic 17674. This MR drops the milestone half of that footnote and removes the footnote marker from the Search milestones row. The marker and the remaining text stay on Search confidential issues and comments, which this change does not affect.

References

Screenshots or screen recordings

Not applicable -- this is a backend search authorization fix with no UI changes.

How to set up and validate locally

  1. Create a project (Issues enabled) and create a milestone in it

  2. Update the project settings: set issues_access_level = disabled, keep merge_requests_access_level = enabled, and set visibility to private

    (Note: the Milestones UI page 404s once Issues is disabled unless you're an authorized member with Admin Mode enabled, so create the milestone before disabling Issues)

  3. Add a user with the Planner role and a separate user with the Guest role to the project

  4. Enable Elasticsearch and index the project

  5. As the Planner user, then as the Guest user, search for the milestone title in Advanced Search

  6. Before fix: no results returned for either user

  7. After fix: the milestone appears in results for both the Planner and the Guest user

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.

Edited by Ravi Kumar

Merge request reports