@katebutler opened this Pull Request on May 27th 2019 Member

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

@mattab commented on May 28th 2019 Member

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 commented on May 29th 2019 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 commented on June 7th 2019 Member

@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 commented on June 7th 2019 Contributor

@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 commented on June 7th 2019 Member

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

@katebutler commented on June 10th 2019 Member

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 commented on June 11th 2019 Member

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 commented on June 11th 2019 Member

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.
@mattab commented on June 12th 2019 Member

Any update @katebutler ?

@katebutler commented on June 12th 2019 Member

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 commented on June 12th 2019 Member

:+1: 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 commented on June 12th 2019 Member

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 commented on June 13th 2019 Member

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.

@katebutler commented on June 13th 2019 Member

Everything is working now! Just need to redo the UI test images and tidy up my code a bit.

@katebutler commented on June 13th 2019 Member

Have been testing in BrowserStack this morning with limited success - neither Safari nor Edge will allow serviceworkers, even on localhost and with self-signed SSL, and I can't find any settings to force it. Works OK in Firefox and Chrome.

@mattab commented on June 13th 2019 Member

@tsteur good point. the service worker would possible keep the sessions active, which is a security concern.

Current status:

  • when user logs in without remember me:
    • If the service worker is polling less frequently eg. every 2 hours, then polling wouldn't work and notifications wouldn't work. But we do want notifications to be displayed even if user hasn't selected "Remember me", correct?
  • when user logs in with remember me:
    • polling would work and would always keep the session active (it would never expire) <- security issue
  • when user logs out
    • polling and notifications would stop working <- desired behavior, correct?

So the fact we keep the session opened forever (when "Remember me") is a concern.

Proposed steps:

  • Introduce a report_token for each notification report (we have a function to generate a random string).
  • When Matomo page loads, it gets the list of notification reports for this user, and the report_tokens, and outputs the array in the page in JavaScript.
  • Pass this array of report_token to the service worker.
  • Service worker passes the list of report_token when it polls the API
  • [..API magic, what's the easiest solution here?..]
  • When user logs out, unregister service worker entirely

@katebutler can you complete the list of steps?

@mattab commented on June 13th 2019 Member

@katebutler can you check why you didn't see Thomas comment yesterday in your emails?

@mattab commented on June 13th 2019 Member

Because reports are "per user" and can't be shared across users, maybe it's easier to introduce a new browser_notification_token per user in the user table (new column), and use that to get the notifications for this user?
(if we had already https://github.com/matomo-org/matomo/issues/6559 we could use it)

@tsteur commented on June 14th 2019 Member

Because reports are "per user" and can't be shared across users, maybe it's easier to introduce a new browser_notification_token per user in the user table (new column), and use that to get the notifications for this user?

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.

(if we had already #6559 we could use it)

We wouldn't use #6559 as it would grant way too many permissions. If you just log in once into a random network or so even without logging in to Matomo it could potentially leak the app token giving users access to everything.

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.

@Findus23 commented on June 14th 2019 Member

We wouldn't use #6559 as it would grant way too many permissions. If you just log in once into a random network or so even without logging in to Matomo it could potentially leak the app token giving users access to everything.

I thought the point of #6559 was that those multiple tokens are all limited to a very small scope, so in this case we could generate a token that could only read notifications. (or someone could embed a widget without giving access to all other reports).
Otherwise if the token still have full access, we just increase the issue of token_auth as they can still be used to take over account, but are more and therefore harder to revoke.
I just can't find an issue where this is described at the moment.

One other thing I want to mention: Service worker have a lifetime independent of the file on the server and the cache header, so unless we specifically fix this, they will stay the same after Matomo upgrades which will break things in subtile ways. (And I don't want to add delete the service worker to my list of troubleshooting steps as that's really hard to explain everyone)

@tsteur commented on June 14th 2019 Member

I thought the point of #6559 was that those multiple tokens are all limited to a very small scope, so in this case we could generate a token that could only read notifications. (or someone could embed a widget without giving access to all other reports).

That will be a totally different issue and is not planned as part of that issue. In many other applications an app specific token still gives you full access to everything. Eventually this can be implemented on top though and in general in Matomo. Also for regular login. Feel free to create an issue for this.

@katebutler commented on June 16th 2019 Member

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 commented on June 16th 2019 Member

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?

@tsteur commented on June 17th 2019 Member

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.

@katebutler commented on June 17th 2019 Member

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 commented on June 17th 2019 Member

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

  1. 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 commented on June 18th 2019 Member

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 https://github.com/matomo-org/matomo/issues/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 commented on June 18th 2019 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 commented on June 18th 2019 Member

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 commented on June 18th 2019 Member

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 commented on June 18th 2019 Member

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 commented on June 19th 2019 Member

@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 commented on June 19th 2019 Member

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?)
Powered by GitHub Issue Mirror