[backend] fix(multi-tenant): scenario tenant isolation (#4864)#5619
[backend] fix(multi-tenant): scenario tenant isolation (#4864)#5619corinnekrych wants to merge 8 commits intorelease/currentfrom
Conversation
github-actions
Bot
added
the
filigran team
corinnekrych
changed the title
| + "LEFT JOIN tags tg ON tg.scenario_id = s.scenario_id " | ||
| + "LEFT JOIN workflows w ON w.workflow_scenario_id = s.scenario_id " | ||
| + "WHERE s.scenario_id = :scenarioId", | ||
| + "WHERE s.scenario_id = :scenarioId AND s.tenant_id = :#{#tenantContext.currentTenant}", |
There was a problem hiding this comment.
nitpick: Since we are on a specific scenario linked to a tenant, this is not required. But we can do "centure et bretelles"
| actionPerformed = Action.READ, | ||
| resourceType = ResourceType.SCENARIO) | ||
| public List<TeamOutput> scenarioTeams(@PathVariable @NotBlank final String scenarioId) { | ||
| this.scenarioService.scenario(scenarioId); // Tenant-scoped validation |
There was a problem hiding this comment.
question: Why we need it ?
There was a problem hiding this comment.
To test it:
- either comment out the line and see integration test failing.
- or you can also test it via CURL:
# 1. Login
curl -s -c cookies.txt -X POST "http://localhost:3001/api/login" \
-H "Content-Type: application/json" \
-d '{
"login": "admin@openaev.io",
"password": "admin"
}'
# 2a. Create Tenant XXX
curl -s -b cookies.txt -X POST "http://localhost:3001/api/tenants" \
-H "Content-Type: application/json" \
-d '{
"tenant_name": "Tenant XXX",
"tenant_description": "First test tenant"
}'
==> "tenant_id":"270f0438-7713-4112-a6a8-7ffb1641c520" for tenant XXX
# 2b. Create Tenant YYY
curl -s -b cookies.txt -X POST "http://localhost:3001/api/tenants" \
-H "Content-Type: application/json" \
-d '{
"tenant_name": "Tenant YYY",
"tenant_description": "Second test tenant"
}'
==> "tenant_id":"ef689b73-7417-4d38-877c-739c2d716e5e" for tenant YYY
# 3. Create a scenario in tenant XXX
curl -s -b cookies.txt -X POST "http://localhost:3001/api/tenants/270f0438-7713-4112-a6a8-7ffb1641c520/scenarios" \
-H "Content-Type: application/json" \
-d '{
"scenario_name": "Test Scenario"
}'
==> "scenario_id":"5f91427b-67c5-40b7-9e91-e3dc767cb05e for tenant XXX (270f0438-7713-4112-a6a8-7ffb1641c520)
# 4. Read the scenario from tenant YYY (cross-tenant test — expect 403/404)
curl -s -b cookies.txt -X GET "http://localhost:3001/api/tenants/ef689b73-7417-4d38-877c-739c2d716e5e/scenarios/5f91427b-67c5-40b7-9e91-e3dc767cb05e"
==> 200, return the scenario detail
ACTUAL: We can retrieve scenarios that does not belong to our current tenant
EXPECTED: Reading scenario from tenant YYY should fail with 403/404, confirming tenant isolation is working correctly.
Why we need it:
So the Hibernate tenantFilter is automatically enabled for any method annotated with @transactional. This means:
For example Team has @filter(name = "tenantFilter") ✅ — all Hibernate queries (findAll, findById, Specification-based queries) will automatically filter by tenant_id when the filter is active.
The filter is activated by HibernateFilterTransactionAspect on any @transactional method ✅
However, there's a catch: the filter only applies to Hibernate Session queries (JPQL, Criteria, findAll, etc.), not to native SQL queries (like @query(nativeQuery = true) — which is why getScenarioById needed a manual WHERE clause fix). Standard JPA repository methods like findById() also bypass the filter unless called within a Hibernate Session with the filter enable
There was a problem hiding this comment.
The way I find missing coverage is by:
- write integration tests for CRUD for each entity (with correct RBAC but we could use isAdmin is we want to bypass capa)
- see failing tests, fix them
I've started with scenario entity and then look at embedded entities: teams, variables, channels etc...
| @Operation(description = "Get a team", summary = "Get team") | ||
| public Team getTeam(@PathVariable @Schema(description = "ID of the team") String teamId) { | ||
| return teamRepository.findById(teamId).orElseThrow(ElementNotFoundException::new); | ||
| return teamRepository.findByIdAndTenant(teamId).orElseThrow(ElementNotFoundException::new); |
There was a problem hiding this comment.
question: team has tenantFilter, so can you explain why we need this ?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/current #5619 +/- ##
=====================================================
+ Coverage 63.76% 63.80% +0.04%
- Complexity 5825 5828 +3
=====================================================
Files 1145 1145
Lines 34375 34386 +11
Branches 2652 2650 -2
=====================================================
+ Hits 21920 21941 +21
+ Misses 11161 11154 -7
+ Partials 1294 1291 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
This pull request introduces comprehensive tenant isolation for team and scenario operations throughout the API, ensuring that all relevant endpoints enforce tenant scoping. This is achieved by updating repository calls to use tenant-aware queries, adding explicit tenant-scoped validation in services and controllers, and expanding integration tests to verify correct isolation and error handling.
Tenant Isolation Enforcement:
TeamApiensuring that teams can only be accessed, updated, or deleted within the current tenant context.ScenarioApiand dependent API.<!--
Thank you very much for your pull request to the OpenAEV project! We as a community
driven project depend on support and contributions like this!
Thus already a BIG THANK YOU upfront to you for choosing to help with your PR.
-->
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...