feature: update user on login #4060
Conversation
There was a problem hiding this comment.
Thank you for offering this PR.
I've reviewed the changes and added some comments.
If you have any questions, or need any help, just shout.
In addition to those, it would also be ideal to have this added functionality covered by testing. Would you be willing to add testing to cover your changes?
| $user->update([ | ||
| 'name' => $name, | ||
| 'email' => $email, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Could this logic be moved to it's own method within this class instead?
Passing a flag argument to trigger extra logic starts to make this method a little muddy, would prefer a focused method that's optionally called (depending on setting) from the Saml2service side.
If you want to specifically only update new instances I think there might be something within the Eloquent model for that, or you could resort to checking the created date.
| } else if ($updateUser && ($user->name !== $name || $user->email !== $email)) { | ||
| $user->update([ | ||
| 'name' => $name, | ||
| 'email' => $email, |
There was a problem hiding this comment.
The email should be user-unique so it might be wise to have a check before updating, but there becomes a question of what to do on error.
There's also a question of how this interacts with email confirmation and domain restriction.
Don't want to complicate things thought so it might just be that those are left alone but we ensure there are warnings against the setting documentation about email confirmation/domain restriction.
There was a problem hiding this comment.
Is there really the need for email-verification in registrations from SSO-Services? In my oppinion this information must be verified by the identity-prodiver. I would suggest that in such cases a verified address is assumed.
There was a problem hiding this comment.
What do you think about removing email confirmation/domain restriction from SSO-Logins in general?
There was a problem hiding this comment.
What do you think about removing email confirmation/domain restriction from SSO-Logins in general?
I am not willing to remove a feature that could be in use, just in the interest of making a PR easier to merge.
We can ignore these specific elements though and, as mentioned above, we'd just need to ensure these edge cases are in the documentation for the setting.
There was a problem hiding this comment.
This was not meant for making the PR easier to merge. I thought this would be a solution for preventing an inconsistent email-confirmation-feature in SAML-auth rather just "ignoring" it.
Now the question is how to resolve this, while maintaining consistency in function. How about a new config-parameter for email-domain-restrictions in SAML (and another parameter for every other auth-method)? So you can use domain-restriction on local-auth and disable it on saml-auth.
There was a problem hiding this comment.
As said above, I'd advise we ignore these specific elements in functionality and defer to documenting the implications instead, rather than increasing the surface area and scope of the PR.
| 'email' => $email, | ||
| ]); | ||
|
|
||
| Activity::add(ActivityType::USER_UPDATE, $user); |
There was a problem hiding this comment.
Please could this be removed?
Activities generally reflect user actions or high-level processes, rather than raw changes to data.
While the user data is technically being updated, no one is (intentionally) actioning a user update themselves.
|
This is related to #3771. Ideally we'd align functionality across these methods, but SAML2 can be a starting point. |
To simplify user-management this pull request implements a feature, which updates the user's display name and email on log-ins using SAML2.
It is enabled using a new config-parameter: SAML2_UPDATE_USER_ON_LOGIN.