Keycloak/PR #5

Add Client resource type and scopes to authorization schema

keycloak/keycloak#36880
Golden recall: 33.3% Extra findings: 2

Golden Comments (1/3 found)

#CommentSeverityStatusFriendlyReviewer Detail
#1Inconsistent feature flag bug causing orphaned permissions — event listener cleanup is gated by ADMIN_FINE_GRAINED_AUTHZ (V1) instead of V2. If V2 is enabled but V1 is not, cleanup never runs HIGH✓ Found FR: "Event listener cleanup is gated only for V1; V2 lacks lifecycle management for removed clients/roles/groups." Exactly identified.
#2hasPermission(ClientModel client, String scope) uses findByName(server, client.getId(), server.getId()) but getOrCreateResource creates resources with resourceServer.getClientId() — lookup never finds per-client resources HIGH✗ Missed FR did not identify that findByName uses the wrong parameter compared to resource creation.
#3getClientsWithPermission(String scope) iterates resourceStore.findByType(server, CLIENTS_RESOURCE_TYPE) and returns resource.getName() — but per-client resources have no type, so only the type-level "Clients" resource is found HIGH✗ Missed FR did not detect that resourceStore.findByType misses per-client resources (no type), making the method inoperative.

Supplementary Findings (8 findings)

FindingFileSeverityLegitimate?
Potential NullPointerException when getResourceTypeResource returns null — findByResource crashesClientPermissionsV2.java:203HIGH✓ Real bug
Assertion assertThat(existing, nullValue()) expects null but findAll() returns a List (empty) — will fail at runtimeUserResourceTypePermissionTest.java:132MEDIUM✓ Test bug
Unused static import TOKEN_EXCHANGEClientPermissionsV2.java:1LOW✓ Dead code
Unused loggerClientPermissionsV2.java:34LOW✓ Cleanup
Dead method getEvaluationContext not usedClientPermissionsV2.java:249LOW✓ Dead code
Fragile test: indexed access getDefaultClientScopes().get(1) assumes at least 2 scopesPermissionClientTest.java:159LOW✓ Fragile
Test verifies success by absence of exception — missing explicit assertionsPermissionClientTest.java:347LOW✓ Incomplete test
getPolicies() is non-static while other methods are staticAbstractPermissionTest.java:73LOW✓ Inconsistency

Honest Analysis

FriendlyReviewer found 1 of 3 golden bugs (the V1 vs V2 feature flag issue, #1) but missed the two resource lookup bugs (#2 and #3) which are subtler problems requiring understanding of how the authorization store creates versus looks up per-client resources.

The 8 supplementary findings include a HIGH-severity NPE when getResourceTypeResource returns null, and a test bug where assertThat(existing, nullValue()) will fail at runtime because findAll() returns a List (empty), not null.

The remaining 6 findings are mostly LOW-severity: dead code, unused imports, fragile tests. This is a noticeable drop in finding quality compared to other PRs — more noise and less deep analysis.

FriendlyReviewer's full review

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

friendly-reviewerbotcommented now

Hi there 👋

🌥️ Tech
2 high, 1 medium, 6 low

The MR introduces the Client resource type to the FGAP V2 authorization schema, including new ClientPermissionsV2 implementation, scopes, and integration tests. The overall design is sound, but several issues require attention: a potential NullPointerException in permission checks, missing cleanup event handling for V2 resources, a test assertion bug that expects null instead of an empty list, a fragile test relying on index-based access, and minor dead code/unused imports. Addressing these will improve correctness, reliability, and maintainability.

These might need a close look

  • 🔴 services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (L203)
    Potential NullPointerException when getResourceTypeResource returns null, leading to NPE in findByResource.
  • 🔴 services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java (L74)
    Event listener cleanup is gated only for V1; V2 lacks lifecycle management for removed clients/roles/groups.

Worth checking

  • 🟡 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authz/fgap/UserResourceTypePermissionTest.java (L132)
    Assertion bug: assertThat(existing, nullValue()) expects null, but findAll() returns an empty List, not null. This assertion will fail at runtime. Change to assertThat(existing, empty()).

Small things (take or leave)

  • 🔵 services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (L1)
    Unused static import: TOKEN_EXCHANGE. Remove it.
  • 🔵 services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (L34)
    Unused logger. Remove it.
  • 🔵 services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (L249)
    Dead code: getEvaluationContext method is defined but never called anywhere in the codebase. Remove it.
  • 🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authz/fgap/PermissionClientTest.java (L159)
    Fragile test: getDefaultClientScopes().get(1) assumes at least 2 default client scopes exist. Use a named/descriptive check instead.
  • 🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authz/fgap/PermissionClientTest.java (L347)
    Test only checks that no exception is thrown, without asserting the expected outcome. Add explicit assertions on the response.
  • 🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authz/fgap/AbstractPermissionTest.java (L73)
    Inconsistency: getPolicies() is non-static while all other utility methods in this class are static. Make it static for consistency.