feat(multitenancy): API isolation v2 kill chain phase (#6402) - #7485
Draft
Romuald Lemesle (RomuDeuxfois) wants to merge 1 commit into
Draft
feat(multitenancy): API isolation v2 kill chain phase (#6402)#7485Romuald Lemesle (RomuDeuxfois) wants to merge 1 commit into
Romuald Lemesle (RomuDeuxfois) wants to merge 1 commit into
Conversation
Contributor
|
📖 Documentation check — ✅ Passed 31 functional file(s), 0 doc file(s) changed. No documentation gaps detected. |
Romuald Lemesle (RomuDeuxfois)
marked this pull request as draft
August 17, 2026 15:59
Copilot started reviewing on behalf of
Romuald Lemesle (RomuDeuxfois)
August 17, 2026 15:59
View session
Copilot stopped reviewing on behalf of
Romuald Lemesle (RomuDeuxfois) due to an error
August 17, 2026 16:00
Contributor
|
Core CI reports these findings in advisory mode. Review the JSON reports before merging.
View workflow run · Standard JSON report · UBI9 JSON report Updated from CI run attempt 2. |
Romuald Lemesle (RomuDeuxfois)
force-pushed
the
feat/tenant-v2-kill-chain-phase
branch
from
August 20, 2026 12:44
005e57b to
7439151
Compare
Switch kill_chain_phases from v1 @filter isolation to v2 (TenantStatementInspector + can_access_tenant), go-live included: the @filter removal, the TenantBaseListener removal and the active-tables entry ship together, never split. Reads are scoped by threading TxCtx to every transaction that touches the table. Beyond KillChainPhaseApi itself, that covers four less obvious surfaces: - The association. AttackPattern#killChainPhases is a LAZY @manytomany re-derived by Inject, Scenario, Exercise and InjectorContract. The tenant scope is transaction-local (set_config(..., true)) and open-in-view renders JSON after the commit, where can_access_tenant is fail-closed: a lazy load at rendering time returns zero rows and silently serializes an EMPTY phase list. KillChainPhaseInitializer hydrates the association inside the scoped transaction instead (same blind spot as #7025, applied to this table), and @batchsize keeps a page of attack patterns to one IN-clause query instead of one per 50. - Propagation.SUPPORTS handlers hold no transaction of their own, so TxCtx is threaded down to the service method that opens one, mirroring bulkDeleteScenarios. - The scenario-detail projection joins kill_chain_phases inside a CTE, so the fail-closed rewriter now has to accept that shape (#7007 class of regression); a test pins the real production SQL read off the repository by reflection. - findDistinctByExerciseId joins the table through attack_patterns_kill_chain_phases, which carries no tenant column: under a multi-tenant scope the join could return another tenant's copy of a phase, since can_access_tenant only caps it at the request scope. The query correlates the phase tenant with the simulation's row to row - read from the row, never from a thread-local, so unlike a v1 filter it cannot disagree with the v2 scope. A test fails without the predicate; a TODO v2 marker hands it to the eventual attack_patterns activation. Write attribution is explicit through TenantWriteScopeResolver. The upsert resolves the write tenant BEFORE any lookup and matches on the per-tenant unique keys: every unique key on this table carries tenant_id, so a lookup by the bare business key could match one row per in-scope tenant under a multi-tenant scope and link another tenant's row to the write. TenantBaseListener is gone with it - every create path attributes the tenant explicitly (both endpoints, the upsert service, the importer), so the listener was only a TenantContext fallback able to mask a missing attribution. The shared fixture takes the tenant as a parameter now, and a test pins that an unattributed write is refused by the non-nullable column. The importer follows the same rule. V1_DataImporter creates phases on the import paths, and its write tenant now comes from the TxCtx threaded down from the import entrypoints instead of TenantContext, which is only set on the /api/tenants route and falls back to the default tenant everywhere else: a header-scoped import would look the phase up in one tenant and insert it in another, an INSERT ... VALUES the inspector never validates. There is no fallback, a scope that does not pin exactly one tenant is refused. Its find-or-create goes through KillChainPhaseService rather than the repository, which shortens the reviewed access surface. The inject-level import endpoints (scenario, simulation, atomic testing) carried no TxCtx at all and now do, so their reads stop being fail-closed too, and tenant provisioning declares its own scope at the datapack call site. Guards: the access arch test gains a rule refusing a joined @query on a tenant-active table that does not correlate the tenant, and TenantScopedEntrypointsTxCtxArchTest had six entrypoints declared twice in its Set.of - which throws at class initialisation, so that guard had stopped running at all. Client impact: creates and upserts now need a single-tenant scope. Tenant-path and single-tenant callers are unaffected; a multi-tenant caller without a selector gets 400 on create/upsert. Also, outside the activation itself: KillChainPhase implements Auditable (the manual updatedAt stamps become redundant and stop forcing an UPDATE on every no-op upsert), convert-to-manual returns the resulting scenario id instead of the whole entity nobody read, and the two chaining duplication endpoints that never had a caller are removed with their tests. Tests: isolation through the real endpoints (tenant A vs B, header route, non-admin, per-tenant upsert duplication, write attribution asserted at the SQL level), the association path, the simulation-tenant correlation, the import path (the phase follows the request scope rather than TenantContext, an ambiguous scope is refused), a regression test pinning the real production SQL of the scenario projection, plus the arch and production-config guards required at go-live.
Romuald Lemesle (RomuDeuxfois)
force-pushed
the
feat/tenant-v2-kill-chain-phase
branch
from
August 21, 2026 06:42
7439151 to
28eaf91
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6402.
Activates
kill_chain_phaseson multi-tenancy v2. Go-live included: the v1@Filterremoval and theactive-tablesentry ship together.Why the diff is wide.
AttackPattern#killChainPhasesis a LAZY@ManyToManyre-derived byInject/Scenario/Exercise/InjectorContract. The scope is transaction-local andopen-in-viewrenders after the commit, wherecan_access_tenantis fail-closed — so a lazy load at rendering time serializes an empty phase list, with no error.KillChainPhaseInitializerhydrates it inside the scoped transaction, and every handler serializing those entities carriesTxCtx. Same blind spot as #7025.Also handled:
SUPPORTShandlers hold no transaction, soTxCtxis threaded into the service that opens one; the scenario-detail CTE query joining the table is pinned by a regression test on the real production SQL (#7007 class).Writes go through
TenantWriteScopeResolver. The upsert resolves the write tenant before any lookup and matches on the per-tenant unique keys — a bare business-key lookup could link another tenant's row.TenantBaseListeneris removed: every create path attributes the tenant explicitly (the two endpoints, the upsert service, the importer), and a test pins that an unattributed write is now refused by the non-nullabletenant_idinstead of silently landing onTenantContext's tenant.Child table.
attack_patterns_kill_chain_phasescarries notenant_idand rides along with its parents; it is not added to the allowlist. Because it holds no tenant, nothing in the join keeps a phase in the simulation's tenant —findDistinctByExerciseIdcorrelates the two explicitly, with a// TODO v2:marker for the eventualattack_patternsactivation and a test that fails without the predicate.Hot path (gate 0.4). The table is small (a few dozen rows per tenant) but it is not cold: it is joined by the scenario-detail projection and reached from every attack-pattern page. Read amplification is bounded by
@BatchSize(size = 1000)on the association, so a page of attack patterns hydrates in oneINquery instead of one per 50. No plan regression observed on the rewritten SQL; the CTE shape is pinned by test.Client impact: creates and upserts need a single-tenant scope. Tenant-path and single-tenant callers are unaffected; a multi-tenant caller with no selector gets 400.
Tests: full
openaev-apisuite green — 5692 tests, the 2 failures are order-dependent flakes that pass in isolation and touch neither this table nor a modified file. Plus 22 isolation tests (tenant A vs B, header route, non-admin, per-tenant upsert duplication, attribution asserted at SQL level, association path, simulation-tenant correlation), the arch and production-config guards,spotless:check.Rollback: revert the whole commit. Never drop only the property — with the
@Filtergone that is an isolation hole, which the config guard fails the build to prevent.