fix(security): enforce team membership validation in admin UI

[madhav165]

🐛 Bug-fix PR

Fixes https://github.ibm.com/contextforge-org/mcp-context-forge/issues/44

---

📌 Summary

Fixes a team scoping validation issue in the admin UI where an invalid team_id query parameter could result in broader visibility than intended. The endpoint now correctly enforces team membership checks and rejects unauthorized team contexts.

🐞 Root Cause

The admin_ui endpoint in mcpgateway/admin.py had a permissive fallback path during team validation that silently relaxed scoping constraints instead of enforcing them.

💡 Fix Description

• Replaced the permissive fallback with strict 403 rejection when team membership validation fails.
• The fix is scoped to the two validation branches inside the admin_ui function; the default no-team-id path remains unchanged.
• Added three unit tests covering the invalid, unavailable, and null team_id cases.

🧪 Verification

Check	Command	Status
Lint suite	make lint	pass
Unit tests	make test	pass
Coverage ≥ 80 %	make coverage	pass

📐 MCP Compliance (if relevant)

• No breaking change to MCP clients
• Admin UI only; no protocol-level changes

✅ Checklist

• Code formatted (make black isort pre-commit)
• No secrets/credentials committed
• Deny-path regression tests included

[crivetimihai]

Thanks @madhav165 — important security fix. Replacing the permissive fallback with strict 403 rejection is the correct approach. Three unit tests covering the edge cases is good. LGTM.

[MohanLaksh]

PR Review: Team Scoping Validation Fix (PR #3399)

Executive Summary

Verdict: ✅ APPROVED

Architecture Compliance

Two-Layer Security Model:

Layer 1 (Token Scoping) → Layer 2 (RBAC) → Operation
     ↓                          ↓
normalize_token_teams()    require_permission()
     ↓                          ↓
  [PASS]                     [PASS]  → admin_ui() validates team_id

This fix operates at the application layer (after both security layers):

• Token scoping already filtered what user CAN SEE
• RBAC already checked if user CAN ACCESS admin dashboard
• This fix ensures user can't REQUEST a team context they don't belong to

Correct placement in security stack:

• ✅ After authentication (get_current_user_with_permissions)
• ✅ After RBAC check (@require_permission("admin.dashboard"))
• ✅ Before data fetching (lines 3227+)

---

🚨 Potential Issues & Mitigations

Issue 1: Breaking Change for Existing Clients

Risk: Low
Impact: Clients relying on fallback behavior will now get 403
Mitigation: This is the intended behavior - the fallback was a bug. Clients should not be requesting team contexts they don't belong to.

Issue 2: User Experience

Risk: Low
Impact: Users might see 403 instead of empty results
Mitigation: Error messages are clear ("Not a member of the requested team"). This is better UX than silently showing different data.

Issue 3: Service Availability

Risk: Low
Impact: If team service fails, users can't access admin UI with team filter
Mitigation: Users can still access without team_id parameter. This is correct fail-closed behavior.

---

🎓 Recommendations

For Merge

✅ APPROVE - This PR should be merged as-is.

Rationale:

1. Fixes the issue
2. Implementation is correct and minimal
3. Test coverage is comprehensive
4. Aligns with project security principles
5. No breaking changes to legitimate use cases

Test Running Results:

tests/unit/mcpgateway/test_admin_module.py::test_admin_ui_rejects_invalid_team_id PASSED
tests/unit/mcpgateway/test_admin_module.py::test_admin_ui_rejects_team_id_when_teams_unavailable PASSED
tests/unit/mcpgateway/test_admin_module.py::test_admin_ui_no_team_id_returns_public_items PASSED

---

🏆 Final Verdict

APPROVED ✅

Recommendation: Merge immediately

[crivetimihai]

Review Summary

Rebased onto main (clean), reviewed, and applied fixes.

Original PR (correct)

• Replaces the permissive fallback (silently dropping team_id to public visibility) with strict 403 rejection when a non-admin user supplies a team_id they don't belong to.
• Adds deny-path regression tests for invalid team_id, unavailable teams, and null team_id.

Review fixes applied

1. Admin bypass with token_teams check
The original PR blocked platform admins from viewing other teams' resources. Added an admin bypass consistent with the codebase convention: requires both is_admin=True AND token_teams is None (unrestricted token). Team-scoped admin tokens (token_teams=["team-1"]) are still subject to membership validation.

2. Test name correction
Renamed test_admin_ui_team_loading_failure_ignores_team_id → test_admin_ui_team_loading_failure_rejects_team_id to match the new 403 behavior.

3. Additional tests

• test_admin_ui_admin_bypasses_team_membership_check — unrestricted admin can select any team
• test_admin_ui_team_scoped_admin_rejected_for_other_team — scoped admin gets 403 for non-member team

4. Docstring Raises section
Added HTTPException to the admin_ui docstring to satisfy darglint (DAR401).

Known pre-existing limitation (not in scope)

admin_ui does not pass token_teams through to service methods, so downstream _apply_access_control falls back to DB-derived team membership. This means even unrestricted admins see empty results for teams they're not members of. This predates this PR and should be addressed separately.

Verification

• All tests pass independently (test_admin_module.py, test_admin.py)
• flake8 DAR401 clean
• 100% differential coverage on new/changed lines

[crivetimihai]

Approved. Security fix is correct — 403 rejection for unauthorized team_id with proper admin bypass (is_admin + token_teams=None). All branches tested.