feat(rbac): enforce session-token team narrowing in Layer 2 permission checks by bogdanmariusc10 · Pull Request #3919 · IBM/mcp-context-forge

bogdanmariusc10 · 2026-03-30T09:37:49Z

🔗 Related Issue

📝 Summary

Fixes a security gap in the two-layer RBAC model where session-token team narrowing (Layer 1 / visibility) was not enforced in permission checks (Layer 2 / RBAC).

Problem: When check_any_team=True, PermissionService.check_permission() aggregated permissions from ALL of a user's team-scoped roles, ignoring the token_teams narrowing applied at Layer 1. This allowed a session narrowed to team A to satisfy RBAC checks using roles the user holds in team B.

Example scenario:

User has developer role in team A (tools.read, tools.execute)
User has team_admin role in team B (teams.*, tools.create, etc.)
User creates session narrowed to team A via JWT teams: ["A"]
Before: Layer 1 scoped to team A, but Layer 2 granted teams.* from team B
After: Both layers correctly restricted to team A permissions only

Solution: Filter team-scoped roles in _get_user_roles() to only include roles from teams specified in token_teams when include_all_teams=True and token_teams is non-empty. Updated cache key generation to prevent cross-contamination between different narrowing scopes.

🏷️ Type of Change

🧪 Verification

Check	Command	Status
Lint suite	`make lint`	Passed ✅
Unit tests	`make test`	Passed ✅
Coverage ≥ 80%	`make coverage`	Passed ✅

✅ Checklist

Code formatted (make black isort pre-commit)
Tests added/updated for changes
Documentation updated (if applicable)
No secrets or credentials committed

📓 Notes

Changes Made
mcpgateway/services/permission_service.py:

get_user_permissions() (line 212):

Added token_teams: Optional[List[str]] = None parameter
Updated docstring to document Layer 2 enforcement behavior
Modified cache key generation (lines 241-244) to include token_teams for proper isolation
Pass token_teams to _get_user_roles() (line 253)

_get_user_roles() (line 461):

Added token_teams: Optional[List[str]] = None parameter
Updated docstring to explain narrowing enforcement
Added filtering logic (lines 505-512) to restrict team-scoped roles when include_all_teams=True and token_teams is non-empty

check_permission() (line 134):

Pass token_teams=token_teams to get_user_permissions()

tests/unit/mcpgateway/services/test_permission_service_token_narrowing.py (NEW):
Created comprehensive unit test suite with 16 tests covering:

1. Cache Key Generation (4 tests):

Cache key includes token_teams to prevent cross-contamination
Different token_teams produce different cache keys
Token teams are sorted in cache key for consistency
Un-narrowed sessions (token_teams=None) use base cache key

2. Role Filtering (4 tests):

_get_user_roles() filters by token_teams when narrowed
No filtering when token_teams is None (un-narrowed)
No filtering when token_teams is empty list (public-only)
Global team roles (scope_id=None) preserved when narrowed

3. End-to-End Permission Checks (8 tests):

Narrowed session restricts to token_teams only
Un-narrowed session uses all team roles
Public-only token has no team permissions
Admin bypass unaffected by narrowing
Public-only blocks admin.* even for admin users
Multiple teams in token_teams aggregates from all specified
Specific team_id overrides token_teams

Test Results: All 16 new tests pass ✅

… checks Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>

Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>

- Fix test_check_permission_public_only_token that passed due to wildcard string mismatch rather than actual role filtering; renamed to test_check_permission_public_only_token_retains_non_admin_team_perms with realistic permissions and accurate assertion (Layer 1 handles public-only visibility, not Layer 2 role filtering) - Remove unused imports (datetime, timezone) and unused variable (role_c) - Remove trailing "Made with Bob" comment - Apply black/isort formatting to match project standards - Clean up duplicate inline comments on or_() clause Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

…sion test Address two findings from code review: 1. Cache key deduplication: token_teams with duplicates (e.g. ["team-a", "team-a"]) now produces the same cache key as the deduplicated form (["team-a"]), preventing avoidable cache fragmentation. Added test_cache_key_deduplicates_token_teams. 2. Strengthen test_check_permission_narrowed_session_restricts_to_token_teams: replaced unrealistic "teams.*" wildcard permissions with explicit permissions matching bootstrap_db.py built-in roles (teams.create, teams.read, etc.). The previous test passed even when team-B roles leaked because "teams.create" != "teams.*" in exact string matching. The new test would correctly fail if role filtering broke. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

crivetimihai · 2026-03-30T12:16:24Z

Review changes

Rebased onto main (clean, no conflicts) and added three review-fix commits on top of the original three:

Commit: `fix(rbac): correct misleading test and clean up token narrowing tests`

Fixed test_check_permission_public_only_token_no_team_permissions — the original test mocked get_effective_permissions() to return ["teams.*"] then checked "teams.read". The assertion (False) held only because "teams.read" != "teams.*" in exact string match, not because token_teams=[] actually filters roles. Renamed to test_check_permission_public_only_token_retains_non_admin_team_perms with realistic permissions (["teams.read", "teams.create"]) and corrected the assertion to True, with a docstring explaining the two-layer design (Layer 1 handles public-only visibility; Layer 2 role filtering only triggers for non-empty token_teams).
Removed unused from datetime import datetime, timezone import (flake8 F401).
Removed unused role_c variable (ruff F841).
Removed trailing # Made with Bob comment.
Applied black/isort formatting to match project standards (200 char line length).
Cleaned up duplicate inline comments on the or_() clause in _get_user_roles().

Commit: `fix: add missing Returns docstring sections in _Unset sentinel (DAR201)`

Pre-existing flake8 DAR201 in team_management_service.py — added Returns: sections to _Unset.__repr__ and _Unset.__bool__.

Commit: `fix(rbac): deduplicate token_teams in cache key and strengthen regression test`

Cache key deduplication: sorted(set(token_teams)) — prevents ["team-a", "team-a"] from creating a different cache entry than ["team-a"]. Added test_cache_key_deduplicates_token_teams.
Strengthened regression test: test_check_permission_narrowed_session_restricts_to_token_teams now uses explicit permissions (teams.create, teams.read, teams.update, teams.delete, tools.create) matching bootstrap_db.py built-in roles. The previous ["teams.*"] wildcard meant the test passed even when team-B roles leaked into the result (since "teams.create" != "teams.*" in exact string match). Verified the blind spot by reproducing it before fixing.

Pre-existing gaps (not introduced by this PR)

Filed as tracking issue: #3924 — covers 6 residual token_teams scoping gaps in other code paths (explicit team_id bypass, admin bypass with token_teams=[], unscoped check_admin_permission, unscoped token scope containment, admin UI batch path, and token_teams=[] design decision).

Verification

16 tests in test_permission_service_token_narrowing.py — all pass
441 tests across all RBAC-related test suites — all pass
black, isort, ruff, flake8, bandit — all clean

crivetimihai

Reviewed and approved. Core logic is correct — token_teams narrowing is properly enforced in the check_any_team path, cache keys are isolated, and global team roles (scope_id=NULL) are correctly preserved. Added review-fix commits for cache key deduplication, strengthened regression test with realistic permissions, and cleaned up lint/formatting issues. Pre-existing scoping gaps in other code paths tracked in #3924.

lucarlig

Core narrowing fix looks good. The check_any_team path, cache isolation, and regression test cleanup all make sense. Remaining broader token_teams=[] semantics can stay tracked separately in #3924.

bogdanmariusc10 requested review from crivetimihai, kevalmahajan and madhav165 as code owners March 30, 2026 09:37

bogdanmariusc10 added enhancement New feature or request security Improves security labels Mar 30, 2026

bogdanmariusc10 linked an issue Mar 30, 2026 that may be closed by this pull request

[FEATURE]: Narrow RBAC Layer 2 permission checks to match session-token team scope #3799

Closed

bogdanmariusc10 added rbac Role-based Access Control MUST P1: Non-negotiable, critical requirements without which the product is non-functional or unsafe api REST API Related item labels Mar 30, 2026

crivetimihai self-assigned this Mar 30, 2026

Bogdan-Marius-Catanus and others added 6 commits March 30, 2026 11:50

fix(rbac): enforce session-token team narrowing in Layer 2 permission…

8121fe9

… checks Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>

Added unit tests for token narrowing

38533c1

Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>

Fixed pre-commit issues

d712918

Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>

fix: add missing Returns docstring sections in _Unset sentinel (DAR201)

1f8f8a1

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

crivetimihai mentioned this pull request Mar 30, 2026

[BUG][SECURITY]: token_teams narrowing not enforced consistently across Layer 2 permission paths #3924

Closed

2 tasks

crivetimihai force-pushed the 3799-feature-narrow-rbac-layer-2-permission-checks-to-match-session-token-team-scope branch from 809a8b8 to a27c51a Compare March 30, 2026 12:16

crivetimihai approved these changes Mar 30, 2026

View reviewed changes

crivetimihai requested review from brian-hussey and gcgoncalves March 30, 2026 13:19

lucarlig approved these changes Mar 30, 2026

View reviewed changes

crivetimihai merged commit 79bf23c into main Mar 30, 2026
28 checks passed

crivetimihai deleted the 3799-feature-narrow-rbac-layer-2-permission-checks-to-match-session-token-team-scope branch March 30, 2026 13:40

crivetimihai mentioned this pull request Mar 30, 2026

fix(security): enforce token_teams narrowing across all Layer 2 RBAC paths #3932

Merged

10 tasks

jonpspri mentioned this pull request Mar 31, 2026

feat(discovery): add automatic tool discovery with hot/cold classification #3839

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rbac): enforce session-token team narrowing in Layer 2 permission checks#3919

feat(rbac): enforce session-token team narrowing in Layer 2 permission checks#3919
crivetimihai merged 6 commits intomainfrom
3799-feature-narrow-rbac-layer-2-permission-checks-to-match-session-token-team-scope

bogdanmariusc10 commented Mar 30, 2026

Uh oh!

crivetimihai commented Mar 30, 2026

Uh oh!

crivetimihai left a comment

Uh oh!

lucarlig left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bogdanmariusc10 commented Mar 30, 2026

🔗 Related Issue

📝 Summary

🏷️ Type of Change

🧪 Verification

✅ Checklist

📓 Notes

Uh oh!

crivetimihai commented Mar 30, 2026

Review changes

Commit: fix(rbac): correct misleading test and clean up token narrowing tests

Commit: fix: add missing Returns docstring sections in _Unset sentinel (DAR201)

Commit: fix(rbac): deduplicate token_teams in cache key and strengthen regression test

Pre-existing gaps (not introduced by this PR)

Verification

Uh oh!

crivetimihai left a comment

Choose a reason for hiding this comment

Uh oh!

lucarlig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Commit: `fix(rbac): correct misleading test and clean up token narrowing tests`

Commit: `fix: add missing Returns docstring sections in _Unset sentinel (DAR201)`

Commit: `fix(rbac): deduplicate token_teams in cache key and strengthen regression test`