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

Update role based sync #298

Merged
merged 2 commits into from
Jun 24, 2024
Merged

Update role based sync #298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
374f0d2
feat: update role based sync
f-necas Jun 21, 2024
b2eb18e
fix: update method name
pmauduit Apr 24, 2024
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
11 changes: 10 additions & 1 deletion ...src/main/java/org/geonetwork/security/external/integration/AbstractGroupSynchronizer.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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");

Comment on lines +46 to +48
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have expected that such a set would have been defined somewhere into georchestra/georchestra (ldap-account-mgmt, georchestra-commons, or security-proxy-spring-integration), but it does not look like it's actually the case, so, fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's https://github.com/georchestra/datadir/blob/master/console/protectedroles.properties ... but so far only used by the console. So that would be two lists to sync ? or move those to default.properties ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that would be two lists to sync ? or move those to default.properties ?

I think such a constant should be defined into one of the shared geOrchestra libraries across components, but this is another topic (georchestra/georchestra#4031) ; here, since we already are into a GN module specific to geOrchestra, I think it's fine to leave it as it is.

@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::notMatchesGeorchestraDefaultRoleNameFilter).filter(this::matchesRoleNameFilter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/notMatches/noMatches/ or doesNotMatch... ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was tired 😄 doesNotMatch is better, I fix this

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::notMatchesGeorchestraDefaultRoleNameFilter).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 notMatchesGeorchestraDefaultRoleNameFilter(String roleName) {
requireNonNull(roleName);
return !georchestraDefaultRoleNames.contains(roleName);
}

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

}
114 changes: 40 additions & 74 deletions ...rc/test/java/org/geonetwork/security/external/integration/RoleBasedSynchronizationIT.java
View file
Open in desktop
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
Loading