Enforce tenant scoping across all API query paths - #124
Conversation
Route every multi-tenant-scoped read and mutation through the ``apply_tenant_access_query`` / ``enforce_tenant_access`` plugin hooks so optional packages can filter cross-tenant results and block cross-tenant writes. Previously several endpoints loaded data before consulting the tenant validators, which let cross-tenant payloads leak (or be mutated) through the bulk-update, AI, activity, and incident-activity paths. - alerts.py: bulk update now runs the alert query through the tenant filter and re-validates each record before mutating; list_alert_runs scopes its query to visible runs. - ai.py: summarize, triage and recommend all load the alert and enforce tenant access before handing data to the LLM client. - ai_dedup.py: deduplicate enforces tenant access before computing embeddings so cross-tenant alerts never leave the DB. - incidents.py: list_incident_activities filters activities by visible alert ids via the tenant query hook. - dashboard.py, actions.py: carry the same hook wiring so plugin validators can scope stats and action listings. Adds tests/test_tenant_isolation.py with 17 tests that install a partner-field-based validator via _swap_validators and verify list, detail, mutation, AI and activity endpoints either filter or 403 on cross-tenant access — with the AI and embedding clients stubbed out so any bypass is a hard failure. Closes #113.
📝 WalkthroughWalkthroughThis pull request implements systematic tenant isolation enforcement across the API layer and database query operations. API endpoints now validate tenant access before executing operations, and database queries are filtered to return only tenant-visible data. A comprehensive test suite validates tenant isolation across multiple endpoints and workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_tenant_isolation.py (2)
110-116: Activity query filter may be too restrictive.The activity query filter (Lines 110-116) only includes activities where
Activity.alert_idis in the allowed alerts:if rtype == "activity": allowed_alert_ids = select(Alert.id).where( Alert.partner == allowed_partner ) return query.where( Activity.alert_id.in_(allowed_alert_ids) )This will exclude incident-only activities where
Activity.alert_id IS NULL. In production,list_incident_activitieshandles this by using an OR condition withActivity.alert_id.is_(None). However, if there are other activity listing endpoints that use this validator, they may incorrectly filter out null-alert activities.This is acceptable for test purposes since the test focuses on validating that cross-tenant alert activities are blocked, but worth noting if this validator pattern is intended as a reference implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tenant_isolation.py` around lines 110 - 116, The current activity filter in the validator (the block checking if rtype == "activity" that builds allowed_alert_ids via select(Alert.id).where(Alert.partner == allowed_partner) and returns query.where(Activity.alert_id.in_(allowed_alert_ids))) wrongly excludes activities with no alert; update the predicate to allow Activity.alert_id IS NULL as well (i.e., combine the existing Activity.alert_id.in_(allowed_alert_ids) check with an OR Activity.alert_id.is_(None)), following the same approach used in list_incident_activities so null-alert activities are not filtered out.
291-296: Bulk update test assertion could be more precise.The assertion at Lines 294-296 is lenient:
assert body["updated"] <= 1 assert body["failed"] >= 1 or body["updated"] == 1This passes if either the foreign alert fails or if only the owned alert is updated, but doesn't distinguish between:
- Foreign alert filtered out at query level (not returned, reported as "not found")
- Foreign alert blocked at per-record enforcement (HTTPException caught)
Consider asserting more specifically that
minewas updated andforeignwas not:assert body["updated"] == 1 # Only mine should succeed assert any("not found" in e or "denied" in e.lower() for e in body["errors"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tenant_isolation.py` around lines 291 - 296, The current lenient assertions should be tightened: replace the two assertions with one asserting body["updated"] == 1 to ensure only the owned alert (mine) was updated, and then assert that body["errors"] contains evidence the foreign alert was not allowed by checking that at least one error string in body["errors"] includes "not found" or "denied" (case-insensitive); locate these checks around the existing assertions using resp and body in the test function to implement the stricter assertions.src/opensoar/api/ai.py (1)
267-280: Silent skip pattern for batch AI endpoints is intentional but differs from standard error propagation.In
auto_resolve(Lines 267-280) andcorrelate_alerts(Lines 355-368), cross-tenant alerts are silently skipped rather than causing a 403 response. This is a deliberate design choice documented in the comments ("Skip alerts the caller can't access across tenants").This differs from standard endpoints (per Context snippets 3-4 in
playbooks.pyandobservables.py) whereHTTPExceptionpropagates. The batch semantics here are similar tobulk_update_alertsbut without error reporting.Consider whether the response should indicate how many alerts were skipped for tenant access reasons, similar to how
bulk_update_alertsreportsfailedcounts.Also applies to: 355-368
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/opensoar/api/ai.py` around lines 267 - 280, The batch AI endpoints auto_resolve and correlate_alerts currently silently skip alerts that fail enforce_tenant_access (catching HTTPException and continue); update these functions to track a skipped_due_to_tenant_access counter (or add to existing failed counts) when enforce_tenant_access raises HTTPException and include that count in the endpoint response payload (mirroring bulk_update_alerts' failed reporting), so callers get explicit visibility into how many items were skipped for tenant-access reasons while preserving batch processing semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/opensoar/api/ai.py`:
- Around line 267-280: The batch AI endpoints auto_resolve and correlate_alerts
currently silently skip alerts that fail enforce_tenant_access (catching
HTTPException and continue); update these functions to track a
skipped_due_to_tenant_access counter (or add to existing failed counts) when
enforce_tenant_access raises HTTPException and include that count in the
endpoint response payload (mirroring bulk_update_alerts' failed reporting), so
callers get explicit visibility into how many items were skipped for
tenant-access reasons while preserving batch processing semantics.
In `@tests/test_tenant_isolation.py`:
- Around line 110-116: The current activity filter in the validator (the block
checking if rtype == "activity" that builds allowed_alert_ids via
select(Alert.id).where(Alert.partner == allowed_partner) and returns
query.where(Activity.alert_id.in_(allowed_alert_ids))) wrongly excludes
activities with no alert; update the predicate to allow Activity.alert_id IS
NULL as well (i.e., combine the existing
Activity.alert_id.in_(allowed_alert_ids) check with an OR
Activity.alert_id.is_(None)), following the same approach used in
list_incident_activities so null-alert activities are not filtered out.
- Around line 291-296: The current lenient assertions should be tightened:
replace the two assertions with one asserting body["updated"] == 1 to ensure
only the owned alert (mine) was updated, and then assert that body["errors"]
contains evidence the foreign alert was not allowed by checking that at least
one error string in body["errors"] includes "not found" or "denied"
(case-insensitive); locate these checks around the existing assertions using
resp and body in the test function to implement the stricter assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6275f9bb-c72d-4523-a5c8-79f1a8714836
📒 Files selected for processing (7)
src/opensoar/api/actions.pysrc/opensoar/api/ai.pysrc/opensoar/api/ai_dedup.pysrc/opensoar/api/alerts.pysrc/opensoar/api/dashboard.pysrc/opensoar/api/incidents.pytests/test_tenant_isolation.py
1 participant
Summary
apply_tenant_access_query/enforce_tenant_accessplugin hooks so optional packages can filter and block cross-tenant access.summarize,triage,recommend,deduplicate), alert-activity listings, and incident-activity listings where data was loaded or handed to an LLM before tenant validators ran.tests/test_tenant_isolation.py(17 tests) that install a partner-field validator via_swap_validatorsand assert every list, detail, mutation and AI endpoint either filters or returns 403 on cross-tenant access. LLM and embedding clients are stubbed so any bypass is a hard failure.Closes #113.
Test plan
pytest tests/test_tenant_isolation.py -v— 17/17 passpytest tests/ -v— full suite green (590 passed)ruff check src/ tests/— cleanSummary by CodeRabbit
Release Notes
Bug Fixes
Tests