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
Browser notifications for scheduled reports #14492
Conversation
…ification on server-side when scheduled report is run
Feedback:
|
Some very basic example of asking the user for consent and not doing anything if he denied. function notifyMe() {
// Let's check if the browser supports notifications
if (!("Notification" in window)) {
alert("This browser does not support desktop notification");
}
// Let's check whether notification permissions have alredy been granted
else if (Notification.permission === "granted") {
// If it's okay let's create a notification
var notification = new Notification("promato QBM", { body: "Ein Chat hat eine neue Antwort erhalten.", icon: "/styles/pages/default/images/dash/visitors.png" });
}
// Otherwise, we need to ask the user for permission
else if (Notification.permission !== 'denied') {
Notification.requestPermission(function (permission) {
// If the user accepts, let's create a notification
if (permission === "granted") {
var notification = new Notification("promato QBM", { body: "Ein Chat hat eine neue Antwort erhalten.", icon: "/styles/pages/default/images/dash/visitors.png" });
}
});
}
// At last, if the user has denied notifications, and you
// want to be respectful there is no need to bother them any more.
} |
@tsteur can you confirm that browser notifications would only be sent when the user has the Matomo Web app opened in a tab? Or would you expect to have some kind of service worker or so (if that is even possible) to send notifications even when the Matomo app is not opened? |
Would definitely expect to receive them when the Matomo app is not open. |
Next steps:
|
We had previously decided not to use the Push API as this means sending notifications through Firebase (a Google service) when users are on Chrome. I have moved my code for polling into a service worker but this will need to be reworked to remove the dependency on ajaxHelper (ajaxHelper depends on jQuery which depends on the DOM which is not available to a service worker). Also need to add logic to ensure that the service worker is registered if a user who has at least one browser report visits Matomo in a new browser. Some UI tests for the report editor have been moved from The service worker is in the root directory of the Matomo project because I've seen warnings in tutorials that it may not be able to accept notifications from scripts that aren't on the same path otherwise. I don't know whether this is true or not, feel free to experiment and see what happens. How to test the service worker on your local Matomo instance
Implementing PushIf we do decide to use the Push API we'll need to subscribe for these after registering the service worker (example in Some useful resources for implementing the Push API:
|
Notes from call:
|
} | ||
} | ||
|
||
class XssProcessedMetric extends ProcessedMetric |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed to copy this code or can we do without maybe? or reuse the other fixture?
… serviceworker to controller method to get notifications
Any update @katebutler ? |
The serviceworker runs in a separate context, but when our app receives requests from it they are recognised as authenticated. Therefore we can use our normal auth logic rather than generating tokens for each report. |
👍 Seems Service worker can pass cookies when calling see also that the behavior was changed recently meaning the credentials:include is not required, but should help to get it working in most browsers: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
@katebutler since we don't need to implement the per-report token complexity (great we can keep this simpler!), what are the next steps to finish this project? |
Next steps:
|
I'm not so much into the topic @katebutler @mattab but seeing 2 problems re the sessions/cookies:
Those notifications should be likely independent of the regular session. |
…ER_TYPE to NOTIFICATION_TYPE, hide notification type from report editor when not on HTTPS, tidying up
Rewritten with a per-user notification. Token is generated in |
Interesting point... we were not too worried about this but it is a "data leak" issue indeed. Re-generating the token once a week could work. One concern, how can we tell the difference between these two:
(same as bullet point 1. above). What about if you're in an Internet cafe or shared computer, have allowed notifications in the shared browser, and you realise later you don't want the notifications enabled, how would you remove the authorizations to make sure that nobody else can see the notifications? @tsteur would be great if you can review what is done so far and leave your thoughts on the secure token re-generation process? |
We could update the token for example whenever the user logs in... or if the user logs in and was last updated a week ago. Ideally we would just need to make sure there won't be a race condition like we kind of need to make sure that the service worker has received the updated token etc but that be maybe an edge case. Always using the same token not sure... it gives access to sensitive data and who knows what it will be used for in the future (but scared about this since it's on the user now and not the report).
Any data leak can be a serious issue for many organisations, businesses, governments, etc. |
// Check periodically for new notifications | ||
this.timerId = setInterval(function() { | ||
fetchNotifications(); | ||
}, 30 * 1000); // Timeout is in ms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I see this right we're fetching for notifications every 30 seconds? Could we do this maybe only once per hour or maybe even only every 3 hours? or 6 hours?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, 30 seconds for dev purposes while it's still a WIP, will be changed to something much less frequent once it's ready to go. 1 hour, 4 hours and 6 hours have all been suggested previously...
self.token = new URL(location).searchParams.get('token'); | ||
}); | ||
|
||
self.addEventListener('activate', async function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so much into service workers. The activate event might be only triggered once? Will it still work after closing and opening browser, after updating the service worker, and after updating to a new browser version and all these things? I reckon it looks good to use the activate event here like this just wanting to be sure.
I reckon the activate event will be executed again if there's an update. Do we need to do anything when the service worker?
The activate event fires after a successful installation. You can use it to phase out older versions of the worker. We'll look at a basic example where we deleted stale cache entries.
I guess here's nothing to phase out of the older versions so should be all good I suppose.
notifications-sw.js
Outdated
fetch('index.php?module=MobileMessaging&action=getBrowserNotifications&token=' + self.token, | ||
{credentials: "omit"} | ||
).then(function(response) { | ||
if (response.status === 200) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon a 301 or 302 might be fine as well? We've seen users sometimes redirect these requests. Although in this case shouldn't happen as much.
FYI: I know some users have basic auth etc set up. We may need to mention in an FAQ that it won't work in this case.
Because we will get reports from users that for some reason it doesn't work, can we log errors to the console? Eg status code, message, ... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested it yet, only quickly went over the PR. Be good to look at the comments.
notifications-sw.js
Outdated
|
||
var displayNotifications = function(response) { | ||
response.json().then(function(notifications) { | ||
notifications.forEach(function(notification) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be good to check that notifications is an array. We've had random problems there sometimes in the past
@@ -155,4 +158,32 @@ public function getCredentialFields() | |||
|
|||
return $view->render(); | |||
} | |||
|
|||
public function getBrowserNotifications() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this into an API method please? In the request URL you then maybe need to append &format=json
.
This allows other users to use the token as well and embed notifications into other apps using other formats etc.
$login = null; | ||
$token = Common::getRequestVar('token'); | ||
|
||
if ($token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check how we can prevent brute force here.
In theory we could add something like this 'API.MobileMessaging. getBrowserNotifications' => 'beforeLoginCheckBruteForce',
here: https://github.com/matomo-org/matomo/blob/3.10.0-b3/plugins/Login/Login.php#L52
And then we would also need to call BruteForceDetection::addFailedAttempt
on a failed attempt. The problem: If the service worker plays up and doesn't have the right token, it may cause the user to be no longer able to log in as the IP would be always blocked. However, if we request the notifications only once per hour or only a few times per day anyway then it shouldn't be a problem.
Btw: should the method be more like getNotifications
or getUserNotifications
since the type is no longer called browser
?
@@ -169,4 +170,25 @@ public function unsubscribe() | |||
|
|||
return $view->render(); | |||
} | |||
|
|||
public function getNotifications() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no permission check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should check at least some view access.
@@ -169,4 +170,25 @@ public function unsubscribe() | |||
|
|||
return $view->render(); | |||
} | |||
|
|||
public function getNotifications() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually not even seeing where this method is used?
*/ | ||
private static function canSupportBrowserNotifications() | ||
{ | ||
return ProxyHttp::isHttps() || Development::isEnabled() || defined('PIWIK_TEST_MODE'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be || (defined('PIWIK_TEST_MODE') && PIWIK_TEST_MODE)
$this->migration = $factory; | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove all the method and class comments in this class? we usually remove them as they would be always the same otherwise.
public function updateReport($idReport, $idSite, $description, $period, $hour, $reportType, $reportFormat, $reports, $parameters, $idSegment = false, | ||
$evolutionPeriodFor = 'prev', $evolutionPeriodN = null, $periodParam = null) | ||
{ | ||
public function updateReport($idReport, $idSite, $description, $period, $hour, $reportType, $reportFormat, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there changes from a different PR? Same above and in the update script
public function getUserByNotificationToken($token) | ||
{ | ||
$db = $this->getDb(); | ||
return $db->fetchRow('SELECT * FROM ' . $this->table . ' WHERE notification_token = ?', $token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we store the token hashed ideally? It would be great to have this hashed in case there is a SQL injection, or a DB gets hacked, etc. see https://www.php.net/manual/en/function.password-hash.php
we could reuse Piwik\Auth\Password
We've always had the trouble with the token_auth
being in plain text which we will now finally hash in Matomo 4 and be great to avoid storing passwords in plain text in the future.
It means we need to generate the token when we install the service worker basically. If we don't have the token at a certain time when we need it, then we could possibly regenerate the token and update the service worker.
It would be fine to store the token in the session for a very short time I suppose (only if really needed but ideally not since sessions are stored in the DB).
@@ -335,6 +341,13 @@ public function updateUserTokenAuth($userLogin, $tokenAuth) | |||
)); | |||
} | |||
|
|||
public function updateNotificationToken($userLogin, $token) | |||
{ | |||
$this->updateUserFields($userLogin, array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above, we should hash the tokens.
What would happen if the user was already logged in though? Their notifications on all browsers would stop working within a week, until next time they interacted with Matomo, even if they were still actually logged in (i.e. if they had remember me turned on). |
I'm trying to understand hashing the token would work, have I got it right?
So far so good, but ... So we would need some way of keeping the raw tokens in sync across multiple browsers/devices - or is it OK to say they can only receive them on one device at a time? |
I suppose in this case we would actually need to create multiple tokens one for each device. I know it's getting really complicated for this issue. On the other side we can't really store such tokens in plain text. It does start to go into the direction of #6559 and maybe we could have a table that can be reused by that feature later like
In theory we could even make it more secure by restricting a browser type to a specific token like I'm wondering how would another device be able to get this token. Like in the Android/iOS mobile app or another browser. I suppose we would be able to generate such a token through the API by providing the Like usually you would log in to the mobile app, then we notice there are notifications configured, then the OS asks for permissions to show notifications, then it works. But if the API requires a password we would either before or after the OS asks for permissions also ask for a password. Might be secure enough to generate a token by only providing the @mattab @diosmosis any thoughts? I suppose it would need to require the actual password to generate such a |
Haven't read all the comments, but here are some thoughts:
This always happens for me after a user logs in, so it seems like the credentials would be available at this point.
Could this be solved by only allowing one active token per device at a time? Ie, as soon as one token is created, the old becomes invalid. Actually if an attacker has the token auth, they could probably do worse things than get notifications. |
You would just need to generate and store another token to ensure there is only one token per device. And an attacker could just send it from different devices or pretend to be different devices.
It's just that if an attacker created multiple users, that would be visible. And if you noticed your token leaked, you could generate a new token. Those other tokens would currently not be visible anywhere and you would maybe never notice these were created. Unless we also built some management for it but then we would maybe need to wait with this feature for #6559 .
In the mobile app the credentials wouldn't be available since the auth token is stored on the device. |
Yes, but then a user might notice if their notifications stop working. For a device they don't use they wouldn't
Perhaps having an "authorized devices" feature would be helpful (w/ notification emails like "your login was just used on a new device, if this wasn't you... blah blah blah ..."). Did we add this? I remember working on or talking about something like this in the fairly recent past... Anyway, this would make it a lot less of a problem to use the token auth. It would work like github's Sessions section in the Security settings page. Of course it would still mean working on something else before merging this, but a feature like this might provide enough value to warrant it. |
# Conflicts: # tests/UI/specs/UIIntegration_spec.js
Have addressed most PR comments. We still don't seem to have a clear picture of how the token re-generation is going to allow "legitimate" devices to continue receiving notifications, while invalidating any that shouldn't. |
@katebutler let's postpone this feature to Matomo 4 or when we implement #6559 This might be a good idea cause similar logic will be needed there and in Matomo 4 when we basically remove |
Shelved until after #6659 is implemented. Next steps would be:
|
@mattab I'd simply close this issue as it's not high value but be still quite some time to make this work. Initially, we thought it be quick to do so it was ok to work on it but it's actually quite some work. Plus we'd need to maintain it later and be able to debug things etc. Besides, we would need to keep storing the token in the browser etc and I reckon it's all complex and not worth it. |
Next steps:
Implement storage of multiple notifications (in the options table as is? somewhere else? in a new table?)
Invalidate notifications in DB after they have been shown to the user
PHP tests for:
-- Writing notifications to server
-- Fetching current notifications via Controller route
-- Invalidating notifications
Angularify the JS for polling the server
Decide how to handle browsers with notifications blocked
Find/make an icon for browser notifications (currently just using the SMS one)
UI tests for:
-- Setting up browser-type reports (e.g. changes the format, parameters etc appropriately when user chooses "Browser", lets them save)
-- Actually receiving the notifications (might be tricky with user needing to grant permission first)
Test other browsers on BrowserStack
-- Firefox (requires HTTPS), Safari (desktop only), Edge, IE (not sure if supported)
Finish rewriting existing functionality
Ask browser to register service worker on new visit (but only if user has browser notification reports)
Test other browsers on BrowserStack
Fixes #13989