Skip to content

fix(auth): normalize IdP group names before matching scope mappings - #1713

Open
AmirF194 wants to merge 1 commit into
agentic-community:mainfrom
AmirF194:fix/1689-idp-group-name-normalize
Open

fix(auth): normalize IdP group names before matching scope mappings#1713
AmirF194 wants to merge 1 commit into
agentic-community:mainfrom
AmirF194:fix/1689-idp-group-name-normalize

Conversation

@AmirF194

Copy link
Copy Markdown
Contributor

Root cause

_normalize_server_name (auth_server/server.py) strips leading/trailing
slashes so a scope entry written as /cloudflare-docs still matches a
request for cloudflare-docs. Group names never got the same treatment, so
every comparison point matched verbatim:

  • group_filter.py::_filter_by_scope_mappings (Design C): [g for g in groups if g in mapped]
  • group_filter.py::filter_session_groups (Design B): [g for g in groups if g in allowed]
  • server.py::map_groups_to_scopes, which calls get_group_mappings_bulk(groups) (a MongoDB $in against unslashed stored group_mappings) and falls back to the same exact-match lookup against SCOPES_CONFIG

Keycloak's Group Membership mapper emits the full group path (/mcp-admins)
when "Full group path" is enabled, while scope mappings are seeded without
the leading slash. A user in that group then silently resolves to zero
scopes for it, both in the session (dropped by the login-time filter) and on
every subsequent authorization check.

Fix

Added a small _normalize_group_name helper (same logic as
_normalize_server_name: strip surrounding slashes) at each comparison
point:

  • filter_session_groups normalizes the incoming groups once, before either
    the Design B allowlist or the Design C scope-derived branch; the allowlist
    and the scope-mapped set are normalized too, so both sides agree.
  • map_groups_to_scopes normalizes its groups argument before calling
    get_group_mappings_bulk and before the in-memory fallback lookup, which
    covers all five call sites of this function.

Scope

This addresses the normalization gap only (the first half of #1689). The
issue also asks for the group filter to log dropped group names, not just
counts, plus a documentation note on the bearer-token/session asymmetry;
neither is touched here, so this is Refs #1689 rather than Fixes #1689.

Verification

  • New regression tests in both affected modules (leading-slash IdP claim
    matching an unslashed mapping, allowlist entry, and the bulk scope query),
    confirmed failing against unmodified auth_server/group_filter.py and
    auth_server/server.py and passing against this branch, in a clean
    python:3.14-slim container.
  • Full tests/auth_server/ suite: 820 passed, no regressions, same
    container.
  • ruff check, ruff format --check, and the repo's scoped mypy invocation
    (--ignore-missing-imports --no-strict-optional on registry|auth_server)
    all clean; bandit reports no new findings.
  • Did not run the registry-wide test suite (needs a MongoDB service
    container); this diff touches only auth_server/, and its own tests
    exercise every changed line.

Keycloak's Group Membership mapper can emit the full group path (e.g.
"/mcp-admins") instead of the bare name. Scope mappings are seeded
without the leading slash, so filter_session_groups, its Design C
helper _filter_by_scope_mappings, and map_groups_to_scopes all
compared the prefixed claim against the unslashed mapping and matched
nothing, silently dropping the group's scopes.

Normalize group names at the comparison points, mirroring
_normalize_server_name, which already handles the identical slash
mismatch for server names.

Refs agentic-community#1689
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants