Fixes 31740: Enforce caller policies on the Incident Manager listing (#31741) [2.0 backport] - #32017
Conversation
…31741) * fix(dq): enforce caller policies on the incident manager listing The Incident Manager listing returned incidents from every domain to a domain-restricted user. Clicking one was correctly blocked, but the row itself already exposed the test case name, the table name and the full FQN. RuleEvaluator#hasDomain short-circuits to true for list operations, because a listing has no single resource to evaluate a condition against, and defers to "post-filtering" on the search query. The two search-backed listing paths in EntityTimeSeriesRepository never received a SubjectContext, so that post-filtering never happened and no hasDomain()/noDomain() policy could take effect. Upgrading does not help: 7c9d85a added a user-supplied `domain` filter parameter, not policy enforcement. Thread the caller's SubjectContext through both paths so the existing RBAC search machinery filters the query: - listFromSearchWithOffset now has a SubjectContext-aware overload that calls SearchRepository#listWithOffset(..., subjectContext), which already applies RBAC in both engines. - listLatestFromSearch does the same through SearchRepository#aggregate, so `latest=true` cannot bypass the filtering applied to the plain listing. This adds a SubjectContext-aware aggregate() down through AggregationManagementClient and both search clients/aggregation managers, mirroring the existing genericAggregation() overload. - TestCaseResolutionStatusResource passes the caller on both branches. Existing callers keep the subject-less overloads and are unaffected. Filtering still honours the global searchSettings.enableAccessControl gate, as elsewhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(dq): add Playwright coverage for incident manager domain isolation Adds DomainIncidentIsolation.spec.ts alongside the existing DomainIsolation specs, covering the listing through the UI. The user under test deliberately holds DomainOnlyAccessRole with an EMPTY domain list. A user WITH a domain is not a valid regression guard: useIncidentList passes activeDomain as the `domain` query param, and that caller-supplied filter alone hides foreign incidents even on a server carrying the bug — the spec would pass against unfixed code and protect nothing. With no domain the page sends no `domain` param, so the server-side policy is the only thing that can filter the list. Assertions are made against both the payload the page requested and the rendered rows. The list is paginated, so a leaked row can sit on page 2 while page 1 looks clean; asserting only on the DOM would let a real leak pass. Indexing is handled by seedFailedIncidents, which polls until every seeded incident is searchable, so the assertions never race Elasticsearch. Verified against a locally built server: - fixed build: 2/2 pass; 10/10 with --repeat-each=5 - reverted build: the isolation test fails on the leaked FQN while the admin control still passes, confirming the spec detects the regression - full DomainIsolation lane: 16/16 pass Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(dq): address review feedback on incident domain isolation Review fixes: - SearchRepository#aggregate(4-arg) calls the subject-less client method again instead of delegating to the new 5-arg overload. Delegating rerouted every existing caller onto a different client method and broke SearchRepositoryBehaviorTest#reportingWrappersDelegateToSearchClient, which stubs the 4-arg overload. Keeping the subject-less path byte-identical also keeps the blast radius of this change to the endpoint being fixed. - DomainIsolationIT now asserts both latest=false and latest=true. The endpoint defaults to false, so the aggregation branch — the one `latest=true` uses, and a trivial bypass if unguarded — had no regression coverage. Playwright spec: - Declare the tag via the { tag: [...] } option rather than embedding it in the describe title. 57 specs use the option against 8 with an in-title tag; --grep @domain-isolation still selects both tests. - Drop describe-level test.slow(). The tests run in ~2.7s against a 60s budget, so it bought nothing. What actually needs a budget is the hook, and test.slow() does not extend one — so beforeAll sets an explicit timeout for entity creation plus waiting on indexing, matching IncidentManagerPagination. - Match waitForResponse on URL only and assert the status separately. Narrowing the predicate by status never resolves on an error response, turning a failing API into an opaque timeout instead of a status assertion. - Parallelise independent setup and teardown with Promise.all, in phases that respect the real dependencies: the table must carry its domain before the incident is indexed, and the tables are deleted before the domain. Verified on a locally built server: both listing paths filter for a domainless user; the spec passes 6/6 and still fails against a reverted build; SearchRepositoryBehaviorTest 127/127 and the search package 2187/2187. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(test): keep the incident isolation IT entity names inside the column limit DomainIsolationIT#test_incidents_restrictedUserSeesOnlyOwnDomain failed in CI on both engines: Data truncation: Data too long for column 'name' INSERT INTO test_suite ... Creating a test case implicitly creates a test suite whose name is the table's FQN plus a suffix. createSchema derives the service, database and schema names from TestNamespace, which embeds the test method name at every level, so the resulting name ran to roughly 410 characters. The sibling tests in this class do not hit it because none of them create a test suite. Use short, explicitly named database and schema entities under the shared MySQL service — the pattern IncidentPaginationIT already uses for the same reason — which brings the test suite name to about 84 characters. Shorten the test method name too, since it no longer has to carry namespace duplication. Not reproducible locally: the IT harness builds a server image from the dist tarball and boots its own containers, so CI is the first place this runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(dq): consolidate aggregation RBAC filtering and fail closed genericAggregation() in the Elasticsearch and OpenSearch aggregation managers inlined the same evaluate-then-bool(must/filter) RBAC block that applyRbacQuery() already encapsulates. Route genericAggregation() through applyRbacQuery() so each manager keeps one copy used by both the aggregate() and genericAggregation() paths. Also fail closed in applyRbacQuery(): when access control requires filtering for the caller but evaluateConditions() yields no query, return a match-none query instead of the unfiltered query. evaluateConditions() returns matchAll today so this branch is currently unreachable, but for a security filter the safe default on an unexpected null must be to hide rather than leak. Behaviour-preserving: the subject-less path (null subject / admin / bot / access control disabled) still returns the query unchanged, and the search listing path (listWithOffset) is untouched. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: sonika-shah <58761340+sonika-shah@users.noreply.github.com> (cherry picked from commit 8df0bb5)
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
safe to test
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private Query applyRbacQuery(Query query, SubjectContext subjectContext) { | ||
| if (!SearchUtils.shouldApplyRbacConditions(subjectContext, rbacConditionEvaluator)) { | ||
| return query; | ||
| } | ||
| OMQueryBuilder rbacQueryBuilder = rbacConditionEvaluator.evaluateConditions(subjectContext); | ||
| if (rbacQueryBuilder == null) { | ||
| // Fail closed: policies had to be applied for this caller (access control on, not admin/bot) | ||
| // but produced no query. Returning the unfiltered query would leak; match nothing instead. | ||
| return Query.of(qb -> qb.matchNone(m -> m)); | ||
| } | ||
| Query rbacQuery = ((ElasticQueryBuilder) rbacQueryBuilder).buildV2(); | ||
| if (query == null) { | ||
| return rbacQuery; | ||
| } | ||
| final Query existingQuery = query; | ||
| return Query.of(qb -> qb.bool(b -> b.must(existingQuery).filter(rbacQuery))); | ||
| } |
There was a problem hiding this comment.
The new RBAC helper has several early returns and exceeds the repository’s focused-method guideline; duplicating this structure in the OpenSearch implementation makes the policy-enforcement behavior harder to review and keep aligned.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const response = await incidentResponse; | ||
|
|
||
| expect(response.status()).toBe(200); | ||
|
|
||
| await waitForAllLoadersToDisappear(page); | ||
|
|
There was a problem hiding this comment.
The response predicate embeds the endpoint directly in includes(), while the filter-bar test ID is another raw identifier later in the helper. Defining these identifiers as constants follows the repository guidance and prevents future endpoint or selector changes from requiring synchronized literal updates.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| try { | ||
| String p = ns.shortPrefix(); | ||
| Domain ownDomain = createDomain(admin, p + "_d1", cleanup); | ||
| Domain foreignDomain = createDomain(admin, p + "_d2", cleanup); | ||
| DatabaseSchema schema = createShortNamedSchema(admin, p, cleanup); | ||
| Table ownTable = createTable(admin, p + "_own", schema, ownDomain, cleanup); | ||
| Table foreignTable = createTable(admin, p + "_foreign", schema, foreignDomain, cleanup); | ||
|
|
||
| String testDefinitionFqn = | ||
| admin | ||
| .testDefinitions() | ||
| .list(new ListParams().withLimit(1)) | ||
| .getData() | ||
| .get(0) | ||
| .getFullyQualifiedName(); |
There was a problem hiding this comment.
The added Java test methods introduce multiple parameters and local variables that are never reassigned without declaring them final. This conflicts with the repository’s Java guidance and makes mutability less explicit throughout the new setup and helper code.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Code Review ✅ ApprovedBackport of the fix to enforce caller policies on Incident Manager listings by threading the SubjectContext through EntityTimeSeriesRepository search paths, preventing domain isolation leaks. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
❌ UI Checkstyle Failed❌ Core Components - I18n SyncCore-components ❌ Antd + Less Deprecation GuardA new Affected filesat Function._resolveFilename (node:internal/modules/cjs/loader:1401:15) Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
`TestCaseResolutionStatusResource` imports `CommonUtil.listOrEmpty` and `CommonUtil.nullOrEmpty` but no longer uses either, so `mvn spotless:check` fails and the `java-checkstyle` job is red for every PR targeting this branch -- #32095 and #32093 fail identically. The usages went away in #32017 while the imports stayed. Import-only change, produced by `mvn spotless:apply`; no logic touched. Needed here only so this backport can be validated. `main` needs no equivalent fix: both symbols are imported and used five times each in that file there. On `1.13` the imports do not exist. This is 2.0-only.
1 participant
Describe your changes:
Fixes #31740
2.0 backport of #31741.
A domain-restricted user saw failed test case incidents from every domain on the Incident Manager page. Opening one was correctly blocked, but the listed row already leaked the test case name, the table name and the full FQN.
RuleEvaluator#hasDomainshort-circuits totruefor list operations (a listing has no single resource to evaluate the condition against) and defers enforcement to search-side RBAC filtering. The two search-backed listing paths inEntityTimeSeriesRepositorynever received aSubjectContext, so that filtering never ran and no policy could restrict the listing.This threads the caller's
SubjectContextthrough both paths so the existing RBAC search machinery filters the query.Clean cherry-pick
Unlike the 1.13 backport,
2.0is close enough tomainthat8df0bb573dcherry-picks almost cleanly —2.0already has the "ContextMemory" featuremain'sElasticSearchAggregationManager/OpenSearchAggregationManagerbuild on, the fullDomainIsolationIT.javatest infrastructure, theDomainIsolationPlaywright folder (already wired as its own dedicated CI project —--project=DomainIsolationis already invoked inplaywright-postgresql-e2e.yml), and theincident-filter-bartestid the Playwright spec asserts on. Only one trivial import-ordering conflict inTestCaseResolutionStatusResource.java, resolved by hand.All 10 files match the original PR's file list exactly — no hand-adaptation, no reconstructed test files, no dead code.
Type of change:
Tests:
Backend integration tests
DomainIsolationIT#test_incidents_restrictedUserSeesOnlyOwnDomain(existing test class on2.0, extended by the cherry-picked commit) — asserts bothlatest=falseandlatest=true.Playwright (UI) tests
DomainIncidentIsolation.spec.ts— cherry-picked as-is, runs under the existingDomainIsolationproject (already scheduled in CI).Manual testing performed
Verified
mvn compileforopenmetadata-service,mvn test-compileforopenmetadata-integration-tests,mvn spotless:apply(only removed two genuinely-unused imports left over from the conflict resolution),yarn tsc:playwrightshows no new type errors, andeslint/prettierpass clean on the touched files. Not run against a live server in this session — first execution will be in CI.Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Bug fix:
🤖 Generated with Claude Code
Greptile Summary
This backport threads the caller’s subject context through both Incident Manager search paths and applies RBAC predicates before plain-result retrieval or latest-status aggregation. It also adds backend and Playwright coverage for domain-isolated incident listings.
latest=falseandlatest=true.Confidence Score: 4/5
The PR is safe to merge, with only non-blocking repository-style cleanup needed in the RBAC helpers, Java test declarations, and Playwright constants.
The caller context reaches both incident search paths, and both search backends apply the resulting RBAC query before returning or aggregating documents; the accepted concerns are maintainability issues rather than functional defects.
Files Needing Attention: openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchAggregationManager.java, openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchAggregationManager.java, openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java, openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/DomainIsolation/DomainIncidentIsolation.spec.ts
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Incident Manager participant Resource as Resolution Status Resource participant Repo as Time-Series Repository participant Search as Search Repository participant Engine as Elasticsearch/OpenSearch UI->>Resource: List incidents (latest true/false) Resource->>Resource: Authorize request and resolve SubjectContext alt "latest=true" Resource->>Repo: listLatestFromSearch(..., subject) Repo->>Search: aggregate(..., subject) else "latest=false" Resource->>Repo: listFromSearchWithOffset(..., subject) Repo->>Search: listWithOffset(..., subject) end Search->>Engine: Query with caller RBAC predicate Engine-->>UI: Policy-filtered incident rowsReviews (1): Last reviewed commit: "Fixes 31740: Enforce caller policies on ..." | Re-trigger Greptile
Context used: