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
akshayutture-augment 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();
Copy link

@greptile-apps greptile-apps bot Nov 14, 2025

Choose a reason for hiding this comment

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

logic: Violates interface contract - GroupModel.getSubGroupsCount() javadoc states "Never returns {@code null}". Should return 0L instead.

Suggested change
return model == null ? null : model.getSubGroupsCount();
return model == null ? 0L : model.getSubGroupsCount();
Prompt To Fix With AI 
This is a comment left during a code review.
Path: model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java
Line: 275:275

Comment:
**logic:** Violates interface contract - `GroupModel.getSubGroupsCount()` javadoc states "Never returns {@code null}". Should return `0L` instead.

```suggestion
        return model == null ? 0L : model.getSubGroupsCount();
```

How can I resolve this? If you propose a fix, please make it concise.

}

@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:[email protected]">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();
Comment on lines +139 to +149
Copy link

@greptile-apps greptile-apps bot Nov 14, 2025

Choose a reason for hiding this comment

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

logic: Race condition - thread may not start before groups are deleted. Add a small delay or synchronization to ensure the reader thread is actually running before deletion starts.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java
Line: 139:149

Comment:
**logic:** Race condition - thread may not start before groups are deleted. Add a small delay or synchronization to ensure the reader thread is actually running before deletion starts.

How can I resolve this? If you propose a fix, please make it concise.


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

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

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