@diosmosis opened this Pull Request on February 18th 2019 Member

API requests to UsersManager.updateUser will now trigger an email verification email. When the email is verified, the new email is set, until then the old email is used.

I think this has the possibility to break some API uses. If anyone is updating user emails automatically, in a seamless fashion, they may not want the email verification to go out. I suppose, we could add an INI config setting to disable it... or another parameter to disable if the user is a superuser. In either case, users will have to do something for their code to work.

Fixes #13533

@tsteur commented on February 18th 2019 Member

I didn't realise this issue was scheduled in a milestone. @Findus23 @diosmosis @mattab I'm not sure this issue is actually needed? And if so, we should almost maybe undo the asking for password again.

I think it's actually often that you create users with a fake email eg for API etc. If the password is lost fo this user, you just set a new password as an admin or delete and create the user. For these users you would probably not change the email address so it's fine. Also when you add users you would need to verify email in this case...

Not sure #13533 is really needed? From a security point of view I'm not 100% getting it as we're already asking for the user's password. From a usability point it can make sense, but might not be really needed?

@diosmosis commented on February 18th 2019 Member

I think the idea is if the user itself is acting maliciously, don't allow them to set the email to any email. I ignored addUser since there's another ticket in 3.9 about inviting users via email, which would act as verification.

@tsteur commented on February 18th 2019 Member

if the user itself is acting maliciously, don't allow them to set the email to any email

not sure what would be the malicious part here? maybe could simply ask to enter email address twice in case user has a typo in the email?

In general we might also need a feature to disable this check but can be always added later. Thinking of web hosters and other users that may use those features in the background and wouldn't want the user to validate the email address as it's already validated in their system. Also ldap and other plugins may not need this feature?

@diosmosis commented on February 18th 2019 Member

not sure what would be the malicious part here?

Just to spam someone w/ reports or alerts I suppose. Though I suppose there are easier ways of doing that than going through Matomo.

@tsteur commented on February 18th 2019 Member

Yeah I would definitely say there are easier ways to spam people. Eg you could still just enter any email address in scheduled reports etc. Not sure what the benefit would be of spamming people like this through Matomo. Most of the emails that are sent would be basically only scheduled reports or alerts. And there you could still email anyone pretty much.

@diosmosis commented on February 18th 2019 Member

Actually, this is an important email, w/o it, it can be difficult to, eg, reset your password (and I know, I would very much prefer not to have to email an admin and wait for them to do something). I think there's value in making sure emails can be successfully sent and viewed there, which is maybe why my bank, for instance, requires both entering your password and verifying the email address.

@tsteur commented on February 18th 2019 Member

Yeah that's why I mentioned lost password here: https://github.com/matomo-org/matomo/pull/14110#issuecomment-464536180

thinking though it could be easier to just ask to enter email twice for example. Just wanting to avoid an overly complex solution when it maybe doesn't have to be. There are also use cases where you maybe want to use a non existent email but that's not too much of an issue.

If PR will be merged then be good to have a config setting to fully ignore this feature. This will be needed for users who integrate Matomo into other systems.

@diosmosis commented on February 18th 2019 Member

CC @mattab

@mattab commented on February 20th 2019 Member

Not sure #13533 is really needed? From a security point of view I'm not 100% getting it as we're already asking for the user's password. From a usability point it can make sense, but might not be really needed?

I think the part about verifying the new email is not needed (for now anyway...)

But the other part is valuable (from security point of view), where we send an email to inform a user their user's email address was changed in Matomo (and which email address it was changed to, and by which IP address)

Actually, this is an important email, w/o it, it can be difficult to, eg, reset your password (and I know, I would very much prefer not to have to email an admin and wait for them to do something). I think there's value in making sure emails can be successfully sent and viewed there, which is maybe why my bank, for instance, requires both entering your password and verifying the email address.

I feel it's not necessary to have the email verification added, and it keeps things a bit simpler maybe (otherwise we also need to report the status in the UI, invite users to check & verify their email, maybe report in the list of users which users have an un-verified email etc.), so i've removed the milestone from https://github.com/matomo-org/matomo/issues/13533

@sgiehl commented on February 25th 2019 Member

@mattab @tsteur any common sense how to proceed with this PR? Or shall we move the PR out of the current milestone as well?

@tsteur commented on February 25th 2019 Member

But the other part is valuable (from security point of view), where we send an email to inform a user their user's email address was changed in Matomo (and which email address it was changed to, and by which IP address)

👍 this sounds good and quick to do.

Could also ask twice for the email and verify both fields contain same value but maybe not needed.

@diosmosis commented on February 26th 2019 Member
This Pull Request was closed on February 26th 2019
Powered by GitHub Issue Mirror