Keycloak / PR #30

Fix concurrent group access to prevent NullPointerException

keycloak/keycloak#40940
Golden recall: 100% Extra findings: 6

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Returning null from getSubGroupsCount() violates the GroupModel contract (Javadoc says it never returns null) and may lead to NPEs in callers that expect a non-null count. HIGH ✓ Found "getSubGroupsCount() now returns null when the model is null, violating the GroupModel contract ('Never returns null'). GroupUtils.populateSubGroupCount passes the null straight into GroupRepresentation, silently dropping the value, and any unboxing caller gets an NPE — return a safe non-null value, e.g. counting from the cached snapshot" (cache/infinispan/GroupAdapter.java L275)
#2 The reader thread isn't waited for; flipping deletedAll to true and asserting immediately can race and miss exceptions added just after the flag change, making this test flaky. MEDIUM ✓ Found "The reader thread is started and caughtExceptions is asserted without ever joining the thread; deletedAll is only checked at the loop top, so an in-flight exception appended just after the flag change is silently missed — a false negative. Join the thread (with a timeout) before the assertion" (admin/group/GroupTest.java L162)

Supplementary Findings (6 findings)

Finding File Severity Legitimate?
The NPE fix is incomplete: only getSubGroupsCount() is guarded — the three getSubGroupsStream(...) overloads (lines 231, 236, 241) still call modelSupplier.get() without a null check and throw the same NPE under the concurrent-deletion race GroupAdapter.java:231 HIGH ✓ Real: the same race still NPEs through the sibling accessors
modelSupplier = new LazyModel<>(this::getGroupModel) caches the resolved model in a plain non-volatile field with no synchronization — a data race that can also cache a null result after concurrent deletion, which the new null-guard depends on GroupAdapter.java:42 MEDIUM ✓ Borderline: adapters are usually created per session, but this is a concurrency-fix PR
The reader spins in a busy-loop with no initial synchronization — nothing guarantees it has actually started or is in-flight when deletion begins, so the test can pass trivially without reproducing the race GroupTest.java:145 MEDIUM ✓ Real: the regression guard can pass without exercising the race
In populateGroupHierarchyFromSubGroups, session.groups().getGroupById(realm, currGroup.getParentId()) can return null if the parent is concurrently deleted, with no null check before groupEvaluator.canView(parentModel) and toRepresentation(...) GroupUtils.java:44 MEDIUM ✓ Borderline: pre-existing and out of this PR's diff, but the same race family
All 100 groups are created as top-level groups, so the while (currGroup.getParentId() != null) parent-chain traversal in GroupUtils.populateGroupHierarchyFromSubGroups is never entered — parent lookups, computeIfAbsent/merge logic are not exercised under concurrent deletion GroupTest.java:124 LOW ✓ Real: the hierarchy-building paths the fix touches are untested
List<String> groupUuuids is misspelled (triple 'u') GroupTest.java:126 LOW ✓ Borderline: cosmetic

Honest Analysis

FriendlyReviewer found both golden bugs. On getSubGroupsCount(), the review frames the fix as trading one NPE for another: the null now violates the GroupModel "never returns null" contract and becomes a downstream unboxing NPE once passed through GroupUtils.populateSubGroupCount into GroupRepresentation, with a concrete alternative (counting from the cached snapshot). On the test, it identifies the unjoined reader thread and the loop-top-only check of deletedAll as the false-negative mechanism.

The strongest supplementary finding is arguably the most important item on the page: the fix is incomplete. Only getSubGroupsCount() is guarded; the three getSubGroupsStream(...) overloads still call modelSupplier.get() unguarded and throw the same NPE under the same race. The test-quality findings reinforce the point — the busy-loop has no synchronization guaranteeing overlap, so the test can pass trivially, and with only top-level groups the parent-chain paths are never exercised. A regression in the sibling accessors would not be caught.

Two caveats. Severity is arguably inverted between the two HIGH items: the incomplete-fix finding (GroupAdapter.java L231) is the actual remaining crash path and could be considered the more severe of the two, while the golden contract violation (L275) is a latent, downstream failure. And two findings are conditional by nature — the LazyModel thread-safety point matters only if adapters outlive their session, and the GroupUtils null-parent path is pre-existing and outside this PR's diff. The "Not there yet" checklist verdict is consistent with the review's own evidence.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
2 high, 4 medium, 2 low 1 issue

This merge request addresses Keycloak #40368, an NPE raised when a group is accessed concurrently with its deletion. The core fix in GroupAdapter.getSubGroupsCount() guards against a null lazily-loaded model, and the associated cache-layer changes (@Override on CachedGroup.getRealm(), thread-safe DefaultLazyLoader) are correct. However, the fix is incomplete: sibling getSubGroupsStream(...) overloads still dereference the model unguarded and can throw the same NPE, and the change to getSubGroupsCount() now violates the GroupModel "never returns null" contract, trading the original NPE for a contract violation and potential downstream unboxing NPE. The new integration test has several weaknesses (reader thread not joined, no synchronization to guarantee overlap, only top-level groups covered) that make it an unreliable regression guard and leave parent-chain deletion races uncovered. Additionally, LazyModel is not thread-safe, which is relevant to the PR's stated purpose. Overall, the MR needs further hardening before merge.

These might need a close look

  • 🔴 org/keycloak/models/cache/infinispan/GroupAdapter.java (L231)
    Incomplete NPE fix: only getSubGroupsCount() is guarded. The three getSubGroupsStream(...) overloads (lines 231, 236, 241) still call modelSupplier.get() without a null check and will throw an NPE under the same concurrent-deletion race. The no-arg getSubGroupsStream() (line ~218) already handles this by iterating cached.getSubGroups(...), which falls back to an empty set. Apply the same pattern or null-guard the overloads consistently.
  • 🔴 org/keycloak/models/cache/infinispan/GroupAdapter.java (L275)
    getSubGroupsCount() now returns null when the model is null, violating the GroupModel contract ("Never returns null"). GroupUtils.populateSubGroupCount passes the null straight into GroupRepresentation, silently dropping the value, and any unboxing caller would get an NPE. Recommend returning a safe non-null value, e.g., counting from the cached snapshot: (long) cached.getSubGroups(keycloakSession, modelSupplier).size().

Worth checking

  • 🟡 org/keycloak/models/cache/infinispan/GroupAdapter.java (L42)
    modelSupplier = new LazyModel<>(this::getGroupModel) uses LazyModel, which caches the resolved model in a plain non-volatile field with no synchronization. This is a data race and can also cache a null result after concurrent deletion, which the new null-guard depends on. While the adapter is typically created per RealmCacheSession, the PR's concurrency-fix intent warrants making LazyModel thread-safe (volatile + double-checked locking) or documenting why it is guaranteed single-threaded.
  • 🟡 org/keycloak/tests/admin/group/GroupTest.java (L162)
    The reader thread is started and the main thread asserts caughtExceptions is empty without ever joining the thread. deletedAll is only checked at the loop top, so an in-flight exception appended after the assertion is silently missed, producing a false negative. Join the thread (with a timeout) before the assertion and handle InterruptedException.
  • 🟡 org/keycloak/tests/admin/group/GroupTest.java (L145)
    The reader spins in a busy-loop with no initial synchronization, so nothing guarantees the reader has actually started or is in-flight when deletion begins. If the reader is slow to schedule, deletions may complete first and the test passes trivially without reproducing the race. Use a CountDownLatch/barrier to ensure overlap, plus a short sleep in the loop to reduce server load.
  • 🟡 org/keycloak/utils/GroupUtils.java (L44)
    In populateGroupHierarchyFromSubGroups, session.groups().getGroupById(realm, currGroup.getParentId()) can return null if the parent is concurrently deleted, and there is no null check before groupEvaluator.canView(parentModel) and toRepresentation(...). This pre-existing path is not changed by this MR and is not covered by the new test. If the concurrency hardening is meant to cover hierarchy building, guard against a null parent.
Small things (take or leave)
  • 🔵 org/keycloak/tests/admin/group/GroupTest.java (L124)
    All 100 groups are created as top-level groups (via managedRealm.admin().groups().add(group)), so the while (currGroup.getParentId() != null) loop in GroupUtils.populateGroupHierarchyFromSubGroups is never entered. The parent-chain traversal, parent lookups, and computeIfAbsent/merge logic are not exercised under concurrent deletion. Consider adding a hierarchy with nested subgroups and having the reader search for subgroups to cover these paths.
  • 🔵 org/keycloak/tests/admin/group/GroupTest.java (L126)
    List<String> groupUuuids is misspelled (triple 'u'). Minor readability fix.

✅ Feature-level checklist

Not there yet

  • ❌ Fix the NPE when accessing a group concurrently (per PR description closing #40368).
    The fix is incomplete: sibling getSubGroupsStream overloads still throw the same NPE, and getSubGroupsCount() now returns null, violating the GroupModel contract and introducing a potential downstream NPE. The new test does not reliably reproduce the race or cover parent-chain deletion paths, so the requirement is not fully satisfied.