Keycloak PR #25 — Live Benchmark

Golden Comments (2/3 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	Inconsistent feature flag bug causing orphaned permissions. The `AdminPermissions` event listener, responsible for cleaning up permissions upon role, client, or group removal, is incorrectly guarded by the `ADMIN_FINE_GRAINED_AUTHZ` (V1) feature flag. Consequently, if `ADMIN_FINE_GRAINED_AUTHZ_V2` is enabled but V1 is not, the permission cleanup logic will not execute, leading to orphaned permission data. Cleanup should occur regardless of which fine-grained authorization version is enabled.	HIGH	✓ Found	🔴 "Removal-cleanup listener is gated on V1 only (`Profile.ADMIN_FINE_GRAINED_AUTHZ`), producing two problems: (1) With only V2 enabled, role/client/group removal events never trigger cleanup, leaving FGAP resources orphaned; (2) With V1 and V2 both enabled, the guard is true but `management(...)` selects `MgmtPermissionsV2`, whose `setPermissionsEnabled` methods throw `UnsupportedOperationException`, so removing any role/client/group throws inside the event listener. Guard should be derived from the same feature-flag decision used by `management(...)`" (AdminPermissions.java L81)
#2	In `hasPermission(ClientModel client, String scope)`, the resource lookup uses `findByName(server, client.getId(), server.getId())`, but `AdminPermissionsSchema.getOrCreateResource` creates per-client resources with the owner set to `resourceServer.getClientId()`, so this lookup will never find those resources and will always fall back to the 'all-clients' resource, effectively ignoring client-specific permissions.	HIGH	✗ Not found	The review flags the neighboring consequence in `getClientsWithPermission` (🔴 ClientPermissionsV2.java L135 — `findByType` with a null type) but not the defect in `hasPermission` itself: `findByName(server, client.getId(), server.getId())` can never match per-client resources created with owner set to `resourceServer.getClientId()`, so the lookup always falls back to the 'all-clients' resource. Two different lookups, two different mismatched attributes (type vs owner) — the review did not connect the per-client resource-creation defect to the `hasPermission` lookup. See `services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java:214`
#3	In `getClientsWithPermission(String scope)`, iterating `resourceStore.findByType(server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE)` and returning `resource.getName()` will only ever consider the type-level 'Clients' resource (per-client resources have no type) and return its name, while `AvailableRoleMappingResource#getRoleIdsWithPermissions` expects actual client IDs to pass to `realm.getClientById`, which can lead to incorrect behavior or a null client and subsequent failures.	HIGH	✓ Found	🔴 "getClientsWithPermission returns the wrong set: per-client resources created for the CLIENTS resource type via `AdminPermissionsSchema.getOrCreateResource` are never assigned a resource type (type is null), so `findByType(server, "Clients", ...)` only matches the all-clients resource and not per-client resources. This breaks fine-grained available role mappings (returns empty for per-client grants, or returns the literal string "Clients" used as a client id, causing NPE)" (ClientPermissionsV2.java L135)

Supplementary Findings (7 findings)

Finding	File	Severity	Legitimate?
`canExchangeTo` throws `UnsupportedOperationException` (HTTP 500 instead of a 403 denial) when V2 is enabled; reachable from the standard client-to-client token-exchange flow (`AbstractTokenExchangeProvider.exchangeClientToClient`)	ClientPermissionsV2.java:148	HIGH	✓ Real reachable production path breaking token exchange whenever V2 is on
Indexing into default client scopes by fixed position is brittle and the "before" block tests the wrong call — `getDefaultClientScopes()` already throws, so `removeDefaultClientScope` is never exercised before the grant	PermissionClientTest.java:148	MEDIUM	✓ Real test that silently exercises the wrong path and assumes fixture size
Trailing whitespace added after the class declaration, before `USERS_RESOURCE_TYPE`	AdminPermissionsSchema.java:53	LOW	✓ Borderline: cosmetic noise only
Unused helper `createClientPolicy` added but never called by any target test class (only `createUserPolicy` is used)	AbstractPermissionTest.java:120	LOW	✓ Real dead code in the shared base class
Inconsistent static vs. instance helper design after refactor: `getPolicies()` and the two `createPermission(ManagedClient, resourceId, ...)` overloads remain instance methods depending on the injected `client` field while siblings are static	AbstractPermissionTest.java:146	LOW	✓ Borderline: consistency/style only, no behavior impact
Test-created client roles (`myclient-role`, `myclient-subRole`) are not cleaned up within the test method — re-running fails with duplicate-role 409s if the realm is shared	PermissionClientTest.java:328	LOW	✓ Real robustness issue if the realm lifecycle ever changes
Policy cleanup callback dereferences `findByName(name).getId()` without a null guard — the teardown lambda NPEs and masks the root cause if creation failed	AbstractPermissionTest.java:112	LOW	✓ Real: cleanup is not idempotent and can mask test failures

Honest Analysis

FriendlyReviewer found 2 of the 3 golden comments of this PR. The two found goldens are the most severe ones. On the V1-only cleanup listener (AdminPermissions.java L81), the review covers both failure modes — orphaned FGAP resources under V2-only, and an UnsupportedOperationException thrown inside the event listener when V1 and V2 coexist — and names the fix direction (derive the guard from the same feature-flag decision as management()). On getClientsWithPermission (L135), the coverage matches the reference comment: the missing resource type on per-client resources, the literal "Clients" string returned as a client id, and the resulting NPE in AvailableRoleMappingResource are all identified.

The missed golden is narrow but real. hasPermission(ClientModel, scope) resolves resources with findByName(server, client.getId(), server.getId()) while per-client resources are created with owner = resourceServer.getClientId(), so client-specific permissions are always bypassed in favor of the all-clients fallback. The review found the sibling defect in the same class — getClientsWithPermission's findByType with a null type — but did not generalize the root cause (per-client resources carry mismatched lookup attributes) to hasPermission. The miss sits on the same file, the same feature and the same underlying flaw: it is a generalization gap rather than a total blind spot, though the two lookups mismatch different attributes (type vs owner), so it is not a trivial carry-over.

The seven supplementary findings include one additional reachable production bug of golden caliber: canExchangeTo (and isPermissionsEnabled/setPermissionsEnabled) throw UnsupportedOperationException, turning client-to-client token exchange into an HTTP 500 in V2-enabled realms instead of a clean 403 denial. The rest is one test-design issue (fixed-position indexing that tests the wrong call before the grant) and five low-severity test/cosmetic items (unused helper, inconsistent static/instance split, missing role cleanup, missing null guard, trailing whitespace). The supplementary set is honest: the most damaging finding outranks several goldens, and the lows are genuinely minor.

Overall verdict: two severe defects confirmed at depth, one missed by a narrow margin, and a bonus finding of the same severity class as the goldens. The review's blocking stance is justified by the token-exchange 500 alone; the miss is a judgment call on how far to push a root cause the review had already identified in one of its own findings.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech	🌤️ Feat
3 high, 1 medium, 5 low	1 mitigated

This MR adds the Client resource type and its scopes to the authorization schema (ClientsPermissionsV2). While the schema and scope definitions are consistent with the existing V2 (Users) pattern and the new tests align with the evaluator contracts, several reachable production paths break when the V2 feature is enabled: canExchangeTo throws UnsupportedOperationException (500 instead of denial), getClientsWithPermission returns the wrong set (leading to NPE for all-clients), and the removal-cleanup listener is gated on V1 only, leaving orphaned resources and throwing when V1 and V2 coexist. The test suite also has several robustness issues (fixed-index scope access, unused helper, missing cleanup, missing null guard). These blocking issues require changes before merge.

These might need a close look

🔴 services/.../admin/permissions/ClientPermissionsV2.java (L148)
UnsupportedOperationException on canExchangeTo breaks token exchange (500 instead of denial) when V2 is enabled. This method is reachable from the standard client-to-client token-exchange flow (AbstractTokenExchangeProvider.exchangeClientToClient), and because AdminPermissions.management(...) selects MgmtPermissionsV2 purely on ADMIN_FINE_GRAINED_AUTHZ_V2, any such exchange while V2 is enabled throws instead of returning a clean 403 denial. The same applies to isPermissionsEnabled (line 188) and setPermissionsEnabled (line 192), which are reached from ClientResource when V1 and V2 are both enabled. At minimum, canExchangeTo should return false rather than throw, or callers must be gated on the V1 flag.
🔴 services/.../admin/permissions/ClientPermissionsV2.java (L135)
getClientsWithPermission returns the wrong set: per-client resources created for the CLIENTS resource type via AdminPermissionsSchema.getOrCreateResource are never assigned a resource type (type is null), so findByType(server, "Clients", ...) only matches the all-clients resource (name/type = "Clients") and not per-client resources. This breaks fine-grained available role mappings (returns empty for per-client grants, or returns the literal string "Clients" used as a client id, causing NPE). Either create per-client resources with type=CLIENTS_RESOURCE_TYPE or change the discovery logic.
🔴 services/.../admin/permissions/AdminPermissions.java (L81)
Removal-cleanup listener is gated on V1 only (Profile.ADMIN_FINE_GRAINED_AUTHZ), producing two problems: (1) With only V2 enabled, role/client/group removal events never trigger cleanup, leaving FGAP resources orphaned; (2) With V1 and V2 both enabled, the guard is true but management(...) selects MgmtPermissionsV2, whose setPermissionsEnabled methods throw UnsupportedOperationException, so removing any role/client/group throws inside the event listener. Guard should be derived from the same feature-flag decision used by management(...) and the V2 path must perform its own cleanup.

Worth checking

🟡 tests/.../fgap/PermissionClientTest.java (L148)
Indexing into default client scopes by fixed position is brittle and the "before" block tests the wrong call. In testManageOnlyOneClient, the try block calls getDefaultClientScopes().get(1) but getDefaultClientScopes() already throws ForbiddenException, so removeDefaultClientScope is never exercised before grant. After grant, fixed-index access (.get(1)/.get(0)) assumes at least two default scopes, risking IndexOutOfBoundsException on fixture changes. Assert size() first or pick a scope by name.

Small things (take or leave)

🔵 server-spi-private/.../AdminPermissionsSchema.java (L53)
Minor: trailing whitespace added after the class declaration, before USERS_RESOURCE_TYPE. Cosmetic noise; consider trimming.
🔵 tests/.../fgap/AbstractPermissionTest.java (L120)
Unused helper createClientPolicy added but never called by any target test class (only createUserPolicy is used). Either wire it in or remove to avoid dead code in the shared base class.
🔵 tests/.../fgap/AbstractPermissionTest.java (L146)
Inconsistent static vs. instance helper design after refactor: most helpers are static taking an explicit ManagedClient, but getPolicies() and the two createPermission(ManagedClient, resourceId, ...) overloads remain instance methods depending on the injected client field. Since these use no instance state, making them static would align with siblings and remove the confusing API split.
🔵 tests/.../fgap/PermissionClientTest.java (L328)
Test-created client roles (myclient-role, myclient-subRole) are not cleaned up within the test method. This only passes if the framework recreates the realm per test method; if the realm is shared, re-running fails with duplicate-role 409s. Add a realm.cleanup() for these roles to make the test robust and symmetric with the rest of the suite.
🔵 tests/.../fgap/AbstractPermissionTest.java (L112)
Policy cleanup callback dereferences findByName(name).getId() without a null guard. If a test deletes the policy before teardown (or creation failed), the teardown lambda NPEs and masks the root cause. Add a null guard to make cleanup idempotent.

✅ Feature-level checklist

Partially covered

⚠️ Add Client resource type and its scopes to authorization schema (ClientsPermissionsV2).
The schema and scope definitions are added and consistent with the V2 (Users) pattern, but the V2 evaluation has blocking defects: canExchangeTo throws instead of denying (500), getClientsWithPermission returns the wrong set leading to NPE, and the removal-cleanup listener is V1-only, leaving orphaned resources or throwing when V1 and V2 coexist. These prevent the feature from fully meeting the requirement.

Add Client resource type and scopes to authorization schema