Index project visibility on project snippet documents

Ravi Kumarrequested to merge
35760-index-project-snippet-access into master

What does this MR do and why?

Advanced Search never returns public or internal project snippets to users who are not project members. The same is true for admins without admin mode. Elastic::Latest::SnippetClassProxy#filter_project_snippets only matches snippets in user.authorized_projects. Basic search does return these snippets.

The snippet documents in the main index have the snippet's own visibility_level. They have nothing about the parent project. So the filter cannot check the project's visibility or its snippets feature setting.

This MR is step 1 of 3. It changes no search results on its own. It only puts the data in the index that the later steps need.

What changes:

  • Adds a project_visibility_level short field to the main index mapping in ee/lib/elastic/latest/config.rb.
  • Adds the Advanced Search migration AddProjectVisibilityLevelToSnippets (using UpdateMappingsHelper) to add that mapping to existing indices.
  • Elastic::Latest::SnippetInstanceProxy now writes project_visibility_level and snippets_access_level on project snippet documents.
    • It writes them only after the migration has finished. The main index mapping is dynamic: 'strict', so writing an unmapped field would fail.
    • snippets_access_level is already mapped, because project documents use it.
    • Personal snippets get neither field.
    • SCHEMA_VERSION goes from 23_08 to 26_01. Documents keep 23_08 until the migration has finished, so no document claims a schema version whose fields it doesn't carry. Elastic::Latest::NoteInstanceProxy does the same.
  • SnippetClassProxy#preload_indexing_data now preloads project: :project_feature through a new Snippet.preload_for_indexing scope, like Milestone, Epic, and Project. This stops bulk indexing from running a query per snippet.
  • A project's snippets are reindexed when the project's visibility_level changes (through elastic_index_dependant_association, once the migration has finished). They are also reindexed when its snippets_access_level changes (in EE::ProjectFeature), also only once the migration has finished.
  • Snippets are indexed even for projects outside indexing limits (Elastic::SnippetsSearch#use_elasticsearch?). The refresh paths were gated on the project being indexed, so on an instance that limits indexing, a non-indexed project's snippet documents would keep a stale visibility. Three changes close that:
    • ElasticAssociationIndexerWorker keeps the snippets association when the object is not indexed, and drops the others as before.
    • EE::ProjectFeature enqueues the snippets refresh for non-indexed projects too.
    • ProcessInitialBookkeepingService.backfill_projects! re-tracks the snippets of a non-indexed project. A group transfer that lowers project visibility goes through it, because update_all skips the model callbacks.

Next steps:

  • Step 2, !259752 (merged): a backfill migration for existing snippet documents. It reindexes every snippet below schema version 26_01, which also covers changes made before the mapping migration finished.
  • Step 3, !259759: adds a public and internal project branch to filter_project_snippets, gated on the backfill. It also removes the 8 pending rows in ee/spec/services/ee/search/snippet_service_spec.rb.

Both are stacked on this MR and merge after it.

Access rule for step 3

I checked this rule against Ability.allowed?(user, :read_snippet, snippet). I covered project visibility, snippets access level, snippet visibility, and user type. I found no differences.

A non-member can read a project snippet when both of these are true:

  • The project's visibility and the snippet's visibility are both in Gitlab::VisibilityLevel.levels_for_user(user).
  • snippets_access_level is enabled.

Advanced Search migration

  • Runtime: one update_mapping call on the main index. No batching or throttling.
  • Reversibility: Elasticsearch can't remove a field from an existing mapping, so the migration can't be reverted. An unused mapped field is harmless. To drop it, remove it from config.rb and reindex.
  • Multi-version compatibility: code from before this MR never writes the field, so it can't fail against the strict mapping. New code writes it only after the migration has finished. snippets_access_level is already mapped.

Database

No schema change. This MR adds read queries in two places.

Snippet reindex on a project visibility or snippets access level change. ElasticAssociationIndexerWorker reads the project's snippets with find_in_batches:

SELECT "snippets".* FROM "snippets"
WHERE "snippets"."type" = 'ProjectSnippet' AND "snippets"."project_id" = <project_id>
ORDER BY "snippets"."id" ASC LIMIT 1000;

snippets has no (project_id, id) index. The planner reads the project's rows through index_snippets_on_project_id_and_visibility_level and sorts them by id, so it does not walk snippets_pkey, even when the project has no snippets.

Project Rows Buffers Time Plan
gitlab-org/gitlab 603 606 688 ms (cold) 166294
gitlab-org/cli 3 9 15 ms 166295
A project ID with no snippets 0 6 8 ms 166296

Preload during bulk indexing. Snippet.preload_for_indexing adds two primary-key lookups per batch of snippets:

SELECT "projects".* FROM "projects" WHERE "projects"."id" IN (<project ids>);
SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" IN (<project ids>);

They use projects_pkey and index_project_features_on_project_id. For 10 project IDs: 9 rows, 54 buffers, 65 ms (166297) and 9 rows, 49 buffers, 50 ms (166298). They replace one query per snippet that as_indexed_json would otherwise run.

Tests

The specs assert that:

  • project_visibility_level and snippets_access_level are written for project snippets once the migration has finished.
  • Neither field is written for personal snippets.
  • Neither field is written before the migration has finished, and documents keep the previous schema version until then.
  • The migration adds a short project_visibility_level mapping that matches config.rb. The shared 'migration adds mapping' example now checks this for every mapping migration.
  • Indexing preloads the project and the project feature.
  • Project declares its snippets as dependants of visibility_level.
  • Changing a project's snippets_access_level reindexes its snippets, including for a project that isn't indexed, and doesn't reindex them before the migration has finished.
  • A project visibility change queues snippets alongside the other associations.
  • ElasticAssociationIndexerWorker still updates snippets for an object that isn't indexed, and skips the other associations.
  • backfill_projects! tracks the snippets of a project that isn't indexed.

Removing each change makes its spec fail.

References

Screenshots or screen recordings

Not applicable. This is a backend-only change.

How to set up and validate locally

  1. Enable Advanced Search in GDK.

  2. In a Rails console, run Elastic::MigrationWorker.new.perform until this returns true. The worker runs pending migrations in order, so other pending migrations may run first:

    Elastic::DataMigrationService.migration_has_finished?(:add_project_visibility_level_to_snippets)

  3. Pick a project snippet, or create one in a public project if your GDK has none, and reindex it:

    snippet.maintain_elasticsearch_update
Elastic::ProcessBookkeepingService.new.execute

  4. Fetch its document:

    Snippet.__elasticsearch__.client.get(index: Snippet.__elasticsearch__.index_name, id: "snippet_#{snippet.id}")

  5. Check that the document has project_visibility_level and snippets_access_level.

  6. Change the project's visibility, then repeat steps 3 to 5. project_visibility_level should change.

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