Configurable groups claim key for SSO providers#9367
Conversation
There was a problem hiding this comment.
1 issue found across 11 files
Confidence score: 5/5
- This PR looks low risk to merge: the only flagged item is low severity (3/10) and is primarily a maintainability concern rather than a functional bug.
- In
backend/infrahub/config.py, the newgroups_claimlogic is duplicated across two base classes, which could cause future drift if one copy is updated without the other. - Pay close attention to
backend/infrahub/config.py- keep the duplicatedgroups_claimbehavior aligned or extract a shared helper/mixin to prevent divergence.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/config.py">
<violation number="1" location="backend/infrahub/config.py:604">
P3: New groups_claim logic duplicated in two base classes. Easy drift later. Extract one shared mixin/helper and reuse it.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| pkce_enabled: bool = Field( | ||
| default=True, description="Enable PKCE (RFC 7636) with S256 method for authorization code flow" | ||
| ) | ||
| groups_claim: str = Field( |
There was a problem hiding this comment.
P3: New groups_claim logic duplicated in two base classes. Easy drift later. Extract one shared mixin/helper and reuse it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/config.py, line 604:
<comment>New groups_claim logic duplicated in two base classes. Easy drift later. Extract one shared mixin/helper and reuse it.</comment>
<file context>
@@ -601,6 +601,21 @@ class SecurityOIDCBaseSettings(BaseSettings):
pkce_enabled: bool = Field(
default=True, description="Enable PKCE (RFC 7636) with S256 method for authorization code flow"
)
+ groups_claim: str = Field(
+ default="groups",
+ description=(
</file context>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
e28bef5 to
355e88f
Compare
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
| userinfo_body = { | ||
| "sub": "u1", | ||
| "name": "Otto", | ||
| "email": "o@x.com", |
There was a problem hiding this comment.
Since we are using fake data, we should use a RFC 2606/BCP 32 reserved domain rather than domain owned by an actual company.
| *, | ||
| claim_key: str = "groups", | ||
| provider_name: str = "", |
There was a problem hiding this comment.
Every caller passes claim_key and provider_name explicitly. The defaults ("groups" / "") only exist for tests. Suggest dropping the defaults and making both required.
| provider=provider_name, | ||
| source=source, | ||
| configured_claim=claim_key, | ||
| available_keys=sorted(payload.keys()), |
There was a problem hiding this comment.
The miss log dumps all top-level claim keys at WARN. Key names aren't usually PII, but URI-namespaced claims can embed tenant/customer identifiers (e.g. https://acme-corp.example/claims/...). Is WARN the right level for this, or would DEBUG be more appropriate?
Why
Infrahub maps SSO users into Infrahub groups by reading a hardcoded top-level
groupskey from the identity provider's claim payload. Identity providers that emit group memberships under a different claim name (e.g.roles,memberships) cannot drive group mapping, forcing operators to reshape claims or give up on SSO-driven group assignment.Goal: Let operators configure, per provider, which claim key Infrahub reads group memberships from.
Non-goals: No path/JSONPath expressions, namespace expansion, or case-folding — the key is matched literally. No changes to how groups map to Infrahub permissions once extracted.
Closes
What changed
Behavioral changes:
groups_claimsetting (default"groups") on both OAuth2 and OIDC providers. Set it via TOML (groups_claim = "roles") or env var (INFRAHUB_OIDC_PROVIDER1_GROUPS_CLAIM,INFRAHUB_OAUTH2_PROVIDER1_GROUPS_CLAIM).id_token.groups_claimis rejected at startup (config validation error).WARNlog identifying the provider, source, configured claim, and miss reason.Implementation notes:
extract_sso_groups()helper inbackend/infrahub/auth.py, replacing the three previously-divergent inline.get("groups")lookups so behavior can't drift between sites.groups_claimlives on the shared base settings classes (SecurityOAuth2BaseSettings,SecurityOIDCBaseSettings) so OAuth2 and OIDC share one definition.What stayed the same:
groups.How to review
backend/infrahub/auth.py— the newextract_sso_groups()helper (validation + structured logging) is the core of the change.backend/infrahub/config.py— thegroups_claimfield and its validator on the two base settings classes.backend/infrahub/api/oidc.py/backend/infrahub/api/oauth2.py— call sites wired to the helper; note_get_id_token_groupsnow takesclaim_key/provider_name.docs/archive/guides/sso.mdxare straightforward to skim.Area wanting extra scrutiny: the strict list-of-strings enforcement is intentionally stricter than the old code (which returned whatever
.get()produced). Confirm this matches expectations for IdPs that may emit groups in other shapes.How to test
Expected: all pass. The new tests cover the default
groupsbehavior, a custom claim key, each miss reason (absent / not-a-list / non-string member), and rejection of emptygroups_claimat config load.Impact & rollout
groups. The only behavioral tightening is strict list-of-strings extraction; an IdP previously returning a non-list undergroupswould now yield no groups (and log a WARN) instead of passing the raw value through.groups_claimper provider (INFRAHUB_OIDC_PROVIDER<N>_GROUPS_CLAIM,INFRAHUB_OAUTH2_PROVIDER<N>_GROUPS_CLAIM).Checklist
uv run towncrier create ...)🤖 Generated with Claude Code
Summary by cubic
Adds a per-provider
groups_claimsetting for OAuth2 and OIDC so IdPs using keys likerolesormembershipscan drive group mapping without rewrites. Centralizes group extraction with strict validation and structured WARN logs across userinfo andid_token.New Features
groups_claimon each provider (defaultgroups). Configure via TOML or env varsINFRAHUB_OIDC_PROVIDER<N)_GROUPS_CLAIMandINFRAHUB_OAUTH2_PROVIDER<N>_GROUPS_CLAIM.extract_sso_groups()for OAuth2 userinfo, OIDC userinfo, and OIDCid_token; enforces list-of-strings.groups_claimrequired at startup; on miss/wrong type, returns [] and logs provider, source, claim, keys, and reason. Tests and docs updated.Migration
groups.groups_claim(e.g.,roles) and ensure the claim is a list of strings.Written for commit 935b6ac. Summary will update on new commits.
Review in cubic