feat(executors): align api with v2 tenant isolation (#6409) - #6811
Draft
corinnekrych wants to merge 61 commits into
Draft
feat(executors): align api with v2 tenant isolation (#6409)#6811corinnekrych wants to merge 61 commits into
corinnekrych wants to merge 61 commits into
Conversation
#6399) Add TxCtx parameters to CollectorApi and InjectExpectationTraceApi handlers so their transactions carry a tenant scope once collectors is activated on v2 isolation. Fix TenantStatementInspector to pass through TableFunction/LateralSubSelect FROM items (needed by the collector indexing native query using jsonb_array_elements LATERAL joins). Register the new entrypoints in TenantScopedEntrypointsTxCtxArchTest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…coped transaction primitive (#6399) ExpectationsExpirationManagerJob now iterates tenants via TenantScopedTransaction.forEachTenant() instead of the v1 TenantContext loop, so its writes to the collectors table carry a v2 scope. ManagerIntegrationsSyncJob wraps monitorTenantIntegrations() in tenantTx.execute(TxCtx.forTenant(tenantId), ...) plus a TenantContext bridge for the v1 tables it still touches, fixing a PK violation caused by ManagerFactory.getManager() looking up an unscoped collectors row on cache miss. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#6399) CollectorHttpIsolationTest proves read/list/write attribution across tenants for admin users via the tenant path and X-Tenant-Ids header. CollectorNonAdminIsolationTest proves the same list isolation holds for a non-admin user granted ACCESS_TENANT_SETTINGS on two tenants, modeled on ImportMapperNonAdminIsolationTest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… tests (#6399) CollectorService.register() now looks up the existing row with the v2-scoped findByCollectorId() instead of findByIdAndTenantId(), letting the inspector own tenant scoping instead of an explicit tenant_id predicate; findByIdAndTenantId stays on the repository for test ground-truth assertions only. Fix CollectorApiTest to hit the tenant path with tenantUri() (which also grants tenant membership) instead of the header route, and drop the broken nested TenantIsolation class (it needed a per-class @TestPropertySource, impossible on a @nested class; isolation is now fully covered by CollectorHttpIsolationTest/CollectorNonAdminIsolationTest). Fix PayloadApiTest.processDeprecatedPayloads to use the tenant path with an explicit membership grant. Set Tenant.DEFAULT_TENANT_UUID explicitly in tests that build Collector entities directly, now that TenantIdBaseListener no longer backfills it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
One-commit go-live: remove the v1 @filter and TenantIdBaseListener from Collector, add collectors to openaev.tenant.active-tables, and extend the production-config guard so dropping collectors from the allowlist fails the build. The table is now fully scoped by TenantStatementInspector/can_access_tenant; write attribution goes through TenantWriteScopeResolver. Rollback: revert this whole commit together. Removing only the allowlist entry without restoring the @filter would leave the table with no tenant isolation at all. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…table skill (#6399) Make explicit that TenantBaseListener/TenantIdBaseListener must be removed from the entity at go-live, never kept as a fallback. Tests that create the entity directly must set tenantId explicitly instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… unreachable (#6399) isXtmComposerReachable() caught BadRequestException from throwIfXtmComposerNotReachable() and returned false, but both methods share the same class-level @transactional, so by the time the catch block ran the transaction was already marked rollback-only. The commit at the end of the request then threw UnexpectedRollbackException, surfacing as a 500 instead of the intended false response. This is pre-existing and unrelated to the collectors v2 activation: it only reproduces when XTM Composer is genuinely not configured/ reachable (true in some dev/local environments, not on the main remote environment where it is configured), which is why it hadn't been observed before. Marking the read as noRollbackFor = Exception.class keeps the transaction commitable when the expected not-reachable case is caught. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nantStatementInspector (#6399)
…Collector An unrelated parallel PR (security management revamp, e1b9cd7) merged from main added a brand-new deleteCollector endpoint on CollectorApi using the v1 pattern (collectorRepository.deleteByIdAndTenantId(id, TenantContext.getCurrentTenant())). This collided with the collectors table's v2 activation: the endpoint carried no TxCtx and relied on the legacy thread-local tenant context, which is gone once the table's @filter is removed. Fix: deleteCollector now takes a TxCtx ctx parameter and calls a new v2-scoped repository method, collectorRepository.deleteByCollectorId(id), a JPQL delete relying on the TenantStatementInspector for tenant scoping instead of an explicit tenant_id parameter. Register the endpoint in TenantScopedEntrypointsTxCtxArchTest so a future regression on this entrypoint is caught at build time. deleteByIdAndTenantId is retained on CollectorRepository: it is still legitimately used by ConnectorInstanceService with an explicit, caller-supplied tenantId (not TenantContext), which remains correct regardless of the table's isolation mechanism.
…practices Add a section to tenant-isolation.md classifying the three shapes of frozen background-transaction violations (A1 dead inner annotation, A2/A3 live self-invocation bug, B/C job transactional plumbing bug) with their actual runtime risk, using UserService.changePassword's self-invocation as a worked A1 example. List 5 concrete best practices for developers to avoid adding new entries to the baseline going forward (no self-calls to @transactional siblings, jobs always go through TenantScopedTransaction, new violations must be fixed not re-frozen, merges can smuggle in debt, shrinking is opportunistic via reduce-tx-baseline).
Switch the executors table from v1 Hibernate @filter isolation to v2 SQL rewriting (TenantStatementInspector + can_access_tenant GUC). Go-live (one commit): - Remove @filter("tenantFilter") and TenantIdBaseListener from Executor entity - Add executors to openaev.tenant.active-tables in application.properties Read path — TxCtx wired on all handlers: - ExecutorApi: all 6 handlers (executors, getExecutor, getExecutorRelatedIds, updateExecutor, deleteExecutor, registerExecutor) - EndpointApi#upsertEndpoint: reads executors via findById(OPENAEV_EXECUTOR_ID) Write path — explicit attribution via TenantWriteScopeResolver: - ExecutorApi#registerExecutor: writeScopeResolver.tenantForWrite(ctx, null) - EndpointApi#upsertEndpoint: same pattern for endpoint registration - ExecutorService.register(): tenantId threaded as parameter from callers v1 remnant removal: - Remove all TenantContext.getCurrentTenant() from ExecutorService - Remove findByIdAndTenantId/findByTypeAndTenantId from ExecutorRepository (replaced by inspector-scoped findById/findByType) - Remove TenantContext from all 5 integration innerStart() methods (replaced by Integration.getTenantId() reading from ConnectorInstancePersisted) - Remove TenantIdBaseListener (write attribution now explicit) Background paths: - ManagerIntegrationsSyncJob: already on TenantScopedTransaction primitive (forTenant scope), threads tenantId to ExecutorService.register() - InventoryMetricCollector: wrapped in tenantTx.execute(TxCtx.allTenants()) for cross-tenant metric collection Architecture guards: - TenantScopedEntrypointsTxCtxArchTest: 7 entries added - TenantActiveTableAccessArchTest: executors in GUARDED_TABLES with reviewed allowlist (ExecutorApi, ExecutorService, ConnectorInstanceService, InventoryMetricCollector, EndpointService) - ExecutorActivationConfigTest: production config guard Tests: - ExecutorHttpIsolationTest: 9 tests (read, cross-tenant 404, list path/header, create attributed, no-selector 400, update/delete cross-tenant, related-ids) - EndpointCrossTenantRegistrationTest: cross-tenant endpoint registration - Existing tests fixed for v2 (explicit tenantId, users_tenants membership) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Filigran-Automation
changed the title
Contributor
|
🤖 [AI-generated] Hey @corinnekrych! 👋 Thanks a lot for opening PR #6811 — really appreciate the contribution! 🙏 I just had a quick look and I think the description could be enhanced a little to help reviewers get through it faster. I haven't changed anything in your description — just a gentle suggestion:
No rush at all — thanks again for contributing to the project! 🚀 |
…pectationTraceApi (#6399) The collectors activation wired TxCtx on createInjectExpectationTraceForCollector, which resolved the frozen self-invocation violation (it now delegates properly). Refresh the baseline to reflect the solved entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tform/openaev into issue-6399/api-v2-collectors
…enant (#6399) The native query findExpectationsNotFilledAndExpired had no WHERE tenant_id clause. Since injects_expectations has no tenant_id column, the query now JOINs through the injects table (same pattern as findExpectationsNotFilled). Without this fix, the first tenant processed by forEachTenant would consume all expired expectations across every tenant, leaving subsequent tenants with zero rows to process. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tivation (#6399) ExpectationApi.updateInjectExpectation looks up the collector by ID via CollectorService.collector(). Since the collectors table is now on v2 active-tables, the transaction needs a TxCtx scope for the inspector to resolve the tenant. Without it, the query returns zero rows and throws ElementNotFoundException. Added TxCtx parameter to the three PUT endpoints that reach the collectors table (single update, single update by collector input, and bulk update). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ction for collectors v2 (#6399) InjectsExecutionJob processes injects across all tenants in parallel but never set a v2 tenant scope on its transactions. Once the collectors table was activated on v2 (TenantStatementInspector), the nested call chain executeInject → Executor.execute → InjectExpectationService.computeAndSaveExpectations → CollectorService.securityPlatformCollectors(tenantId) returned zero rows because can_access_tenant had no scope to resolve. Wrap each executeInject() call in tenantTx.execute(TxCtx.forTenant(tenantId)) so the inspector can resolve the tenant for all activated tables read during inject execution. Checked exceptions from executeInject are caught and re-thrown as RuntimeException (the primitive's Runnable overload does not support checked exceptions), then unwrapped in the outer catch block. Tested E2E: inject execution now correctly links security-platform collectors to DETECTION/PREVENTION expectations, and the collector polling + reporting flow works end-to-end under tenant isolation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dpoints (#6399) Add log.debug() to the three collector-facing endpoints: - PUT /expectations/{id}: logs source, score on update - GET /expectations/prevention/{sourceId}: logs pending count - GET /expectations/detection/{sourceId}: logs pending count Helps diagnose collector polling and reporting issues in dev/staging without impacting production (DEBUG level, off by default). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tform/openaev into issue-6399/api-v2-collectors
Contributor
Resolved in |
…r test to avoid polluted record in ci logs
corinnekrych
commented
Jul 27, 2026
| - "[0-9]*.[0-9]*.[0-9]*" | ||
| pull_request: | ||
| branches: [main] | ||
| branches: [main, issue-6399/api-v2-collectors] |
Contributor
Author
There was a problem hiding this comment.
Note to reviewers: I will remove that prior to merge.
…callback (#6399) TenantScopedTransaction.forEachTenant() now hands the loop's Consumer the tenant id (String) instead of the raw TxCtx scope. Every current caller only ever unwrapped the scope via a cast to TxCtx.Restricted to get the tenant id, so exposing the TxCtx object added a repeated, awkward cast for no benefit: inside the loop the scope is always TxCtx.forTenant(tenantId), trivially reconstructible from the id alone if a nested call ever needs the TxCtx object explicitly. Updates the only production caller (ExpectationsExpirationManagerJob) and the two integration tests exercising the same idiom. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…6409) executorService.remove(executorId) declares a checked ConnectorStatusException that deleteExecutor() neither caught nor declared, breaking compilation. ExecutorApi extends RestBehavior, which already has an @ExceptionHandler(UnprocessableContentException) mapping (ConnectorStatusException's parent), matching the pattern already used elsewhere in the API layer (e.g. ScenarioImportApi, ExerciseImportApi) — declare the checked exception on the controller method and let it map to the same HTTP response. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…remove() (#6409) ExecutorServiceTest.Remove stubbed executorRepository.findByIdAndTenantId(id, tenantId), a method that never existed on ExecutorRepository, breaking the build. Executor is fully on v2 tenant isolation (TenantStatementInspector + can_access_tenant, see its class javadoc): remove() correctly resolves the row through the unscoped findByExecutorId(id), the same pattern already used by CollectorRepository.findByCollectorId. Update the test's stubs to match, so it exercises the actual (and correct) production code path instead of a superseded pre-v2-activation design. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (7.28%) 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 #6811 +/- ##
============================================
+ Coverage 44.43% 44.58% +0.14%
- Complexity 9513 9543 +30
============================================
Files 2720 2720
Lines 82514 82585 +71
Branches 12548 12549 +1
============================================
+ Hits 36669 36822 +153
+ Misses 43139 43049 -90
- Partials 2706 2714 +8
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:
|
…athRunWiringTest (#6409) ExecutorFixture#getDefaultExecutor() hardcoded Tenant.DEFAULT_TENANT_UUID for its fallback-creation path. Executor is fully on v2 tenant isolation (no @Filter/listener auto-default), unlike Agent, which still relies on TenantBaseListener plus ambient TenantContext. AttackPathRunWiringTest creates its own tenant and persists an Agent under it via AgentFixture.createDefaultAgentSession(), but the Executor the agent references was still being created under the default tenant, tripping the agent_executor_id_fk constraint (agent and executor tenant_id mismatch). Add an explicit-tenant overload, getDefaultExecutor(String tenantId), and use it here instead of reaching for TenantContext.getCurrentTenant() ambiently, consistent with this PR direction of explicit tenant-id passing rather than ambient thread-local reliance. The no-arg overload is preserved unchanged for existing callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ite-scope resolution (#6409) Two related test-side gaps surfaced by the v2 executor/collector tenant isolation work, both pre-existing but only now exercised by CI: - AttackPathExecutionIngestionServiceTest and AttackPathIngestionTenantAttributionTest called ExecutorFixture#getDefaultExecutor() (hardcoded to the default tenant) right after switching TenantContext to a per-test tenant for the Agent. Agent and Executor ended up under different tenants, tripping the composite agent_executor_id_fk. Same root cause already fixed in AttackPathRunWiringTest; these two call sites now pass the explicit tenant id too. - AgentRuntimeAccessControlTest exercises EndpointApi#upsertEndpoint (POST /register), which now resolves its write tenant from a TxCtx parameter (TenantWriteScopeResolver), itself derived from the caller's users_tenants membership. WithMockUser never grants the mock user any tenant membership, so the scope resolved to TxCtx.Missing and every create was refused with 400, independent of isAdmin/capabilities. Link the mock user to the default tenant in a @beforeeach, matching the existing pattern already used in EndpointApiTest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
…6409) TenantBackgroundTransactionArchTest.no_transactional_self_invocation failed in CI with StoreUpdateFailedException. The frozen store recorded ExecutorService.register's self-invocation at ExecutorService.java:159, but this PR's own v2 change (dropping the ambient TenantContext lookup, adding an explicit tenantId parameter and expanding the delegating call across multiple lines) shifted that call site down to line 158. Since CI runs the freeze store locked (no create/update), an exact line-number mismatch on an otherwise identical, already-accepted violation surfaces as a hard failure instead of a silent match. This is not a new violation and not a fix of the underlying self-invocation (out of scope here): the method still calls its own @transactional overload directly. Correct the stored line reference to match the current source so the frozen baseline is accurate again. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Resolved by merging |
…form/openaev into issue-6409/api-v2-executors
) Constrain the raw installationMode value from agent register/upgrade requests to the known allow-list (AgentUtils.getSupportedInstallationMode) before it reaches file/resource-path resolution in EndpointService.loadAgentScriptTemplate and ExecutorApi's agent package download, addressing a CodeQL java/path-injection finding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…form/openaev into issue-6409/api-v2-executors
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.
Proposed changes
Testing Instructions
Related issues
Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...