From 374f0d21e91760b5e1507375ebbee1d8948d94d1 Mon Sep 17 00:00:00 2001 From: Florian Necas Date: Fri, 21 Jun 2024 13:53:10 +0200 Subject: [PATCH] feat: update role based sync --- .../AbstractGroupSynchronizer.java | 11 +- .../OrgsBasedGroupSynchronizer.java | 10 +- .../RolesBasedGroupSynchronizer.java | 35 +++--- .../RoleBasedSynchronizationIT.java | 114 ++++++------------ 4 files changed, 69 insertions(+), 101 deletions(-) diff --git a/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/AbstractGroupSynchronizer.java b/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/AbstractGroupSynchronizer.java index 4956267686..a36b13fe23 100644 --- a/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/AbstractGroupSynchronizer.java +++ b/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/AbstractGroupSynchronizer.java @@ -38,6 +38,7 @@ import org.fao.geonet.repository.GroupRepository; import org.fao.geonet.repository.LanguageRepository; import org.geonetwork.security.external.configuration.ExternalizedSecurityProperties; +import org.geonetwork.security.external.configuration.ProfileMappingProperties; import org.geonetwork.security.external.model.CanonicalGroup; import org.geonetwork.security.external.model.CanonicalUser; import org.geonetwork.security.external.model.GroupLink; @@ -50,6 +51,7 @@ import org.springframework.context.ConfigurableApplicationContext; import com.google.common.collect.Sets; +import org.springframework.lang.NonNull; abstract class AbstractGroupSynchronizer implements GroupSynchronizer { static final Logger log = LoggerFactory.getLogger(AbstractGroupSynchronizer.class.getPackage().getName()); @@ -186,7 +188,9 @@ protected Profile resolveDefaultProfile(CanonicalUser user) { return configProperties.getProfiles().resolveHighestProfileFromRoleNames(user.getRoles()); } - protected abstract Privilege resolvePrivilegeFor(CanonicalUser user, Group group); + private Privilege resolvePrivilegeFor(CanonicalUser user, Group group) { + return new Privilege(group, resolveUserProfile(user.getRoles())); + } private Map getExistingGroupLinksById() { return toIdMap(this.externalGroupLinks.findAll(), g -> g.getCanonical().getId()); @@ -224,4 +228,9 @@ private Map toIdMap(List list, Function idExtractor return actual; } + private Profile resolveUserProfile(@NonNull List roles) { + ProfileMappingProperties profileMappings = configProperties.getProfiles(); + return profileMappings.resolveHighestProfileFromRoleNames(roles); + } + } diff --git a/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/OrgsBasedGroupSynchronizer.java b/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/OrgsBasedGroupSynchronizer.java index 66e574df6c..f47de1a4aa 100644 --- a/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/OrgsBasedGroupSynchronizer.java +++ b/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/OrgsBasedGroupSynchronizer.java @@ -67,14 +67,6 @@ private Supplier notFound(final String orgNa "Organization with name '" + orgName + "' not found in internal nor external repository"); } - protected @Override Privilege resolvePrivilegeFor(CanonicalUser user, Group groupFromOrganization) { - Profile profile = resolveUserProfile(user.getRoles()); - return new Privilege(groupFromOrganization, profile); - } - private Profile resolveUserProfile(@NonNull List roles) { - ProfileMappingProperties profileMappings = configProperties.getProfiles(); - return profileMappings.resolveHighestProfileFromRoleNames(roles); - } - + } diff --git a/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/RolesBasedGroupSynchronizer.java b/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/RolesBasedGroupSynchronizer.java index 31c4c1179e..4fc3f0263d 100644 --- a/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/RolesBasedGroupSynchronizer.java +++ b/georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/RolesBasedGroupSynchronizer.java @@ -18,10 +18,8 @@ */ package org.geonetwork.security.external.integration; -import static java.util.Objects.requireNonNull; - -import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -29,10 +27,7 @@ import java.util.stream.IntStream; import java.util.stream.Stream; -import org.fao.geonet.domain.Group; -import org.fao.geonet.domain.Profile; import org.geonetwork.security.external.configuration.ExternalizedSecurityProperties; -import org.geonetwork.security.external.configuration.ProfileMappingProperties; import org.geonetwork.security.external.model.CanonicalGroup; import org.geonetwork.security.external.model.CanonicalUser; import org.geonetwork.security.external.model.GroupLink; @@ -42,10 +37,15 @@ import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; +import static java.util.Objects.*; + public class RolesBasedGroupSynchronizer extends AbstractGroupSynchronizer { public static final Logger log = LoggerFactory.getLogger(RolesBasedGroupSynchronizer.class.getPackage().getName()); + static final Set georchestraDefaultRoleNames = Set.of("SUPERUSER","ORGADMIN","MAPSTORE_ADMIN","REFERENT","EMAILPROXY", + "ADMINISTRATOR", "IMPORT", "USER", "GN_EDITOR", "GN_REVIEWER", "GN_ADMIN"); + @Autowired public ExternalizedSecurityProperties config; @@ -71,7 +71,7 @@ public RolesBasedGroupSynchronizer(CanonicalAccountsRepository canonicalAccounts */ public @Override List fetchCanonicalGroups() { List roles = canonicalAccounts.findAllRoles(); - Stream matches = roles.stream().filter(this::matchesRoleNameFilter); + Stream matches = roles.stream().filter(this::notMatchesGeorchestraDefaultRoleNameFilter).filter(this::matchesRoleNameFilter); return matches.map(this::renameRoleUsingConfigPattern).collect(Collectors.toList()); } @@ -92,7 +92,7 @@ private CanonicalGroup renameRoleUsingConfigPattern(CanonicalGroup role) { } protected @Override List resolveGroupsOf(CanonicalUser user) { - Stream roleNames = user.getRoles().stream().filter(config::matchesRoleNameFilter); + Stream roleNames = user.getRoles().stream().filter(this::notMatchesGeorchestraDefaultRoleNameFilter).filter(config::matchesRoleNameFilter); Stream roleGroups = roleNames.map(role -> this.externalGroupLinks.findByName(role)// .map(GroupLink::getCanonical)// @@ -103,15 +103,6 @@ private CanonicalGroup renameRoleUsingConfigPattern(CanonicalGroup role) { return roleGroups.collect(Collectors.toList()); } - protected @Override Privilege resolvePrivilegeFor(CanonicalUser user, Group groupFromRole) { - final String roleName = groupFromRole.getName(); - - ProfileMappingProperties profileMappings = configProperties.getProfiles(); - Profile profile = profileMappings.resolveHighestProfileFromRoleNames(Collections.singletonList(roleName)); - - return new Privilege(groupFromRole, profile); - } - private Supplier notFound(String role) { return () -> new IllegalArgumentException("Role " + role + " not found in internal or external repository"); } @@ -123,4 +114,14 @@ private boolean matchesRoleNameFilter(CanonicalGroup role) { return config.matchesRoleNameFilter(name); } + private boolean notMatchesGeorchestraDefaultRoleNameFilter(String roleName) { + requireNonNull(roleName); + return !georchestraDefaultRoleNames.contains(roleName); + } + + private boolean notMatchesGeorchestraDefaultRoleNameFilter(CanonicalGroup role) { + requireNonNull(role); + return notMatchesGeorchestraDefaultRoleNameFilter(role.getName()); + } + } diff --git a/georchestra-integration/externalized-accounts/src/test/java/org/geonetwork/security/external/integration/RoleBasedSynchronizationIT.java b/georchestra-integration/externalized-accounts/src/test/java/org/geonetwork/security/external/integration/RoleBasedSynchronizationIT.java index 23e2bfda7b..acba761dfc 100644 --- a/georchestra-integration/externalized-accounts/src/test/java/org/geonetwork/security/external/integration/RoleBasedSynchronizationIT.java +++ b/georchestra-integration/externalized-accounts/src/test/java/org/geonetwork/security/external/integration/RoleBasedSynchronizationIT.java @@ -73,7 +73,6 @@ public class RoleBasedSynchronizationIT extends AbstractAccountsReconcilingServi createOnlyGeonetworkUsersAndGroupsFromRoles(users, roles); Map existingUsers = getExistingUsers(users); - Map existingGroups = getExistingGroups(roles); // current state is that users and groups exist in GN db but no links to // canonical versions exist @@ -84,10 +83,7 @@ public class RoleBasedSynchronizationIT extends AbstractAccountsReconcilingServi // succeeded) roles.forEach(gu -> { Optional link = support.groupLinkRepository.findById(gu.getId()); - assertTrue(link.isPresent()); - Group actual = link.get().getGeonetworkGroup(); - Group expected = existingGroups.get(actual.getName()); - assertEquals(expected.getId(), actual.getId()); + assertTrue(link.isEmpty()); }); users.forEach(cu -> { @@ -136,7 +132,8 @@ private void createOnlyGeonetworkUsersAndGroupsFromRoles(List use public @Test void Synchronize_on_empty_geonetwork_db_creates_all_users_and_groups_from_roles() { List users = super.defaultUsers; - List roles = super.defaultRoles; + //Roles are empty due to removing default roles + List roles = new ArrayList<>(); assertEquals(0, support.gnUserRepository.count()); assertEquals(0, support.gnGroupRepository.count()); @@ -145,70 +142,10 @@ private void createOnlyGeonetworkUsersAndGroupsFromRoles(List use verify(users, roles); } - public @Test void Synchronize_updates_group_members_when_role_members_changed() { - support.synchronizeDefaultUsersAndGroups(); - - List origUsers = super.defaultUsers; - - List newRoles = Arrays.asList(roleUser, roleGnEditor, roleGnReviewer); - List usersWithChangedRoles = origUsers.stream().map(u -> swapRoles(u, newRoles)) - .collect(toList()); - - when(canonicalAccountsRepositoryMock.findAllUsers()).thenReturn(usersWithChangedRoles); - - service.synchronize(); - - for (CanonicalUser expected : usersWithChangedRoles) { - UserLink link = support.assertUserLink(expected); - for (CanonicalGroup expectedRole : newRoles) { - support.assertGroup(link.getInternalUser(), expectedRole); - } - } - } - private CanonicalUser swapRoles(CanonicalUser user, List newRoles) { return super.withRoles(user, newRoles.toArray(new CanonicalGroup[newRoles.size()])); } - public @Test void Synchronize_creates_updates_and_deletes_users_and_groups_based_on_roles() { - support.synchronizeDefaultUsersAndGroups(); - - List roles = new ArrayList<>(super.defaultRoles); - CanonicalGroup newrole1; - roles.add(newrole1 = super.createRole("newrole1")); - roles.add(super.createRole("newrole2")); - - CanonicalGroup removedRole = super.roleOrgAdmin; - roles.remove(removedRole); - - final CanonicalGroup changedRoleOrig = super.roleGnEditor; - CanonicalGroup changedRole = super.withName(changedRoleOrig, changedRoleOrig.getName() + "Modified"); - roles.remove(changedRoleOrig); - roles.add(changedRole); - when(canonicalAccountsRepositoryMock.findRoleByName(changedRoleOrig.getName())).thenReturn(Optional.empty()); - when(canonicalAccountsRepositoryMock.findRoleByName(changedRole.getName())) - .thenReturn(Optional.of(changedRole)); - - List users = new ArrayList<>(super.defaultUsers); - users.add(super.setUpNewUser("newuser1", changedRole, roleAdministrator)); - users.add(super.setUpNewUser("newuser2", newrole1, roleUser)); - - users.remove(super.testeditor); - - CanonicalUser changedUser = super.withRoles(super.testuser, roleAdministrator, roleGnAdmin); - users.remove(super.testuser); - users.add(changedUser); - // just to be sure.. - users.forEach(u -> assertNotEquals(u.toString(), removedRole.getName(), u.getOrganization())); - users.forEach(u -> assertNotEquals(u.toString(), changedRoleOrig.getName(), u.getOrganization())); - - when(canonicalAccountsRepositoryMock.findAllRoles()).thenReturn(roles); - when(canonicalAccountsRepositoryMock.findAllUsers()).thenReturn(users); - - service.synchronize(); - verify(users, roles); - } - /** * In * {@link ExternalizedSecurityProperties#setSyncRolesFilter(java.util.regex.Pattern)}, @@ -222,11 +159,19 @@ private CanonicalUser swapRoles(CanonicalUser user, List newRole */ public @Test void Role_based_synchronization_respects_regex_filter_from_config_and_applies_pattern_group_filter() { ExternalizedSecurityProperties config = support.getConfig(); - config.setSyncRolesFilter(Pattern.compile("GN_(.*)")); + config.setSyncRolesFilter(Pattern.compile("PSC_(.*)")); + + CanonicalGroup psc1 = super.createRole("PSC_COMMUNITY"); + CanonicalGroup psc2 = super.createRole("PSC_GEOCOM"); + List roles = super.defaultRoles; + roles.add(super.createRole("NOPSC_BUILDINGS")); + roles.add(psc1); + roles.add(psc2); + service.synchronize(); - Set origGroups = rolesMatchingPattern(config); + Set origGroups = rolesMatchingPattern(config, Set.of(psc1, psc2)); Set syncedGroups = getSavedCanonicalGroups(); assertEquals(origGroups.size(), syncedGroups.size()); assertNotEquals("group names should differ due to pattern grouping", origGroups, syncedGroups); @@ -253,6 +198,27 @@ private CanonicalUser swapRoles(CanonicalUser user, List newRole } } + public @Test void Role_correctly_added_with_editor_privilege() { + support.synchronizeDefaultUsersAndGroups(); + ExternalizedSecurityProperties config = support.getConfig(); + config.setSyncRolesFilter(Pattern.compile("(.*)")); + + List roles = new ArrayList<>(); + CanonicalGroup newrole1; + roles.add(newrole1 = super.createRole("MySuperNewRole")); + + + List users = new ArrayList<>(super.defaultUsers); + users.add(super.setUpNewUser("newuser1", orgPsc, newrole1, roleGnEditor)); + + when(canonicalAccountsRepositoryMock.findAllRoles()).thenReturn(roles); + when(canonicalAccountsRepositoryMock.findAllUsers()).thenReturn(users); + + service.synchronize(); + verify(users, roles); + //TODO find a way to read privileges + } + private Set getSavedCanonicalGroups() { Set syncedGroups = support.groupLinkRepository.findAll().stream().map(GroupLink::getCanonical) .collect(toSet()); @@ -261,17 +227,17 @@ private Set getSavedCanonicalGroups() { private Set stripOffRolePrefixFromGroupNames(Set origGroups) { Set expectedGroups = origGroups.stream() - .map(g -> CanonicalGroup.builder().init(g).withName(g.getName().replace("GN_", "")).build()) + .map(g -> CanonicalGroup.builder().init(g).withName(g.getName().replace("PSC_", "")).build()) .collect(toSet()); return expectedGroups; } - private Set rolesMatchingPattern(ExternalizedSecurityProperties config) { - Set origGroups = super.defaultRoles.stream() + private Set rolesMatchingPattern(ExternalizedSecurityProperties config, Set expectedGroups) { + Set origGroups = super.defaultRoles.stream().filter(r -> !RolesBasedGroupSynchronizer.georchestraDefaultRoleNames.contains(r.getName())) .filter(r -> config.matchesRoleNameFilter(r.getName())).collect(toSet()); - assertEquals(3, origGroups.size()); - assertEquals("preflight check failed", - Sets.newLinkedHashSet(roleGnAdmin, roleGnEditor, roleGnReviewer), origGroups); + assertEquals(10, super.defaultRoles.size()); + assertEquals(2, origGroups.size()); + assertEquals("preflight check failed", expectedGroups, origGroups); return origGroups; }