feat(realm): tenant translation contract + single-tenant guard (Phase 3, #2196) - #2239
Conversation
…ructorfabric#2196) - Canonical insight scope: tenant_id mapper aggregates over groups, so a per-tenant group's attribute serves single-registration-multi-tenant installs with no scope change; idp_sub passthrough (upstream directory id for the login-bootstrap) formalized in the canonical contract. - Translation-table pattern documented (advanced claim-to-group mapper, one entry per external tenant, unmapped fails closed) — mapper shape validated against KC 26. - CI guard (tenant_translation_guard.py + workflow): boots a throwaway Keycloak, imports the ACTUAL canonical realm, and asserts DD-AUTH-04: fail-closed with no source, correct group translation, and detection of the two ambiguities token inspection cannot see — two tenant groups, and a group silently shadowing the pinned attribute (both observed live: Keycloak emits one arbitrary value). - tenant_claim freeze noted at the chart surface (defaults tenant_id per ADR-0003; configurable only for non-broker third-party IdPs). Refs constructorfabric#2196 Part of constructorfabric#2193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR defines pinned ChangesTenant translation contract
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Guard
participant KeycloakContainer
participant KeycloakAdminAPI
Guard->>KeycloakContainer: Start disposable Keycloak
Guard->>KeycloakAdminAPI: Import realm and configure users
KeycloakAdminAPI-->>Guard: Return token and group data
Guard->>Guard: Validate tenant contract
Guard->>KeycloakContainer: Remove disposable container
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
deploy/compose/keycloak/tests/tenant_translation_guard.py (3)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Keycloak image by digest.
quay.io/keycloak/keycloak:26.4is a mutable tag. A rebuilt tag can change mapper behavior and turn this guard red on an unrelated pull request, or hide a regression. The workflow pins its actions by SHA. Apply the same rule to the container image.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/compose/keycloak/tests/tenant_translation_guard.py` at line 38, Update the KC_IMAGE constant to reference the Keycloak 26.4 image by its immutable digest instead of the mutable tag, preserving the intended version while following the repository’s digest-pinning convention.
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a check for the
idp_subclaim.This pull request adds the
idp_submapper to the canonical scope. The guard does not assert it. The login-bootstrap person lookup depends on that claim. A future scope edit can drop the mapper and this lane stays green.Add a check that sets the
idp_subuser attribute and asserts the example token carries the value as a string.Also guard against a
Noneresponse intenant_claim:_callreturnsNonefor an empty body, and.getthen raisesAttributeError.♻️ Proposed additions
def tenant_claim(admin: Admin, client_id: str, user_id: str): token = admin.realm(f"/clients/{client_id}/evaluate-scopes/generate-example-id-token?userId={user_id}&scope=openid") - return token.get("tenant_id") + return (token or {}).get("tenant_id") + + +def claim(admin: Admin, client_id: str, user_id: str, name: str): + token = admin.realm(f"/clients/{client_id}/evaluate-scopes/generate-example-id-token?userId={user_id}&scope=openid") + return (token or {}).get(name)Also applies to: 182-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/compose/keycloak/tests/tenant_translation_guard.py` around lines 108 - 110, Update the tenant_claim test helper to handle a None token response before reading claims, returning an appropriate empty result instead of calling .get on None. Extend the related canonical-scope assertions to set the idp_sub user attribute and verify the generated example ID token contains that value as a string, alongside the existing tenant_id check.
69-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe admin token is fetched once and never refreshed.
Admin.__init__stores oneaccess_token. The master realmadmin-cliaccess token lifespan is short by default. The guard then performs the realm import plus roughly twenty admin calls. If the realm import is slow, later calls return HTTP 401 and the guard fails with a traceback instead of a contract result.Re-authenticate on 401, or refresh the token before each phase.
♻️ Proposed refactor
class Admin: def __init__(self) -> None: - self.token = self._call( + self.token = self._login() + + def _login(self) -> str: + self.__dict__.pop("token", None) + return self._call( "/realms/master/protocol/openid-connect/token", method="POST", raw=urllib.parse.urlencode( {"grant_type": "password", "client_id": "admin-cli", "username": "admin", "password": "admin"} ).encode(), )["access_token"]Then retry once on
HTTPErrorwithcode == 401inside_call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/compose/keycloak/tests/tenant_translation_guard.py` around lines 69 - 77, Update the Admin authentication flow so the token cannot expire during the guard’s multi-call execution: refresh or re-authenticate before each phase, or preferably make _call retry once after replacing the token when it receives an HTTPError with code 401. Preserve normal error propagation and avoid unbounded retries..github/workflows/keycloak-realm-contract.yml (1)
39-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture Keycloak logs when the guard fails.
The guard removes the container in its
finallyblock. If the realm import or the boot fails, the job output holds a Python traceback and no server log. Diagnosis then requires a local reproduction.Add a failure step that prints the container logs. The guard must skip the removal when a debug flag is set, otherwise the container is already gone.
Also pin the PyYAML version so a new release cannot change the parse result between runs.
🔧 Proposed workflow change
- name: Install PyYAML - run: python3 -m pip install --quiet pyyaml + run: python3 -m pip install --quiet 'pyyaml==6.0.2' - name: Tenant translation guard run: python3 deploy/compose/keycloak/tests/tenant_translation_guard.py + + - name: Keycloak logs on failure + if: failure() + run: docker logs tenant-guard-kc || echo "container already removed"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/keycloak-realm-contract.yml around lines 39 - 43, Update the Tenant translation guard workflow to pin the PyYAML dependency to a specific version and add a failure-only step that prints the Keycloak container logs. Pass a debug flag from the workflow to tenant_translation_guard.py, and update its finally cleanup logic to skip container removal when that flag is enabled so logs remain available after failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/keycloak-realm-contract.yml:
- Around line 12-15: The workflow trigger in keycloak-realm-contract.yml is
broader than the validation performed by tenant_translation_guard.py, so
non-local realm changes can pass without being checked. Update the path filter
to match only the local realm path that the guard actually reads, or expand
tenant_translation_guard.py to iterate over every realm file under
deploy/gitops/environments/*/keycloak/realms/ so the trigger scope and assertion
scope stay aligned.
In `@deploy/compose/keycloak/tests/tenant_translation_guard.py`:
- Around line 79-91: Update the tenant translation guard’s _call method to pass
a finite timeout to urllib.request.urlopen for every admin API request, using
the existing guard timeout convention. Wrap the realm-import operation in
HTTPError handling so failures are reported with a clear named check result
instead of an uncaught traceback.
In `@deploy/gitops/environments/local/keycloak/realms/insight-broker.yaml`:
- Around line 57-65: Add a canonical Keycloak identity-provider mapper for
idp_sub that writes the idp_sub user attribute before the existing
oidc-usermodel-attribute-mapper emits it. Update the realm configuration near
the existing idp_sub mapper, using the established tenant
hardcoded-attribute-idp-mapper shape and preserving the idp_sub claim mapping.
In `@deploy/gitops/README.md`:
- Around line 279-285: Revise the README text around the CI guard reference to
describe it as a contract test that creates a synthetic user in a throwaway
realm and validates the canonical realm shape and detection logic. Remove the
implication that it audits committed YAML, live realms, or blocks existing
deployed users from invalid memberships.
---
Nitpick comments:
In @.github/workflows/keycloak-realm-contract.yml:
- Around line 39-43: Update the Tenant translation guard workflow to pin the
PyYAML dependency to a specific version and add a failure-only step that prints
the Keycloak container logs. Pass a debug flag from the workflow to
tenant_translation_guard.py, and update its finally cleanup logic to skip
container removal when that flag is enabled so logs remain available after
failures.
In `@deploy/compose/keycloak/tests/tenant_translation_guard.py`:
- Line 38: Update the KC_IMAGE constant to reference the Keycloak 26.4 image by
its immutable digest instead of the mutable tag, preserving the intended version
while following the repository’s digest-pinning convention.
- Around line 108-110: Update the tenant_claim test helper to handle a None
token response before reading claims, returning an appropriate empty result
instead of calling .get on None. Extend the related canonical-scope assertions
to set the idp_sub user attribute and verify the generated example ID token
contains that value as a string, alongside the existing tenant_id check.
- Around line 69-77: Update the Admin authentication flow so the token cannot
expire during the guard’s multi-call execution: refresh or re-authenticate
before each phase, or preferably make _call retry once after replacing the token
when it receives an HTTPError with code 401. Preserve normal error propagation
and avoid unbounded retries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2bfd1bc-60c0-4e3c-883f-2747ae0008c9
📒 Files selected for processing (5)
.github/workflows/keycloak-realm-contract.ymlcharts/insight/templates/secrets.yamldeploy/compose/keycloak/tests/tenant_translation_guard.pydeploy/gitops/README.mddeploy/gitops/environments/local/keycloak/realms/insight-broker.yaml
Per review: the tenant is ALWAYS the fixed per-registration pin; an IdP's tenancy assertions are never consulted; a customer with several IdPs pins the same tenant on each. Drops the group aggregation added earlier in this branch (which itself created the group-shadows-pin hazard) and the translation-table docs. The guard now asserts the pinned contract: clean baseline, fail-closed, pin verbatim as a single string, tenant-bearing groups INERT and flagged by the realm scan. ADR-0003 amended (translation rejected); cfs clean for authenticator artifacts. Refs constructorfabric#2196 Part of constructorfabric#2193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/compose/keycloak/tests/tenant_contract_guard.py`:
- Around line 114-115: Update the group-loading logic in the tenant contract
guard to paginate the `/groups` endpoint and collect every realm group before
inspecting attributes. Use this complete group set when resolving `tenant-a`,
while preserving the existing detail lookup through `admin.realm` for each
group.
- Around line 169-174: Update the registration-level validation around
tenant_claim so it no longer relies on writing tenant_id directly to the
synthetic user. Scan each configured IdP’s hardcoded-attribute-idp-mapper and
fail when its mapped source attribute is not INSIGHT_TENANT_ID, ensuring the
pinned-tenant assertion exercises the canonical IdP mapping.
In `@deploy/gitops/README.md`:
- Line 257: Update the guard reference in the README to use the repository’s
actual filename, tenant_contract_guard.py, instead of
tenant_translation_guard.py.
In
`@docs/components/backend/authenticator/specs/ADR/0003-keycloak-identity-broker.md`:
- Around line 12-18: Update the Decision Outcome section around the claim
importer and tenancy mapping statements to remove claim-to-group translation,
upstream tenancy import, and upstream tenancy mapping. State that each
registration uses its fixed environment-provided tenant_id pin, while identity
data uses the separate idp_sub importer contract; preserve the fail-closed
behavior and keep tenant-bearing groups inert.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce3e1e6d-2dcf-4933-9ec8-f84b3f014f4e
📒 Files selected for processing (5)
.github/workflows/keycloak-realm-contract.ymldeploy/compose/keycloak/tests/tenant_contract_guard.pydeploy/gitops/README.mddeploy/gitops/environments/local/keycloak/realms/insight-broker.yamldocs/components/backend/authenticator/specs/ADR/0003-keycloak-identity-broker.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/keycloak-realm-contract.yml
- Guard statically validates EVERY committed realm file (each IdP registration: exactly one INSIGHT_TENANT_ID pin + an idp_sub stamp; no tenant-bearing groups), which makes the workflow's wide path trigger honest and catches a registration missing the pin — the live import covers the canonical realm's token behavior. - Admin calls carry timeouts; realm-import rejection reports as a named failure; group scan paginates. - README: per-registration mapper snippet (the idp_sub source the canonical shape implies), guard described as a contract test, stale filename fixed. - ADR-0003 Decision Outcome rewritten to the pinned-only model — the prescriptive claim-to-group passage the status entry rejected is gone. Refs constructorfabric#2196 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The guard talks to a throwaway Keycloak on 127.0.0.1 over plain http by design; URLs are module constants, never user input. nosemgrep on the two urlopen sites (dynamic-urllib-use, insecure-urlopen); verified locally with the same semgrep image (0 findings). Refs constructorfabric#2196 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/compose/keycloak/tests/tenant_contract_guard.py (1)
229-231: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
all_groupsto resolvetenant-a.
admin.realm("/groups")may return the first page only, sotenant-acan be absent from the lookup and raiseKeyErrorbefore the runtime assertions run. Build the mapping with the existing paginated helper before indexing by name.Proposed fix
- groups = {g["name"]: g["id"] for g in admin.realm("/groups")} + groups = {g["name"]: g["id"] for g in all_groups(admin)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/compose/keycloak/tests/tenant_contract_guard.py` around lines 229 - 231, Update the group lookup in the tenant setup flow to build the name-to-ID mapping from the existing paginated all_groups helper instead of the single admin.realm("/groups") response. Keep the tenant-a lookup and subsequent group assignment unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/compose/keycloak/tests/tenant_contract_guard.py`:
- Around line 116-132: Update all_groups() to recursively traverse each group’s
subGroups and include nested groups before tenant_bearing_groups() checks
attributes. Preserve pagination for top-level results, recurse through every
child hierarchy, and add a negative test fixture with tenant_id on a nested
group to verify the guard detects it.
In
`@docs/components/backend/authenticator/specs/ADR/0003-keycloak-identity-broker.md`:
- Around line 114-125: Update registration_violations() to reject any additional
IdP or client-scope mapper that produces tenant_id, allowing only the single
pinned tenant mapper and required idp_sub mapping described by the ADR. Add a
negative fixture containing an extra tenant-producing mapper and assert the
guard fails, ensuring no overlapping validator is relied on for this check.
---
Outside diff comments:
In `@deploy/compose/keycloak/tests/tenant_contract_guard.py`:
- Around line 229-231: Update the group lookup in the tenant setup flow to build
the name-to-ID mapping from the existing paginated all_groups helper instead of
the single admin.realm("/groups") response. Keep the tenant-a lookup and
subsequent group assignment unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d35144e1-132a-4e85-aff2-1e1b866f88f0
📒 Files selected for processing (4)
.github/workflows/keycloak-realm-contract.ymldeploy/compose/keycloak/tests/tenant_contract_guard.pydeploy/gitops/README.mddocs/components/backend/authenticator/specs/ADR/0003-keycloak-identity-broker.md
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/gitops/README.md
- .github/workflows/keycloak-realm-contract.yml
| def all_groups(admin: Admin) -> list[dict]: | ||
| groups, first, page = [], 0, 100 | ||
| while True: | ||
| batch = admin.realm(f"/groups?first={first}&max={page}") | ||
| groups.extend(batch) | ||
| if len(batch) < page: | ||
| return groups | ||
| first += page | ||
|
|
||
|
|
||
| def tenant_bearing_groups(admin: Admin) -> list[str]: | ||
| """Policy scan: NO group in the realm may carry a tenant_id attribute.""" | ||
| offenders = [] | ||
| for g in all_groups(admin): | ||
| detail = admin.realm(f"/groups/{g['id']}") | ||
| if (detail.get("attributes") or {}).get("tenant_id"): | ||
| offenders.append(g["name"]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'deploy/compose/keycloak/tests/tenant_contract_guard.py|compose|keycloak|tenant_contract_guard' || true
echo
echo "Target file outline/stat:"
wc -l deploy/compose/keycloak/tests/tenant_contract_guard.py
sed -n '1,220p' deploy/compose/keycloak/tests/tenant_contract_guard.py
echo
echo "Search tenant_id and group checks:"
rg -n "tenant_id|all_groups|tenant_bearing_groups|registrations|clean-baseline|subGroups|groups" deploy/compose/keycloak/tests/tenant_contract_guard.py deploy/compose/keycloak 2>/dev/null || trueRepository: constructorfabric/insight
Length of output: 20625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect likely Keycloak API shape and behavior from available docs/repo examples.
echo "Relevant API docs/search:"
rg -n "subGroups|tenant_id|groups/\\{" . 2>/dev/null | head -200 || true
echo
echo "Read only structural verifier for tenant_bearing_groups implementation:"
python3 - <<'PY'
from pathlib import Path
p = Path('deploy/compose/keycloak/tests/tenant_contract_guard.py')
txt = p.read_text()
start = txt.index('def tenant_bearing_groups')
end = txt.index('\ndef ', start+1) if '\ndef ' in txt[start+1:] else len(txt)
body = txt[start:end]
print(body)
print("references subGroups:", 'subGroups' in txt)
print("recursive group traversal present:", any(tok in body for tok in ['iterate_recursive', 'subGroups', 'all_groups(admin)']))
PYRepository: constructorfabric/insight
Length of output: 22676
🌐 Web query:
Keycloak REST API groups subGroups group representation nested groups tenant_id attribute
💡 Result:
In Keycloak, group management via the REST API is handled through the Groups and Group resources. Group Representation and Subgroups The GroupRepresentation object represents a group and includes a subGroups field, which is a list of nested GroupRepresentation objects [1]. While the data model supports a hierarchical structure, retrieving this hierarchy depends on the API endpoint and parameters used [2]. When listing groups using GET /admin/realms/{realm}/groups, you can control the detail level of the response using the briefRepresentation query parameter [2][3]. Setting briefRepresentation to false includes full details, such as attributes and role mappings, whereas true (the default) limits the response to basic information like ID, name, path, and parentId [4][2][3]. Retrieving subgroups can be done in a few ways: 1. When listing all groups or searching, subgroups may be populated based on the parameters used [2]. 2. You can explicitly list subgroups for a specific parent group by calling the children endpoint: GET /admin/realms/{realm}/groups/{parentId}/children [5]. 3. The GroupRepresentation includes a subGroupCount field and a corresponding getter/setter, which can be useful for managing or displaying hierarchical data [1]. Attributes and tenant_id The GroupRepresentation class contains an attributes field, which is a Map<String, List> used to store custom key-value pairs [6]. There is no specialized "tenant_id" field hardcoded into the GroupRepresentation model; instead, if you need to associate a group with a specific tenant, you should add a "tenant_id" key to the attributes map [6]. Key Administrative Actions - Create a child group: POST /admin/realms/{realm}/groups/{parentId}/children [5]. - Update a child group: POST /admin/realms/{realm}/groups/{id}/children [5]. - Retrieve group details: GET /admin/realms/{realm}/groups/{group-id} [2]. Note that for large hierarchies, retrieving full representations for every nested group can be resource-intensive, so using briefRepresentation: true is generally recommended unless full attribute data is required for all nodes [2][3][7].
Citations:
- 1: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/representations/idm/GroupRepresentation.html
- 2: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/services/resources/admin/GroupsResource.html
- 3: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/admin/client/resource/GroupsResource.html
- 4: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/admin/client/resource/GroupResource.html
- 5: https://github.com/keycloak/keycloak/blob/213ff926/js/libs/keycloak-admin-client/src/resources/groups.ts
- 6: https://www.keycloak.org/docs-api/26.2.2/javadocs/org/keycloak/representations/idm/GroupRepresentation.html
- 7: Is there a way to get all mapped attributes for all groups/roles in one API request? keycloak/keycloak#24377
🌐 Web query:
"GET /admin/realms/{realm}/groups" "subGroups" Keycloak docs-api
💡 Result:
In modern Keycloak versions (since release 23), the behavior of the GET /admin/realms/{realm}/groups endpoint regarding the subGroups field has changed [1][2]. The subGroups field is not populated by default when querying the root group list [3]. To retrieve subgroups, the following points are authoritative based on current Keycloak documentation and behavior: 1. Use the Dedicated Children Endpoint: The recommended way to retrieve subgroups for a specific group is now the dedicated endpoint [1][2]: GET /admin/realms/{realm}/groups/{group-id}/children This endpoint supports pagination and is designed specifically to handle subgroup retrieval efficiently [1][2]. 2. Limitations of the Main Groups Endpoint: When using the root GET /admin/realms/{realm}/groups endpoint, the subGroups field is only populated if you provide a search or q (query) parameter [3]. If these parameters are not provided, the API returns only top-level groups without expanding the hierarchy [3]. 3. API Parameters: - populateHierarchy: While this parameter exists, it does not automatically force the expansion of the subGroups tree when querying the root group list without search criteria [2][3]. - subGroupsCount: A parameter introduced to control whether the count of subgroups is returned for each group, which defaults to true [4]. If you are developing against the Keycloak Admin API, it is best practice to migrate logic that previously relied on the full hierarchy being returned in a single call to the /children endpoint for specific group IDs [1][2].
Citations:
- 1: auth/admin/realms/{realm-name}/groups?briefRepresentation=false returns empty subgroups. keycloak/keycloak#27694
- 2: GET
/admin/realms/{realm}/groupsEndpoint Not ReturningsubGroups,clientRoles,realmRolesDespitepopulateHierarchyandbriefRepresentationsettings" keycloak/keycloak#25053 - 3: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/services/resources/admin/GroupsResource.html
- 4: https://github.com/keycloak/keycloak/blob/main/docs/documentation/upgrading/topics/changes/changes-26_3_0.adoc
Traverse nested groups before accepting the policy.
all_groups() only lists top-level groups, and GET /admin/realms/insight-broker/groups does not expand subGroups. A nested group can carry attributes.tenant_id while both registrations(...) and clean-baseline pass. Recurse through group children before checking attributes and add a negative case with tenant_id on a nested group.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/compose/keycloak/tests/tenant_contract_guard.py` around lines 116 -
132, Update all_groups() to recursively traverse each group’s subGroups and
include nested groups before tenant_bearing_groups() checks attributes. Preserve
pagination for top-level results, recurse through every child hierarchy, and add
a negative test fixture with tenant_id on a nested group to verify the guard
detects it.
| - **Claims are shaped at the broker.** Every provider registration carries a | ||
| `hardcoded-attribute-idp-mapper` pinning the fixed per-registration Insight `tenant_id` from | ||
| environment values, plus an attribute-importer stamping `idp_sub` (the upstream's stable | ||
| directory id, e.g. Entra `oid` -- the login-bootstrap external id). An upstream's own tenancy | ||
| assertions are never consulted (amended 2026-08-06; the claim-to-group translation once | ||
| sketched here is rejected): a customer with several IdPs pins the same tenant on each | ||
| registration, and two customers sharing an IdP vendor never intersect -- realm-per-customer | ||
| gives each its own registration, client, and pin. The client's **protocol mappers / client | ||
| scopes** are the allow-list: the token contains only what is explicitly emitted -- `sub`, | ||
| `email`, one string `tenant_id`, `idp_sub` -- and deliberately does NOT aggregate attributes | ||
| over groups, so a group-sourced tenant is mechanically impossible; a CI guard asserts the | ||
| contract on every committed realm file and against a live import. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'identityProviderMappers|protocolMappers|clientScopes|tenant_id|idp_sub|claim-to-user-attribute|group' \
deploy/compose/keycloak/tests/tenant_contract_guard.py \
deploy/gitops/environmentsRepository: constructorfabric/insight
Length of output: 23826
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- tenant contract guard relevant sections ---\n'
sed -n '1,260p' deploy/compose/keycloak/tests/tenant_contract_guard.py
printf '\n--- local broker scope mappers around tenant_id/idp_sub ---\n'
sed -n '1,80p' deploy/gitops/environments/local/keycloak/realms/insight-broker.yaml
printf '\n--- find identity_provider_mapper related checks/tests ---\n'
rg -n 'identityProviderMapper|hardcoded-attribute-idp-mapper|tenant_id.*mapper|clientScopes|protocolMappers|tenant-contract guard|tenant contract|registration_violations|tenant_bearing_groups' deploy/compose/keycloak/tests deploy/gitops/environments docs/components/backend/authenticator/specs/ADR || trueRepository: constructorfabric/insight
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tenant contract guard relevant sections ---'
sed -n '1,260p' deploy/compose/keycloak/tests/tenant_contract_guard.py
echo
echo '--- local broker scope mappers around tenant_id/idp_sub ---'234
sed -n '1,80p' deploy/gitops/environments/local/keycloak/realms/insight-broker.yaml
echo
echo '--- find identity_provider_mapper related checks/tests ---'
rg -n 'identityProviderMapper|hardcoded-attribute-idp-mapper|tenant_id.*mapper|clientScopes|protocolMappers|tenant-contract guard|tenant contract|registration_violations|tenant_bearing_groups' deploy/compose/keycloak/tests deploy/gitops/environments docs/components/backend/authenticator/specs/ADR || trueRepository: constructorfabric/insight
Length of output: 16122
🌐 Web query:
Keycloak 26 oidc claim to user attribute mapper tenant_id claim behavior keycloak-admin-api
💡 Result:
In Keycloak 26, to map a user attribute—such as a tenant_id—to an OIDC token claim, you use the User Attribute Mapper (type: oidc-usermodel-attribute-mapper) [1][2]. This mapper retrieves a value stored in the user's attributes (UserModel.attribute) and inserts it into the ID token, access token, or userinfo response [1][3][4]. Configuration via Keycloak Admin API To create this mapper programmatically, you must send a POST request to the protocol-mappers endpoint for your specific client (e.g., /admin/realms/{realm}/clients/{id}/protocol-mappers/models) [5]. The JSON body should follow this structure [1]: { "name": "tenant-id-mapper", "protocol": "openid-connect", "protocolMapper": "oidc-usermodel-attribute-mapper", "config": { "user.attribute": "tenant_id", "claim.name": "tenant_id", "jsonType.label": "String", "id.token.claim": "true", "access.token.claim": "true", "userinfo.token.claim": "true" } } Behavioral Details 1. Token Claim Name: The claim.name property defines the key name in the resulting JSON token [1]. You can use dot notation (e.g., company.tenant_id) to create nested JSON objects within the claim [1][3][4]. 2. Multivalued Attributes: If the tenant_id attribute on the user can contain multiple values, set the "multivalued": "true" configuration option to ensure all values are correctly mapped into an array in the token [3][4][2]. 3. Attribute Source: The mapper looks for the value within the Keycloak user's attribute map [1][3]. Ensure the user profile or identity provider import process correctly populates this attribute for the user session [6][7]. 4. Token Inclusion: The boolean flags (id.token.claim, access.token.claim, userinfo.token.claim) allow you to granularly control which tokens receive this specific claim [1][8]. For advanced scenarios, such as needing to prefix the tenant_id value during mapping (e.g., to ensure uniqueness in multi-tenant brokering scenarios), check if your specific patch level of Keycloak 26 includes the Prefixed User Attribute Mapper functionality, which was introduced to handle scenarios where attribute values must be transformed before inclusion in the token [9].
Citations:
- 1: https://www.keycloak.org/admin-api/protocol-mappers
- 2: https://stackoverflow.com/questions/66822016/where-are-all-of-the-keycloak-protocol-mapper-config-options-documented
- 3: https://www.keycloak.org/docs-api/26.4.6/javadocs/org/keycloak/protocol/oidc/mappers/UserAttributeMapper.html
- 4: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/protocol/oidc/mappers/UserAttributeMapper.html
- 5: https://stackoverflow.com/questions/72363344/add-protocol-mapper-to-client-via-keycloak-rest-api
- 6: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/broker/oidc/mappers/UserAttributeMapper.html
- 7: https://github.com/keycloak/keycloak/blob/main/docs/documentation/server_admin/topics/identity-broker/mappers.adoc
- 8: https://github.com/keycloak/keycloak/blob/main/docs/documentation/server_admin/topics/clients/con-protocol-mappers.adoc
- 9: Prefixed User Attribute Importer for OIDC IdP keycloak/keycloak#39929
Enforce the tenant mapper allowlist in the guard.
registration_violations() only requires the pinned IdP mapper and idp_sub destination, then accepts any additional IdP mapper. A tenant-valued IdP claim mapper that writes tenant_id would still pass the guard while breaking the ADR’s single-pinned-tenant contract. Add explicit mapper allowlist/denylist checks, include a negative fixture with an extra tenant-producing IdP or client-scope mapper, and ensure no other validator covers this gap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/components/backend/authenticator/specs/ADR/0003-keycloak-identity-broker.md`
around lines 114 - 125, Update registration_violations() to reject any
additional IdP or client-scope mapper that produces tenant_id, allowing only the
single pinned tenant mapper and required idp_sub mapping described by the ADR.
Add a negative fixture containing an extra tenant-producing mapper and assert
the guard fails, ensuring no overlapping validator is relied on for this check.
semgrep honors nosemgrep only on the match's first line or the line before; ruff-format had parked the annotations on closing parens where CI ignored them. Standalone line-before comments are format-stable; verified with the exact CI invocation (--config auto + p/rust): 0 findings. Refs constructorfabric#2196 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
3 participants
What (boxes 1–3 of #2196, per the pinned-tenant decision)
Tenant policy — pinned only, translation REJECTED. Review decision on this PR: the tenant is always the fixed per-registration pin (
INSIGHT_TENANT_IDfrom environment values); an IdP's own tenancy assertions are never consulted; a customer with several IdPs pins the same tenant on each registration. The claim-to-group translation sketched in ADR-0003 is rejected — the ADR is amended in this PR (status-history entry;cfsclean for authenticator artifacts). Two customers sharing an IdP vendor never intersect: realm-per-customer gives each its own registration, client, and pin.Canonical claim contract — the
insightscope formalizesidp_sub(upstream directory id feeding the login-bootstrap, proven on the first migrated stand). No group aggregation: without it a tenant-bearing group cannot influence the token, eliminating the shadowing hazard at the mechanism level rather than policing it.Pinned-contract CI guard —
deploy/compose/keycloak/tests/tenant_contract_guard.py+ a small workflow lane (docker Keycloak, no cluster). It imports the actual canonical realm file and asserts against real token evaluation:All four verified green locally against KC 26.4.
tenant_claimfreeze — already defaults totenant_idat every surface; the chart comment records the freeze per ADR-0003, configurable only for third-party consumers wiring a non-broker IdP directly.Refs #2196
Part of #2193
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation