Fix concurrent group access to prevent NullPointerException

[malinosqui]

Here is a precise description for the pull request based on the provided code changes:

Summary
This pull request addresses a stability issue where concurrent operations on groups (specifically reading groups while they are being deleted) could result in a NullPointerException. It also includes minor code cleanup and introduces a concurrency test to prevent future regressions.

Key Changes

• Concurrency Fix: Modified the Infinispan GroupAdapter to safely handle the retrieval of subgroup counts. It now explicitly checks if the underlying group model exists before attempting to fetch its count, preventing a NullPointerException if the group was concurrently deleted.
• Code Cleanup:
  • Removed the unused private method groupMatchesSearchOrIsPathElement from GroupUtils to clean up dead code.
  • Added a missing @Override annotation to getRealm() in CachedGroup for better code hygiene.
• Testing: Introduced a new concurrency test (createMultiDeleteMultiReadMulti) in GroupTest. This test simulates a high-load scenario by creating 100 groups, and then continuously reading the group list in a background thread while the main thread deletes them, verifying that no exceptions are thrown during the process.

[malinosqui]

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration

Interacting with Kody

• Request a Review: Ask Kody to review your PR manually by adding a comment with the @kody start-review command at the root of your PR.

• Validate Business Logic: Ask Kody to validate your code against business rules by adding a comment with the @kody -v business-logic command.

• Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration

Review Options

The following review options are enabled or disabled:

Options	Enabled
Bug	✅
Performance	✅
Security	✅
Business Logic	✅

Access your configuration settings here.

​

[malinosqui]

NullPointerException vulnerability in the getSubGroupsStream methods arises from modelSupplier.get() returning null during concurrent group deletions. Assign the result to a variable and return Stream.empty() if null, mirroring the getSubGroupsCount implementation.

@Override
public Stream<GroupModel> getSubGroupsStream(String search, Integer firstResult, Integer maxResults) {
    if (isUpdated()) return updated.getSubGroupsStream(search, firstResult, maxResults);
    GroupModel model = modelSupplier.get();
    return model == null ? Stream.empty() : model.getSubGroupsStream(search, firstResult, maxResults);
}

@Override
public Stream<GroupModel> getSubGroupsStream(Integer firstResult, Integer maxResults) {
    if (isUpdated()) return updated.getSubGroupsStream(firstResult, maxResults);
    GroupModel model = modelSupplier.get();
    return model == null ? Stream.empty() : model.getSubGroupsStream(firstResult, maxResults);
}

@Override
public Stream<GroupModel> getSubGroupsStream(String search, Boolean exact, Integer firstResult, Integer maxResults) {
    if (isUpdated()) return updated.getSubGroupsStream(search, exact, firstResult, maxResults);
    GroupModel model = modelSupplier.get();
    return model == null ? Stream.empty() : model.getSubGroupsStream(search, exact, firstResult, maxResults);
}

Prompt for LLM

File model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java:

Line 271:

WHAT: The same NullPointerException vulnerability fixed in `getSubGroupsCount` still exists in the three `getSubGroupsStream` methods directly above it. WHY: If a group is concurrently deleted, `modelSupplier.get()` will return null, causing an NPE when chained with `.getSubGroupsStream(...)`. HOW: Assign `modelSupplier.get()` to a variable and return `Stream.empty()` if it is null, similar to the fix applied to `getSubGroupsCount`.

Suggested Code:

    @Override
    public Stream<GroupModel> getSubGroupsStream(String search, Integer firstResult, Integer maxResults) {
        if (isUpdated()) return updated.getSubGroupsStream(search, firstResult, maxResults);
        GroupModel model = modelSupplier.get();
        return model == null ? Stream.empty() : model.getSubGroupsStream(search, firstResult, maxResults);
    }

    @Override
    public Stream<GroupModel> getSubGroupsStream(Integer firstResult, Integer maxResults) {
        if (isUpdated()) return updated.getSubGroupsStream(firstResult, maxResults);
        GroupModel model = modelSupplier.get();
        return model == null ? Stream.empty() : model.getSubGroupsStream(firstResult, maxResults);
    }

    @Override
    public Stream<GroupModel> getSubGroupsStream(String search, Boolean exact, Integer firstResult, Integer maxResults) {
        if (isUpdated()) return updated.getSubGroupsStream(search, exact, firstResult, maxResults);
        GroupModel model = modelSupplier.get();
        return model == null ? Stream.empty() : model.getSubGroupsStream(search, exact, firstResult, maxResults);
    }

_{Talk to Kody by mentioning @kody}

_{Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.}

​

​