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.

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

Sync Groups on Login Support #31

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

bmlong137
Copy link

This is a follow up to pull request #17 .

This branch has two commits. The first extracts the "map authorities" feature from the "handleUserTokens()" method and places it into a "TokenProcessor". This is in preparation for the addition of other custom token processing implementations. I think this is a good route to go, but feel free to push back. I don't think it breaks anything functionally.

I am using a priority field to sort the beans, rather than relying on the Spring bean declaration order.

The 2nd commit is the addition of an "authority sync" feature. This was described in the previous pull request.

bmlong137 and others added 3 commits July 11, 2022 18:42
- split authority mapper into granted authority and person processor
- rename authority sync to group sync (since it only handles groups not
  generic authorities), with slight rename of config properties
- add javadoc
- add copyright headers
- add test configuration
- use direct component injection instead of auth component pass-along
- add additional "global-and-subsystem-properties" to Spring context for
  subsystem to handle dynamic config specified in
  alfresco-global.properties for which there is no pre-defined default in
  subsystem defaults (found during test with acme-group role mapping in
  alfresco-global.addition.properties)
@AFaust
Copy link
Member

AFaust commented Jul 16, 2022

@bmlong137 I've done a pass at this PR yesterday + today, and pushed an additional commit to your branch with some revisions, clean-up, and test config. I have renamed the core class, done a bit of pruning (i.e. LOGGER.isDebugEnabled(), comments that seemed redundant due to simple-to-read code...), and minor refactoring/simplification. I'd be willing to merge this state as-is, but please have a look before I do so and check if any of my changes may cause issues in your opinion.

Since not everyone may want to use roles for mapping to groups, I also tried to see whether I could add an additional authority extractor to handle a potential groups claim in the token, but I could not get a Keycloak config to work reliably which mapped the user's groups into the token, and would also have to decide how to handle group paths / nesting in that case in your group sync / processor. So I am definitely leaving that for a future enhancement.

@AFaust
Copy link
Member

AFaust commented Jul 16, 2022

Oh - and forgot to mention the most important change in my opinion: I made the group sync processor independent of the granted authority handling. I found it made more sense that it did not require the other processor to be enabled, as it would (partially) solve the same/similar case by synchronising group memberships (if configured), so some of the granted authorites would not actually be necessary anymore. I can see that in some configurations the groups processor is enabled but granted authority processor is not.

@nschwalbe
Copy link

nschwalbe commented Mar 24, 2023

Hi @AFaust , any news here? I'm also interested in setting the groups on login because the synchronize job is to late for newly created users. They do not see their site because of missing groups and have to wait until the synchronize jobs runs. We synchronize every 15min but its still not a satisfactory solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants