New Group Sync for GN#350
Conversation
|
I like the fact that this is an extension of the org-based sync. Can you explain whether this will be be a new option for the |
|
If the community agrees on this move, it has to be properly documented in addition to this PR. |
|
Looks very interesting indeed. |
|
i dont understand much of it but i dont have the time to properly look, but shouldnt the 'profile in GN' be |
We compute the highest profile in order for user to have the good minimal profile acording to his roles.
Yep wanted to do so but I didn't remembered some things I did. It was a shot i gave a few months ago...
Sure will do, if community agrees |
|
I think this deserves a GIP :-) |
There was a problem hiding this comment.
Pull request overview
Adds a new role_per_org group synchronization strategy for GeoNetwork/geOrchestra integration to support granular per-organization group privileges via prefixed roles (e.g. PSC:GN_REVIEWER), while keeping org-based mapping for “root” roles.
Changes:
- Introduces
GroupSyncMode.role_per_organd a newRolePerOrgBasedGroupSynchronizerimplementation, wired viaGroupSynchronizerProxy. - Deprecates the existing roles-based group synchronizer class (annotation) and adjusts integration test support to compute user profile using “root roles” provided by the active synchronizer.
- Adds a new integration test covering the new sync mode.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| georchestra-integration/externalized-accounts/src/test/java/org/geonetwork/security/external/integration/RolePerOrgBasedSynchronizationIT.java | New integration test exercising the new sync mode and prefixed role behavior. |
| georchestra-integration/externalized-accounts/src/test/java/org/geonetwork/security/external/integration/IntegrationTestSupport.java | Updates profile assertion to use synchronizer-provided “root roles” for profile resolution. |
| georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/model/GroupSyncMode.java | Adds the role_per_org sync mode (and marks roles as deprecated in a comment). |
| georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/RolesBasedGroupSynchronizer.java | Marks the old roles-based synchronizer class as @Deprecated. |
| georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/RolePerOrgBasedGroupSynchronizer.java | New synchronizer implementing org group mapping + prefixed-role per-group privileges. |
| georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/IntegrationConfiguration.java | Registers the new synchronizer as a Spring bean. |
| georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/GroupSynchronizerProxy.java | Routes role_per_org mode to the new synchronizer and delegates getRootRolesForUser. |
| georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/GroupSynchronizer.java | Adds getRootRolesForUser to the interface. |
| georchestra-integration/externalized-accounts/src/main/java/org/geonetwork/security/external/integration/AbstractGroupSynchronizer.java | Provides a default getRootRolesForUser implementation (returns raw roles). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!StringUtils.hasLength(orgName)) { | ||
| return Collections.emptyList(); | ||
| } | ||
| Stream<String> groupsName = userRoles(user) | ||
| .map(r -> r.contains(separator) ? r.split(separator)[0] : orgName | ||
| ).distinct(); |
| .map(r -> r.contains(separator) ? r.split(separator)[0] : orgName | ||
| ).distinct(); | ||
| Stream<CanonicalGroup> roleGroups = groupsName.map(role -> this.externalGroupLinks.findByName(role)// | ||
| .map(GroupLink::getCanonical) |
| private Stream<String> userRoles(CanonicalUser user) { | ||
| return user.getRoles().stream() | ||
| .filter(r -> Pattern.compile(".+" + separator + ".+").matcher(r).matches() || config.getProfiles().getRolemappings().keySet().contains(r)); | ||
| } |
| } else { | ||
| return group.getName().equals(user.getOrganization()); | ||
| } | ||
| }) //e.g filter roles for this group PSC:GN_REVIEWER and GN_EDITOR //e.g filter roles for this group PSC:GN_REVIEWER and GN_EDITOR |
|
|
||
| @Override | ||
| public List<String> getRootRolesForUser(CanonicalUser user) { | ||
| // Not used in RolePerOrg mode |
| if (role.contains(separator)) { | ||
| return role.split(separator)[1]; |
| * Returns the names of the root roles (non group-specific) assigned to the given user in the | ||
| * external system. |
| @@ -237,4 +237,8 @@ private Profile resolveUserProfile(@NonNull List<String> roles) { | |||
| return profileMappings.resolveHighestProfileFromRoleNames(roles); | |||
| } | |||
|
|
|||
| CanonicalGroup org = orgGroups.get(0); // just to be explicit | ||
| CanonicalGroup role = super.createRole("PSC:GN_REVIEWER"); // role name in external repo | ||
| List<CanonicalGroup> orgs = new ArrayList<>(super.defaultGroups); | ||
| orgs.add(org); |
I introduce here a new synchronization for geonetwork in georchestra in order to solve the current situation of:
How does it work
This new sync works (almost) like Org based sync. Organization in georchestra are mapped to GN groups.
But, we can also create new roles in georchestra like PSC:GN_REVIEWER which will allow user to be REVIEWER of PSC group.
In this way, we can set multiple and differents rights for user like PSC:GN_REVIEWER, C2C:GN_EDITOR and so on.
Top level roles like GN_EDITOR, GN_REVIEWER set this role for organization/group where user belongs to (just as org based sync).
Requires: georchestra/georchestra#4616
and georchestra/georchestra#4656
Examples
GIP: georchestra/improvement-proposals#16