NO-ISSUE: Enable Keycloak organizations - #464
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
@jhernand: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR updates Keycloak configuration to expand the role requirements for the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
charts/keycloak/templates/_start.tpl (1)
87-107: Consider validating required credentials before writing the config.If neither the URL contains credentials nor the
user/passwordfiles exist,db_useranddb_passwordwill be empty. Keycloak will fail at runtime with potentially unclear database authentication errors.🔧 Optional: Add validation for required credentials
+# Validate required credentials: +if [[ -z "${db_user}" ]]; then + echo "Database user is not set (provide 'user' file or include in URL)" + exit 1 +fi +if [[ -z "${db_password}" ]]; then + echo "Database password is not set (provide 'password' file or include in URL)" + exit 1 +fi + # Write the Keycloak configuration file: conf_file="/opt/keycloak/conf/keycloak.conf"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/keycloak/templates/_start.tpl` around lines 87 - 107, The startup script writes keycloak.conf using db_url, db_user and db_password but does not validate credentials; add a validation step before creating conf_file (check db_url for embedded credentials or that db_user and db_password are non-empty / the files providing them exist) and if missing emit a clear error and exit non-zero instead of writing an incomplete config; modify the segment around conf_file, db_url, db_user, db_password and the exec /opt/keycloak/bin/kc.sh start --import-realm call to perform this check and fail-fast with a helpful message.it/it_tool.go (1)
942-947: Conditionally emitclientRolesonly when role mappings exist.In Keycloak's
UserRepresentationmodel,clientRolesis defined asMap<String, List<String>>and should be omitted (not null) when empty. Line 946 always includes this field; provide it only whendata.ClientRolesis non-empty.Suggested patch
- users = append( - users, map[string]any{ - "username": fmt.Sprintf("service-account-%s", data.ClientId), - "enabled": true, - "serviceAccountClientId": data.ClientId, - "clientRoles": data.ClientRoles, - }, - ) + user := map[string]any{ + "username": fmt.Sprintf("service-account-%s", data.ClientId), + "enabled": true, + "serviceAccountClientId": data.ClientId, + } + if len(data.ClientRoles) > 0 { + user["clientRoles"] = data.ClientRoles + } + users = append(users, user)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/it_tool.go` around lines 942 - 947, The map literal passed into users always includes "clientRoles" even when empty; change this to build the attributes map first (e.g., attrs := map[string]any{ "username": fmt.Sprintf("service-account-%s", data.ClientId), "enabled": true, "serviceAccountClientId": data.ClientId }) and then only set attrs["clientRoles"] = data.ClientRoles when data.ClientRoles is non-nil and has length > 0 (len(data.ClientRoles) > 0). Finally pass attrs to users instead of the inline literal so "clientRoles" is omitted when empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@charts/keycloak/templates/_start.tpl`:
- Around line 87-107: The startup script writes keycloak.conf using db_url,
db_user and db_password but does not validate credentials; add a validation step
before creating conf_file (check db_url for embedded credentials or that db_user
and db_password are non-empty / the files providing them exist) and if missing
emit a clear error and exit non-zero instead of writing an incomplete config;
modify the segment around conf_file, db_url, db_user, db_password and the exec
/opt/keycloak/bin/kc.sh start --import-realm call to perform this check and
fail-fast with a helpful message.
In `@it/it_tool.go`:
- Around line 942-947: The map literal passed into users always includes
"clientRoles" even when empty; change this to build the attributes map first
(e.g., attrs := map[string]any{ "username": fmt.Sprintf("service-account-%s",
data.ClientId), "enabled": true, "serviceAccountClientId": data.ClientId }) and
then only set attrs["clientRoles"] = data.ClientRoles when data.ClientRoles is
non-nil and has length > 0 (len(data.ClientRoles) > 0). Finally pass attrs to
users instead of the inline literal so "clientRoles" is omitted when empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab64f358-7122-43ae-8b0e-41e2e2cd5e5b
📒 Files selected for processing (7)
charts/README.mdcharts/keycloak/README.mdcharts/keycloak/files/realm.jsoncharts/keycloak/templates/_start.tplcharts/keycloak/templates/deployment.yamlit/it_tool.gomanifests/README.md
This enables the Keycloak organizations feature in the realm configuration and grants the `osac-controller` service account the roles it needs to manage the realm, users and organizations. The roles assigned from the `realm-management` client are: - `manage-realm` - Manage the realm configuration, including organizations. - `manage-users` - Create, update and delete users. - `view-realm` - View the realm configuration. - `view-users` - View users. The documentation and integration test tooling are updated accordingly. Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
90c8c18 to
e219099
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Summary
This enables the Keycloak organizations feature and grants the
osac-controllerservice account the permissions it needs to managethem.
The realm configuration is updated to set
organizationsEnabledtotrue, and theosac-controllerservice account is granted thefollowing roles from the
realm-managementclient:manage-realm- Manage the realm configuration, including organizations.manage-users- Create, update and delete users.view-realm- View the realm configuration.view-users- View users.The documentation and integration test tooling are updated accordingly.
Summary by CodeRabbit
Documentation
Configuration