From 7282944609c9deb0b54a3eac655d366bcf7aca7a Mon Sep 17 00:00:00 2001 From: sadilchamishka Date: Thu, 7 Nov 2024 08:19:41 +0530 Subject: [PATCH] Improve identity and non-identity claim filtering --- .../scim2/common/impl/SCIMUserManager.java | 117 +++++++++++------- .../common/impl/SCIMUserManagerTest.java | 88 +++++++------ pom.xml | 2 +- 3 files changed, 119 insertions(+), 88 deletions(-) diff --git a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java index d4021373d..09b5155ef 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java +++ b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java @@ -66,6 +66,7 @@ import org.wso2.carbon.user.core.UserStoreManager; import org.wso2.carbon.user.core.claim.ClaimManager; import org.wso2.carbon.user.core.common.AbstractUserStoreManager; +import org.wso2.carbon.user.core.common.PaginatedUserResponse; import org.wso2.carbon.user.core.constants.UserCoreClaimConstants; import org.wso2.carbon.user.core.constants.UserCoreErrorConstants; import org.wso2.carbon.user.core.jdbc.JDBCUserStoreManager; @@ -2110,66 +2111,78 @@ private UsersGetResponse getMultiAttributeFilteredUsers(Node node, Map filteredUsers = new ArrayList<>(); - Set users; + List filteredUserDetails; + int maxLimit = getMaxLimitForTotalResults(domainName); + int totalResults = 0; + PaginatedUserResponse paginatedUserResult; + // Handle pagination. if (limit > 0) { - users = getMultiAttributeFilteredUsersWithMaxLimit(node, offset, sortBy, sortOrder, domainName, limit, + paginatedUserResult = getMultiAttributeFilteredUsersWithMaxLimit(node, offset, sortBy, sortOrder, domainName, limit, true); } else { - int maxLimit = getMaxLimit(domainName); - users = getMultiAttributeFilteredUsersWithMaxLimit(node, offset, sortBy, sortOrder, domainName, maxLimit, + paginatedUserResult = getMultiAttributeFilteredUsersWithMaxLimit(node, offset, sortBy, sortOrder, domainName, maxLimit, false); } - filteredUsers.addAll(getFilteredUserDetails(users, requiredAttributes)); + filteredUserDetails = getFilteredUserDetails(new LinkedHashSet<>(paginatedUserResult.getFilteredUsers()), + requiredAttributes); // Check that total user count matching the client query needs to be calculated. - if (isJDBCUSerStore(domainName) || isAllConfiguredUserStoresJDBC() || - SCIMCommonUtils.isConsiderTotalRecordsForTotalResultOfLDAPEnabled()) { - int maxLimit = getMaxLimitForTotalResults(domainName); - if (!SCIMCommonUtils.isConsiderMaxLimitForTotalResultEnabled()) { - maxLimit = Math.max(maxLimit, limit); + if (paginatedUserResult.getTotalResults() > 0) { + /* If the total result is provided with the paginated user result, give the priority instead of calculating + user total count. */ + totalResults = paginatedUserResult.getTotalResults(); + if (totalResults > maxLimit && SCIMCommonUtils.isConsiderMaxLimitForTotalResultEnabled()) { + totalResults = maxLimit; } - // Get total users based on the filter query. - totalResults += getMultiAttributeFilteredUsersCount(node, 1, domainName, maxLimit); - } else { - totalResults += filteredUsers.size(); - if (totalResults == 0 && filteredUsers.size() > 1) { - totalResults = filteredUsers.size() - 1; + if (isJDBCUSerStore(domainName) || isAllConfiguredUserStoresJDBC() || + SCIMCommonUtils.isConsiderTotalRecordsForTotalResultOfLDAPEnabled()) { + // Get total users based on the filter query. + totalResults += getMultiAttributeFilteredUsersCount(node, 1, domainName, maxLimit); + + } else { + totalResults += paginatedUserResult.getFilteredUsers().size(); + if (totalResults == 0 && paginatedUserResult.getFilteredUsers().size() > 1) { + totalResults = paginatedUserResult.getFilteredUsers().size() - 1; + } } } - return getDetailedUsers(filteredUsers, totalResults); + + return getDetailedUsers(filteredUserDetails, totalResults); } - private Set getMultiAttributeFilteredUsersWithMaxLimit(Node node, int offset, + private PaginatedUserResponse getMultiAttributeFilteredUsersWithMaxLimit(Node node, int offset, String sortBy, String sortOrder, String domainName, int maxLimit, boolean paginationRequested) throws CharonException, BadRequestException { - Set users = new LinkedHashSet<>(); if (StringUtils.isNotEmpty(domainName)) { - users = getFilteredUsersFromMultiAttributeFiltering(node, offset, maxLimit, sortBy, sortOrder, domainName, + return getFilteredUsersFromMultiAttributeFiltering(node, offset, maxLimit, sortBy, sortOrder, domainName, false); - return users; } else { + PaginatedUserResponse combinedPaginatedUserResult = new PaginatedUserResponse(); AbstractUserStoreManager tempCarbonUM = carbonUM; // If domain name are not given, then perform filtering on all available user stores. - while (tempCarbonUM != null && maxLimit > 0) { - // If carbonUM is not an instance of Abstract User Store Manger we can't get the domain name. - if (tempCarbonUM instanceof AbstractUserStoreManager) { - domainName = tempCarbonUM.getRealmConfiguration().getUserStoreProperty("DomainName"); - users.addAll(getFilteredUsersFromMultiAttributeFiltering(node, offset, maxLimit, sortBy, sortOrder, - domainName, paginationRequested)); - } + while (tempCarbonUM != null && checkIterateOverDomainRequired(maxLimit, combinedPaginatedUserResult)) { + domainName = tempCarbonUM.getRealmConfiguration().getUserStoreProperty("DomainName"); + PaginatedUserResponse paginatedUserResult = getFilteredUsersFromMultiAttributeFiltering(node, offset, + maxLimit, sortBy, sortOrder, domainName, paginationRequested); + // Accumulate the filtered users. + combinedPaginatedUserResult.getFilteredUsers().addAll(paginatedUserResult.getFilteredUsers()); + + // Accumulate the total user counts. + combinedPaginatedUserResult.setTotalResults(combinedPaginatedUserResult.getTotalResults() + + paginatedUserResult.getTotalResults()); + // If secondary user store manager assigned to carbonUM then global variable carbonUM will contains the // secondary user store manager. tempCarbonUM = (AbstractUserStoreManager) tempCarbonUM.getSecondaryUserStoreManager(); // Calculating new offset and limit parameters. - int numberOfFilteredUsers = users.size(); - if (numberOfFilteredUsers <= 0 && offset > 1) { + int numberOfFilteredUsers = combinedPaginatedUserResult.getFilteredUsers().size(); + if (numberOfFilteredUsers == 0 && offset > 1) { if (log.isDebugEnabled()) { log.debug(String.format("Filter returned no results for original offset: %d.", offset)); } @@ -2181,7 +2194,7 @@ private Set getMultiAttributeFilteredUser maxLimit = calculateLimit(maxLimit, numberOfFilteredUsers); } } - return users; + return combinedPaginatedUserResult; } } /** @@ -2386,13 +2399,10 @@ private Map getMappedAttributes(String extClaimDialectName, Stri * @return * @throws CharonException */ - private Set getFilteredUsersFromMultiAttributeFiltering(Node node, - int offset, - int limit, - String sortBy, - String sortOrder, - String domainName, - Boolean paginationRequested) + private PaginatedUserResponse getFilteredUsersFromMultiAttributeFiltering(Node node, int offset, int limit, + String sortBy, String sortOrder, + String domainName, + Boolean paginationRequested) throws CharonException, BadRequestException { Set coreUsers; @@ -2422,9 +2432,13 @@ private Set getFilteredUsersFromMultiAttr if (paginationRequested) { checkForPaginationSupport(condition); } - coreUsers.addAll(carbonUM.getUserListWithID(condition, domainName, - UserCoreConstants.DEFAULT_PROFILE, limit, offset, sortBy, sortOrder)); - return coreUsers; + PaginatedUserResponse paginatedUserResult = carbonUM.getPaginatedUserListWithID(condition, domainName, + UserCoreConstants.DEFAULT_PROFILE, limit, offset, sortBy, sortOrder); + + // Remove duplicates. + coreUsers.addAll(paginatedUserResult.getFilteredUsers()); + paginatedUserResult.setFilteredUsers(new ArrayList<>(coreUsers)); + return paginatedUserResult; } catch (UserStoreException e) { throw resolveError(e, "Error in filtering users by multi attributes in domain: " + domainName); } @@ -6668,13 +6682,22 @@ private int calculateOffsetForMultiAttributeFilter(Node node, int offset, String // Checking the number of matches till the original offset. boolean paginationRequest = false; - Set skippedUsers = - getFilteredUsersFromMultiAttributeFiltering(node, initialOffset, offset, sortBy, sortOrder, domainName, - paginationRequest); - - int skippedUserCount = skippedUsers.size(); + int skippedUserCount = getFilteredUsersFromMultiAttributeFiltering(node, initialOffset, offset, sortBy, + sortOrder, domainName, paginationRequest).getFilteredUsers().size(); // Calculate the new offset and return return offset - skippedUserCount; } + + private boolean checkIterateOverDomainRequired(int limit, PaginatedUserResponse paginatedUserResponse) { + + /* When paginated user response contains total result, need to iterate over all the user stores and accumulate + total matching user count as total count is not calculated afterward. */ + if (paginatedUserResponse.getTotalResults() > 0) { + return true; + } + + // When required users are retrieved and limit is set to zero, skip iterating the user stores. + return limit > 0; + } } diff --git a/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManagerTest.java b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManagerTest.java index c939c7c8a..26c66e958 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManagerTest.java +++ b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManagerTest.java @@ -47,6 +47,7 @@ import org.wso2.carbon.identity.scim2.common.group.SCIMGroupHandler; import org.wso2.carbon.identity.scim2.common.internal.SCIMCommonComponentHolder; import org.wso2.carbon.user.core.UserStoreClientException; +import org.wso2.carbon.user.core.common.PaginatedUserResponse; import org.wso2.charon3.core.exceptions.NotImplementedException; import org.wso2.charon3.core.extensions.UserManager; import org.wso2.charon3.core.objects.plainobjects.UsersGetResponse; @@ -573,6 +574,9 @@ public void testFilteringUsersWithGET(List()); when(mockedUserStoreManager.getSecondaryUserStoreManager("PRIMARY")).thenReturn(mockedUserStoreManager); when(mockedUserStoreManager.isSCIMEnabled()).thenReturn(true); @@ -643,8 +647,8 @@ public Object[][] userInfoForFiltering() { @Test(dataProvider = "getDataForFilterUsersWithPagination") public void testFilteringUsersWithGETWithPagination(List users, String filter, - List filteredUsersWithPagination, - List filteredUsersWithoutPagination, + PaginatedUserResponse filteredUsersWithPagination, + PaginatedUserResponse filteredUsersWithoutPagination, boolean isConsiderTotalRecordsForTotalResultOfLDAPEnabled, boolean isConsiderMaxLimitForTotalResultEnabled, String domain, int count, int configuredMaxLimit, int expectedResultCount, @@ -666,23 +670,27 @@ public void testFilteringUsersWithGETWithPagination(List()); @@ -836,87 +844,87 @@ public Object[][] getDataForFilterUsersWithPagination() { // value of the 'totalResult'. {users, "name.givenName eq testUser", - new ArrayList() {{ + new PaginatedUserResponse(new ArrayList() {{ add(testUser1); add(testUser2); - }}, - new ArrayList() {{ + }}), + new PaginatedUserResponse(new ArrayList() {{ add(testUser1); add(testUser2); add(testUser3); - }}, + }}), true, false, "PRIMARY", 2, 4, 2, 3}, {users, "name.givenName eq testUser", - new ArrayList() {{ + new PaginatedUserResponse(new ArrayList() {{ add(testUser1); add(testUser2); - }}, - new ArrayList() {{ + }}), + new PaginatedUserResponse(new ArrayList() {{ add(testUser1); add(testUser2); add(testUser3); - }}, + }}), false, false, "PRIMARY", 2, 4, 2, 2}, {users, "name.givenName eq testUser", - new ArrayList() {{ + new PaginatedUserResponse(new ArrayList() {{ add(testUser1); add(testUser2); - }}, - new ArrayList() {{ + }}), + new PaginatedUserResponse(new ArrayList() {{ add(testUser1); add(testUser2); add(testUser3); - }}, + }}), true, false, "SECONDARY", 2, 4, 2, 3}, {users, "name.givenName eq testUser", - new ArrayList() {{ + new PaginatedUserResponse(new ArrayList() {{ add(testUser1); add(testUser2); - }}, - new ArrayList() {{ + }}), + new PaginatedUserResponse(new ArrayList() {{ add(testUser1); add(testUser2); add(testUser3); - }}, + }}), true, true, "SECONDARY", 2, 4, 2, 3}, {users, "name.givenName sw testUser and name.givenName co New", - new ArrayList() {{ + new PaginatedUserResponse(new ArrayList() {{ add(testUser4); - }}, - new ArrayList() {{ + }}), + new PaginatedUserResponse(new ArrayList() {{ add(testUser4); add(testUser5); - }}, + }}), true, false, "PRIMARY", 1, 4, 1, 2}, {users, "name.givenName sw testUser and name.givenName co New", - new ArrayList() {{ + new PaginatedUserResponse(new ArrayList() {{ add(testUser4); - }}, - new ArrayList() {{ + }}), + new PaginatedUserResponse(new ArrayList() {{ add(testUser4); add(testUser5); - }}, + }}), false, false, "PRIMARY", 1, 4, 1, 1}, {users, "name.givenName sw testUser and name.givenName co New", - new ArrayList() {{ + new PaginatedUserResponse(new ArrayList() {{ add(testUser4); - }}, - new ArrayList() {{ + }}), + new PaginatedUserResponse(new ArrayList() {{ add(testUser4); add(testUser5); - }}, + }}), true, false, "SECONDARY", 1, 4, 1, 2}, {users, "name.givenName sw testUser and name.givenName co New", - new ArrayList() {{ + new PaginatedUserResponse(new ArrayList() {{ add(testUser4); - }}, - new ArrayList() {{ + }}), + new PaginatedUserResponse(new ArrayList() {{ add(testUser4); add(testUser5); - }}, + }}), false, false, "SECONDARY", 1, 4, 1, 2}, }; diff --git a/pom.xml b/pom.xml index d1f85100f..0a853d7be 100644 --- a/pom.xml +++ b/pom.xml @@ -278,7 +278,7 @@ 3.3.7 6.5.3 3.2.0.wso2v1 - 4.10.17 + 4.10.24 7.5.109 4.13.1 20030203.000129