Send email notifications to a user whenever their settings are changed (2FA, token_auth, user is updated or deleted), and to super users when security settings are changed (brute force logic, 2FA, cors changes) #17531
Conversation
|
One thing: To anticipate an issue like #14447, I guess it would be useful to have an option to disable those E-Mails (even though I find it hard to know where to draw the line) |
For MVP this might not be needed and we could wait until this comes up as a request and then someone could even develop a plugin for this etc. |
There was a problem hiding this comment.
@diosmosis I updated my pr but added some questions as comments, can you please check those?
and still waiting for some comments from the other, I will modify those parts after the comment.
thanks
| if (count($this->superuserEmails)) { | ||
| foreach ($this->superuserEmails as $login => $email) { | ||
| return $email; | ||
| } | ||
| } |
There was a problem hiding this comment.
here it's possible we send the email to ourselves. should we find the first superuser email which is not us?
what if there is no other superuser? what is the recommendation in the email to the user?
There was a problem hiding this comment.
Not sure I understand, it should be sent to the user that enabled the two factor and nobody else?
There was a problem hiding this comment.
Like you can't enable it for a different user AFAIK?
There was a problem hiding this comment.
Oh i see now it's for who we mention to contact, I'll have a think
There was a problem hiding this comment.
Let's keep it simple and do same thing as for password change etc.
We could reuse the same translation key : UsersManager_IfThisWasYouPasswordChange. which says If this was you, feel free to ignore this email. If this was not you, please contact your Matomo administrator immediately, as your account might have been compromised! and this would maybe also make the other comments obsolete.
I wonder if we can document this for future reference somewhere but wouldn't really know where right now. It's really good to have this SecurityNotificationEmail base class now maybe this could provide such a method or constant or something to be reused across emails?
There was a problem hiding this comment.
I reused that translation key. I'll think about that method or constant.
|
@tsteur can you look at the comments pinging you above? There are some questions I can't answer. |
flamisz
force-pushed
the
new-email-notifications
branch
from
c7d1ae1
to
2147117
Compare
mattab
added
c: Security
mattab
changed the title
mattab
changed the title
|
I believe that this change breaks install via the ExtraTools. I really wish team Matomo would realize what the time is and #10257 got some attention. |
|
@bluikko I've had a look through the PR and not sure where this could break ExtraTools. Have you created an issue for this problem on the ExtraTools issue tracker? It may be related to something else or maybe the plugin developer can work around it. Re #10257 we may put some focus on it in the near future but I can't promise anything. |
Description:
Send email notifications when critical action happened.
Critical actions:
Review