plugin-LoginLdap
plugin-LoginLdap copied to clipboard
matomo-org
Unnecesarry warning on first LDAP user sync
When LDAP user has already their account in matomo DB created manually, then on their first login warning is logged:
Unable to synchronize LDAP user 'userloginhere', non-LDAP user with same name exists.
No such warning on further logins of same user.
Seems this warning makes no sense because after this warning user is marked as synced from LDAP and further updates from LDAP work fine for them.
Consider removing this warning and always update user from LDAP if LDAP user sync is enabled and user matches.
Hi @pboguslawski . Thank you for taking the time to create this issue. It looks like that warning is there to be informative. I suppose we could change it from WARNING to INFO and possibly reword the message to be more accurate. What do you think @AltamashShaikh ?
Problem is not only in message prio: now user is not synced from LDAP after first login (and synced from LDAP after further logins). Should be synced after first login too if LDAP sync is enabled.
Just checking if I understand the ticket correctly:
- when LDAP is enabled
- and a user creates an account in matomo DB manually
- and someone logs in with that account
- the first time they log in, they see a warning with the text
Unable to synchronize LDAP user 'userloginhere', non-LDAP user with same name exists. - however, the account is being synced
- and the next time they log in this message doesn't show up because the account is now synced
Did I understand that correctly?
If so, I see various options:
- sync could happen before login (but that would be a workflow change, introduce the risk of serious bugs, and probably quite some work)
- show/hide logic of the message could be updated to skip the first login (potentially easier, but some amount of engineering and testing work would be needed)
- message can be augmented with something like
If this is your first time logging in with this account, you can ignore this message.. That would be easy, but not very polished. - delete the message altogether. Not sure if the message is still relevant for any edge cases though, e.g. if the sync didn't work
Keen to read your thoughts
Hi @pboguslawski . Thank you for taking the time to create this issue. It looks like that warning is there to be informative. I suppose we could change it from WARNING to INFO and possibly reword the message to be more accurate. What do you think @AltamashShaikh ?
@snake14 Yes a warning is not required, we should change that for sure and @Stan-vw since the user gets synced after first login we could do the same at the time of login
Just checking if I understand the ticket correctly:
[...]
- however, the account is being synced
[...]
Did I understand that correctly?
It's synced but not on the first login (here only warning is logged and account is set as LDAP account without syncing) but on further logins (here LDAP sync after every login).
Keen to read your thoughts
I don't see any point in logging such message and different sync behaviour on first and further logins. Consider removing this warning and update user from LDAP after every login if LDAP user sync is enabled and user matches.
@pboguslawski You are correct doesn't make sense to have the if else logic and we can remove the same as the existing user is marked as LDAP user on first login but not synced
Is the message relevant in later logins if for some reason the sync failed? Or is there no downside to simply removing the message?
To be honest, since the message doesn't have a clear "do this in case you see this error" I'm not even sure the message has value in case it would only show up in the exact scenario where it is relevant. This might be more relevant to show in a superuser's config screen than in the normal user's screen after logging in
@Stan-vw Checking the code it doesn't seems useful to have this extra case just for 1st time login as the skipped part would be done next time you login.