-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
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? |
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. |
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? |
Just to spam someone w/ reports or alerts I suppose. Though I suppose there are easier ways of doing that than going through Matomo. |
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. |
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. |
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. |
CC @mattab |
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)
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 |
👍 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. |
Closing in favor of #14136 |
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