Skip to content
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

Improve pagination for scim2/groups API #620

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+137 −32
Open

Improve pagination for scim2/groups API #620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c78665
Improve pagination for scim2/groups API
sadilchamishka Mar 5, 2025
ab35043
Add unit tests for group pagination
sadilchamishka Mar 5, 2025
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
92 changes: 61 additions & 31 deletions ...cim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1406,9 +1406,9 @@ private void handleAndThrowClientExceptionForActionFailure(UserStoreException e)
private int handleLimitEqualsNULL(Integer limit) {

// Limit equal to null implies return all users. Return all users scenario handled by the following methods by
// expecting count as zero.
// expecting count as integer max value.
if (limit == null) {
limit = 0;
limit = Integer.MAX_VALUE;
}
return limit;
}
Expand Down Expand Up @@ -3051,8 +3051,6 @@ public GroupsGetResponse listGroupsWithGET(Node rootNode, Integer startIndex, In
startIndex = handleStartIndexEqualsNULL(startIndex);
if (sortBy != null || sortOrder != null) {
throw new NotImplementedException("Sorting is not supported");
} else if (startIndex != 1 && count != null) {
throw new NotImplementedException("Pagination is not supported");
} else if (rootNode != null) {
return filterGroups(rootNode, startIndex, count, sortBy, sortOrder, domainName, requiredAttributes);
} else {
Expand Down Expand Up @@ -3096,6 +3094,7 @@ private GroupsGetResponse listGroups(int startIndex, Integer count, String sortB

GroupsGetResponse groupsResponse = new GroupsGetResponse(0, Collections.emptyList());
List<Group> groupList = new ArrayList<>();
int totalGroupCount;
try {
Set<String> groupNames;
if (carbonUM.isRoleAndGroupSeparationEnabled()) {
Expand All @@ -3104,6 +3103,25 @@ private GroupsGetResponse listGroups(int startIndex, Integer count, String sortB
groupNames = getRoleNamesForGroupsEndpoint(domainName);
}

totalGroupCount = groupNames.size();
// Adjust startIndex and endIndex to ensure they are within bounds
if (startIndex > 0) {
if (startIndex > totalGroupCount) {
startIndex = totalGroupCount;
} else {
startIndex -= 1;
}
} else {
startIndex = 0;
}
int endIndex;
if (count == null) {
endIndex = totalGroupCount;
} else {
endIndex = Math.min(startIndex + count, groupNames.size());
}
groupNames = new HashSet<>(new ArrayList<>(groupNames).subList(startIndex, endIndex));

for (String groupName : groupNames) {
String userStoreDomainName = IdentityUtil.extractDomainFromName(groupName);
if (isInternalOrApplicationGroup(userStoreDomainName) || isSCIMEnabled(userStoreDomainName)) {
Expand Down Expand Up @@ -3154,7 +3172,7 @@ private GroupsGetResponse listGroups(int startIndex, Integer count, String sortB
} catch (IdentitySCIMException | BadRequestException e) {
throw new CharonException("Error in retrieving SCIM Group information from database.", e);
}
groupsResponse.setTotalGroups(groupList.size());
groupsResponse.setTotalGroups(totalGroupCount);
groupsResponse.setGroups(groupList);
return groupsResponse;
}
Expand Down Expand Up @@ -3222,19 +3240,15 @@ private Set<String> getRoleNamesForGroupsEndpoint(String domainName)
private Set<String> getGroupNamesForGroupsEndpoint(String domainName)
throws UserStoreException, IdentitySCIMException {

if (StringUtils.isEmpty(domainName)) {
Set<String> groupsList = new HashSet<>(Arrays.asList(carbonUM.getRoleNames()));
// Remove roles.
groupsList.removeIf(SCIMCommonUtils::isHybridRole);
return groupsList;
} else {
String searchValue = SCIMCommonConstants.ANY;
if (StringUtils.isNotEmpty(domainName)) {
// If the domain is specified create a attribute value with the domain name.
String searchValue = domainName + CarbonConstants.DOMAIN_SEPARATOR + SCIMCommonConstants.ANY;
// Retrieve roles using the above attribute value.
List<String> roleList = Arrays
.asList(carbonUM.getRoleNames(searchValue, MAX_ITEM_LIMIT_UNLIMITED, true, true, true));
return new HashSet<>(roleList);
searchValue = domainName + CarbonConstants.DOMAIN_SEPARATOR + SCIMCommonConstants.ANY;
}
// Retrieve roles using the above attribute value.
List<String> roleList = Arrays
.asList(carbonUM.getRoleNames(searchValue, MAX_ITEM_LIMIT_UNLIMITED, true, true, true));
return new HashSet<>(roleList);
}

/**
Expand Down Expand Up @@ -3311,22 +3325,38 @@ private GroupsGetResponse filterGroupsBySingleAttribute(ExpressionNode node, int
groupsList.removeIf(SCIMCommonUtils::isHybridRole);
}

if (groupsList != null) {
for (String groupName : groupsList) {
if (groupName != null && carbonUM.isExistingRole(groupName, false)) {
// Skip internal roles.
if (CarbonConstants.REGISTRY_ANONNYMOUS_ROLE_NAME.equals(groupName) || UserCoreUtil
.isEveryoneRole(groupName, carbonUM.getRealmConfiguration())) {
continue;
}
Group group = getRoleWithDefaultAttributes(groupName, requiredAttributes);
if (group != null && group.getId() != null) {
filteredGroups.add(group);
}
} else {
// Returning null will send a resource not found error to client by Charon.
return new GroupsGetResponse(0, null);
int totalGroupCount = groupsList.size();
// Adjust startIndex and endIndex to ensure they are within bounds
if (startIndex > 0) {
if (startIndex > totalGroupCount) {
startIndex = totalGroupCount;
} else {
startIndex -= 1;
}
} else {
startIndex = 0;
}
int endIndex;
if (count < 0) {
count = 0;
}
endIndex = Math.min(startIndex + count, groupsList.size());
groupsList = groupsList.subList(startIndex, endIndex);

for (String groupName : groupsList) {
if (groupName != null && carbonUM.isExistingRole(groupName, false)) {
// Skip internal roles.
if (CarbonConstants.REGISTRY_ANONNYMOUS_ROLE_NAME.equals(groupName) || UserCoreUtil
.isEveryoneRole(groupName, carbonUM.getRealmConfiguration())) {
continue;
}
Group group = getRoleWithDefaultAttributes(groupName, requiredAttributes);
if (group != null && group.getId() != null) {
filteredGroups.add(group);
}
} else {
// Returning null will send a resource not found error to client by Charon.
return new GroupsGetResponse(0, null);
}
}
} catch (org.wso2.carbon.user.core.UserStoreException e) {
Expand Down
77 changes: 76 additions & 1 deletion ....common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,22 @@ public Object[][] groupNameWithFilters() throws Exception {
};
}

@DataProvider(name = "groupPagination")
public Object[][] groupWithPagination() throws Exception {

return new Object[][]{
// start Index = 1, count = 2, results = 2, total = 6
{1, 2, 2, 6},
// start Index = 2, count = not specified, results = 5, total = 6
{2, null, 5, 6},
// start Index = not specified, count = not specified, results = 6, total = 6
{null, null, 6, 6},
// start Index = 7, count = 1, results = 0, total = 6
{7, 1, 0, 6}

};
}

@Test(dataProvider = "groupNameWithFilters")
public void testListGroupsWithFilter(String filter, String roleName, String userStoreDomain) throws Exception {

Expand Down Expand Up @@ -500,7 +516,31 @@ public void testListGroupsWithFilter(String filter, String roleName, String user
null, requiredAttributes);

assertEquals(groupsResponse.getGroups().size(), 1);
}

@Test(dataProvider = "groupPagination")
public void testListGroups(Integer startIndex, Integer count, Integer results, Integer totalResult)
throws Exception {

String[] groups = new String[]{"group1", "group2", "group3", "group4", "group5", "group6"};
mockedUserStoreManager = mock(AbstractUserStoreManager.class);
when(mockedUserStoreManager.isRoleAndGroupSeparationEnabled()).thenReturn(true);
when(mockedUserStoreManager.getRoleNames(anyString(), anyInt(), anyBoolean(), anyBoolean(), anyBoolean()))
.thenReturn(groups);
when(mockedUserStoreManager.getSecondaryUserStoreManager(anyString())).thenReturn(mockedUserStoreManager);
when(mockedUserStoreManager.isSCIMEnabled()).thenReturn(true);

when(mockIdentityUtil.extractDomainFromName(anyString())).thenReturn("PRIMARY");
for (String group : groups) {
when(mockedUserStoreManager.getGroupByGroupName(group, null)).
thenReturn(buildUserCoreGroupResponse(group, "123456789", null));
}
SCIMUserManager scimUserManager = new SCIMUserManager(mockedUserStoreManager, mockedClaimManager);
GroupsGetResponse groupsResponse = scimUserManager.listGroupsWithGET(null, startIndex, count, null, null,
null, null);

assertEquals(groupsResponse.getGroups().size(), results);
assertEquals(groupsResponse.getTotalGroups(), totalResult);
}

@Test(dataProvider = "listUser")
Expand Down Expand Up @@ -625,9 +665,44 @@ public void testFilteringUsersWithGET(List<org.wso2.carbon.user.core.common.User
ClaimMapping[] claimMappings = getTestClaimMappings();
when(mockedClaimManager.getAllClaimMappings(anyString())).thenReturn(claimMappings);

String claimDialectUri = SCIMCommonConstants.SCIM_USER_CLAIM_DIALECT;

AttributeMapping givenNameAttributePrimary = new AttributeMapping("PRIMARY", "http://wso2.org/claims/givenname");
AttributeMapping givenNameAttributeSecondary = new AttributeMapping("SECONDARY", "http://wso2.org/claims/givenname");
List<AttributeMapping> givenNameAttributeList = new ArrayList<>();
givenNameAttributeList.add(givenNameAttributePrimary);
givenNameAttributeList.add(givenNameAttributeSecondary);

AttributeMapping emailAttributePrimary = new AttributeMapping("PRIMARY", "http://wso2.org/claims/emailaddress");
AttributeMapping emailAttributeSecondary = new AttributeMapping("SECONDARY", "http://wso2.org/claims/emailaddress");
List<AttributeMapping> emailAttributeList = new ArrayList<>();
emailAttributeList.add(emailAttributePrimary);
emailAttributeList.add(emailAttributeSecondary);

Map<String, String> supportedByDefaultProperties = new HashMap<String, String>() {{
put("SupportedByDefault", "true");
put("ReadOnly", "true");
}};

List<LocalClaim> localClaimList = new ArrayList<LocalClaim>() {{
add(new LocalClaim(GIVEN_NAME_LOCAL_CLAIM, givenNameAttributeList, supportedByDefaultProperties));
add(new LocalClaim(EMAIL_ADDRESS_LOCAL_CLAIM, emailAttributeList, supportedByDefaultProperties));
}};

List<ExternalClaim> externalClaimList = new ArrayList<ExternalClaim>() {{
add(new ExternalClaim(claimDialectUri, claimDialectUri + ":name.givenName", GIVEN_NAME_LOCAL_CLAIM));
add(new ExternalClaim(claimDialectUri, claimDialectUri + ":emails", EMAIL_ADDRESS_LOCAL_CLAIM));
}};

when(mockClaimMetadataManagementService.getLocalClaims(anyString())).thenReturn(localClaimList);
when(mockClaimMetadataManagementService.getExternalClaims(anyString(), anyString()))
.thenReturn(externalClaimList);

HashMap<String, Boolean> requiredClaimsMap = new HashMap<>();
requiredClaimsMap.put("urn:ietf:params:scim:schemas:core:2.0:User:userName", false);
SCIMUserManager scimUserManager = new SCIMUserManager(mockedUserStoreManager, mockedClaimManager);

SCIMUserManager scimUserManager = new SCIMUserManager(mockedUserStoreManager,
mockClaimMetadataManagementService, "carbon.super");

Node node = null;
if (StringUtils.isNotBlank(filter)) {
Expand Down