Skip to content

Commit 7d08ba5

Browse files
fix(search): token-scope team results for admins in unified search (#5668)
* fix(search): token-scope team results for admins in unified search Complements the non-admin team-scope fix: the admin branch of admin_search_teams returns all teams (the admin management view) and ignored token scope, so an admin using a token explicitly narrowed to a team subset still saw every team through /v1/search, /admin/search, and /admin/teams/search. Honor explicit token_teams even for admins (Layer 1 constrains visibility independently of admin status): None means full admin bypass (unchanged), while an explicit list narrows the result. The caller's own personal team stays visible. Admin UI session tokens and unscoped admin API tokens resolve to None and are unaffected. Add real-data tests through /v1/search: an unscoped admin sees all teams (including one they do not belong to), while a token scoped to one team narrows to it. Verified empirically by disabling the filter and observing the scoped admin token still see every team. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * fix(search): apply admin team scope in the query before pagination The admin branch of admin_search_teams fetched list_teams(page=1, per_page=limit) and then filtered by token scope, so a scoped-admin token whose allowed team sorted past the first `limit` matches returned empty/incomplete results even though that team was visible. Add an optional team_ids filter to TeamManagementService.list_teams (applied to the query before pagination; an empty list matches nothing) and pass the normalized token scope from the admin branch. Scope is now enforced in the DB query rather than on an already-limited page. As a consequence an explicit scope (including [] = public-only) no longer surfaces the personal team, which aligns the admin branch with the token model; the non-admin branch carve-out is addressed separately. Verified empirically: an admin scoped to a team that sorts last among 12 teams with limit=8 now returns that team instead of an empty page. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * fix(search): drop personal-team carve-out; cover public-only and past-page scope Finding 1 (non-admin branch): remove the getattr(is_personal) carve-out so an explicit token scope (including [] = public-only) no longer surfaces the caller's personal team. An unscoped caller's memberships already include their personal team via _get_user_team_ids, so the common case is unchanged; this matches normalize_token_teams()/get_team_from_token(), which define [] as public-only with no personal fallback. (The admin branch was already aligned by pushing scope into the query.) Add the two regression cases the review flagged: - public-only admin token (token_teams=[]) sees no teams, not a bypass view. - an admin scoped to a team that sorts past the per-page limit still gets it (proves scope is applied in the query before pagination). Verified empirically: disabling the list_teams team_ids filter makes all three scope-dependent admin tests fail (narrowing leaks, public-only non-empty, past-page team dropped). Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * fix(search): rename admin-branch team-id var to avoid mypy type collision The admin branch declared scoped_team_ids: Optional[list[str]] while the non-admin branch reused the same name for a set[str], which strict mypy flagged as an incompatible reassignment (harmless at runtime since the branches are mutually exclusive). Rename the admin-branch variable to admin_scoped_team_ids. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> * test(search): add unit coverage for admin scope extraction and list_teams team_ids CI diff-cover flagged two changed lines as uncovered because they are only exercised by the integration suite (gated behind --with-integration): - admin.py: the admin-branch token-scope extraction - team_management_service.py: the list_teams team_ids where-clause Add unit tests that run in the standard pytest job: - admin_search_teams with a scoped admin token (mixed str/dict token_teams) forwards the normalized team_ids to list_teams. - list_teams(team_ids=[...]) restricts results in the query; None = no filter. Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com> --------- Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
1 parent 4ed6b01 commit 7d08ba5

5 files changed

Lines changed: 163 additions & 13 deletions

File tree

mcpgateway/admin.py

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5581,9 +5581,25 @@ async def admin_search_teams(
55815581
# The CALLER (admin.py) distinguishes.
55825582

55835583
if current_user.is_admin:
5584-
# Admin sees all non-personal teams plus their own personal team (single query)
5584+
# Honor explicit token narrowing even for admins (Layer 1 constrains
5585+
# visibility independently of RBAC/admin status). token_teams is None for
5586+
# full admin bypass (unrestricted); an explicit list (including []) scopes
5587+
# the result. The scope is pushed into the query so it applies before
5588+
# pagination (an allowed team must not be dropped for sorting past the
5589+
# first page) and so an explicit scope no longer surfaces the personal team.
5590+
raw_token_teams = user.get("token_teams")
5591+
admin_scoped_team_ids: Optional[list[str]] = None
5592+
if raw_token_teams is not None:
5593+
admin_scoped_team_ids = [team["id"] if isinstance(team, dict) else team for team in raw_token_teams]
55855594
result = await team_service.list_teams(
5586-
page=1, per_page=limit, include_inactive=include_inactive, visibility_filter=visibility, include_personal=False, search_query=search_query, personal_owner_email=user_email
5595+
page=1,
5596+
per_page=limit,
5597+
include_inactive=include_inactive,
5598+
visibility_filter=visibility,
5599+
include_personal=False,
5600+
search_query=search_query,
5601+
personal_owner_email=user_email,
5602+
team_ids=admin_scoped_team_ids,
55875603
)
55885604
# Result is dict {data, pagination...} (since page provided)
55895605
teams = result["data"]
@@ -5595,12 +5611,14 @@ async def admin_search_teams(
55955611
# returns every membership and ignores token scope, so a token narrowed to
55965612
# a team subset would otherwise leak sibling teams the caller belongs to but
55975613
# is scoped out of. _get_user_team_ids honors token_teams/_cached_team_ids;
5598-
# the caller's own personal team stays visible (owner is always visible).
5614+
# an unscoped caller's own memberships (including their personal team) are in
5615+
# this set, while an explicit scope (including [] = public-only) does not add
5616+
# a personal-team fallback, matching normalize_token_teams()/get_team_from_token().
55995617
scoped_team_ids = set(await _get_user_team_ids(user, db))
56005618
# Filter in memory
56015619
filtered = []
56025620
for t in all_teams:
5603-
if not getattr(t, "is_personal", False) and t.id not in scoped_team_ids:
5621+
if t.id not in scoped_team_ids:
56045622
continue
56055623
if not include_inactive and not t.is_active:
56065624
continue

mcpgateway/services/team_management_service.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1856,6 +1856,7 @@ async def list_teams(
18561856
include_personal: bool = False,
18571857
search_query: Optional[str] = None,
18581858
personal_owner_email: Optional[str] = None,
1859+
team_ids: Optional[List[str]] = None,
18591860
) -> Union[Tuple[List[EmailTeam], Optional[str]], Dict[str, Any]]:
18601861
"""List teams with pagination support (cursor or page based).
18611862
@@ -1871,6 +1872,7 @@ async def list_teams(
18711872
include_personal: Whether to include personal teams
18721873
search_query: Search term for name/slug/description
18731874
personal_owner_email: When set (and include_personal=False), includes this user's personal team alongside non-personal teams
1875+
team_ids: When set, restrict results to these team IDs before pagination (e.g. token-scoped callers). An empty list matches no teams.
18741876
18751877
Returns:
18761878
Union[Tuple[List[EmailTeam], Optional[str]], Dict[str, Any]]:
@@ -1887,6 +1889,11 @@ async def list_teams(
18871889
search_description=True,
18881890
)
18891891

1892+
# Restrict to specific team IDs before pagination so token-scoped callers
1893+
# are filtered in the query rather than on an already-limited page.
1894+
if team_ids is not None:
1895+
query = query.where(EmailTeam.id.in_(team_ids))
1896+
18901897
# Choose ordering based on pagination mode:
18911898
# - Page-based (UI): alphabetical by name for user-friendly display
18921899
# - Cursor-based (API): created_at DESC, id DESC to match unified_paginate expectations

tests/integration/test_search_endpoint.py

Lines changed: 81 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -287,15 +287,17 @@ def _real_data_env():
287287
now = datetime.now(timezone.utc)
288288
team_a = uuid.uuid4().hex
289289
team_b = uuid.uuid4().hex
290+
team_c = uuid.uuid4().hex # exists but user_b is NOT a member; only visible via the admin all-teams view
290291
# Unique emails per invocation: get_user_teams and role/permission lookups are
291292
# cached by email at process scope, so a fixed email would leak one test's
292293
# temp-DB team ids into another test using the same user.
293294
suffix = uuid.uuid4().hex
294295
user_b = f"user-b-{suffix}@example.com"
296+
admin_user = f"admin-{suffix}@example.com"
295297
owner = f"owner-{suffix}@example.com" # owns the tools so user_b access is never via ownership
296298

297-
def _user(email):
298-
return EmailUser(id=uuid.uuid4().hex, email=email, password_hash="x", full_name=email, is_admin=False, is_active=True, auth_provider="local", email_verified_at=now) # pragma: allowlist secret
299+
def _user(email, is_admin=False):
300+
return EmailUser(id=uuid.uuid4().hex, email=email, password_hash="x", full_name=email, is_admin=is_admin, is_active=True, auth_provider="local", email_verified_at=now) # pragma: allowlist secret
299301

300302
def _tool(name, visibility, team_id):
301303
return Tool(
@@ -316,24 +318,27 @@ def _tool(name, visibility, team_id):
316318
)
317319

318320
db = TestSessionLocal()
319-
db.add_all([_user(user_b), _user(owner)])
321+
db.add_all([_user(user_b), _user(owner), _user(admin_user, is_admin=True)])
320322
db.add_all(
321323
[
322324
EmailTeam(id=team_a, name=f"{SEARCH_TERM} Team A", slug="team-a", created_by=owner, is_personal=False, visibility="public"),
323325
EmailTeam(id=team_b, name=f"{SEARCH_TERM} Team B", slug="team-b", created_by=owner, is_personal=False, visibility="public"),
326+
EmailTeam(id=team_c, name=f"{SEARCH_TERM} Team C", slug="team-c", created_by=owner, is_personal=False, visibility="public"),
324327
]
325328
)
326329
db.commit()
327330

328-
# user_b is a real member of BOTH teams (so team-A is genuinely accessible),
329-
# and holds a real GLOBAL tools.read role (Layer 2 pass-through).
331+
# user_b is a real member of teams A and B (so team-A is genuinely accessible),
332+
# and holds a real GLOBAL tools.read+teams.read role (Layer 2 pass-through).
333+
# admin_user is a DB admin holding the same role (teams.read has no admin bypass).
330334
role_id = uuid.uuid4().hex
331335
db.add_all(
332336
[
333337
EmailTeamMember(id=uuid.uuid4().hex, team_id=team_a, user_email=user_b, role="member", is_active=True),
334338
EmailTeamMember(id=uuid.uuid4().hex, team_id=team_b, user_email=user_b, role="member", is_active=True),
335339
Role(id=role_id, name="test-reader", scope="global", permissions=["tools.read", "teams.read"], created_by=owner, is_active=True),
336340
UserRole(id=uuid.uuid4().hex, user_email=user_b, role_id=role_id, scope="global", scope_id=None, granted_by=owner, is_active=True),
341+
UserRole(id=uuid.uuid4().hex, user_email=admin_user, role_id=role_id, scope="global", scope_id=None, granted_by=owner, is_active=True),
337342
]
338343
)
339344

@@ -353,7 +358,9 @@ def _tool(name, visibility, team_id):
353358
"server_public": s_public.id,
354359
"team_a": team_a,
355360
"team_b": team_b,
361+
"team_c": team_c,
356362
"user_b": user_b,
363+
"admin_user": admin_user,
357364
}
358365
db.close()
359366

@@ -368,22 +375,24 @@ def _tool(name, visibility, team_id):
368375
os.unlink(path)
369376

370377

371-
def _inject_identity(app, TestSessionLocal, email, token_teams=None):
372-
"""Override the auth dependency to yield a non-admin context, optionally token-scoped.
378+
def _inject_identity(app, TestSessionLocal, email, token_teams=None, is_admin=False):
379+
"""Override the auth dependency to yield a caller context, optionally token-scoped.
373380
374381
Args:
375382
app: The FastAPI app to override on.
376383
TestSessionLocal: Session factory bound to the temp DB.
377384
email (str): Caller email.
378385
token_teams: When provided, narrows the caller's visible team scope
379386
(list of team-id strings); when ``None``, the key is omitted so the
380-
caller's full DB team membership applies.
387+
caller's full DB team membership applies (non-admin) or full admin
388+
bypass applies (admin).
389+
is_admin (bool): Whether the context is flagged admin.
381390
"""
382391

383392
async def _ctx():
384393
session = TestSessionLocal()
385394
try:
386-
context = {"email": email, "is_admin": False, "ip_address": "127.0.0.1", "user_agent": "test-client", "db": session}
395+
context = {"email": email, "is_admin": is_admin, "ip_address": "127.0.0.1", "user_agent": "test-client", "db": session}
387396
if token_teams is not None:
388397
context["token_teams"] = token_teams
389398
yield context
@@ -503,3 +512,66 @@ def test_public_only_token_sees_only_public_tools(self, _real_data_env):
503512
assert ids["public"] in returned # public tool visible
504513
assert ids["teama"] not in returned # team-private hidden under public-only scope
505514
assert ids["teamb"] not in returned # team-private hidden under public-only scope
515+
516+
def test_admin_unscoped_token_sees_all_teams(self, _real_data_env):
517+
"""An admin with no token narrowing sees every team, including one they don't belong to."""
518+
app, TestSessionLocal, ids = _real_data_env
519+
_inject_identity(app, TestSessionLocal, ids["admin_user"], token_teams=None, is_admin=True)
520+
521+
client = TestClient(app, raise_server_exceptions=False)
522+
resp = client.get(f"/v1/search?q={SEARCH_TERM}&entity_types=teams", headers={"Authorization": "Bearer x"})
523+
524+
assert resp.status_code == 200
525+
returned = {team["id"] for team in resp.json()["results"]["teams"]}
526+
# Admin all-teams view: team-C is returned even though admin is not a member.
527+
assert {ids["team_a"], ids["team_b"], ids["team_c"]} <= returned
528+
529+
def test_admin_scoped_token_narrows_teams(self, _real_data_env):
530+
"""An admin with a token scoped to team-B sees only team-B, not the other teams.
531+
532+
Explicit token_teams constrains Layer-1 visibility regardless of admin status;
533+
None (full bypass) is the only unrestricted case.
534+
"""
535+
app, TestSessionLocal, ids = _real_data_env
536+
_inject_identity(app, TestSessionLocal, ids["admin_user"], token_teams=[ids["team_b"]], is_admin=True)
537+
538+
client = TestClient(app, raise_server_exceptions=False)
539+
resp = client.get(f"/v1/search?q={SEARCH_TERM}&entity_types=teams", headers={"Authorization": "Bearer x"})
540+
541+
assert resp.status_code == 200
542+
returned = {team["id"] for team in resp.json()["results"]["teams"]}
543+
assert ids["team_b"] in returned # in-scope team visible (positive control)
544+
assert ids["team_a"] not in returned # narrowed out
545+
assert ids["team_c"] not in returned # narrowed out
546+
547+
def test_admin_public_only_token_sees_no_teams(self, _real_data_env):
548+
"""A public-only admin token (token_teams=[]) sees no teams, not a full-bypass all-teams view.
549+
550+
[] is public-only per the token model (normalize_token_teams), and there is no
551+
personal-team fallback, so an admin scoped to [] must see zero teams.
552+
"""
553+
app, TestSessionLocal, ids = _real_data_env
554+
_inject_identity(app, TestSessionLocal, ids["admin_user"], token_teams=[], is_admin=True)
555+
556+
client = TestClient(app, raise_server_exceptions=False)
557+
resp = client.get(f"/v1/search?q={SEARCH_TERM}&entity_types=teams", headers={"Authorization": "Bearer x"})
558+
559+
assert resp.status_code == 200
560+
assert resp.json()["results"]["teams"] == [] # empty scope -> no teams (not bypass)
561+
562+
def test_admin_scoped_team_returned_even_when_it_sorts_past_the_limit(self, _real_data_env):
563+
"""Scope is applied in the query before pagination, so a scoped team that sorts past the page limit is still returned.
564+
565+
Teams are ordered by name (Team A < B < C). With limit_per_type=1 the admin
566+
page holds only Team A; a filter-after-pagination approach would drop the
567+
scoped Team C entirely.
568+
"""
569+
app, TestSessionLocal, ids = _real_data_env
570+
_inject_identity(app, TestSessionLocal, ids["admin_user"], token_teams=[ids["team_c"]], is_admin=True)
571+
572+
client = TestClient(app, raise_server_exceptions=False)
573+
resp = client.get(f"/v1/search?q={SEARCH_TERM}&entity_types=teams&limit_per_type=1", headers={"Authorization": "Bearer x"})
574+
575+
assert resp.status_code == 200
576+
returned = {team["id"] for team in resp.json()["results"]["teams"]}
577+
assert ids["team_c"] in returned # returned despite sorting past the 1-item page

tests/unit/mcpgateway/services/test_team_management_service.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1063,6 +1063,34 @@ async def test_list_teams_personal_owner_email(self):
10631063
assert "shared-team" in names
10641064
assert "other-personal" not in names
10651065

1066+
@pytest.mark.asyncio
1067+
async def test_list_teams_team_ids_filters_before_pagination(self):
1068+
"""team_ids restricts results in the query; None applies no filter."""
1069+
from sqlalchemy import create_engine
1070+
from sqlalchemy.orm import Session as OrmSession
1071+
1072+
from mcpgateway.db import Base
1073+
1074+
engine = create_engine("sqlite:///:memory:", connect_args={"check_same_thread": False})
1075+
Base.metadata.create_all(engine)
1076+
with OrmSession(engine) as db:
1077+
db.add_all(
1078+
[
1079+
EmailTeam(id="id-a", name="Alpha", slug="alpha-x", created_by="o@example.com", is_personal=False),
1080+
EmailTeam(id="id-b", name="Beta", slug="beta-x", created_by="o@example.com", is_personal=False),
1081+
]
1082+
)
1083+
db.commit()
1084+
1085+
svc = TeamManagementService(db)
1086+
scoped, _ = await svc.list_teams(team_ids=["id-b"])
1087+
scoped_names = {t.name for t in scoped}
1088+
unfiltered, _ = await svc.list_teams(team_ids=None)
1089+
unfiltered_names = {t.name for t in unfiltered}
1090+
1091+
assert scoped_names == {"Beta"} # restricted to the given id
1092+
assert {"Alpha", "Beta"} <= unfiltered_names # None = no filter
1093+
10661094
@pytest.mark.asyncio
10671095
async def test_list_teams_with_search_query_page(self, service, mock_db):
10681096
"""Test list_teams applies search_query and page-based ordering."""

tests/unit/mcpgateway/test_admin.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19846,6 +19846,31 @@ async def test_admin_search_teams_admin(self, monkeypatch, allow_permission, moc
1984619846
assert len(result) == 1
1984719847
assert result[0]["name"] == "Alpha"
1984819848

19849+
@pytest.mark.asyncio
19850+
async def test_admin_search_teams_admin_scoped_token_forwards_team_ids(self, monkeypatch, allow_permission, mock_db):
19851+
"""A scoped admin token forwards normalized team_ids to list_teams (dict/str extraction)."""
19852+
mock_auth = MagicMock()
19853+
admin_user = SimpleNamespace(is_admin=True)
19854+
mock_auth.get_user_by_email = AsyncMock(return_value=admin_user)
19855+
monkeypatch.setattr("mcpgateway.admin.EmailAuthService", lambda db: mock_auth)
19856+
19857+
team = SimpleNamespace(id="tb", name="Beta", slug="beta", description="", visibility="public", is_active=True)
19858+
ts = MagicMock()
19859+
ts.list_teams = AsyncMock(return_value={"data": [team]})
19860+
monkeypatch.setattr("mcpgateway.admin.TeamManagementService", lambda db: ts)
19861+
19862+
# token_teams mixes a raw str id and a dict id to exercise both extraction branches
19863+
result = await admin_search_teams(
19864+
q="Beta",
19865+
include_inactive=False,
19866+
limit=10,
19867+
visibility=None,
19868+
db=mock_db,
19869+
user={"email": "admin@test.com", "token_teams": ["tb", {"id": "tc"}]},
19870+
)
19871+
assert result[0]["id"] == "tb"
19872+
assert ts.list_teams.await_args.kwargs["team_ids"] == ["tb", "tc"]
19873+
1984919874
@pytest.mark.asyncio
1985019875
async def test_admin_search_teams_non_admin_filters(self, monkeypatch, allow_permission, mock_db):
1985119876
"""Cover visibility and q filter continue branches in admin_search_teams (non-admin)."""

0 commit comments

Comments
 (0)