fix: add admin-permissions client to isInternal denylist - #51634
Open
waterWang wants to merge 1 commit into
Open
fix: add admin-permissions client to isInternal denylist#51634waterWang wants to merge 1 commit into
waterWang wants to merge 1 commit into
Conversation
The `ClientPermissions.isInternal` method denylists only the `realm-management` client (and, in the master realm, client IDs ending in the `-realm` suffix). The `admin-permissions` client — the resource server holding the realm's entire FGAP V2 model — is not in that list. As a result, a delegated administrator holding a FGAP `Clients:manage` permission (and no realm-management roles) can update and disable the `admin-permissions` client through the Admin API, even though `getAccess()` correctly returns `manage: false` for this client. This fix adds `Constants.ADMIN_PERMISSIONS_CLIENT_ID` to the `isInternal` predicate, making the enforcement path consistent with the access report. Closes keycloak#51630 Signed-off-by: waterWang <waterWang@users.noreply.github.com>
waterWang
force-pushed
the
fix/fgap-v2-admin-permissions-internal
branch
from
August 12, 2026 02:38
87a360d to
074b7df
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Extends internal-client protection to the FGAP V2 admin-permissions client.
Changes:
- Adds
admin-permissionsto theisInternalpredicate.
Suppressed comments (1)
services/src/main/java/org/keycloak/services/resources/admin/fgap/ClientPermissions.java:729
- This authorization regression has no automated coverage. Add an FGAP V2 test that grants only
Clients:manageand verifies that updating or disabling the admin-permissions client is forbidden; code inspection alone will not guard this enforcement path.
|| Constants.ADMIN_PERMISSIONS_CLIENT_ID.equals(client.getClientId());
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| return Constants.REALM_MANAGEMENT_CLIENT_ID.equals(client.getClientId()); | ||
| return Constants.REALM_MANAGEMENT_CLIENT_ID.equals(client.getClientId()) | ||
| || Constants.ADMIN_PERMISSIONS_CLIENT_ID.equals(client.getClientId()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
ClientPermissions.isInternalmethod denylists only therealm-managementclient (and, in the master realm, client IDs ending in the-realmsuffix). Theadmin-permissionsclient — the resource server holding the realm's entire FGAP V2 model — is not in that list.As a result, a delegated administrator holding a FGAP
Clients:managepermission (and no realm-management roles) can update and disable theadmin-permissionsclient through the Admin API, even thoughgetAccess()correctly returnsmanage: falsefor this client.Fix
Added
Constants.ADMIN_PERMISSIONS_CLIENT_IDto theisInternalpredicate, making the enforcement path consistent with the access report.Root Cause
ClientPermissions.isInternal(line 719) checks only forrealm-managementand-realmsuffix clients.Constants.ADMIN_PERMISSIONS_CLIENT_IDwas missing from the predicate.How Has This Been Tested?
realm-managementis handledgetAccess()method already special-cases this client at line 676; the fix makesisInternalconsistent with that checkCloses #51630