feat(multi-tenancy): tenant transaction scope and SQL filtering foundations (#6212)#6255
feat(multi-tenancy): tenant transaction scope and SQL filtering foundations (#6212)#6255laugiov wants to merge 67 commits into
Conversation
bc25355 to
698b8a9
Compare
a9c61a0 to
c6ae59a
Compare
e141fb9 to
5ad2db4
Compare
69e48a6 to
5b4007b
Compare
There was a problem hiding this comment.
Pull request overview
Introduces the foundations for transaction-scoped tenant isolation by carrying an explicit tenant scope (TxCtx) into a transaction-local PostgreSQL setting, then using a Hibernate StatementInspector to rewrite SQL so tenant-scoped tables are filtered via can_access_tenant(...) (inert by default via an empty activation allowlist).
Changes:
- Add
TxCtxscope model + a transaction aspect to write scope intoapp.current_tenants. - Add PostgreSQL
can_access_tenant(...)function and a SQL rewriter (TenantStatementInspector) with activation gating viaTenantTables. - Add wiring/configuration and extensive tests (inspector behavior, resolver behavior, ordering with
@Lock, and end-to-end table activation examples).
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| openaev-model/src/main/java/io/openaev/database/repository/FindingRepository.java | Splits a previously unparseable modifying CTE into parseable statements to work with SQL rewriting. |
| openaev-model/src/main/java/io/openaev/context/TxCtx.java | Adds a sealed tenant transaction-scope contract (missing vs restricted). |
| openaev-model/src/main/java/io/openaev/aop/TenantScopeTransactionAspect.java | Writes TxCtx scope into app.current_tenants at transactional method entry. |
| openaev-api/src/main/java/io/openaev/rest/helper/RestBehavior.java | Maps tenant-filtering refusal to a stable 500 error code. |
| openaev-api/src/main/java/io/openaev/rest/finding/FindingWriter.java | Adds a REQUIRES_NEW writer for the split finding upsert/link statements. |
| openaev-api/src/main/java/io/openaev/rest/finding/FindingService.java | Routes the agent-finding persistence path through FindingWriter. |
| openaev-api/src/main/java/io/openaev/migration/V5_22__Add_can_access_tenant_function.java | Adds the PostgreSQL can_access_tenant(row_tenant_id, allow_platform) function. |
| openaev-api/src/main/java/io/openaev/config/TxCtxArgumentResolver.java | Resolves TxCtx for controller params from path/header selectors constrained by user rights. |
| openaev-api/src/main/java/io/openaev/config/TenantTables.java | Derives and manages strict vs dual-scope tenant table sets (plus activation allowlist). |
| openaev-api/src/main/java/io/openaev/config/TenantStatementInspector.java | Implements the SQL rewrite/fail-closed logic for tenant-table filtering. |
| openaev-api/src/main/java/io/openaev/config/TenantScopeResolver.java | Converts request selector + authorized tenants into a deterministic TxCtx. |
| openaev-api/src/main/java/io/openaev/config/TenantFilteringException.java | Defines the fail-closed exception surfaced when SQL can’t be safely filtered. |
| openaev-api/src/main/java/io/openaev/config/TenantFilteringConfig.java | Wires tenant table discovery + statement inspector into Hibernate (inert by default via empty allowlist). |
| openaev-api/src/main/java/io/openaev/config/MvcConfig.java | Registers the TxCtxArgumentResolver with Spring MVC. |
| openaev-api/src/main/java/io/openaev/config/AppConfig.java | Reorders transaction advisor to ensure @Lock -> tx -> tenant-scope precedence. |
| openaev-api/src/main/java/io/openaev/aop/lock/LockAspect.java | Adjusts aspect ordering so locking wraps the transaction. |
| openaev-api/src/main/resources/spring.properties | Forces Spring Data JPA native-query parser to regex to avoid jsqlparser enhancer incompatibilities. |
| openaev-api/pom.xml | Adds explicit jsqlparser dependency + excludes it from JaCoCo instrumentation. |
| openaev-api/src/test/java/io/openaev/rest/helper/RestBehaviorTest.java | Tests exception-to-response mapping for tenant-filtering failures. |
| openaev-api/src/test/java/io/openaev/context/TxCtxTest.java | Unit tests for TxCtx semantics/validation/immutability. |
| openaev-api/src/test/java/io/openaev/context/CanAccessTenantFunctionTest.java | Integration tests for the can_access_tenant SQL function behavior. |
| openaev-api/src/test/java/io/openaev/config/TxCtxArgumentResolverIntegrationTest.java | End-to-end HTTP resolution tests for TxCtx argument binding. |
| openaev-api/src/test/java/io/openaev/config/TenantTablesTest.java | Tests tenant table derivation from entity model + allowlist restriction behavior. |
| openaev-api/src/test/java/io/openaev/config/TenantTablesModelTest.java | Verifies tenant table derivation from the real Hibernate metamodel. |
| openaev-api/src/test/java/io/openaev/config/TenantStatementInspectorTest.java | Extensive rewrite behavior tests for SELECT/UPDATE/DELETE/INSERT shapes and the activation gate. |
| openaev-api/src/test/java/io/openaev/config/TenantSqlReplayMeasurementTest.java | Optional replay test for captured real SQL to detect tenant-read leaks. |
| openaev-api/src/test/java/io/openaev/config/TenantSqlLeakOracleTest.java | Tests for the parser-independent leak detector. |
| openaev-api/src/test/java/io/openaev/config/TenantSqlLeakOracle.java | Adds a parser-independent tenant-table leak oracle used by empirical tests. |
| openaev-api/src/test/java/io/openaev/config/TenantScopeTransactionAspectTest.java | Unit tests for TxCtx argument detection and set_config behavior. |
| openaev-api/src/test/java/io/openaev/config/TenantScopeTransactionAspectIntegrationTest.java | Integration tests ensuring scope is set inside active transactions across propagations and doesn’t leak. |
| openaev-api/src/test/java/io/openaev/config/TenantScopeResolverTest.java | Unit tests for selector-vs-rights tenant scope resolution rules. |
| openaev-api/src/test/java/io/openaev/config/TenantScopeLockOrderingIntegrationTest.java | Integration tests proving @Lock wraps the transaction and scope is set inside it. |
| openaev-api/src/test/java/io/openaev/config/TenantIsolationIntegrationTest.java | Base utilities for end-to-end tenant isolation tests against the DB layer. |
| openaev-api/src/test/java/io/openaev/config/TenantIsolationExampleTest.java | End-to-end tenant isolation example test (tags table) under activated filtering. |
| openaev-api/src/test/java/io/openaev/config/TenantInspectorEmpiricalGateTest.java | Empirical barrier test capturing real emitted SQL per entity and checking for leaks/fail-closed. |
| openaev-api/src/test/java/io/openaev/config/TenantFilteringConfigTest.java | Tests schema-derived tenant table classification and Hibernate wiring correctness. |
| openaev-api/src/test/java/io/openaev/config/ImportMapperTenantIsolationTest.java | End-to-end readiness test for first intended activated table (import_mappers). |
| openaev-api/src/test/java/io/openaev/config/CapturingStatementInspector.java | Test-only inspector to capture emitted SQL in-memory or to a file for replay. |
5ad2db4 to
89627a8
Compare
5610ca2 to
35fb2de
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 66 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
openaev-api/src/main/java/io/openaev/rest/mapper/MapperApi.java:95
- issue (moderate): This controller method returns a JPA entity (
ImportMapper) directly. This makes the API contract harder to evolve (entity changes become breaking API changes) and is contrary to the project’s API-layer conventions (use Output DTOs + mapper).
Consider introducing an ImportMapperOutput DTO and mapping ImportMapper to it in the controller responses (including this endpoint).
@GetMapping("/{mapperId}")
@Transactional
@AccessControl(
resourceId = "#mapperId",
actionPerformed = Action.READ,
resourceType = ResourceType.MAPPER)
// TxCtx is resolved from the request and applied by the transaction aspect; it scopes this read
// to the caller's tenants. The handler does not use it directly.
public ImportMapper getImportMapperById(TxCtx ctx, @PathVariable String mapperId) {
return importMapperRepository
.findById(UUID.fromString(mapperId))
.orElseThrow(ElementNotFoundException::new);
}
367bebe to
024e334
Compare
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (2.96%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6255 +/- ##
============================================
+ Coverage 43.79% 44.08% +0.29%
- Complexity 7043 7174 +131
============================================
Files 2254 2265 +11
Lines 62116 62400 +284
Branches 8183 8240 +57
============================================
+ Hits 27203 27512 +309
+ Misses 33180 33132 -48
- Partials 1733 1756 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
024e334 to
4975a12
Compare
… its HTTP isolation (#6212)
…ve its HTTP isolation (#6212)
… to avoid a rebase clash (#6212)
…'s tenant scope (#6212)
…ints and prove their HTTP isolation (#6212)
…'s tenants and remove the unused unscoped repositories on import_mapper child tables(#6212)
…'s tenant scope (#6212)
…cise import endpoints (#6212)
…dmin spanning two tenants (#6212)
…tenant and guard the v1/v2 header double-filter (#6212)
…ble gate so a literal cannot fail-close an unrelated statement (#6212)
…e tenant-filtered subquery (#6212)
… the other modifying queries in InjectRepository (#6212)
… to avoid the clash with main's V5_24 (#6212)
… createImportMapper thread-local fallback (#6212)
…d clash with main's V5_25 (#6212)
d347224 to
7e51131
Compare
What this is
The v2 tenant isolation. The tenant scope lives on the transaction, and a SQL rewriter filters every tenant table against it. The mechanism is behind an allowlist that is empty by default, so for almost every table merging this changes nothing in production.
The one exception is
import_mappers: this PR switches it fully onto v2 (its v1 filter removed, the table added to the allowlist), so it ships as the first table isolated entirely by the new mechanism rather than as dormant code.The old v1 isolation (the
@Filterdriven by theTenantContextthread-local) is untouched and still runs for every other table. v2 sits next to it, dormant for those tables.What's in here
The mechanism is live infrastructure, but dormant for a table until that table is in the allowlist:
TxCtx, the scope contract. Two states, no "all tenants" wildcard: a restricted list of tenant ids, or no scope (fail-closed, denies everything).can_access_tenant(...), the function the filter calls (FlywayV5_26, fail-closed,PARALLEL SAFE).set_config(..., true), so it is transaction-local and never leaks. The order is pinned explicitly (lock, then transaction, then scope), and a nested transaction cannot redefine a scope that is already set.TenantStatementInspector, the rewriter: SELECT (joins, sub-queries, CTEs, unions), UPDATE andUPDATE ... FROM, DELETE andDELETE ... USING, INSERT (with tenant validation on the write side). Anything it cannot safely filter on an active table is refused, not let through.TxCtxfrom the request (path tenant wins, else theX-Tenant-Idsheader, else your full set of rights). The selector is intersected with your actual rights, and asking for a tenant you are not a member of is a 403, before RBAC. Rights are membership, not the admin flag.The first table:
import_mappers(activated here)I took one table all the way through and switched it on in this PR.
import_mappersis HTTP-only (nothing but HTTP touches it), which is why it goes first.What "fully on v2" means here:
@Filteris removed (v1 and v2 must not both filter the same table: v1 forces a single tenant, v2 wants the request scope, and on the header route the two together returned ~0 rows);import_mappersis added toopenaev.tenant.active-tables;The whole
MapperApisurface is scoped, with real two-tenant tests against Postgres (no mocks on the isolation path): reads, update and delete, duplicate, create and import, plus the mapper lookup in the scenario and exercise import endpoints. There are non-admin tests (a foreign tenant is refused at the binding; a non-admin in two tenants only sees the one in the path), and the header-route test is now enabled (it was parked while v1 still filtered the table).Behaviour change at merge: create and import now require a single-tenant scope (else 400). The frontend already tenant-path-scopes every API call, so its mapper create and import keep a single tenant, and no other caller (backend, connectors) creates mappers, so nothing breaks. The api suite is green.
What's left, and how
Coordination, roughly in this order.
1. Confirm the aspect order. The three
@Ordervalues move the global transaction-advisor precedence, which is your call (transaction layer). They are committed and the tests pin the behaviour either way, so it is a yes/no on keeping them, not work.2. The next tables. Same recipe as
import_mappers, but first every path touching the table has to carry aTxCtx. The HTTP paths do; the background ones (jobs, event handlers, indexing, startup seeding) do not yet, and an active table with no scope is fail-closed (zero rows). So those tables wait on the Track A work to carry the scope off the request thread.import_mapperswent first only because nothing but HTTP touches it.3. Drop v1. Once enough tables are on v2, remove the
TenantContextthread-local and the filter aspect (overlaps #6229). End state, not a per-table step.