diff --git a/source/java/org/alfresco/rest/api/impl/GroupsImpl.java b/source/java/org/alfresco/rest/api/impl/GroupsImpl.java index f250ec4025..f02be54f9e 100644 --- a/source/java/org/alfresco/rest/api/impl/GroupsImpl.java +++ b/source/java/org/alfresco/rest/api/impl/GroupsImpl.java @@ -64,6 +64,7 @@ import org.alfresco.rest.framework.resource.parameters.Parameters; import org.alfresco.rest.framework.resource.parameters.SortColumn; import org.alfresco.rest.framework.resource.parameters.where.Query; import org.alfresco.rest.framework.resource.parameters.where.QueryHelper; +import org.alfresco.rest.workflow.api.impl.MapBasedQueryWalker; import org.alfresco.rest.workflow.api.impl.MapBasedQueryWalkerOrSupported; import org.alfresco.service.cmr.security.AuthorityService; import org.alfresco.service.cmr.security.AuthorityType; @@ -248,25 +249,6 @@ public class GroupsImpl implements Groups return CollectionWithPagingInfo.asPaged(paging, groups, pagingResult.hasMoreItems(), totalItems); } -// private boolean groupInZones(Group group, List zonesFilter) -// { -// Set groupZones = group.getZones(); -// // The zones may not have been "included" in the request. -// if (groupZones == null) -// { -// groupZones = authorityService.getAuthorityZones(group.getId()); -// } -// -// for (String zone : zonesFilter) -// { -// if (groupZones.contains(zone)) -// { -// return true; -// } -// } -// return false; -// } - @Override public CollectionWithPagingInfo getGroupsByPersonId(String requestedPersonId, Parameters parameters) { @@ -323,7 +305,7 @@ public class GroupsImpl implements Groups Paging paging) { PagingResults pagingResult; - // TODO: make sure impl works for isRootParam = true,false,null. + if (isRootParam != null) { List groupList; @@ -369,7 +351,18 @@ public class GroupsImpl implements Groups } // Post process paging - this should be moved to service layer. - // TODO: filter groupList to remove zones NOT matching the zoneFilter + + if (zoneFilter != null) + { + // Only AND (e.g. isRoot=true AND zones IN ('MY.APP')) currently supported, + // as we can easily post-filter for zones. + // If we introduce OR, then a different approach will be needed. + groupList = groupList.stream(). + filter(authority -> { + Set zones = authorityService.getAuthorityZones(authority.getAuthorityName()); + return zones.contains(zoneFilter); + }).collect(Collectors.toList()); + } pagingResult = Util.wrapPagingResults(paging, groupList); } else @@ -906,10 +899,16 @@ public class GroupsImpl implements Groups return AuthorityType.GROUP.equals(authorityType) || AuthorityType.EVERYONE.equals(authorityType); } - private static class GroupsQueryWalker extends MapBasedQueryWalkerOrSupported + private static class GroupsQueryWalker extends MapBasedQueryWalker { private List zones; + @Override + public void and() + { + // allow AND, e.g. isRoot=true AND zones in ('BLAH') + } + @Override public void in(String propertyName, boolean negated, String... propertyValues) { diff --git a/source/test-java/org/alfresco/rest/api/tests/GroupsTest.java b/source/test-java/org/alfresco/rest/api/tests/GroupsTest.java index be696c53b3..6304dc2f55 100644 --- a/source/test-java/org/alfresco/rest/api/tests/GroupsTest.java +++ b/source/test-java/org/alfresco/rest/api/tests/GroupsTest.java @@ -68,6 +68,7 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest protected AuthorityService authorityService; private String rootGroupName = null; + private Group rootGroup = null; private Group groupA = null; private Group groupB = null; private GroupMember groupMemberA = null; @@ -358,9 +359,10 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest List groups = response.getList(); // We know exactly which groups are in the selected zone. - assertEquals(2, groups.size()); - assertEquals(groupA, groups.get(0)); - assertEquals(groupB, groups.get(1)); + assertEquals(3, groups.size()); + assertEquals(rootGroup, groups.get(0)); + assertEquals(groupA, groups.get(1)); + assertEquals(groupB, groups.get(2)); groups.forEach(group -> assertTrue(group.getZones().contains("APITEST.MYZONE"))); otherParams.put("where", "(zones in ('APITEST.ANOTHER'))"); @@ -388,31 +390,51 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest List groups = response.getList(); // We know exactly which groups are in the selected zone. - assertEquals(2, groups.size()); - assertEquals(groupA, groups.get(0)); - assertEquals(groupB, groups.get(1)); + assertEquals(3, groups.size()); + assertEquals(rootGroup, groups.get(0)); + assertEquals(groupA, groups.get(1)); + assertEquals(groupB, groups.get(2)); // We haven't included the zones info. groups.forEach(group -> assertNull(group.getZones())); } // Filter zones while using where isRoot=true // (this causes a different query path to be used) - /*{ + { Paging paging = getPaging(0, Integer.MAX_VALUE); Map otherParams = new HashMap<>(); // Ensure predictable result ordering addOrderBy(otherParams, org.alfresco.rest.api.Groups.PARAM_DISPLAY_NAME, true); + otherParams.put("include", org.alfresco.rest.api.Groups.PARAM_INCLUDE_ZONES); - // TODO: how will we support all possible boolean operators? AND, OR...? - otherParams.put("where", "(isRoot=true AND zones in ('APITEST.SPECIAL'))"); + otherParams.put("where", "(isRoot=true AND zones in ('APITEST.MYZONE'))"); ListResponse response = getGroups(paging, otherParams); List groups = response.getList(); assertEquals(1, groups.size()); - assertEquals(rootGroupName, groups.get(0)); - assertTrue(groups.get(0).getZones().contains("APITEST.SPECIAL")); - }*/ + assertEquals(rootGroup, groups.get(0)); + assertTrue(groups.get(0).getZones().contains("APITEST.MYZONE")); + } + + // Filter zones while using where isRoot=false + { + Paging paging = getPaging(0, Integer.MAX_VALUE); + Map otherParams = new HashMap<>(); + // Ensure predictable result ordering + addOrderBy(otherParams, org.alfresco.rest.api.Groups.PARAM_DISPLAY_NAME, true); + + otherParams.put("where", "(isRoot=false AND zones in ('APITEST.MYZONE'))"); + + ListResponse response = getGroups(paging, otherParams); + List groups = response.getList(); + + assertEquals(2, groups.size()); + assertEquals(groupA, groups.get(0)); + assertEquals(groupB, groups.get(1)); + // We haven't included the zones info. + groups.forEach(group -> assertNull(group.getZones())); + } // -ve test: invalid zones clause { @@ -439,6 +461,10 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest // A zone that isn't in use or doesn't exist otherParams.put("where", "(zones in ('NON.EXISTENT'))"); getGroups(paging, otherParams, "Incorrect response", 404); + + // OR operator not currently supported + otherParams.put("where", "(isRoot=true OR zones in ('APP.DEFAULT'))"); + getGroups(paging, otherParams, "Incorrect response", 400); } } @@ -479,7 +505,7 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest AuthenticationUtil.setAdminUserAsFullyAuthenticatedUser(); rootGroupName = authorityService.createAuthority(AuthorityType.GROUP, groupName); - authorityService.addAuthorityToZones(rootGroupName, zoneSet("APITEST.SPECIAL")); + authorityService.addAuthorityToZones(rootGroupName, zoneSet("APITEST.MYZONE")); String groupBAuthorityName = authorityService.createAuthority(AuthorityType.GROUP, "Test_GroupB" + GUID.generate()); authorityService.addAuthority(rootGroupName, groupBAuthorityName); @@ -492,6 +518,9 @@ public class GroupsTest extends AbstractSingleNetworkSiteTest authorityService.addAuthority(groupAAuthorityName, user1); authorityService.addAuthority(groupBAuthorityName, user2); + rootGroup = new Group(); + rootGroup.setId(rootGroupName); + groupA = new Group(); groupA.setId(groupAAuthorityName);