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

Web Push Notifications #3243

Merged
merged 85 commits into from Jul 13, 2017

Conversation

Projects
None yet
7 participants
@sorin-davidoi
Collaborator

sorin-davidoi commented May 23, 2017

This PR introduces the ability to send Web Push Notifications. The basic idea is to save a push subscription to the backend, which can then be used to wake up the Service Worker and display a notification when something happened, even if Mastodon is not opened. Together with #3052 this can make Mastodon pretty much indistinguishable from a native application (at least on Android).

Features to implement:

  • Register Service Worker
  • Generate VAPID keys in the backend
  • Pass the public VAPID key to the frontend
  • Register push subscription to the backend
  • Delete invalid push subscriptions
  • Send push notifications
    • Mentions
    • New followers
      • Follow
      • Follow request
    • Favorites
    • Boosts
  • Localize messages
  • Handle notification interactions
  • Integrate in the UI - one should be able to control the settings for each individual subscription (e.g. for what type of events it gets triggered). This is not a per account preference, as I assume people want different settings for each device (e.g. only push for mentions on mobile, but push all notifications on desktop).
  • Handle backend changing the applicationServerKey
  • Handle RTL
  • Display attachment
  • Unregister push subscription when logging out - can not figure out how to associate a subscription with a session, such that when the user logs out the subscription is destroyed
  • Set VAPID keys via configuration
  • Move logic out of SettingsController - I could not figure out how to create a new endpoint for this, so I've hijacked this controller for adding and updating subscriptions.
  • Add actions to the notification
    • Favourite / boost status

Browser support: Chrome and Firefox. Cannot be polyfilled.

Closes #1122, #1039.

@sorin-davidoi sorin-davidoi changed the title from Web Push Notifications [WIP] to Web Push Notifications May 25, 2017

@nolanlawson

This comment has been minimized.

Show comment
Hide comment
@nolanlawson

nolanlawson May 26, 2017

Collaborator

I was just about to ask how you managed to get Chrome to do push notifications without a GCM/FCM ID, but it looks like they've removed that requirement, which is going to make our lives a lot easier. Hooray! 🎉

Collaborator

nolanlawson commented May 26, 2017

I was just about to ask how you managed to get Chrome to do push notifications without a GCM/FCM ID, but it looks like they've removed that requirement, which is going to make our lives a lot easier. Hooray! 🎉

@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi May 26, 2017

Collaborator

I think this is ready for a first review. Updated the description with stuff that still needs to be done. Tested on Firefox and Chrome on Linux (KDE Plasma) and on Android.

photo_2017-05-27_00-31-24
image
photo_2017-05-27_00-30-57
photo_2017-05-27_00-31-16

Collaborator

sorin-davidoi commented May 26, 2017

I think this is ready for a first review. Updated the description with stuff that still needs to be done. Tested on Firefox and Chrome on Linux (KDE Plasma) and on Android.

photo_2017-05-27_00-31-24
image
photo_2017-05-27_00-30-57
photo_2017-05-27_00-31-16

@sorin-davidoi sorin-davidoi changed the title from Web Push Notifications to [REVIEW ME] Web Push Notifications May 26, 2017

@akihikodaki

I have some questions and found something suspicious, so please review them. I'm looking forward this feature.

Show outdated Hide outdated app/controllers/api/web/settings_controller.rb Outdated
Show outdated Hide outdated app/javascript/mastodon/web_push.js Outdated
Show outdated Hide outdated db/schema.rb Outdated
Show outdated Hide outdated app/controllers/api/web/push_notification_controller.rb Outdated
Show outdated Hide outdated app/controllers/api/web/settings_controller.rb Outdated
Show outdated Hide outdated app/controllers/api/web/push_notification_controller.rb Outdated
Show outdated Hide outdated app/controllers/api/web/settings_controller.rb Outdated
@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi May 28, 2017

Collaborator

Will try to see tomorrow how Rails tests work and maybe add some specs if I can figure things out.

Collaborator

sorin-davidoi commented May 28, 2017

Will try to see tomorrow how Rails tests work and maybe add some specs if I can figure things out.

@akihikodaki

I found an error with CI.

Show outdated Hide outdated app/controllers/api/web/settings_controller.rb Outdated
@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi May 29, 2017

Collaborator

If this spec gets traction we might even be able to reply from the notification.

Collaborator

sorin-davidoi commented May 29, 2017

If this spec gets traction we might even be able to reply from the notification.

@sorin-davidoi sorin-davidoi changed the title from [REVIEW ME] Web Push Notifications to Web Push Notifications May 30, 2017

@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron May 30, 2017

Member

33 files change - I'm already scared :D Is this ready for review?

Member

Gargron commented May 30, 2017

33 files change - I'm already scared :D Is this ready for review?

@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi May 30, 2017

Collaborator

I still haven't figured out how to attach the subscription to the session, so that when the user logs out the subscription gets deleted. Also need to pass the VAPID keys via config files / environment (right now they are generated when the server starts).

Collaborator

sorin-davidoi commented May 30, 2017

I still haven't figured out how to attach the subscription to the session, so that when the user logs out the subscription gets deleted. Also need to pass the VAPID keys via config files / environment (right now they are generated when the server starts).

@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi May 30, 2017

Collaborator

@akihikodaki I explored a bit our options for clearing the subscriptions. We have the following in doorkeeper.rb:

  # Reuse access token for the same resource owner within an application (disabled by default)
  # Rationale: https://github.com/doorkeeper-gem/doorkeeper/issues/383
  reuse_access_token

What I understand from this is that we do not keep track of each user session (we use one token for all sessions), so there is no way to associate a push subscription with the session that created it. What is the reason for this? What would be the implications of issuing a new token for each login and deleting the token when the user logs out (attaching the push subscription to this token and deleting them together).

Another approach would be to manually delete the push subscription from the front-end when the user logs out, but that would create a huge security / privacy issue.

Don't really know how to proceed with this.

Collaborator

sorin-davidoi commented May 30, 2017

@akihikodaki I explored a bit our options for clearing the subscriptions. We have the following in doorkeeper.rb:

  # Reuse access token for the same resource owner within an application (disabled by default)
  # Rationale: https://github.com/doorkeeper-gem/doorkeeper/issues/383
  reuse_access_token

What I understand from this is that we do not keep track of each user session (we use one token for all sessions), so there is no way to associate a push subscription with the session that created it. What is the reason for this? What would be the implications of issuing a new token for each login and deleting the token when the user logs out (attaching the push subscription to this token and deleting them together).

Another approach would be to manually delete the push subscription from the front-end when the user logs out, but that would create a huge security / privacy issue.

Don't really know how to proceed with this.

@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron May 30, 2017

Member

What is the reason for this?

The web UI uses the API like any other app would use the API, so it needs an access token. Reusing of the access token was implemented in response to #1681

Member

Gargron commented May 30, 2017

What is the reason for this?

The web UI uses the API like any other app would use the API, so it needs an access token. Reusing of the access token was implemented in response to #1681

@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki May 31, 2017

Collaborator

Another approach would be to manually delete the push subscription from the front-end when the user logs out, but that would create a huge security / privacy issue.

Could you tell me about your security concern?

Collaborator

akihikodaki commented May 31, 2017

Another approach would be to manually delete the push subscription from the front-end when the user logs out, but that would create a huge security / privacy issue.

Could you tell me about your security concern?

@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi May 31, 2017

Collaborator
  1. Call /auth/sign_out -> success
  2. Call endpoint deleting the current subscription -> fail

Result: the user is logged out, but the browser keeps getting push notifications for that account. Also someone can mess with the code and skip the request to delete the subscription.

Signing out and deleting the subscription should be an atomic operation, handled by the back-end.

Ideally, there would be another model (Device or Session or something similar), with an account having multiple devices (places where the user is signed in). A device has a subscription. When the user logs out, the corresponding device gets deleted together with the attached subscription. This would also enable the user to see where they are logged in and log out remotely (kind of like Dropbox does it, see screenshot below).

image

Collaborator

sorin-davidoi commented May 31, 2017

  1. Call /auth/sign_out -> success
  2. Call endpoint deleting the current subscription -> fail

Result: the user is logged out, but the browser keeps getting push notifications for that account. Also someone can mess with the code and skip the request to delete the subscription.

Signing out and deleting the subscription should be an atomic operation, handled by the back-end.

Ideally, there would be another model (Device or Session or something similar), with an account having multiple devices (places where the user is signed in). A device has a subscription. When the user logs out, the corresponding device gets deleted together with the attached subscription. This would also enable the user to see where they are logged in and log out remotely (kind of like Dropbox does it, see screenshot below).

image

@Gargron

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 1, 2017

Collaborator

@sorin-davidoi

the user is logged out, but the browser keeps getting push notifications for that account. Also someone can mess with the code and skip the request to delete the subscription.

Even now we have access_token, which remains valid after signing out.

Collaborator

akihikodaki commented Jun 1, 2017

@sorin-davidoi

the user is logged out, but the browser keeps getting push notifications for that account. Also someone can mess with the code and skip the request to delete the subscription.

Even now we have access_token, which remains valid after signing out.

@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Jun 1, 2017

Member

I suggest extracting Devise modifications into a separate PR to make reviewing easier

Member

Gargron commented Jun 1, 2017

I suggest extracting Devise modifications into a separate PR to make reviewing easier

@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi Jun 22, 2017

Collaborator

Will continue work once #3616 is merged.

Collaborator

sorin-davidoi commented Jun 22, 2017

Will continue work once #3616 is merged.

Show outdated Hide outdated config/routes.rb Outdated
@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi Jun 25, 2017

Collaborator

Added actions when you are mentioned:

Collaborator

sorin-davidoi commented Jun 25, 2017

Added actions when you are mentioned:

@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi Jun 25, 2017

Collaborator

I think this is ready for review. The service worker is configured not to cache anything. The offline-plugin configuration is mostly copied from #3052 and is only active during in the production build. All the lint errors are logging statements that I've left in to aid testing / debugging.

For testing, after you see the "Subscription registered" notification, go to the settings notifications and:

  • disable desktop notifications
  • enable push notification

This will make it less confusing which type of notifications you are receiving. Would be nice if you could take a look at Chrome and Firefox on Android.

Collaborator

sorin-davidoi commented Jun 25, 2017

I think this is ready for review. The service worker is configured not to cache anything. The offline-plugin configuration is mostly copied from #3052 and is only active during in the production build. All the lint errors are logging statements that I've left in to aid testing / debugging.

For testing, after you see the "Subscription registered" notification, go to the settings notifications and:

  • disable desktop notifications
  • enable push notification

This will make it less confusing which type of notifications you are receiving. Would be nice if you could take a look at Chrome and Firefox on Android.

@nightpool

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Jul 12, 2017

Collaborator

@sorin-davidoi it's hard to see how user-side logging is really going to help in this case (it's difficult enough to get users to tell us when something goes wrong, much less get them to have the dev console open at the right time) but if you think it's important to keep it, I defer to your judgement

Collaborator

nightpool commented Jul 12, 2017

@sorin-davidoi it's hard to see how user-side logging is really going to help in this case (it's difficult enough to get users to tell us when something goes wrong, much less get them to have the dev console open at the right time) but if you think it's important to keep it, I defer to your judgement

@beatrix-bitrot

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot Jul 12, 2017

Collaborator

ah, about the console log statements? i'm indifferent

for most users i don't think they be any use but on bleeding edge instances they may come in handy

maybe leave them in master for a bit and remove before the next tagged release? idk

another option is to leave them in here, then make another PR that removes them so that if anyone needs to enable them for testing all they need to do is revert the commit that removes them

idk

Collaborator

beatrix-bitrot commented Jul 12, 2017

ah, about the console log statements? i'm indifferent

for most users i don't think they be any use but on bleeding edge instances they may come in handy

maybe leave them in master for a bit and remove before the next tagged release? idk

another option is to leave them in here, then make another PR that removes them so that if anyone needs to enable them for testing all they need to do is revert the commit that removes them

idk

@akihikodaki

I have added many comments, but they are trivial and the logic itself looks fine. I hope they will be addressed soon and the change will be merged.

Show outdated Hide outdated app/controllers/api/web/push_subscriptions_controller.rb Outdated
Show outdated Hide outdated app/controllers/api/web/push_subscriptions_controller.rb Outdated
Show outdated Hide outdated app/controllers/api/web/push_subscriptions_controller.rb Outdated
Show outdated Hide outdated app/javascript/mastodon/service_worker/web_push_notifications.js Outdated
Show outdated Hide outdated app/javascript/mastodon/service_worker/web_push_notifications.js Outdated
Show outdated Hide outdated app/javascript/mastodon/web_push_subscription.js Outdated
Show outdated Hide outdated db/migrate/20170625175513_create_web_push_subscriptions.rb Outdated
Show outdated Hide outdated app/services/notify_service.rb Outdated
Show outdated Hide outdated spec/controllers/api/web/push_subscriptions_controller_spec.rb Outdated
Show outdated Hide outdated spec/fabricators/web_push_subscription_fabricator.rb Outdated
@akihikodaki

All problems I have requested are addressed, but I found a few other suspicious things.

Show outdated Hide outdated config/webpack/production.js Outdated
Show outdated Hide outdated spec/fabricators/web_push_subscription_fabricator.rb Outdated

Seems to be addressed. There are some code style issues (from a human PoV, not code climate) that I intend to refactor in the future, but I think overall this is fine

@Gargron Gargron merged commit 0c7c188 into tootsuite:master Jul 13, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ykzts added a commit to ykzts/mastodon that referenced this pull request Jul 14, 2017

Gargron added a commit that referenced this pull request Jul 14, 2017

@sorin-davidoi sorin-davidoi referenced this pull request Jul 14, 2017

Closed

Toot.Cafe Tusky app #17

@sorin-davidoi

This comment has been minimized.

Show comment
Hide comment
@sorin-davidoi

sorin-davidoi Jul 14, 2017

Collaborator

For things that were left unaddressed here see #4200.

Collaborator

sorin-davidoi commented Jul 14, 2017

For things that were left unaddressed here see #4200.

YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017

Web Push Notifications (tootsuite#3243)
* feat: Register push subscription

* feat: Notify when mentioned

* feat: Boost, favourite, reply, follow, follow request

* feat: Notification interaction

* feat: Handle change of public key

* feat: Unsubscribe if things go wrong

* feat: Do not send normal notifications if push is enabled

* feat: Focus client if open

* refactor: Move push logic to WebPushSubscription

* feat: Better title and body

* feat: Localize messages

* chore: Fix lint errors

* feat: Settings

* refactor: Lazy load

* fix: Check if push settings exist

* feat: Device-based preferences

* refactor: Simplify logic

* refactor: Pull request feedback

* refactor: Pull request feedback

* refactor: Create /api/web/push_subscriptions endpoint

* feat: Spec PushSubscriptionController

* refactor: WebPushSubscription => Web::PushSubscription

* feat: Spec Web::PushSubscription

* feat: Display first media attachment

* feat: Support direction

* fix: Stuff broken while rebasing

* refactor: Integration with session activations

* refactor: Cleanup

* refactor: Simplify implementation

* feat: Set VAPID keys via environment

* chore: Comments

* fix: Crash when no alerts

* fix: Set VAPID keys in testing environment

* fix: Follow link

* feat: Notification actions

* fix: Delete previous subscription

* chore: Temporary logs

* refactor: Move migration to a later date

* fix: Fetch the correct session activation and misc bugs

* refactor: Move migration to a later date

* fix: Remove follow request (no notifications)

* feat: Send administrator contact to push service

* feat: Set time-to-live

* fix: Do not show sensitive images

* fix: Reducer crash in error handling

* feat: Add badge

* chore: Fix lint error

* fix: Checkbox label overlap

* fix: Check for payload support

* fix: Rename action "type" (crash in latest Chrome)

* feat: Action to expand notification

* fix: Lint errors

* fix: Unescape notification body

* fix: Do not allow boosting if the status is hidden

* feat: Add VAPID keys to the production sample environment

* fix: Strip HTML tags from status

* refactor: Better error messages

* refactor: Handle browser not implementing the VAPID protocol (Samsung Internet)

* fix: Error when target_status is nil

* fix: Handle lack of image

* fix: Delete reference to invalid subscriptions

* feat: Better error handling

* fix: Unescape HTML characters after tags are striped

* refactor: Simpify code

* fix: Modify to work with tootsuite#4091

* Sort strings alphabetically

* i18n: Updated Polish translation

it annoys me that it's not fully localized :P

* refactor: Use current_session in PushSubscriptionController

* fix: Rebase mistake

* fix: Set cacheName to mastodon

* refactor: Pull request feedback

* refactor: Remove logging statements

* chore(yarn): Fix conflicts with master

* chore(yarn): Copy latest from master

* chore(yarn): Readd offline-plugin

* refactor: Use save! and update!

* refactor: Send notifications async

* fix: Allow retry when push fails

* fix: Save track for failed pushes

* fix: Minify sw.js

* fix: Remove account_id from fabricator

YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment