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.

Sign up for GitHub

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 notification when user email changes. #14136

Merged
merged 8 commits into from
Mar 7, 2019
Merged

Send email notification when user email changes. #14136

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3f1666c
Send email notification when user email changes.
diosmosis Feb 26, 2019
980bba5
Forgot to add file.
diosmosis Feb 26, 2019
ce0d380
Apply pr fixes + send email for password changes too.
diosmosis Mar 1, 2019
3946637
Add quick test for new emails.
diosmosis Mar 1, 2019
8d7f80c
Translate text
diosmosis Mar 4, 2019
a8e5cb6
Refactor according to review.
diosmosis Mar 6, 2019
41c371b
ucfirst device name
diosmosis Mar 7, 2019
9dbc53d
Fixing integration test
diosmosis Mar 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lang/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,9 @@
"YouAreCurrentlyUsing": "You are currently using Matomo %s.",
"YouAreViewingDemoMessage": "You are viewing the demo of %1$sMatomo%2$s",
"YouMustBeLoggedIn": "You must be logged in to access this functionality.",
"YourChangesHaveBeenSaved": "Your changes have been saved."
"YourChangesHaveBeenSaved": "Your changes have been saved.",
"ThankYouForUsingMatomo": "Thank you for using Matomo",
"TheMatomoTeam": "The Matomo Team"
},
"Mobile": {
"AboutPiwikMobile": "About Matomo Mobile",
Expand Down
80 changes: 79 additions & 1 deletion plugins/UsersManager/API.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/
namespace Piwik\Plugins\UsersManager;

use DeviceDetector\DeviceDetector;
use Exception;
use Piwik\Access;
use Piwik\Access\CapabilitiesProvider;
Expand All @@ -17,6 +18,8 @@
use Piwik\Config;
use Piwik\Container\StaticContainer;
use Piwik\Date;
use Piwik\IP;
use Piwik\Mail;
use Piwik\Metrics\Formatter;
use Piwik\NoAccessException;
use Piwik\Option;
Expand All @@ -26,7 +29,7 @@
use Piwik\SettingsPiwik;
use Piwik\Site;
use Piwik\Tracker\Cache;
use Piwik\Url;
use Piwik\View;

/**
* The UsersManager API lets you Manage Users and their permissions to access specific websites.
Expand Down Expand Up @@ -915,6 +918,14 @@ public function updateUser($userLogin, $password = false, $email = false, $alias

Cache::deleteTrackerCache();

if ($email != $userInfo['email']) {
$this->sendEmailChangedEmail($userInfo, $email);
}

if ($passwordHasBeenUpdated) {
$this->sendPasswordChangedEmail($userInfo);
}

/**
* Triggered after an existing user has been updated.
* Event notify about password change.
Expand Down Expand Up @@ -1379,4 +1390,71 @@ private function getRoleAndCapabilitiesFromAccess($access)
}
return [$roles, $capabilities];
}

private function sendEmailChangedEmail($user, $newEmail)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we maybe mostly reuse the same code for sendEmailChangedEmail and sendPasswordChangedEmail and also it's templates and only have different mail subject and also different leading sentence in the email? not too important though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about it, but since there were just two emails, and the code isn't really complicated (just setting things on the view & Mail instance), it didn't seem that important. Maybe if there are more changes to a user that we should notify them about?

{
$initiatingDevice = $this->getInitiatingDeviceInfo();

// send the mail to both the old email and the new email
foreach ([$newEmail, $user['email']] as $emailTo) {
$view = new View('@UsersManager/_emailChangedEmail.twig');
$view->accountName = Common::sanitizeInputValue($user['login']);
$view->newEmail = Common::sanitizeInputValue($newEmail);
$view->ipAddress = IP::getIpFromHeader();
$view->deviceName = $initiatingDevice['deviceName'];
$view->deviceBrand = $initiatingDevice['deviceBrand'];
$view->deviceModel = $initiatingDevice['deviceModel'];

$mail = new Mail();

$mail->addTo($emailTo, $user['login']);
$mail->setSubject(Piwik::translate('UsersManager_EmailChangeNotificationSubject'));
$mail->setDefaultFromPiwik();
$mail->setWrappedHtmlBody($view);

$replytoEmailName = Config::getInstance()->General['login_password_recovery_replyto_email_name'];
$replytoEmailAddress = Config::getInstance()->General['login_password_recovery_replyto_email_address'];
$mail->setReplyTo($replytoEmailAddress, $replytoEmailName);

$mail->send();
}
}

private function sendPasswordChangedEmail($user)
{
$initiatingDevice = $this->getInitiatingDeviceInfo();

$view = new View('@UsersManager/_passwordChangedEmail.twig');
$view->accountName = Common::sanitizeInputValue($user['login']);
$view->ipAddress = IP::getIpFromHeader();
$view->deviceName = $initiatingDevice['deviceName'];
$view->deviceBrand = $initiatingDevice['deviceBrand'];
$view->deviceModel = $initiatingDevice['deviceModel'];

$mail = new Mail();
$mail->addTo($user['email'], $user['login']);
$mail->setSubject(Piwik::translate('UsersManager_PasswordChangeNotificationSubject'));
$mail->setDefaultFromPiwik();
$mail->setWrappedHtmlBody($view);

$replytoEmailName = Config::getInstance()->General['login_password_recovery_replyto_email_name'];
$replytoEmailAddress = Config::getInstance()->General['login_password_recovery_replyto_email_address'];
$mail->setReplyTo($replytoEmailAddress, $replytoEmailName);

$mail->send();
}

private function getInitiatingDeviceInfo()
{
$userAgent = isset($_SERVER['HTTP_USER_AGENT']) ? $_SERVER['HTTP_USER_AGENT'] : '';

$uaParser = new DeviceDetector($userAgent);
$uaParser->parse();

return [
'deviceName' => $uaParser->getDeviceName(),
'deviceBrand' => $uaParser->getBrandName(),
'deviceModel' => $uaParser->getModel(),
];
}
}
8 changes: 7 additions & 1 deletion plugins/UsersManager/lang/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,12 @@
"AreYouSureAddCapability": "Are you sure you want to give %1$s the %2$s capability for %3$s?",
"AreYouSureRemoveCapability": "Are you sure you want to remove the %1$s capability from %2$s for %3$s?",
"IncludedInUsersRole": "Included in this user's role.",
"Capability": "Capability"
"Capability": "Capability",
"EmailChangeNotificationSubject": "Your Matomo account's email address has just been changed",
"EmailChangedEmail1": "The email address associated with your account has been changed to %1$s",
"EmailChangedEmail2": "This change was initiated from the following device: %1$s (IP address = %2$s).",
"IfThisWasYouIgnoreIfNot": "If this was you, feel free to ignore this email. If this was not you, please login, correct your email address, change your password and contact your Matomo administrator.",
"PasswordChangeNotificationSubject": "Your Matomo account's password has just been changed",
"PasswordChangedEmail": "Your password has just been changed. The change was initiated from the following device: %1$s (IP address = %2$s)."
}
}
9 changes: 9 additions & 0 deletions plugins/UsersManager/templates/_emailChangedEmail.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<p>{{ 'General_HelloUser'|translate('<strong>' ~ accountName ~ '</strong>')|raw }}</p>

<p>{{ 'UsersManager_EmailChangedEmail1'|translate('<strong>' ~ newEmail ~ '</strong>')|raw }}.</p>

{% set deviceDescription %}{{ deviceName }}{% if deviceBrand is not empty or deviceModel is not empty %} ({% if deviceBrand is not empty %}{{ deviceBrand }}{% endif %}{% if deviceModel is not empty %} {{ deviceModel }}{% endif %}){% endif %}{% endset %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the device name always set? Looking at the code it may be empty but probably isn't really often the case. May not be too useful though to show whether it was updated from desktop or mobile if no device was detected. Not too important though and fine to leave it just like that.

Copy link
Member

@tsteur tsteur Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reckon there can be various cases where user agent may not be set when details are updated through the API. Therefore device name may be empty on occasion

<p>{{ 'UsersManager_EmailChangedEmail2'|translate(deviceDescription, ipAddress) }} {{ 'UsersManager_IfThisWasYouIgnoreIfNot'|translate }}</p>

<p>{{ 'General_ThankYouForUsingMatomo'|translate }}!
<br/>The Matomo team</p>
diosmosis marked this conversation as resolved.
Show resolved Hide resolved
9 changes: 9 additions & 0 deletions plugins/UsersManager/templates/_passwordChangedEmail.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<p>{{ 'General_HelloUser'|translate('<strong>' ~ accountName ~ '</strong>')|raw }}</p>

{% set deviceDescription %}{{ deviceName }}{% if deviceBrand is not empty or deviceModel is not empty %} ({% if deviceBrand is not empty %}{{ deviceBrand }}{% endif %}{% if deviceModel is not empty %} {{ deviceModel }}{% endif %}){% endif %}{% endset %}
<p>{{ 'UsersManager_PasswordChangedEmail'|translate(deviceDescription, ipAddress) }}</p>

<p>{{ 'UsersManager_IfThisWasYouIgnoreIfNot'|translate }}</p>

<p>{{ 'General_ThankYouForUsingMatomo'|translate }}!
<br/>The Matomo team</p>
diosmosis marked this conversation as resolved.
Show resolved Hide resolved
13 changes: 13 additions & 0 deletions plugins/UsersManager/tests/Integration/APITest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Piwik\Access\Role\Write;
use Piwik\Auth\Password;
use Piwik\Container\StaticContainer;
use Piwik\Mail;
use Piwik\Option;
use Piwik\Piwik;
use Piwik\Plugins\SitesManager\API as SitesManagerAPI;
Expand Down Expand Up @@ -293,6 +294,11 @@ public function test_setUserPreference_throws_whenPreferenceNameContainsUndersco

public function test_updateUser()
{
$capturedMails = [];
Piwik::addAction('Mail.send', function (Mail $mail) use (&$capturedMails) {
$capturedMails[] = $mail;
});

$identity = FakeAccess::$identity;
FakeAccess::$identity = $this->login; // ensure password will be checked against this user
$this->api->updateUser($this->login, 'newPassword', '[email protected]', 'newAlias', false, $this->password);
Expand All @@ -307,6 +313,13 @@ public function test_updateUser()
$passwordHelper = new Password();

$this->assertTrue($passwordHelper->verify(UsersManager::getPasswordHash('newPassword'), $user['password']));

$subjects = array_map(function (Mail $mail) { return $mail->getSubject(); }, $capturedMails);
$this->assertEquals([
'UsersManager_EmailChangeNotificationSubject', // sent twice to old email and new
'UsersManager_EmailChangeNotificationSubject',
'UsersManager_PasswordChangeNotificationSubject',
], $subjects);
}

public function test_updateUser_doesNotChangePasswordIfFalsey()
Expand Down