fix(rbac) admin user visibility issue for personal teams

[kevalmahajan]

🐛 Bug-fix PR

Closes #3376

📌 Summary

Fixed a security issue in the admin UI where platform administrators could see and manage personal teams belonging to other users. The issue violated the fundamental security principle that personal teams should only be visible to their owner, even for admins.

Why this matters:

• Personal teams are private workspaces that should never be exposed to other users
• The vulnerability allowed admins to view, edit, and potentially delete other users' personal teams
• This violated user privacy expectations and the platform's security model
• The fix maintains admin capabilities while respecting personal team privacy boundaries

Another minor fix: All other private teams are still listed for the admin user for governance (managing members, editing, deleting) but when the admin user tries to see the content for that private team which he is not a member of, in that case, error banner will be displayed and will be defaulted to All Teams.

🔁 Reproduction Steps

Original Bug:

1. Create two users: admin@example.com (platform admin) and user@example.com (regular user)
2. Each user automatically gets a personal team (e.g., personal-admin, personal-user)
3. Log in as admin@example.com to the admin UI at /admin
4. Navigate to any team management endpoint:
   • GET /admin/teams - List all teams
   • GET /admin/teams/{team_id} - View team details
   • PUT /admin/teams/{team_id} - Edit team
   • DELETE /admin/teams/{team_id} - Delete team

Expected behavior: Admin should only see:

• Their own personal team (personal-admin)
• All non-personal (shared) teams

Actual behavior (bug): Admin could see:

• Their own personal team (personal-admin)
• Other users' personal teams (personal-user, personal-alice, etc.)
• All non-personal teams

🐞 Root Cause

The issue existed in mcpgateway/admin.py across four endpoints that used TeamManagementService.list_teams():

# BEFORE (vulnerable code)
teams = team_service.list_teams(
    db=db,
    include_personal=True,  # Included ALL personal teams
    include_public=True
)

The problem:

• include_personal=True returned ALL personal teams in the database
• No filtering was applied to restrict personal teams to the current admin's own team
• The service layer correctly implemented the logic, but the admin layer misused it

Affected endpoints:

1. GET /admin/teams - Team listing page
2. GET /admin/teams/{team_id} - Team details page
3. PUT /admin/teams/{team_id} - Team edit endpoint
4. DELETE /admin/teams/{team_id} - Team deletion endpoint

💡 Fix Description

Access model updated:

• Exclude all personal teams from admin queries.
• Explicitly fetch and include only the current admin’s personal team.
• Return 404 for attempts to access other users’ personal teams.

Implementation changes:

• Updated all four admin endpoints to:
  • Call list_teams(include_personal=False)
  • Fetch admin’s personal team via new helper method
• Added TeamManagementService.get_user_personal_team() to retrieve owner-scoped personal team
• Added UI banner clarifying visibility rules

Security Guarantees:

After this change:
Admins cannot:

1. View other users’ personal teams
2. Edit other users’ personal teams
3. Delete other users’ personal teams

Admins can:

1. Manage all shared (non-personal) teams
2. Access their own personal team
3. This enforces strict personal team isolation while preserving administrative control over shared resources.

Tests:
11 new test cases (security + UX)
Validates:

• Personal team isolation
• 404 enforcement on unauthorized access
• Admin self-access
• Shared team visibility
• All tests passing

Design Decision:

• The fix is applied in the admin layer, not the service layer, to preserve service flexibility and avoid impacting other valid use cases of list_teams().

Scenario	Before (Vulnerable)	After (Secure)
Admin views team list	Sees ALL personal teams	Sees only their own personal team
Admin views other user's personal team	✅ Allowed (bug)	❌ Blocked (404 error)
Admin views their own personal team	✅ Allowed	✅ Allowed
Admin manages shared teams	✅ Allowed	✅ Allowed

🧪 Verification

Check	Command	Status
Lint suite	make lint	✅
Unit tests	make test	✅
Coverage ≥ 80 %	make coverage	✅
Manual regression no longer fails	steps / screenshots	✅

📐 MCP Compliance (if relevant)

• Matches current MCP spec
• No breaking change to MCP clients

✅ Checklist

• Code formatted (make black isort pre-commit)
• No secrets/credentials committed

[madhav165]

PR #3413 Review: Admin User Visibility Issue for Personal Teams

Overview

This PR fixes a security vulnerability where platform admins could see, edit, and delete personal teams belonging to
other users. The fix changes admin endpoints to call list_teams(include_personal=False) and then separately fetches
only the admin's own personal team via a new get_user_personal_team() service method. It also adds a UI banner when
admins view non-member teams.

Correctness

The core fix is sound - switching from include_personal=True to include_personal=False at the admin layer is the right
approach, and the decision to keep the service layer unchanged preserves flexibility.

Issues found:

• Duplicated filter logic across 4 endpoints (admin_get_all_team_ids, admin_search_teams, admin_teams_partial_html,
  admin_list_teams). Each has a near-identical block checking is_active, visibility, and search query match for the
  personal team. This is a maintenance risk - if the filtering logic changes, all 4 places need updating. Consider
  extracting a helper like _should_include_personal_team(team, include_inactive, visibility, search_query) -> bool.
• Inconsistent search matching logic between endpoints:
  • admin_get_all_team_ids (line ~65): checks name and slug only
  • admin_search_teams (line ~93): checks name, slug, and description
  • admin_teams_partial_html (line ~124): checks name, slug, and description
  • admin_list_teams (line ~148): checks name, slug, and description

The admin_get_all_team_ids endpoint is missing the description check, which is inconsistent.

• is_admin_user computed twice in admin_ui: once at line 15 of the diff, and again inline at lines 40-41. The second
  occurrence should use the is_admin_user variable already computed.

Security

• The fix correctly blocks access to other users' personal teams across all 4 admin team endpoints.
• The get_user_personal_team service method correctly filters by created_by, is_personal, and is_active.
• Minor concern: The PR description mentions returning 404 for attempts to access other users' personal teams via
  view/edit/delete endpoints, but I don't see those checks in the diff. The diff only covers listing endpoints. Are the
  view/edit/delete guards in a separate commit or missing?

Code Quality

• f-string in logger: team_management_service.py line 183 uses f"Failed to get personal team for user {user_email}:
  {e}" - should use lazy logging: logger.error("Failed to get personal team for user %s: %s", user_email, e) per project
  conventions.
• Template variable redundancy: The admin_viewing_non_member_team computation in the template context (lines 39-44)
  re-derives admin status instead of using is_admin_user.

Test Coverage

• Strong coverage: 4 service-level tests + 4 banner tests + 7 security regression tests + 12 filter edge case tests =
  comprehensive.
• Tests cover the happy path, edge cases (inactive, wrong visibility, search mismatch, limit reached), and both
  admin/non-admin paths.
• The existing tests were properly updated with get_user_personal_team = AsyncMock(return_value=None) to avoid
  breakage.
• Trailing whitespace in several test lines (visible in diff) - should be cleaned with make black.

Recommendations

1. Extract the personal team filter logic into a shared helper to eliminate the 4x duplication
2. Fix the inconsistent search matching in admin_get_all_team_ids (add description check)
3. Use the is_admin_user variable in the template context instead of re-computing it
4. Fix the f-string logger to use lazy %s formatting
5. Clarify: Are view/edit/delete endpoint guards included in this PR or in a follow-up?

Verdict

The security fix itself is correct and well-tested. The main concerns are code duplication and a minor inconsistency
in search logic. These should be addressed before merging to prevent divergence bugs in the future.

[kevalmahajan]

Hi @madhav165, thanks for the review.

Addressed all the requested changes:

1. Duplicated Filter Logic:
   Extracted TeamManagementService.should_include_personal_team() static method. All 4 endpoints now use this helper, eliminating maintenance risk.
2. Inconsistent Search Matching:
   Fixed. admin_get_all_team_ids now checks name, slug, AND description (consistent with other 3 endpoints).
3. is_admin_user Computed Twice:
   Fixed. Verified only computed once at line 3209, reused at line 3574 for admin_viewing_non_member_team.
4. F-string Logger:
   Fixed. Changed line 1362 in team_management_service.py to lazy logging format: logger.error("Failed to get personal team for user %s: %s", user_email, e).
5. Template Variable Redundancy:
   Fixed. admin_viewing_non_member_team now uses existing is_admin_user variable instead of re-computing.

Regarding the view/edit/delete endpoints, they already have the security code that checks and does not allow admin users to edit a user's personal team. In the PR, I have mentioned it for UI, since the admin user will not be able to list the teams in the first place, so no risk of other actions as well.

[madhav165]

Review Feedback

The fix correctly identifies the security issue and the admin-layer approach is reasonable, but the current implementation has a structural problem worth addressing before merge.

Core Issue: Two queries + manual append breaks pagination

All four admin endpoints (admin_get_all_team_ids, admin_search_teams, admin_teams_partial_html, admin_list_teams) follow this pattern:

result = await team_service.list_teams(include_personal=False, ...)
data = result["data"]
admin_personal_team = await team_service.get_user_personal_team(user_email)
if admin_personal_team and not any(t.id == admin_personal_team.id for t in data):
    if team_service.should_include_personal_team(...) and len(data) < per_page:
        data.append(admin_personal_team)

Problems:

1. Pagination metadata is wrong. total_items, total_pages, and has_next from the first query don't account for the appended personal team. The UI count will be off by one.
2. Personal team can silently disappear. If len(data) == per_page, the guard blocks it from being added — it won't appear on any page.
3. Code duplication. The same block is copy-pasted 4 times, and the admin_list_teams variant already has slightly different hardcoded args (True, None vs request params).
4. Two queries instead of one for every admin team listing request.

Suggested approach: single query in the service layer

In list_teams (and get_all_team_ids), add a personal_owner_email parameter:

async def list_teams(self, ..., include_personal=False, personal_owner_email=None, ...):
    ...
    if not include_personal:
        if personal_owner_email:
            query = query.where(
                or_(EmailTeam.is_personal.is_(False), EmailTeam.created_by == personal_owner_email)
            )
        else:
            query = query.where(EmailTeam.is_personal.is_(False))

Then admin.py call sites become:

result = await team_service.list_teams(include_personal=False, personal_owner_email=user_email, ...)

This fixes all four problems: pagination is correct, no silent disappearance, no duplication, one query. The new get_user_personal_team and should_include_personal_team methods can be removed. The admin_viewing_non_member_team banner logic and UI changes are fine as-is.

[kevalmahajan]

Hi @madhav165,

Thanks for the thorough review, all points are valid and have been addressed.

1. Pagination metadata off-by-one: Confirmed. The appended personal team was never reflected in total_items, total_pages, or has_next since pagination came from the first query only. Fixed.

2. Silent disappearance on full pages: Confirmed. The len(data) < per_page guard silently dropped the personal team when the page was full, and since it was excluded from the base query (include_personal=False), it would never appear on any page. Fixed.

3. Code duplication: Confirmed. Same ~8-line pattern was copy-pasted across all 4 endpoints. Eliminated.

4. Hardcoded filter values in admin_list_teams: Good catch, should_include_personal_team(admin_personal_team, True, None, q) was using hardcoded include_inactive=True and visibility=None instead of the endpoint's actual parameters. Fixed as a side effect of the refactor.

Fix applied: Added personal_owner_email: Optional[str] = None to both list_teams() and get_all_team_ids() in the service layer. When set alongside include_personal=False, the SQL query becomes:

WHERE (is_personal = False OR (is_personal = True AND created_by = :email))

All 4 admin endpoints now make a single call with personal_owner_email=user_email. Pagination is automatically correct, no manual append needed, no silent disappearance possible. All filters (active, visibility, search) apply uniformly via SQL.

[crivetimihai]

Thanks @kevalmahajan — good fix for admin visibility of personal teams. The PR is substantial (+1104 lines) — please make sure test coverage is thorough for all edge cases.

[madhav165]

Good fix overall. The core security change is correct — switching from include_personal=True to include_personal=False, personal_owner_email=user_email across all four admin endpoints cleanly isolates personal teams at the query level without breaking the service layer for other callers.

A few things were addressed during review:

Admin banner behavior fixed: The banner message said "Defaulting to All Teams" but selected_team_id was not actually being reset — the page would filter by the selected team and return empty content. Fixed by resetting selected_team_id = None when an admin selects a non-member team, so the content genuinely defaults to All Teams as stated.

Service layer tests replaced: test_list_teams_personal_owner_email and test_get_all_team_ids_personal_owner_email were checking compiled SQL strings for column name presence ("is_personal" in compiled, "created_by" in compiled), which is fragile and does not verify filtering behavior. Replaced with real in-memory SQLite tests that insert teams with different is_personal/created_by values and assert which ones are returned.

One note on the asymmetry between admin and non-admin paths: non-admins with an invalid team_id silently default to All Teams (same as before this PR), while admins now get the banner. This is intentional — admins may legitimately attempt to select a non-member team for governance, so the explanation is warranted.

[crivetimihai]

Reviewed and approved. Changes look correct and well-tested.

What was fixed during review:

• Rebased onto main, resolved 2 merge conflicts in admin.py
• Fixed broken banner test (needed team-scoped admin token, not unrestricted)
• Updated 3 pre-existing tests expecting 403 to match new silent-reset behavior
• Removed redundant _is_admin variable (reused is_admin_user)
• Added personal team guard to admin_get_team_edit() (returns 404 for non-owners)
• Fixed admin_update_team() to check update_team() return value (was showing false success for personal teams)
• Added 4 deny-path tests for direct personal team access
• Renamed misleading test name
• Fixed existing tests mocking update_team with None instead of True

Security: No SQL injection, XSS, or authorization bypass risks. Service layer correctly blocks personal team mutations. Direct-access endpoints now also guard against personal team disclosure.

All 1171 admin/team tests pass.