Update role based sync #298
Conversation
|
nice ! cant pretend i understand the code changes, but as long as it properly syncs as discussed in lille.. and i see there are proper tests. |
There was a problem hiding this comment.
Looks good overall, I was wondering if it will also address georchestra/georchestra#4202
| static final Set<String> georchestraDefaultRoleNames = Set.of("SUPERUSER","ORGADMIN","MAPSTORE_ADMIN","REFERENT","EMAILPROXY", | ||
| "ADMINISTRATOR", "IMPORT", "USER", "GN_EDITOR", "GN_REVIEWER", "GN_ADMIN"); | ||
|
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
s/notMatches/noMatches/ or doesNotMatch... ?
There was a problem hiding this comment.
I was tired 😄 doesNotMatch is better, I fix this
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
…-23.0.x/pr-298 [georchestra-gn4.2.x-23.0.x] Merge pull request #298 from georchestra/roles-sync-update
|
im a bit lost with what is supposed to happen, but in orgs syncmode, now only the organizations console api is polled at each sync interval, while previously in 23.0 both users & orgs apis were called. Not saying this is due to this change, but it feels strange because user role membership (eg testuser being part of edit ive looked at the history of debian updates on the demo, and the |
This PR is a refactoring of roles based sync which allows to directly set privilege to associated.
Documentation will be update in georchestra/georchestra#4256
geOrchestra/geonetwork checklist