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

Send email confirmation when changing a user's email address #14110

Closed
wants to merge 6 commits into from

Conversation

diosmosis
Copy link
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

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Feb 18, 2019
@diosmosis diosmosis added this to the 3.9.0 milestone Feb 18, 2019
@tsteur
Copy link
Member

tsteur commented Feb 18, 2019

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
Copy link
Member Author

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
Copy link
Member

tsteur commented Feb 18, 2019

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
Copy link
Member Author

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
Copy link
Member

tsteur commented Feb 18, 2019

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
Copy link
Member Author

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
Copy link
Member

tsteur commented Feb 18, 2019

Yeah that's why I mentioned lost password here: #14110 (comment)

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
Copy link
Member Author

CC @mattab

@mattab
Copy link
Member

mattab commented Feb 20, 2019

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 #13533

@sgiehl
Copy link
Member

sgiehl commented Feb 25, 2019

@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
Copy link
Member

tsteur commented Feb 25, 2019

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 diosmosis changed the title Send email verification when changing a user's email address Send email confirmation when changing a user's email address Feb 25, 2019
@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Feb 25, 2019
@diosmosis
Copy link
Member Author

Closing in favor of #14136

@diosmosis diosmosis closed this Feb 26, 2019
@diosmosis diosmosis deleted the 13533-verify-email-change branch February 26, 2019 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants