Skip to content

Fix concurrent group access to prevent NullPointerException #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ShashankFC wants to merge 1 commit into
base: feature-group-concurrency-update
Choose a base branch
from
Open

Fix concurrent group access to prevent NullPointerException #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,8 @@ public Stream<GroupModel> getSubGroupsStream(String search, Boolean exact, Integ
@Override
public Long getSubGroupsCount() {
if (isUpdated()) return updated.getSubGroupsCount();
return getGroupModel().getSubGroupsCount();
GroupModel model = modelSupplier.get();
return model == null ? null : model.getSubGroupsCount();
Comment on lines +274 to +275

@coderabbitai coderabbitai Bot Jan 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Null-safety fix is correct, but other similar methods lack the same protection.

The null check here correctly prevents NPE when the group is deleted concurrently. However, the getSubGroupsStream overloads at lines 256, 262, and 268 call modelSupplier.get() directly without null checks:

return modelSupplier.get().getSubGroupsStream(search, firstResult, maxResults);

These would still throw NPE during concurrent deletion. Consider applying the same null-safe pattern consistently.

🔧 Suggested fix for consistency 
 @Override
 public Stream<GroupModel> getSubGroupsStream(String search, Integer firstResult, Integer maxResults) {
     if (isUpdated()) return updated.getSubGroupsStream(search, firstResult, maxResults);
-    return modelSupplier.get().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);
-    return modelSupplier.get().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);
-    return modelSupplier.get().getSubGroupsStream(search, exact, firstResult, maxResults);
+    GroupModel model = modelSupplier.get();
+    return model == null ? Stream.empty() : model.getSubGroupsStream(search, exact, firstResult, maxResults);
 }
🤖 Prompt for AI Agents 
In
@model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java
around lines 274 - 275, In GroupAdapter, make the getSubGroupsStream overloads
null-safe like getSubGroupsCount: call modelSupplier.get() into a local
GroupModel variable, check for null, and if null return an appropriate empty
result (e.g., Stream.empty()) instead of invoking methods on a null reference;
otherwise delegate to model.getSubGroupsStream(...). Ensure you update all three
overloads that currently call modelSupplier.get().getSubGroupsStream(...) so
they use this pattern.

}

@Override
Expand Down
1 change: 1 addition & 0 deletions ...l/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public CachedGroup(Long revision, RealmModel realm, GroupModel group) {
this.type = group.getType();
}

@Override
public String getRealm() {
return realm;
}
Expand Down
10 changes: 0 additions & 10 deletions services/src/main/java/org/keycloak/utils/GroupUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,4 @@ public static GroupRepresentation toRepresentation(GroupPermissionEvaluator grou
rep.setAccess(groupsEvaluator.getAccess(groupTree));
return rep;
}

private static boolean groupMatchesSearchOrIsPathElement(GroupModel group, String search) {
if (StringUtil.isBlank(search)) {
return true;
}
if (group.getName().contains(search)) {
return true;
}
return group.getSubGroupsStream().findAny().isPresent();
}
}
48 changes: 48 additions & 0 deletions tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.CloseableHttpClient;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.keycloak.admin.client.Keycloak;
Expand Down Expand Up @@ -76,6 +77,9 @@
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.stream.IntStream;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.anEmptyMap;
Expand All @@ -90,6 +94,7 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

/**
* @author <a href="mailto:mstrukel@redhat.com">Marko Strukelj</a>
Expand All @@ -109,6 +114,49 @@ public class GroupTest extends AbstractGroupTest {
@InjectHttpClient
CloseableHttpClient httpClient;


@Test
public void createMultiDeleteMultiReadMulti() {
// create multiple groups
List<String> groupUuuids = new ArrayList<>();
IntStream.range(0, 100).forEach(groupIndex -> {
GroupRepresentation group = new GroupRepresentation();
group.setName("Test Group " + groupIndex);
try (Response response = managedRealm.admin().groups().add(group)) {
boolean created = response.getStatusInfo().getFamily() == Response.Status.Family.SUCCESSFUL;
if (created) {
final String groupUuid = ApiUtil.getCreatedId(response);
groupUuuids.add(groupUuid);
} else {
fail("Failed to create group: " + response.getStatusInfo().getReasonPhrase());
}
}
});

AtomicBoolean deletedAll = new AtomicBoolean(false);
List<Exception> caughtExceptions = new CopyOnWriteArrayList<>();
// read groups in a separate thread
new Thread(() -> {
while (!deletedAll.get()) {
try {
// just loading briefs
managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true);
} catch (Exception e) {

caughtExceptions.add(e);
}
}
}).start();

// delete groups
groupUuuids.forEach(groupUuid -> {
managedRealm.admin().groups().group(groupUuid).remove();
});
deletedAll.set(true);

assertThat(caughtExceptions, Matchers.empty());
}
Comment on lines +136 to +158

@coderabbitai coderabbitai Bot Jan 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Race condition: reader thread is not joined before assertion.

The test sets deletedAll.set(true) and immediately asserts on caughtExceptions, but the reader thread may still be executing. This can cause flaky results or miss exceptions from the final loop iteration.

🔧 Proposed fix 
+        Thread readerThread = new Thread(() -> {
-        new Thread(() -> {
             while (!deletedAll.get()) {
                 try {
                     // just loading briefs
                     managedRealm.admin().groups().groups(null, 0, Integer.MAX_VALUE, true);
                 } catch (Exception e) {
                     caughtExceptions.add(e);
                 }
             }
-        }).start();
+        });
+        readerThread.start();

         // delete groups
         groupUuuids.forEach(groupUuid -> {
             managedRealm.admin().groups().group(groupUuid).remove();
         });
         deletedAll.set(true);

+        try {
+            readerThread.join(5000); // Wait up to 5 seconds for reader to finish
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            fail("Reader thread interrupted");
+        }
+
         assertThat(caughtExceptions, Matchers.empty());
🤖 Prompt for AI Agents 
In @tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java
around lines 136 - 158, The reader thread created for calling
managedRealm.admin().groups().groups(...) must be joined before asserting on
caughtExceptions to avoid the race; change the anonymous new Thread(...) to
assign it to a variable (e.g., readerThread), start it, and after
deletedAll.set(true) call readerThread.join() (optionally with a timeout) before
assertThat(caughtExceptions, Matchers.empty()); this ensures the reader loop has
finished and any exceptions have been recorded.


// KEYCLOAK-2716 Can't delete client if its role is assigned to a group
@Test
public void testClientRemoveWithClientRoleGroupMapping() {
Expand Down