Fixes 28463: enforce domain RBAC in Data Quality CSV export and import - #31147
Fixes 28463: enforce domain RBAC in Data Quality CSV export and import#31147TeddyCr wants to merge 4 commits into
Conversation
…open-metadata#28463) DomainOnlyAccessPolicy grants All/All whenever hasDomain() passes, and RuleEvaluator.hasDomain() returns true when the resource context carries no concrete entity — it logs "List operation detected (no specific resource), returning true for post-filtering". The Data Quality CSV export/import fix that follows has to apply exactly that post-filter, from inside a repository and from a background job. DomainAccessFilter mirrors the entity-level decision hasDomain() makes: domainless entities are visible to everyone, otherwise the subject must own the entity's domain or one of its ancestors. It resolves the SubjectContext from a principal name because background CSV jobs never carry a request SecurityContext — the same resolution CsvImportExportJobHandler already performs for search-backed exports. Resolution fails closed: a principal that cannot be resolved to a user aborts the operation instead of letting it run unfiltered, since the subject resolves at execution time and a user deleted or renamed while an async export sat queued would otherwise silently disable filtering. The one exemption is the fixed principal NoopFilter installs when the deployment runs without authentication, and only when it resolves to no user at all. LineageDomainFilter.shouldApply now delegates to the shared predicate instead of repeating it verbatim. No behaviour change there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ort (open-metadata#28463) A user holding DomainOnlyAccessRole could read every test case on the platform via GET /v1/dataQuality/testCases/name/{name}/export, and could create test cases outside their domains via PUT /v1/dataQuality/testCases/name/{name}/import. These endpoints authorize once against a ResourceContext built from the path name, which here is "*", a table FQN or a test-suite FQN — never a TestCase name — so no concrete entity resolves and hasDomain() allows the request on the promise of a post-filter. TestCaseRepository never applied one: getTestCasesForExport("*") returned listAll() over the whole platform, and TestCaseCsv.createEntity called createOrUpdateForImport per row against the row's own attacker-controlled entityFQN. Export now post-filters the resolved test cases through DomainAccessFilter. Import gates every row on the domain of the entity it targets and on the domain of the test suite it names — both columns come from the CSV, and attaching a test case adds a CONTAINS relationship from the suite and bumps its version, so both need the same gate. A row rejected on its target entity is reported as a row failure rather than aborting the import, because rows are applied one by one and are not rolled back, so aborting would misreport the rows already written. A row naming an unusable suite takes one branch whether that suite is absent or out of domain, so the row detail cannot be used as an existence oracle. Attaching to a Bundle Suite named by the request path is rejected outright, since nothing is written yet at that point. The export field lists now request `domains` so the post-filter sees the domain a test case inherits from its linked table; test cases never materialize a domain relationship of their own, so a SQL domainId filter alone would not have closed this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tadata#28463) A CSV import bumps the version of the entity named by the request path, writing a bulkImport ChangeDescription, a ChangeEvent and a version-history row against it. Both the target FQN and the entity type it is resolved against arrive as request input, so the caller chose which entity received that write, and neither the sync nor the async path checked it. The async path was reachable in the Data Quality flow: importing test cases with a foreign table FQN as the path name and entirely legitimate rows bumped that table's version and overwrote its updatedBy, because the single authorize resolves no concrete entity and hasDomain() therefore allows the request. The sync path avoided it for test cases only because TestCaseResource overrides processChangeEventForBulkImport to a no-op; the async handler has no such override, and that asymmetry is what kept it out of sight. Both call sites now resolve the target through DomainAccessFilter.resolveAccessibleVersioningTarget, which loads it with its domains and skips versioning when it is outside the caller's domains. Skipping rather than failing is deliberate: the rows are already written by that point, so failing the job would misreport the import. A missing target now takes the same branch, which additionally stops an uncaught getByName from turning job outcome into an existence oracle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a#28463) Six ITs against a real DomainOnlyAccessRole user, reusing the harness DomainIsolationIT established: - export: platform-wide and table-scoped export must contain the user's own domain test case and must not contain a foreign-domain one; - import, target entity: a row pointing at a foreign-domain table must not report success and must not exist afterwards, asserted separately as admin; - import, target entity with no suite named: the same row with an empty testSuite column, so only the target gate can reject it. Without this the target gate has no guarding coverage — the sibling case is exported verbatim from the foreign table, so its suite column also names a foreign suite and the row is rejected by the suite gate instead; - import, test suite column: a row pointing at the user's own table while naming a foreign-domain basic suite must be rejected and that suite's version must be unchanged; - importAsync: two legitimate own-domain rows sent with a foreign table as the request path must leave that table's version and updatedBy untouched (two rows because versioning only runs when numberOfRowsProcessed > 1); - control: an in-domain row must still be applied, so the fix cannot pass by rejecting everything. Each failing case was watched failing against code with only its own guard removed, so every assertion is attributable to the guard it pins. createRestrictedUserClient asserts the role and domain really attached, then waits until the server resolves the principal as domain-restricted. That wait is a stabilisation measure of unknown cause: an earlier run of this class saw the domain assertions fail as though no narrowing had been applied, and the wait made it reproducibly stable. It asserts nothing and fails loudly on timeout, so it cannot let an assertion pass vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
❌ 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a domain-RBAC bypass in the Data Quality CSV export and bulk-edit import flows by introducing a shared domain-access post-filter/gate and applying it across both sync and async CSV paths, including the bulk-import versioning side effect.
Changes:
- Add
DomainAccessFilterto centralize “domain-only access role” evaluation, principal resolution, and entity accessibility checks. - Enforce domain filtering for test case CSV export and per-row import validation (target entity and
testSuitecolumn), and gate bulk-import version bump targets in both sync and async handlers. - Add an integration test suite covering export disclosure, import write prevention, foreign-suite attachment prevention, and async versioning bypass prevention.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/DomainAccessFilter.java | New centralized domain-access filter utilities (shouldApply/resolveSubject/retainAccessible/isAccessible + versioning target resolution). |
| openmetadata-service/src/main/java/org/openmetadata/service/search/lineage/LineageDomainFilter.java | Delegates domain-only “should apply” decision to DomainAccessFilter to avoid duplicated logic. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/EntityResource.java | Gates bulk-import versioning change-event creation using DomainAccessFilter (skip if inaccessible/missing). |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java | Applies domain post-filter to export results; adds per-row import gates for target entity and CSV-supplied test suite; checks bundle suite accessibility. |
| openmetadata-service/src/main/java/org/openmetadata/service/csv/CsvImportExportJobHandler.java | Applies the same bulk-import versioning target gate for async CSV imports. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseCsvDomainIsolationIT.java | New integration tests validating the domain-RBAC enforcement for export/import and async versioning. |
| if (testSuite == null) { | ||
| importFailure( | ||
| printer, String.format(TEST_SUITE_UNAVAILABLE_MESSAGE, testSuiteFqn), csvRecord); | ||
| importResult.withStatus(ApiStatus.ABORTED); |
| private boolean rejectIfTargetOutOfDomain( | ||
| CSVPrinter printer, CSVRecord csvRecord, String entityFQN, String entityLink) | ||
| throws IOException { | ||
| boolean outOfDomain = false; | ||
| if (DomainAccessFilter.shouldApply(subjectContext)) { | ||
| EntityInterface target = | ||
| Entity.getEntity(EntityLink.parse(entityLink), FIELD_DOMAINS, Include.NON_DELETED); | ||
| outOfDomain = !subjectContext.hasDomains(target.getDomains()); | ||
| } | ||
| if (outOfDomain) { | ||
| importFailure(printer, String.format(OUT_OF_DOMAIN_MESSAGE, entityFQN), csvRecord); | ||
| importResult.withStatus(ApiStatus.FAILURE); | ||
| } | ||
| return outOfDomain; | ||
| } |
There was a problem hiding this comment.
💡 Security: Per-row target gate leaks table existence via distinct error messages
In rejectIfTargetOutOfDomain, an existing but foreign-domain target produces OUT_OF_DOMAIN_MESSAGE, whereas a non-existent target makes Entity.getEntity(...) throw EntityNotFoundException, which the generic catch at line 2254 reports with a different ex.getMessage(). A domain-restricted user can therefore distinguish "exists but outside my domains" from "does not exist" and probe the existence of arbitrary table FQNs — the same cross-domain existence oracle the PR deliberately closed for the testSuite column (resolveAccessibleTestSuite merges absent/foreign into one branch). Consider catching EntityNotFoundException inside rejectIfTargetOutOfDomain and rejecting with the same OUT_OF_DOMAIN_MESSAGE so absent and foreign targets are indistinguishable.
Merge the absent-target and foreign-target cases into one indistinguishable rejection branch.:
private boolean rejectIfTargetOutOfDomain(
CSVPrinter printer, CSVRecord csvRecord, String entityFQN, String entityLink)
throws IOException {
boolean outOfDomain = false;
if (DomainAccessFilter.shouldApply(subjectContext)) {
try {
EntityInterface target =
Entity.getEntity(EntityLink.parse(entityLink), FIELD_DOMAINS, Include.NON_DELETED);
outOfDomain = !subjectContext.hasDomains(target.getDomains());
} catch (EntityNotFoundException e) {
// Absent and foreign take the same branch so the row detail is not an existence oracle.
outOfDomain = true;
}
}
if (outOfDomain) {
importFailure(printer, String.format(OUT_OF_DOMAIN_MESSAGE, entityFQN), csvRecord);
importResult.withStatus(ApiStatus.FAILURE);
}
return outOfDomain;
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsEnforces domain RBAC in Data Quality CSV export and import paths, closing a disclosure and write vulnerability for DomainOnlyAccessRole users. Consider unifying the per-row target check error messages to prevent entity existence probing. 💡 Security: Per-row target gate leaks table existence via distinct error messages📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java:2320-2334 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java:2254-2256 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java:2348-2362 In Merge the absent-target and foreign-target cases into one indistinguishable rejection branch.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
|



Describe your changes:
Fixes #28463
A user restricted to one domain (
DomainOnlyAccessRole) sees only their domain's test cases on the Data Quality dashboard, but Export and Bulk Edit returned test cases from every domain. The reported symptom is disclosure; the code also allowed a write.Root cause. Two domain-RBAC enforcement mechanisms already exist and both work:
GET /v1/dataQuality/testCases) →EntityResource.listInternal→EntityUtil.addDomainQueryParam, which injects adomainIdrestriction;.../search/list) →subjectContextthreaded tosearchClient.listWithOffset→RBACConditionEvaluator.The CSV export/import path implemented neither.
TestCaseRepository.getTestCasesForExportdid a barelistAll(...)— for path name"*", every test case on the platform.The single
authorizer.authorize(...)in front of it does not help, and that is by design rather than by accident:RuleEvaluator.hasDomain()deliberately returnstruefor a null resource entity, logging "List operation detected (no specific resource), returning true for post-filtering".DomainOnlyAccessPolicydelegates enforcement to an endpoint post-filter. Both safe paths implement one; this path had none.Write side.
importCsvInternalhad the same gap, andTestCaseCsv.createEntitycalledcreateOrUpdateForImportper row with no domain check — so a domain-restricted user could attach test cases to out-of-domain tables. Two further write vectors surfaced while fixing it:testSuitecolumn was resolved with a bare existence lookup, so a row could pass the target check while attaching to a foreign-domain test suite — bumping that suite's version and emitting a change event;importAsyncstorestargetFqnandversioningEntityTypeverbatim from request input, andCsvImportExportJobHandler.createBulkImportVersionversioned that target with no domain check — so a payload of entirely legitimate rows could still bump an arbitrary foreign entity's version and overwrite itsupdatedBy. The sync path was safe only becauseTestCaseResourceoverridesprocessChangeEventForBulkImportto a no-op; the async handler had no equivalent.Why an in-memory post-filter and not a SQL
domainIdpredicate. Test cases never materialise a domain relationship row —TestCaseRepository.setInheritedFieldscallsinheritDomains(testCase, fields, table)at read time only.ListFilter.getDomainConditionreads "no domain row OR domain in the user's set", so anaddDomainQueryParam-style filter would have passed every inherited-domain test case and closed nothing. The search index does materialise it, which is why only the ES path was already safe. The fix filters on the effective inherited domain.Why not route export through the RBAC-aware search path. ES RBAC is gated behind
SearchSettings.globalSettings.enableAccessControl, which defaults tofalse— a search-based fix would be inert on a default deployment.Why the repository signature was not changed. Threading a request
SecurityContextthroughEntityRepository.exportToCsv/importFromCsvcannot serve the async endpoints the UI actually calls:CsvImportExportJobHandlerruns on aBackgroundJobwith no request context and already derives its subject fromjob.getCreatedBy(). TheuserString already present in every signature is the authenticated principal name —CatalogPrincipallowercases in its constructor andJwtFilternormalises viafindUserNameFromClaims, so it is byte-identical to what the authorizer feedsSubjectContext.getSubjectContext, and impersonation is safe becauseJwtFiltersets the principal to the impersonated user. So the name reconstructs an authorization-equivalent subject with no information loss, and no entity type is affected by an interface change.Import rejects per row rather than aborting, because rows are applied one at a time and are not rolled back —
ABORTEDwould falsely imply nothing was written. The row is refused before any write, which is the security guarantee. "Absent" and "outside your domains" deliberately take the same branch so the response cannot be used to probe for suites the caller cannot see.Type of change:
High-level design:
DomainAccessFilter(new) centralises theshouldApply/isAccessible/retainAccessibledecisions, matching the predicateLineageDomainFilteralready used — that duplicate is now a one-line delegation, with no behaviour change.TestCaseRepositoryapplies the export post-filter and the two per-row import gates; the bulk-import versioning target is gated at both call sites through one shared helper so the sync/async asymmetry that hid the last bypass cannot recur.Domain semantics mirror
SubjectContext.checkDomainHierarchyAccess(a domainless entity is accessible), consistent withhasDomain()and with theNOT EXISTSdisjunct in the SQL filter.TaskResource.enforceDomainOnlyPolicyForTaskis stricter on writes; reconciling the two is a separate product decision.Cross-cutting behaviour change, deliberate: the versioning gate applies to every CSV import, not just Data Quality. For a domain-restricted user targeting outside their domains the version bump is skipped, and a missing target is skipped rather than failing the job. Admins, bots, unrestricted users and all in-domain imports are unaffected, and no legitimate version history is lost.
Tests:
Use cases covered
name/*) as a domain-restricted user returns only their domain's test cases.Unit tests
Backend integration tests
openmetadata-integration-tests/.../TestCaseCsvDomainIsolationIT.java— 6/6 pass in both failsafe executions.TestCaseResourceIT, DomainIsolationIT, GlossaryResourceIT, DatabaseResourceIT, TableResourceIT, TeamResourceIT, CsvAsyncJobResourceITplus the new class: 1331 run, 0 failures, 0 errors, 53 skipped. The four other entity types exercisetest_importCsvDryRunandtest_importExportRoundTrip, confirming no CSV regression.Platform-wide export must NOT leak a foreign-domain test caseandImport of an out-of-domain row must not report successtestSuitegate onlyImport naming a foreign-domain test suite must not succeedtest_importCsv_foreignTargetIsRejectedEvenWhenNoTestSuiteIsNamedA foreign-domain table must not be versioned … expected: <0.1> but was: <0.2>verify(), no call counts, noThread.sleep(), membership-only so concurrent namespaces cannot perturb them.createBulkImportVersionrequires more than one processed row.Ingestion integration tests
Playwright (UI) tests
Manual testing performed
NOT VERIFIED by hand — the automated evidence above is what was executed. Steps for a reviewer with a stack:
docker compose -f docker/development/docker-compose.yml up -d, log in as admin.d1andd2(type Aggregate).t1,t2; sett1's domain tod1,t2's tod2.tableRowCountToEqualtest tot1(tc1) and tot2(tc2).u1with domaind1and roleDomainOnlyAccessRole; act asu1.GET /api/v1/dataQuality/testCases/name/*/export→ containstc1, nottc2.GET .../name/{t2 FQN}/export→ header row only.GET .../name/{t1 FQN}/export→ containstc1(no regression).t2, rename the test case toevil, andPUT .../name/{t1 FQN}/import?dryRun=false&targetEntityType=tableasu1→statusisfailure, the row detail readsEntity '<t2 FQN>' is outside the domains you have access to, and as adminGET /api/v1/dataQuality/testCases?entityLink=<#E::table::{t2 FQN}>does not listevil.t1→ created and visible.u1, open Data Quality → Bulk Edit → the grid contains onlyd1test cases; saving a row targeting ad2table is reported as a failed row.UI screen recording / screenshots:
Not applicable — backend only.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.DomainOnlyAccessRole, thedomainsrelationships and the RBAC machinery all already exist.Known limitations
DomainAccessFilter.resolveSubjectfails closed — an unresolvable principal now raises rather than silently skipping the filter. The one exemption is the no-authNoopFilter"anonymous" principal, and only when it resolves to no user at all, so a real user namedanonymousin an authenticated deployment is still filtered. Consequence: a user deleted or renamed while an async export sits queued now gets a 403 where they previously got data.inheritDomainson the read side,setFieldsInBulkon the export side — but are not covered by a test. Worth adding, since service-level domain assignment is the common deployment.CsvAsyncJobResourceITpasses.awaitDomainRestrictionActive) exists because an early run saw the domain assertions fail as though no narrowing had been applied. The cause was never established. The wait asserts nothing and fails loudly on timeout, so it cannot let an assertion pass vacuously — every guard-neutralisation probe above still goes RED with it in place.🤖 Generated with Claude Code