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

Unnecesarry warning on first LDAP user sync #374

Open
pboguslawski opened this issue May 24, 2024 · 8 comments
Open

Unnecesarry warning on first LDAP user sync #374

pboguslawski opened this issue May 24, 2024 · 8 comments

Comments

@pboguslawski
Copy link

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.

@snake14
Copy link
Contributor

snake14 commented May 26, 2024

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 ?

@pboguslawski
Copy link
Author

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.

@Stan-vw
Copy link

Stan-vw commented May 26, 2024

Just checking if I understand the ticket correctly:

  1. when LDAP is enabled
  2. and a user creates an account in matomo DB manually
  3. and someone logs in with that account
  4. 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.
  5. however, the account is being synced
  6. 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:

  1. sync could happen before login (but that would be a workflow change, introduce the risk of serious bugs, and probably quite some work)
  2. 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)
  3. 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.
  4. 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

@AltamashShaikh
Copy link
Contributor

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

@pboguslawski
Copy link
Author

Just checking if I understand the ticket correctly:

[...]

  1. 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.

@AltamashShaikh
Copy link
Contributor

@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

@Stan-vw
Copy link

Stan-vw commented May 27, 2024

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

@AltamashShaikh
Copy link
Contributor

@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.

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

No branches or pull requests

4 participants