- Issue created by @minoroffense
- 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
I think it's because [...] so the user doesn't exist yet so I can't assign them to a group.
I suspect that what you say here is true: you can't assign new accounts to a group. But that's on the Group module, or on you, to work around; it's not on the samlauth module to fix. (And also if the Group module does not specify this fact, then is that either a bug or an omission in their documentation? I don't know the module's details.) See below for suggested "fix/workaround".
This quote also suggests you have not really done point 2 of your "steps to reproduce" -- unless I am misreading you and you are implicitly confirming that samlauth's usersync is in fact being dispatched. I have no reason to believe it is not; see the link to samlauth_user_presave() that you point to. (Also: I don't understand point 3. But that's probably unimportant.)
So I'm guessing that 1 of 2 things is happening:
- The groups module is not able to correctly assign to a new user account that is not saved yet (i.e. that has no ID and where isNfonew() is true).
- Your code is somehow buggy. (I quickly looked at your event dispatcher, and I don't think this is the case, so I suspect point 1. I would think you need to call
$event->markAccountChanged()
if you want to be able to sync/change mappings for non-new accounts, but that's separate from the issue you're reporting.)
[...] I figure I'd ask here to see if there's any more context as to why we're using presave instead of login
1. I want to have the same event, where everyone can execute the exact same code, regardless whether an account is new. So no code duplication is necessary for new vs existing accounts.
2. On login, your new user account is already saved, outside the samlauth module's reach. That means the user account would get saved twice: just before login, and during/after login when you update account data from SAML attributes. If that was really necessary, I would live with that, but.... it also means that if, during USER_SYNC, something goes horribly wrong and/or your business logic decides it has to throw prevent login based on SAML attribute values... Drupal now has an already-saved user with half-baked data saved. This can in theory lead to heaps of crap in the users table.
I don't want to enable that possibility. (And it was a legitimate concern with the application I used to work on.) Noone's convinced me so far, to compromise on this point.
(Using the user_presave() hook for this, is very sub-ideal and makes the samlauth code into unfortunate spaghetti. The solution for this will be reorganizing the code / order of events in the underlying externalauth module, so that I can deprecate the USER_SYNC event and use one of the externalauth events. But when that finally hopefully happens... it won't solve your issue. Because that will be dispatched before user save, for the above reason.)
What I think you should probably do in your case:
- If still needed: Check more thoroughly if you really cannot
$group->addMember($account)
on a new account, and have it saved. - If that is the case, then yes you need another event. You should be able to use the ExternalAuthLoginEvent (or hook_user_login but the event probably fits better), and you can inject SamlService and check SamlService->getAttributes(). If that returns any values, you can assume a SAML login just got processed and do your thing. It may feel weirdly disconnected, but it should work without issues.
- (If you want to be pedantic about being sure not to do unnecessary double-saves for existing users: process your new users on ExternalAuthLoginEvent and your existing users on samlauth usersync. But I don't really know why you would.)
It is kind-of strange that ExternalAuthLoginEvent is not mentioned anywhere and I had no earlier conversation thread to refer to, but the topic hasn't come up yet in this way: you could be the first person that has asked here and that IMHO has a documented legitimate reason to use an event listener after the user is already saved. I may point to this thread from the first Q/A in the README.
- 🇨🇦Canada minoroffense Ottawa, Canada
Wow well thanks for the very thorough answer. I'll look into the event from the external auth module and yeah like you said see about handling it with that and the samlauth service.
I've updated the title and changed this to a support request because yeah after going over it again and your answer makes sense that it's not a bug here.
- 🇨🇦Canada minoroffense Ottawa, Canada
Closing as we've used a different event for the group sync. Thanks again for your advice.