agentic-community / agentic-community/mcp-gateway-registry

IdP group names are never normalized, and filter_session_groups drops groups without naming them

Đang mở
#1,689 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Python
Star
911
Fork
234
Merge trung bình
1 ngày 11 giờ
Pull request đã merge (30 ngày)
62

Mô tả

## Summary

Two related gaps around IdP group handling. Neither is an authorization defect — access decisions are correct — but together they make a misconfigured group claim close to invisible, which cost us several days on an unrelated investigation (#1648, withdrawn as not-a-bug).

Both are the same shape as #1647: the system behaves as configured, produces no error, and gives the operator nothing to grep for.

## 1. Group names are never normalised, while server names are

Server names go through `_normalize_server_name` (`auth_server/server.py`), which strips leading and trailing slashes so a scope entry written as `/cloudflare-docs` matches a request for `cloudflare-docs`. The docstring is explicit that this exists because the UI and the URL extraction disagree on the leading slash.

Group names get no such treatment. They are compared verbatim in every place they are used:

- `get_group_mappings_bulk` — `find({"group_mappings": {"$in": unique}})`
- `get_all_mapped_group_names` — set union, exact membership
- `filter_session_groups` / `_filter_by_scope_mappings` — `[g for g in groups if g in mapped]`

This matters because **Keycloak's Group Membership protocol mapper emits full group paths by default** when *Full group path* is enabled: the claim carries `/mcp-admins`, not `mcp-admins`. A scope document seeded with `mcp-admins` then matches nothing.

The consequences are silent and compounding:

1. `get_group_mappings_bulk` returns no scope for that group, so the user resolves to fewer scopes than they should.
2. `filter_session_groups` drops the group from the session entirely, because it is not in the mapped-group union.

The user simply has less access than intended, with no error anywhere. The same applies to any IdP that emits a prefixed or differently-cased group identifier.

**Suggestion:** normalise group names at the comparison points, mirroring what `_normalize_server_name` already does for servers — at minimum strip surrounding slashes and whitespace. Doing it in one shared helper would keep the seeding path and the lookup path from drifting. If verbatim matching is deliberate, a note in `group_filter.py` and in the scope-seeding docs would at least make the trap findable.

## 2. `filter_session_groups` logs counts, never names

`auth_server/group_filter.py` intersects the user's IdP groups with the union of every scope's `group_mappings` at login, and persists only the filtered list in the session. The module docstring argues this is lossless for authorization, which is true — *provided* `group_mappings` is complete and in the matching form.

When it is not, the only trace is:

```
Group filter (scope-derived) user=: 5 -> 1
```

Counts, never names. A user can lose their most important group at login and nothing identifies it. There is no later log line either, because from that point on the session genuinely only has one group, and every downstream line faithfully reports one group.

This is what made #1648 hard: we could see that a user resolved to one scope, but nothing anywhere said which groups had been discarded on the way in, or why.

**Suggestion:** log the dropped group names when the filter removes anything. If group names are considered sensitive — the code already treats them as organisational PII in `validate_server_tool_access`, and Entra group names can encode org units — then DEBUG level, or the count at INFO plus the names at DEBUG, would still be a large improvement over counts alone.

Related: the filter runs only on the session-cookie path (`auth_server/server.py`, at the session write). A bearer-token request reads groups live from the token claim, unfiltered. That asymmetry may well be intentional, but it is not documented, and it means the same user can have different effective permissions depending on how a given request authenticated. A comment saying so would help anyone auditing "what could this user access at 09:43".

## Environment

Found on 1.29.0, Docker Compose, `AUTH_PROVIDER=keycloak` with Keycloak brokering Entra ID over SAML. Both observations are from reading `main` rather than from a reproduction, since our own incident turned out to have a different cause.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.