Repository navigation
fix(admin/api): verify role belongs to master realm when checking for protected admin roles - #51329
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
The security-boundary fix needs regression coverage for both non-master and master realm behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Restricts protected admin-role detection to the configured master realm, fixing false 403 responses in application realms.
Changes:
- Checks the role container’s realm name.
- Protects
adminandcreate-realmonly in the master realm.
File summaries
| File | Description |
|---|---|
services/src/main/java/org/keycloak/services/resources/admin/fgap/RolePermissions.java |
Narrows protected-role detection to master-realm roles. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The regression test renames both roles to the same name, causing the second update to fail with a duplicate-role conflict.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tests/base/src/test/java/org/keycloak/tests/admin/authz/rbac/InternalClientManagementTest.java:117
- Both loop iterations rename their role to
TEMP_ROLE_NAME, so after the first succeeds the second update conflicts with the existing role name instead of verifying permission. Use a distinct target name for each protected-name case.
client.realm("myrealm").rolesById().updateRole(role.getId(), renamed(role));
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The regression test should exercise the name-based endpoint reported in #51323.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tests/base/src/test/java/org/keycloak/tests/admin/authz/rbac/InternalClientManagementTest.java:113
- This regression test exercises the ID-based endpoint, but #51323 reproduces the failure through
PUT /roles/{role-name}; the two endpoints have distinct request paths and the name-based path performs an additional container permission check. Invoke the name-based update here so the test covers the reported regression end to end.
client.realm("myrealm").rolesById().updateRole(role.getId(), renamed(role));
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The focused authorization fix matches the reported regression and preserves master-realm protection with adequate test coverage.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Updated RolePermissions.isRealmAdminRole() to explicitly check that the role belongs to the master realm before blocking it. This ensures that application realms can use and manage custom roles named admin or create-realm without being flagged as protected roles. Closes keycloak#51323 Signed-off-by: alehhu <159355663+alehhu@users.noreply.github.com>
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestCertSubjectAsKeyNameInKeyInfoKeycloak CI - Adapter IT Strict Cookies |
|
Thanks, @alehhu. |
Which problem is this PR solving?
Fixes #51323
Custom realm-level roles named
adminin non-master realms were being incorrectly blocked from updates withHTTP 403 Forbidden. The recent security fix in PR #49624 relied onisRealmAdminRole(), which checked the role name but did not verify if the role belonged to the protectedmasteradmin realm.Short description of the changes
RolePermissions.isRealmAdminRole()to explicitly check that the role belongs to the master realm (Config.getAdminRealm()) before blocking it.adminorcreate-realmwithout being incorrectly flagged as protected server-administration roles.(Note: AI was used in autocomplete mode)