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

Browser notifications for scheduled reports #14492

Closed
wants to merge 22 commits into from
Closed

Browser notifications for scheduled reports #14492

wants to merge 22 commits into from

Conversation

katebutler
Copy link

@katebutler katebutler commented May 27, 2019

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

@katebutler katebutler added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label May 27, 2019
@katebutler katebutler added this to the 3.12.0 milestone May 27, 2019
@mattab
Copy link
Member

mattab commented May 28, 2019

Feedback:

  • we want to keep it as simple as possible (KISS). Maybe we store all browser notifications reports, for one user, in a new row in option table (eg. as json). Then they are polled all at once from the UI and the row is cleared at the same time.
  • Only poll when there is at least one report of "browser notification" created
  • Angularify the JS may not be needed
  • how to handle browsers with notifications blocked
    • whenever a browser notification wants to be triggered, can we simply trigger the standard popup asking whether to allow browser notification.
    • And if the user has 'blocked' these notifications, or not allowed them for some reason (incognito etc.) and we know it (eg. in JavaScript), then maybe we do nothing and just ignore the problem. Could you draft an FAQ that lists the cases where a browser notification may not work?
  • icon, maybe an alarm bell like this https://www.iconfinder.com/search/?q=bell&style=flat&license=2&price=free&maximum=16 ?
  • UI tests, likely not possible to take screenshots of browser notifications, so would be enough to maybe test the API to get/poll the notifications via System tests?

@fdellwing
Copy link
Contributor

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.
}

@mattab
Copy link
Member

mattab commented Jun 7, 2019

@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?

@fdellwing
Copy link
Contributor

fdellwing commented Jun 7, 2019

@mattab Sending desktop notifications from closed websites is very much possible, but alot more complex than from an open website. You need, as you thought a service worker {1} for this and you also need the push API {2} to trigger the notifications server side.

@tsteur
Copy link
Member

tsteur commented Jun 7, 2019

Would definitely expect to receive them when the Matomo app is not open.

@katebutler
Copy link
Author

Next steps:

  • Research how to use Push API without Firebase (which is a Google service)
  • Use Push API instead of polling our server for notifications
  • Test other browsers in BrowserStack

@katebutler
Copy link
Author

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 UIIntegration_spec into MobileMessaging_spec (and arguably some/all of this could be merged into ScheduledReports_spec although it also tests logic specific to editing mobile and browser reports). To make these tests work the ReportSubscription fixture has had several new rows of reports added, meaning that some UI tests in both MobileMessaging_spec and ScheduledReports_spec fail due to having different values.

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

  • Open the report editor and select the 'Browser' notification type, which will trigger the logic in reportParametersScheduledReports_browser.twig. Your browser will ask you to grant the notifications permission, and then register the service worker.
  • When testing with Firefox you'll need to set up self-signed SSL on your localhost, see https://www.digitalocean.com/community/tutorials/how-to-create-a-self-signed-ssl-certificate-for-apache-in-ubuntu-16-04 for advice. It also won't work on Firefox if you are in private browsing mode.
  • I couldn't get Chrome to accept the SSL certificate but it trusts localhost enough to allow it to register service workers when loaded over HTTP. Again, it won't work if you're in incognito mode.
  • Has not been tested on IE, Edge or Safari. From what I've read it won't work on Safari iOS because Apple doesn't allow it.
  • If you reject the permission request, further requests will be blocked by the browser. To unblock them in Chrome, click on the 'site information' icon to the left of the address bar and reset the notifications permission back to 'Ask (default)'.
  • When tinkering with the service worker, you will need to unregister it (through the 'Application' tab of Chrome dev tools), reload the page, and register it again for your changes to take effect.

Implementing Push

If we do decide to use the Push API we'll need to subscribe for these after registering the service worker (example in reportParametersScheduledReports_browser.twig) and then send the endpoint and keys from the subscription object back to the server to be stored in options. We can then make POST requests to that endpoint to send the reports to the browser in the MobileMessaging.sendReportByBrowserNotification method (replaces existing implementation which writes notifications to options table to be polled later).

Some useful resources for implementing the Push API:

  • https://web-push-book.gauntface.com/ explains everything very logically and doesn't assume pre-existing understanding of service workers, node.js etc like many of the other tutorials do. The PDF version is 101 pages long but easy to skim for the important bits.

@mattab
Copy link
Member

mattab commented Jun 11, 2019

Notes from call:

  • Can we rename the type from browser to notification?
    • eventually (later) we may want to send notifications on mobile app as well. With this API we're creating that should be possible. (although not sure if it's possible to send notifications on mobile, without using Firebase or another push notification provider. we'll check later).
  • Service worker can issue the poll request every 6 hours and trigger notifications then.
  • When using a new browser, be important to ask users to enable notifications in this new browser.
  • You can hook into the event AssetManager.getJavaScriptFiles and load the JS file only if they have some browser notification reports. Then that JS can ask user if they want to enable notifications.
  • If Matomo runs under HTTP, you shouldn't even be able to create browser notification report. We just hide the "browser notification" scheduled report option when no SSL.
  • Service worker: there is a security concern around storing the token_auth in the service worker. It's basically not secure and we can't do it. Proposal instead:
    • For each configured scheduled report, we might want to store a hashed password (new column in the report table).
    • This password would be sent to the browser and stored in the service worker (would this use local storage or is there another way to store data in a service worker?)
    • Then to poll the notifications, token_auth wouldn't be used, and instead this new password would be used.

}
}

class XssProcessedMetric extends ProcessedMetric
Copy link
Member

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

mattab commented Jun 12, 2019

Any update @katebutler ?

@katebutler
Copy link
Author

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.
The serviceworker is now working correctly and fetching/displaying notifications when it is registered at the time of creating the report. When I visit in a new browser it is registered and activated, but for some reason it is not 'started' so it doesn't do anything.

@mattab
Copy link
Member

mattab commented Jun 12, 2019

👍 Seems Service worker can pass cookies when calling fetch. we can set credentials: 'include' in the call to fetch, see: https://developers.google.com/web/fundamentals/primers/service-workers/#the_defaults_of_fetch

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

Since Aug 25, 2017. The spec changed the default credentials policy to same-origin. Firefox changed since 61.0b13.

@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?

@katebutler
Copy link
Author

Next steps:

  • Rename type from 'browser' to 'notification'
  • Prevent user from creating notification reports if Matomo not running on HTTPS
  • Test on Safari, Edge, IE on BrowserStack

@tsteur
Copy link
Member

tsteur commented Jun 13, 2019

I'm not so much into the topic @katebutler @mattab but seeing 2 problems re the sessions/cookies:

  • If the service worker pings Matomo eg every hour or more often, it will keep the user's session active even though it shouldn't.
  • If the service worker pings Matomo less frequent than once per often then the session will be expired after 1 hour. In this Matomo release we will log out users if they haven't been active in one hour unless they selected "remember me". The same applies though to "remember me" where we don't want to extend the sessions because of notifications.

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
@katebutler
Copy link
Author

Rewritten with a per-user notification. Token is generated in MobileMessaging.validateReportParameters if report type is "notification" and the user doesn't already have a token (a very hacky way of doing it, other suggestions welcome...).
Have created a piwikNotificationsRegistration directive that is included in the main layout.twig, which checks for a token injected by the controller and does the JS logic needed (request Notification permission and register service worker if not already present) to ensure that the user will receive the notifications in this browser if possible. Again, not sure if this is the ideal way of doing it so open to suggestions.
For the sake of simplicity I've removed the similar JS logic from the report creation form - it will run anyway when the user is redirected back to the list of reports after the form is submitted.
UI tests should hopefully be fixed in this build, but integration tests will be broken.

@mattab
Copy link
Member

mattab commented Jun 16, 2019

Not sure because getting one token will then cause that you can always request the content of notifications. I would only do this if we regenerate the token every week or so. In general it be maybe more secure if it was done per report.

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:

  1. a browser which doesn't want to see notifications and user is not visiting Matomo again,
  2. and a browser where they're not actively visiting Matomo regularly, but they still want to see the notifications. When the notification-user-token is re-generated after a week, how will the service worker know about the new notification-user-token?
    • Maybe the Matomo page would communicate the re-generated notification-user-token to the service worker on the next page load, or once per hour or so? but how would this work if they aren't actively visiting Matomo (loading a new page)?

when user logs out polling and notifications would stop working <- desired behavior, correct?

This is not desired behavior I would say. It should still work when being logged out. That's why it should be independent. You would otherwise almost never get these notifications. We also don't stop sending email reports when not logged in.

(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?

@mattab mattab requested a review from tsteur June 16, 2019 22:30
@mattab mattab added the Needs Review PRs that need a code review label Jun 16, 2019
@tsteur
Copy link
Member

tsteur commented Jun 17, 2019

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).

we were not too worried about this but it is a "data leak" issue indeed.

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

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?

Copy link
Author

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() {
Copy link
Member

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.

fetch('index.php?module=MobileMessaging&action=getBrowserNotifications&token=' + self.token,
{credentials: "omit"}
).then(function(response) {
if (response.status === 200) {
Copy link
Member

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, ... ?

Copy link
Member

@tsteur tsteur left a 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.


var displayNotifications = function(response) {
response.json().then(function(notifications) {
notifications.forEach(function(notification) {
Copy link
Member

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

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

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

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?

Copy link
Member

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

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');
Copy link
Member

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;
}

/**
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 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,
Copy link
Member

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

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

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.

@katebutler
Copy link
Author

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.

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).

@katebutler
Copy link
Author

I'm trying to understand hashing the token would work, have I got it right?

  1. User visits Matomo
  2. There is a variable in the template which tells us whether the user has notification-type reports
  3. If they do, run some JS to call an API method to get a notification token
  4. Generate the token, write a hashed version to DB, return the plain version back to the JS
  5. JS installs the serviceworker with the raw token
  6. Serviceworker passes the raw token to the API when it polls for notifications
  7. API checks raw token against hashed version, and rejects the request if they don't match

So far so good, but ...
8. User now accesses Matomo from a different device
8a. Steps 1->5 above run on the new device ... and now the hashed token in the DB has changed
8b. The original device does its polling and it gets rejected because the token no longer matches

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?

@tsteur
Copy link
Member

tsteur commented Jun 18, 2019

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

login | purpose | password | date_created | date_updated | expire_date (not needed for now) | name (needed later for app specific passwords where user enters a name)
foo   | notifications | 'hash' | ... | ... 

In theory we could even make it more secure by restricting a browser type to a specific token like WINDOWS_CHROME but that doesn't make things more secure.

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 token_auth. Ideally it would also require the original password to generate this new token but that might complicate the UX in the app.

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 token_auth but an attacker could create heaps of tokens without anyone noticing.

@mattab @diosmosis any thoughts? I suppose it would need to require the actual password to generate such a notification token just like you need to do for any app specific password.

@diosmosis
Copy link
Member

Haven't read all the comments, but here are some thoughts:

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.

This always happens for me after a user logs in, so it seems like the credentials would be available at this point.

Might be secure enough to generate a token by only providing the token_auth but an attacker could create heaps of tokens without anyone noticing.

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.

@tsteur
Copy link
Member

tsteur commented Jun 18, 2019

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.

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.

Actually if an attacker has the token auth, they could probably do worse things than get notifications.

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 .

This always happens for me after a user logs in, so it seems like the credentials would be available at this point.

In the mobile app the credentials wouldn't be available since the auth token is stored on the device.

@diosmosis
Copy link
Member

diosmosis commented Jun 18, 2019

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.

Yes, but then a user might notice if their notifications stop working. For a device they don't use they wouldn't

It's just that if an attacker created multiple users, that would be visible.

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.

@katebutler
Copy link
Author

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.

@tsteur
Copy link
Member

tsteur commented Jun 19, 2019

@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 token_auth from user table then the entire UI will rely on such a token like in the notifications very likely.

@katebutler
Copy link
Author

Shelved until after #6659 is implemented. Next steps would be:

  • Implement device-specific tokens. Generate the token in an ajax call at the point of registering the serviceworker - pass raw value back to browser and stored hashed token in DB. The client will need to send some kind of human-readable device/browser ID to the server, which should be stored against the token. This process should also prompt user for their password at some point, so that we can prevent a bad agent which has the user's token_auth from generating notification tokens.
  • If we want to go down the route of regenerating tokens regularly, the serviceworker would also need to handle the expiry of the token and automatically request a new one (either before token expiry if the server has told it the expiry time, or when gracefully handling an auth failure after token has expired). This means IMO that regenerating the tokens adds complexity without actually making the code more secure (since there is no human intervention and every client will keep automatically getting new tokens when the old one expires, forever).
  • Implement brute force protection. If the polling only happens every few hours, the number of failed hits a corrupted serviceworker can generate should be quite low, and thus shouldn't get the IP blacklisted which would prevent the user logging in.
  • Build a UI for invalidating tokens. Should display the unique device/browser ID for each token, and let user invalidate these individually. Implement logic in serviceworker for dealing with this situation (stop polling? unregister itself?)
  • Fix Integration tests to work with new token logic (currently they are failing as they still assume cookie auth will work).
  • Implement logic for upgrading serviceworker when we have made a new version of it. Check whether activate method of serviceworker is called in edge cases suggested by @tsteur above (e.g. when browser is closed and reopened, or browser updates run).
  • Implement logic to unregister the serviceworker on logout, if desired (still some debate about that?)
  • Retest in BrowserStack, especially Edge and Safari as I was unable to get these to work last time ("serviceWorker" in navigator was returning false so these browsers were just not allowing serviceworkers at all, possibly because they didn't like the self-signed cert even on localhost).
  • Change polling period from 30 secs (current dev value) to an appropriate production value (1 hour? 3 hours? 6 hours?)

@katebutler katebutler removed the Needs Review PRs that need a code review label Jun 19, 2019
@tsteur tsteur modified the milestones: 3.12.0, 4.0.0 Jul 5, 2019
@tsteur tsteur changed the base branch from 3.x-dev to 4.x-dev January 14, 2020 03:57
@tsteur
Copy link
Member

tsteur commented Jan 26, 2020

@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.

@mattab mattab closed this Jan 28, 2020
@tsteur tsteur deleted the 13989 branch April 15, 2020 04:33
@Findus23 Findus23 removed this from the 4.0.0 milestone Sep 4, 2020
@mattab mattab added this to the 4.0.0 milestone Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Add type "browser notification" to scheduled reports
6 participants