feat(pr-env): let GitHub users impersonate via Keycloak admin console - #342
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
HyperShell environment destroyedThis ephemeral OpenShift environment has been destroyed. Comment |
Amber reviewStatus: Complete |
|
/pr-extend |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/pr-destroy |
PR environments had no GitHub path into the hypershell-realm admin console, so locking down the test-tier password users would leave humans with no way to impersonate developer or platform-admin. Grant realm-management impersonation on GitHub broker login, hide the password form when GitHub is present, and publish the admin console URL on the access comment. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>
The GitHub IdP mapper grants realm-management.impersonation, which is not target-scoped. Stop claiming interactive impersonation cannot reach other GitHub users; keep that guarantee on token-exchange via hypershell-e2e. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>
f86b904 to
5b2506b
Compare
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a human GitHub-brokered impersonation path via the Keycloak hypershell-realm admin console for ephemeral PR environments, with matching render-gating, login theme, access-comment, banner, and shell-test coverage; the change is env-gated so Kind/hub/local are unaffected. The one prior Minor is now addressed - the spec was corrected to describe the realm-wide role - and I have two small new Minor notes (a malformed access-comment URL when the Keycloak Route is absent, and a defense-in-depth nuance in the login theme).
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Previous concerns
- [Addressed - Minor] Realm-wide
realm-management.impersonationgrant exceeded the documented target set (r4076539509). The prior review asked to either drop the overstated clause or add FGAP target scoping. Commit5b2506btakes the documentation option: the rationale row no longer makes the blanket claim and now reads "Token-exchange impersonation stays onhypershell-e2eand does not include other GitHub-brokered users" (specs/platform/ephemeral-pr-environments.spec.mdL1486); new prose at L1109-1122 explicitly states theimpersonationrole is realm-wide and not target-scoped, framed as a deliberate trade-off for org-gated throwaway envs; andephemeral-test-credentials.spec.mdL774 was scoped to "Token-exchange impersonation SHALL NOT include other GitHub-brokered users." The spec now matches what the role enforces, which resolves the finding. I did not re-post in the existing thread since it is resolved.
Test Diff Scrutiny
The one modified assertion in a pre-existing test (scripts/ci/pr-env-lib_test.sh, the body block that previously asserted the oc login --server=...:6443 --web template) is a re-scope, not a removed guarantee: the same positive oc login --server=https://api.cluster.example.com:6443 --web assertion still runs in the updating_body block (L239), and the negative assertion still rejects the app API Route (L376). The new render-realm-config_test.sh assertions pin both the Kind omission (gateway:creator,github-identity,platform:admin) and the PR-env superset including the three realm-management.* roles, matching the env-gated render logic.
Findings
Two new Minor findings this pass, both posted inline. No blockers or criticals.
Findings Summary (ordered by severity, highest first):
- [Minor] Absent Keycloak Route yields a malformed access-comment URL (
https://admin/hypershell/console/) because the emittedkeycloak_url=https://defeats the-nomission guard - Robustness (.github/actions/deploy-pr-environment/action.ymlL228,scripts/ci/pr-env-lib.shL406-410) - [Minor] Login theme hides but does not disable the password form; the "cannot fall back to a test-tier password" guarantee rests on password secrecy, not the CSS - Security / defense-in-depth (
deploy/base/keycloak/theme/login.css)
Convention Checklist:
| Convention | Result |
|---|---|
| No secrets in logs, comments, or responses | Pass |
| Input handling (env-gated realm render) | Pass |
| Conventional commit messages | Pass |
| Test diff scrutiny (no silently removed guarantees) | Pass |
| Change gated so Kind/hub/local unaffected | Pass |
| Spec matches implemented behavior | Pass |
| { | ||
| echo "api_url=https://${api_host}" | ||
| echo "web_url=https://${web_host}" | ||
| echo "keycloak_url=https://${kc_host}" |
There was a problem hiding this comment.
[Minor] When the Keycloak Route lookup fails, kc_host is empty and this still emits keycloak_url=https:// - a non-empty value. pr_env_comment_body's if [[ -n "${kc_url}" ]] guard (scripts/ci/pr-env-lib.sh L409) then treats it as present and renders | Keycloak admin console | https://admin/hypershell/console/ | (the ${kc_url%/} strip turns https:// into https:/, so the row loses its host). The || true on the oc get route shows absence is anticipated, so the row should be omitted, not rendered broken. Suggest only emitting the line when the host resolved, e.g. [[ -n "${kc_host}" ]] && echo "keycloak_url=https://${kc_host}", or validate a host is present before building the row. Confidence: Medium.
| */ | ||
| body:has(#kc-social-providers) #kc-form, | ||
| body:has(#kc-social-providers) .kc-login-divider { | ||
| display: none !important; |
There was a problem hiding this comment.
[Minor / defense-in-depth] This rule hides the password form when a social provider is present, which satisfies the spec's "SHALL NOT present a username/password form". It does not, however, disable password authentication: the seeded principals still hold passwords and the /login-actions/authenticate endpoint stays reachable, so the stronger claim that "a human cannot fall back to a test-tier principal password" rests on those passwords staying unpublished (cloud secret store), not on the CSS. Acceptable for an org-gated ephemeral env; consider either noting in the comment that this guard is cosmetic, or enforcing it server-side (e.g. dropping the password credential from the seeded users during the brokered window) if the guarantee should not depend on secrecy alone. Confidence: Low.
2 participants
Summary
Enable GitHub users in PR environments to verify lower-privilege roles by impersonating developer and platform-admin accounts through the Keycloak admin console.
Highlights
/admin/hypershell/console/with GitHub and impersonate built-in test rolesScope
Changes are scoped to ephemeral PR environments with GitHub brokering. Kind and local OpenShift are unaffected (GitHub IdP disabled, test credentials unchanged). Spec and test coverage updated to document the new impersonation workflow.
🤖 Generated with Claude Code