Skip to content

Commit

Permalink
Merge pull request #298 from georchestra/roles-sync-update
Browse files Browse the repository at this point in the history 
Update role based sync
  • Loading branch information
@f-necas
f-necas authored Jun 24, 2024
2 parents 4066df2 + b2eb18e commit c12cfbb
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 101 deletions.
11 changes: 10 additions & 1 deletion ...src/main/java/org/geonetwork/security/external/integration/AbstractGroupSynchronizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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());
Expand Down Expand Up @@ -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<String, GroupLink> getExistingGroupLinksById() {
return toIdMap(this.externalGroupLinks.findAll(), g -> g.getCanonical().getId());
Expand Down Expand Up @@ -224,4 +228,9 @@ private <T> Map<String, T> toIdMap(List<T> list, Function<T, String> idExtractor
return actual;
}

private Profile resolveUserProfile(@NonNull List<String> roles) {
ProfileMappingProperties profileMappings = configProperties.getProfiles();
return profileMappings.resolveHighestProfileFromRoleNames(roles);
}

}
10 changes: 1 addition & 9 deletions ...rc/main/java/org/geonetwork/security/external/integration/OrgsBasedGroupSynchronizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,6 @@ private Supplier<? extends IllegalArgumentException> 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<String> roles) {
ProfileMappingProperties profileMappings = configProperties.getProfiles();
return profileMappings.resolveHighestProfileFromRoleNames(roles);
}


}
35 changes: 18 additions & 17 deletions ...c/main/java/org/geonetwork/security/external/integration/RolesBasedGroupSynchronizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,16 @@
*/
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;
import java.util.stream.Collectors;
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;
Expand All @@ -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<String> georchestraDefaultRoleNames = Set.of("SUPERUSER","ORGADMIN","MAPSTORE_ADMIN","REFERENT","EMAILPROXY",
"ADMINISTRATOR", "IMPORT", "USER", "GN_EDITOR", "GN_REVIEWER", "GN_ADMIN");

@Autowired
public ExternalizedSecurityProperties config;

Expand All @@ -71,7 +71,7 @@ public RolesBasedGroupSynchronizer(CanonicalAccountsRepository canonicalAccounts
*/
public @Override List<CanonicalGroup> fetchCanonicalGroups() {
List<CanonicalGroup> roles = canonicalAccounts.findAllRoles();
Stream<CanonicalGroup> matches = roles.stream().filter(this::matchesRoleNameFilter);
Stream<CanonicalGroup> matches = roles.stream().filter(this::doesNotMatchesGeorchestraDefaultRoleNameFilter).filter(this::matchesRoleNameFilter);
return matches.map(this::renameRoleUsingConfigPattern).collect(Collectors.toList());
}

Expand All @@ -92,7 +92,7 @@ private CanonicalGroup renameRoleUsingConfigPattern(CanonicalGroup role) {
}

protected @Override List<CanonicalGroup> resolveGroupsOf(CanonicalUser user) {
Stream<String> roleNames = user.getRoles().stream().filter(config::matchesRoleNameFilter);
Stream<String> roleNames = user.getRoles().stream().filter(this::doesNotMatchesGeorchestraDefaultRoleNameFilter).filter(config::matchesRoleNameFilter);

Stream<CanonicalGroup> roleGroups = roleNames.map(role -> this.externalGroupLinks.findByName(role)//
.map(GroupLink::getCanonical)//
Expand All @@ -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<? extends IllegalArgumentException> notFound(String role) {
return () -> new IllegalArgumentException("Role " + role + " not found in internal or external repository");
}
Expand All @@ -123,4 +114,14 @@ private boolean matchesRoleNameFilter(CanonicalGroup role) {
return config.matchesRoleNameFilter(name);
}

private boolean doesNotMatchesGeorchestraDefaultRoleNameFilter(String roleName) {
requireNonNull(roleName);
return !georchestraDefaultRoleNames.contains(roleName);
}

private boolean doesNotMatchesGeorchestraDefaultRoleNameFilter(CanonicalGroup role) {
requireNonNull(role);
return doesNotMatchesGeorchestraDefaultRoleNameFilter(role.getName());
}

}
114 changes: 40 additions & 74 deletions ...rc/test/java/org/geonetwork/security/external/integration/RoleBasedSynchronizationIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ public class RoleBasedSynchronizationIT extends AbstractAccountsReconcilingServi
createOnlyGeonetworkUsersAndGroupsFromRoles(users, roles);

Map<String, User> existingUsers = getExistingUsers(users);
Map<String, Group> existingGroups = getExistingGroups(roles);

// current state is that users and groups exist in GN db but no links to
// canonical versions exist
Expand All @@ -84,10 +83,7 @@ public class RoleBasedSynchronizationIT extends AbstractAccountsReconcilingServi
// succeeded)
roles.forEach(gu -> {
Optional<GroupLink> 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 -> {
Expand Down Expand Up @@ -136,7 +132,8 @@ private void createOnlyGeonetworkUsersAndGroupsFromRoles(List<CanonicalUser> use

public @Test void Synchronize_on_empty_geonetwork_db_creates_all_users_and_groups_from_roles() {
List<CanonicalUser> users = super.defaultUsers;
List<CanonicalGroup> roles = super.defaultRoles;
//Roles are empty due to removing default roles
List<CanonicalGroup> roles = new ArrayList<>();

assertEquals(0, support.gnUserRepository.count());
assertEquals(0, support.gnGroupRepository.count());
Expand All @@ -145,70 +142,10 @@ private void createOnlyGeonetworkUsersAndGroupsFromRoles(List<CanonicalUser> use
verify(users, roles);
}

public @Test void Synchronize_updates_group_members_when_role_members_changed() {
support.synchronizeDefaultUsersAndGroups();

List<CanonicalUser> origUsers = super.defaultUsers;

List<CanonicalGroup> newRoles = Arrays.asList(roleUser, roleGnEditor, roleGnReviewer);
List<CanonicalUser> 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<CanonicalGroup> 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<CanonicalGroup> 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<CanonicalUser> 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)},
Expand All @@ -222,11 +159,19 @@ private CanonicalUser swapRoles(CanonicalUser user, List<CanonicalGroup> 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<CanonicalGroup> roles = super.defaultRoles;
roles.add(super.createRole("NOPSC_BUILDINGS"));
roles.add(psc1);
roles.add(psc2);


service.synchronize();

Set<CanonicalGroup> origGroups = rolesMatchingPattern(config);
Set<CanonicalGroup> origGroups = rolesMatchingPattern(config, Set.of(psc1, psc2));
Set<CanonicalGroup> syncedGroups = getSavedCanonicalGroups();
assertEquals(origGroups.size(), syncedGroups.size());
assertNotEquals("group names should differ due to pattern grouping", origGroups, syncedGroups);
Expand All @@ -253,6 +198,27 @@ private CanonicalUser swapRoles(CanonicalUser user, List<CanonicalGroup> newRole
}
}

public @Test void Role_correctly_added_with_editor_privilege() {
support.synchronizeDefaultUsersAndGroups();
ExternalizedSecurityProperties config = support.getConfig();
config.setSyncRolesFilter(Pattern.compile("(.*)"));

List<CanonicalGroup> roles = new ArrayList<>();
CanonicalGroup newrole1;
roles.add(newrole1 = super.createRole("MySuperNewRole"));


List<CanonicalUser> 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<CanonicalGroup> getSavedCanonicalGroups() {
Set<CanonicalGroup> syncedGroups = support.groupLinkRepository.findAll().stream().map(GroupLink::getCanonical)
.collect(toSet());
Expand All @@ -261,17 +227,17 @@ private Set<CanonicalGroup> getSavedCanonicalGroups() {

private Set<CanonicalGroup> stripOffRolePrefixFromGroupNames(Set<CanonicalGroup> origGroups) {
Set<CanonicalGroup> 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<CanonicalGroup> rolesMatchingPattern(ExternalizedSecurityProperties config) {
Set<CanonicalGroup> origGroups = super.defaultRoles.stream()
private Set<CanonicalGroup> rolesMatchingPattern(ExternalizedSecurityProperties config, Set<CanonicalGroup> expectedGroups) {
Set<CanonicalGroup> 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;
}

Expand Down

0 comments on commit c12cfbb

Please sign in to comment.