Skip to content

Fix concurrent group access to prevent NullPointerException #1

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 #1

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();
}

@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);
Comment on lines +139 to +155

@codeant-ai codeant-ai Bot Feb 13, 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.

Suggestion: The concurrency test starts a background thread to read groups but never joins it, so the test can complete (and assert that no exceptions occurred) before the reader thread actually runs or finishes, making the test flaky and potentially missing concurrent-access failures it is supposed to detect. [logic error]

Severity Level: Major ⚠️ 
- ⚠️ Concurrency regression test may miss group cache failures.
- ⚠️ CI may report green despite existing concurrent group bug.
Suggested change
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);
Thread reader = 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);
}
}
});
reader.start();
// delete groups
groupUuuids.forEach(groupUuid -> {
managedRealm.admin().groups().group(groupUuid).remove();
});
deletedAll.set(true);
try {
reader.join();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
fail("Reader thread was interrupted");
}
Steps of Reproduction ✅ 
1. Run the JUnit test method `createMultiDeleteMultiReadMulti()` in
`tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java:119-158` (e.g.,
via `mvn test
-Dtest=org.keycloak.tests.admin.group.GroupTest#createMultiDeleteMultiReadMulti`).

2. The test creates and deletes many groups on the main thread at `GroupTest.java:151-154`
while starting a background reader thread at `GroupTest.java:139-149` that repeatedly
calls `managedRealm.admin().groups().groups(...)`.

3. The main thread sets `deletedAll` to `true` at `GroupTest.java:155` and immediately
asserts `caughtExceptions` is empty at `GroupTest.java:157` without joining the reader
thread, so the assertion can run before the reader thread has executed or finished its
loop.

4. If the reader thread encounters a concurrent-access bug and catches an exception later
in its loop at `GroupTest.java:140-147`, it adds it to `caughtExceptions` after the
assertion has already passed, causing the test to report success even though concurrent
group reads are failing.
Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java
**Line:** 139:155
**Comment:**
	*Logic Error: The concurrency test starts a background thread to read groups but never joins it, so the test can complete (and assert that no exceptions occurred) before the reader thread actually runs or finishes, making the test flaky and potentially missing concurrent-access failures it is supposed to detect.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎


assertThat(caughtExceptions, Matchers.empty());
}

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