fix(rbac): enforce tenant/team membership on object routes (OH1/OH2/OH3/H1)#58
Open
PenguinzTech wants to merge 2 commits into
Open
fix(rbac): enforce tenant/team membership on object routes (OH1/OH2/OH3/H1)#58PenguinzTech wants to merge 2 commits into
PenguinzTech wants to merge 2 commits into
Conversation
…H3/H1) Closes IDOR: a global viewer holds global tenants:read/teams:read, so the scope gate passed for everyone and routes then loaded any object by id with no membership check — any user could read any org's tenants/teams + member emails across tenants. - Adds membership helpers in rbac.py; each object-scoped tenant/team route now requires the caller to be a global admin OR a member of that specific tenant/team (admin ops require tenant/team admin scope, resolved with the id so tenant/team-scoped roles are actually consulted). - create_team validates the body tenant_id against caller membership (no more cross-tenant writes). - Regression tests: non-member viewer 403, member 200, global admin 200, foreign-tenant create 403. Also gitignore /worktrees/ (repo-local worktree convention). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reviewer's GuideAdds tenant/team membership enforcement for object-scoped tenant and team routes, introduces RBAC helpers for membership/global-admin checks, wires them into teams and tenants handlers (including create_team), and adds regression tests to prevent cross-tenant/team IDOR reads and writes while preserving global admin behavior. Sequence diagram for create_team with tenant membership enforcementsequenceDiagram
actor User
participant TeamsAPI as create_team
participant RBAC as rbac_helpers
participant DB
User->>TeamsAPI: POST /teams (name, tenant_id)
TeamsAPI->>DB: get_db
TeamsAPI->>User: g.current_user["id"]
TeamsAPI->>RBAC: _has_global_admin_scope(DB, user_id, tenants:admin)
RBAC->>DB: get_user_scopes(user_id)
RBAC-->>TeamsAPI: is_global_admin
alt tenant_id provided
TeamsAPI->>RBAC: _is_tenant_member(DB, user_id, tenant_id)
RBAC->>DB: select from tenant_members
RBAC-->>TeamsAPI: is_tenant_member
alt not is_global_admin and not is_tenant_member
TeamsAPI-->>User: 403 Forbidden
else allowed
TeamsAPI->>DB: teams.insert(name, description, tenant_id, created_by)
DB-->>TeamsAPI: team_id
TeamsAPI-->>User: 201 Created
end
else no tenant_id
TeamsAPI->>DB: teams.insert(name, description, None, created_by)
DB-->>TeamsAPI: team_id
TeamsAPI-->>User: 201 Created
end
Sequence diagram for get_team/get_tenant membership checkssequenceDiagram
actor User
participant TeamsAPI as get_team
participant TenantsAPI as get_tenant
participant RBAC as rbac_helpers
participant DB
User->>TeamsAPI: GET /teams/:team_id
TeamsAPI->>DB: get_db
TeamsAPI->>User: g.current_user["id"]
TeamsAPI->>DB: select from teams where id == team_id
alt team not found
TeamsAPI-->>User: 404 NotFound
else team exists
TeamsAPI->>RBAC: _has_global_admin_scope(DB, user_id, teams:admin)
RBAC->>DB: get_user_scopes(user_id)
RBAC-->>TeamsAPI: is_admin
TeamsAPI->>RBAC: _is_team_member(DB, user_id, team_id)
RBAC->>DB: select from team_members
RBAC-->>TeamsAPI: is_member
alt not is_admin and not is_member
TeamsAPI-->>User: 403 Forbidden
else allowed
TeamsAPI->>DB: select team_members
DB-->>TeamsAPI: members
TeamsAPI-->>User: 200 OK (team + members)
end
end
User->>TenantsAPI: GET /tenants/:tenant_id
TenantsAPI->>DB: get_db
TenantsAPI->>User: g.current_user["id"]
TenantsAPI->>DB: select from tenants where id == tenant_id
alt tenant not found
TenantsAPI-->>User: 404 NotFound
else tenant exists
TenantsAPI->>RBAC: _has_global_admin_scope(DB, user_id, tenants:admin)
RBAC->>DB: get_user_scopes(user_id)
RBAC-->>TenantsAPI: is_admin
TenantsAPI->>RBAC: _is_tenant_member(DB, user_id, tenant_id)
RBAC->>DB: select from tenant_members
RBAC-->>TenantsAPI: is_member
alt not is_admin and not is_member
TenantsAPI-->>User: 403 Forbidden
else allowed
TenantsAPI->>DB: select tenant_members
DB-->>TenantsAPI: members
TenantsAPI-->>User: 200 OK (tenant + members)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
_has_global_admin_scopehelper takes adbparameter but doesn’t use it, and several handlers intenants.pyimport_has_global_admin_scopewithout ever calling it; consider either wiring it into the logic or removing the unused parameter/imports to keep the API and call-sites clean. - The membership checks for tenants/teams (login to get viewer ID, then POST to members) are duplicated across multiple tests; extracting a small helper or fixture to add a user as a tenant/team member would make the tests shorter and easier to maintain.
- The repeated inline membership checks in
teams.py/tenants.py(admin-or-member checks) could be refactored into a shared decorator or helper to avoid drifting behavior between endpoints and to make the access rules easier to audit in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_has_global_admin_scope` helper takes a `db` parameter but doesn’t use it, and several handlers in `tenants.py` import `_has_global_admin_scope` without ever calling it; consider either wiring it into the logic or removing the unused parameter/imports to keep the API and call-sites clean.
- The membership checks for tenants/teams (login to get viewer ID, then POST to members) are duplicated across multiple tests; extracting a small helper or fixture to add a user as a tenant/team member would make the tests shorter and easier to maintain.
- The repeated inline membership checks in `teams.py`/`tenants.py` (admin-or-member checks) could be refactored into a shared decorator or helper to avoid drifting behavior between endpoints and to make the access rules easier to audit in one place.
## Individual Comments
### Comment 1
<location path="services/flask-backend/app/rbac.py" line_range="710-719" />
<code_context>
+ return member is not None
+
+
+def _has_global_admin_scope(db, user_id: int, admin_scope: str) -> bool:
+ """
+ Check if a user has a global admin scope (e.g., 'tenants:admin' or 'system:admin').
+
+ Args:
+ db: Database connection
+ user_id: User ID
+ admin_scope: Admin scope to check (e.g., 'tenants:admin')
+
+ Returns:
+ True if user has the global admin scope
+ """
+ global_scopes = get_user_scopes(user_id)
+ return admin_scope in global_scopes or "system:admin" in global_scopes
+
</code_context>
<issue_to_address>
**suggestion:** The `db` parameter is unused in `_has_global_admin_scope`, which can be misleading.
Since the implementation ignores `db` and calls `get_user_scopes(user_id)` directly, it’s unclear which connection is intended. Either remove `db` from the signature or actually use it in the scope lookup to keep the API consistent and avoid ambiguity about the database context.
Suggested implementation:
```python
def _has_global_admin_scope(db, user_id: int, admin_scope: str) -> bool:
"""
Check if a user has a global admin scope (e.g., 'tenants:admin' or 'system:admin').
Args:
db: Database connection
user_id: User ID
admin_scope: Admin scope to check (e.g., 'tenants:admin')
Returns:
True if user has the global admin scope
"""
global_scopes = get_user_scopes(db, user_id)
return admin_scope in global_scopes or "system:admin" in global_scopes
```
If `get_user_scopes` currently has the signature `get_user_scopes(user_id: int)` without `db`, you should update it (and its call sites) to accept `db` as the first parameter, e.g. `get_user_scopes(db, user_id)`, so that all scope lookups consistently use an explicit database connection.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+710
to
+719
| def _has_global_admin_scope(db, user_id: int, admin_scope: str) -> bool: | ||
| """ | ||
| Check if a user has a global admin scope (e.g., 'tenants:admin' or 'system:admin'). | ||
|
|
||
| Args: | ||
| db: Database connection | ||
| user_id: User ID | ||
| admin_scope: Admin scope to check (e.g., 'tenants:admin') | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
suggestion: The db parameter is unused in _has_global_admin_scope, which can be misleading.
Since the implementation ignores db and calls get_user_scopes(user_id) directly, it’s unclear which connection is intended. Either remove db from the signature or actually use it in the scope lookup to keep the API consistent and avoid ambiguity about the database context.
Suggested implementation:
def _has_global_admin_scope(db, user_id: int, admin_scope: str) -> bool:
"""
Check if a user has a global admin scope (e.g., 'tenants:admin' or 'system:admin').
Args:
db: Database connection
user_id: User ID
admin_scope: Admin scope to check (e.g., 'tenants:admin')
Returns:
True if user has the global admin scope
"""
global_scopes = get_user_scopes(db, user_id)
return admin_scope in global_scopes or "system:admin" in global_scopesIf get_user_scopes currently has the signature get_user_scopes(user_id: int) without db, you should update it (and its call sites) to accept db as the first parameter, e.g. get_user_scopes(db, user_id), so that all scope lookups consistently use an explicit database connection.
…follow-up) The initial isolation fix guarded only read routes; write/admin routes still had just a global scope gate, so a user with global teams:write/tenants:write (e.g. a global maintainer) could modify/delete any team/tenant and grant roles across orgs — privilege escalation. (Also restores object-scoped authz that had regressed when team_id_param was dropped from the decorators.) - Adds _is_team_admin / _is_tenant_admin helpers (global admin scope OR team/tenant- scoped admin role for THAT object). - update/delete/add-member/remove-member on both teams.py and tenants.py now require the caller to be an admin of that specific team/tenant (or global admin). - add_team_member/add_tenant_member especially: they grant roles incl. *_admin, so non-admins of the object are now blocked. - Regression tests: global maintainer (no admin role) → 403 on every write route; team/tenant admin → 200/201. Full suite: 423 passed, 90.10% coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Security fixes (Track A3) — tenant/team isolation (IDOR)
Stacked on #57 (base =
fix/oidc-hardening); merge #56 → #57 → this. Ultimately targetsrelease/v0.1.x.Closes an IDOR where any self-registered global
viewer(holds globaltenants:read/teams:read) could read any org's tenants/teams and member emails across tenants, because the scope gate passed globally and routes then loaded objects by id with no membership check.rbac.py; every object-scoped tenant/team route now requires the caller to be a global admin OR a member of that specific tenant/team. Admin ops (update/delete/member changes) require tenant/team admin scope, resolved WITH the object id so tenant/team-scoped role assignments are actually consulted (also fixes the previously over-restrictive path).create_teamvalidates the bodytenant_idagainst caller membership → no cross-tenant writes.Tests
Regression tests added: non-member viewer → 403, member → 200, global admin → 200, foreign-tenant create → 403. Full suite: 413 passed, 90.58% coverage.
🤖 Generated with Claude Code
Summary by Sourcery
Enforce tenant and team membership, with global admin exceptions, on object-scoped tenant and team routes to close cross-tenant/team IDORs.
New Features:
Bug Fixes:
Tests: