Web Push Notifications #3243

sorin-davidoi · 2017-05-23T00:48:40Z

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:

Browser support: Chrome and Firefox. Cannot be polyfilled.

Closes #1122, #1039.

nolanlawson · 2017-05-26T21:58:43Z

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 · 2017-05-26T22:36:59Z

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.

akihikodaki

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

akihikodaki · 2017-05-28T12:38:07Z

app/controllers/api/web/settings_controller.rb

+    current_account.web_push_subscriptions << web_subscription
+    current_account.save!
+
+    render json: web_subscription.as_payload, status: 200


Does it need explicit status?

Don't think so.

akihikodaki · 2017-05-28T13:01:16Z

app/javascript/mastodon/web_push.js

+
+      try {
+        getRegistration()
+          .then(getPushSubscription())


I suppose you mean getPushSubscription instead of getPushSubscription()?

akihikodaki · 2017-05-28T13:07:01Z

db/schema.rb

@@ -358,6 +358,16 @@
    t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true, using: :btree
  end

+  create_table "web_push_subscriptions", force: :cascade do |t|
+    t.integer  "account_id"


Why not index?

This was generated by the following command, so I cannot answer that 😄 How should it look like?

rails generate model WebPushSubscription account_id:integer endpoint:string:unique key_p256dh:string key_auth:string

Please look at other tables. Here is an example:

t.index ["account_id"], name: "index_reports_on_account_id", using: :btree

akihikodaki · 2017-05-28T13:12:02Z

app/controllers/api/web/push_notification_controller.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true


akihikodaki · 2017-05-28T13:13:16Z

app/controllers/api/web/settings_controller.rb

@@ -1,20 +1,49 @@
 # frozen_string_literal: true

 class Api::Web::SettingsController < ApiController
+  include ActionView::Helpers::TranslationHelper
+
  respond_to :json

  before_action :require_user!

  def update


Spec this, too.

akihikodaki · 2017-05-28T13:19:51Z

app/controllers/api/web/push_notification_controller.rb

+  private
+
+  def web_notification
+    @_web_push_notification ||= WebPushNotifications.where(user: current_user).first_or_initialize(user: current_user)


Where WebPushNotifications comes from?

Ruby magic 😄 ? Guess it is the database model?

Ruby can sometimes do magic, but it should be in schema.rb anyway if it is a database model.

akihikodaki · 2017-05-28T13:23:48Z

app/controllers/api/web/settings_controller.rb

@@ -1,20 +1,49 @@
 # frozen_string_literal: true

 class Api::Web::SettingsController < ApiController
+  include ActionView::Helpers::TranslationHelper


What is it for?

sorin-davidoi · 2017-05-28T21:02:34Z

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.

akihikodaki · 2017-05-29T11:29:30Z

app/controllers/api/web/settings_controller.rb

@@ -1,20 +1,47 @@
 # frozen_string_literal: true

-class Api::Web::SettingsController < ApiController


This makes the spec fail.

sorin-davidoi · 2017-05-29T11:47:44Z

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

Gargron · 2017-05-30T13:28:18Z

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

sorin-davidoi · 2017-05-30T13:35:26Z

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 · 2017-05-30T18:42:43Z

@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 · 2017-05-30T20:58:53Z

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 · 2017-05-31T08:48:39Z

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 · 2017-05-31T10:07:19Z

Call /auth/sign_out -> success
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).

Gargron · 2017-05-31T15:39:45Z

http://www.jonathanleighton.com/articles/2013/revocable-sessions-with-devise/

akihikodaki · 2017-06-01T02:46:39Z

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

sorin-davidoi · 2017-06-01T21:42:19Z

spec/controllers/api/web/push_subscriptions_controller_spec.rb

+    before do
+      sign_in(user)
+
+      ActiveSession.deactivate warden.raw_session['auth_id']


The tests fail because these two lines need to be executed after each call to sign_in (for all tests). Tried putting the following into spec_helper.rb but didn't help. Any pointers?

Devise.setup do |config| Warden::Manager.after_set_user except: :fetch do |user, auth, opts| ActiveSession.deactivate warden.raw_session['auth_id'] warden.raw_session['auth_id'] = user.activate_session end end

I think it is because Devise::Test::ControllerHelpers does not support configuration by Devise.setup.
https://github.com/plataformatec/devise/blob/9fe7040db90be1acb499d2534b077266dbf3b209/lib/devise/test/controller_helpers.rb#L63

Devise::Test::ControllerHelpers is included in rails_helper. You may hook sign_in there. (it's is a bit hacky, but it cannot be helped, I think.)
https://github.com/tootsuite/mastodon/blob/fd66f7cdc0931a67a2e24d887fb79f44fc42e2bc/spec/rails_helper.rb#L26

Gargron · 2017-06-01T22:33:29Z

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

akihikodaki · 2017-06-01T23:26:39Z

spec/controllers/api/web/push_subscriptions_controller_spec.rb

+    before do
+      sign_in(user)
+
+      ActiveSession.deactivate warden.raw_session['auth_id']


I think it is because Devise::Test::ControllerHelpers does not support configuration by Devise.setup.
https://github.com/plataformatec/devise/blob/9fe7040db90be1acb499d2534b077266dbf3b209/lib/devise/test/controller_helpers.rb#L63

Devise::Test::ControllerHelpers is included in rails_helper. You may hook sign_in there. (it's is a bit hacky, but it cannot be helped, I think.)
https://github.com/tootsuite/mastodon/blob/fd66f7cdc0931a67a2e24d887fb79f44fc42e2bc/spec/rails_helper.rb#L26

sorin-davidoi · 2017-06-22T12:11:29Z

Will continue work once #3616 is merged.

sorin-davidoi · 2017-06-24T14:37:38Z

config/routes.rb

@@ -206,6 +206,12 @@

    namespace :web do
      resource :settings, only: [:update]
+      resource :push_subscriptions, only: [:create] do
+        # Why doesn't PUT /api/web/push_subscriptions/:id work?


@Gargron Do you have some insights into this? If I call axios.put('/api/web/push_subscriptions/2' I get this in the logs:

Started PUT "/api/web/push_subscriptions/2" for 127.0.0.1 at 2017-06-24 16:34:57 +0200 Processing by ApplicationController#raise_not_found as HTML

axios.put('/api/web/push_subscriptions') works fine though.

Cannot seem to figure it out.

"resource" means there's no index, show with no ID (e.g. there is always one item)

"resources" is what gives you both index and show with ID

sorin-davidoi · 2017-06-25T13:50:03Z

Added actions when you are mentioned:

sorin-davidoi · 2017-06-25T21:44:16Z

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.

beatrix-bitrot

I, Beatrix Bitrot, being of sound mind and of my own volition APPROVE THESE CHANGES

nightpool · 2017-07-12T00:27:51Z

@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 · 2017-07-12T00:31:51Z

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.

akihikodaki · 2017-07-12T01:21:03Z

app/controllers/api/web/push_subscriptions_controller.rb

+    end
+
+    web_subscription = ::Web::PushSubscription.new(
+      endpoint: params[:data][:endpoint],


Why not params.require?

akihikodaki · 2017-07-12T01:22:03Z

app/controllers/api/web/push_subscriptions_controller.rb

+      active_session.save!
+    end
+
+    web_subscription = ::Web::PushSubscription.new(


Use create!.

Hm, does not seem to work, as this is called from an endpoint.

what's the problem with calling create from an endpoint?

Ah, think I misunderstood the comment. I took it as to change the name of the method from create to create!. Will update it after I fix the conflicts 🙄

akihikodaki · 2017-07-12T01:22:16Z

app/controllers/api/web/push_subscriptions_controller.rb

+
+    web_subscription.save!
+
+    active_session.web_push_subscription = web_subscription


Use update!.

Same comment as create!.

akihikodaki · 2017-07-12T01:34:41Z

app/javascript/mastodon/service_worker/web_push_notifications.js

+      if (action.todo === 'expand') {
+        return expandNotification(event.notification);
+      } else if (action.todo === 'request') {
+        return makeRequest(event.notification, action)


The following statement would be enough. A promise as resolved value will be evaluated and expanded.

resolve(makeRequest(event.notification, action) .then(() => removeActionFromNotification(event.notification, action)));

akihikodaki · 2017-07-12T01:35:22Z

app/javascript/mastodon/service_worker/web_push_notifications.js

+          .catch(reject);
+      }
+
+      return null;


Returned values does not really matter for promises.

akihikodaki · 2017-07-12T01:48:03Z

app/javascript/mastodon/web_push_subscription.js

+        }
+      });
+  } else {
+    // eslint-disable-next-line no-console


I think you are aware of, but here I note you should be consistent, whether you use eslint-disable-next-line or not. If you use, apply for all other console. If you do not, remove this.

Will remove the other logs.

akihikodaki · 2017-07-12T01:50:24Z

db/migrate/20170625175513_create_web_push_subscriptions.rb

@@ -0,0 +1,12 @@
+class CreateWebPushSubscriptions < ActiveRecord::Migration[5.1]
+  def change
+    create_table :web_push_subscriptions do |t|


I guess the following columns cannot not be null, can they?

akihikodaki · 2017-07-12T01:55:40Z

app/services/notify_service.rb

+      rescue Webpush::PayloadTooLarge, Webpush::TooManyRequests => e
+        Rails.logger.error(e)
+      rescue Webpush::Error => e
+        # Failing to send push notifications should not result in Internal Server Error


Failing to send push notifications should not result in Internal Server Error

Yes, but that is controllers' business. If it is just to eliminate Internal Server Error, rescue in controllers.

Not really sure where to move the rescues then (this is called from a ton of places, all of them services - should each of them duplicate the rescues?).

Are we sending notifications synchronously? that seems really bad.

If it's from sidekiq, then we can just let the error propagate so we retry the job.

To be honest I don't really know - don't have a strong grasp on the whole sidekiq thing.

akihikodaki · 2017-07-12T01:58:26Z

spec/controllers/api/web/push_subscriptions_controller_spec.rb

+describe Api::Web::PushSubscriptionsController do
+  render_views
+
+  let!(:user) { Fabricate(:user) }


I think let is fine for these cases.

akihikodaki · 2017-07-12T01:59:46Z

spec/fabricators/web_push_subscription_fabricator.rb

@@ -0,0 +1,6 @@
+Fabricator(:web_push_subscription) do
+  account_id 1
+  endpoint   "MyString"


Use Faker gem.

akihikodaki

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

akihikodaki · 2017-07-13T04:45:20Z

config/webpack/production.js

+        cacheName: 'mastodon',
+        output: '../sw.js',
+        publicPath: '/sw.js',
+        minify: false,


Why don't you minify?

akihikodaki · 2017-07-13T04:49:42Z

spec/fabricators/web_push_subscription_fabricator.rb

@@ -0,0 +1,6 @@
+Fabricator(:web_push_subscription) do
+  account_id 1


Web::PushSubscription does not have account_id column.

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

sorin-davidoi · 2017-07-14T20:04:02Z

For things that were left unaddressed here see #4200.

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

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

akihikodaki suggested changes May 28, 2017

View reviewed changes

akihikodaki suggested changes May 29, 2017

View reviewed changes

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

sorin-davidoi commented Jun 1, 2017

View reviewed changes

akihikodaki previously requested changes Jun 1, 2017

View reviewed changes

akihikodaki added enhancement labels Jun 3, 2017

This was referenced Jun 6, 2017

Revocable sessions #3616

Merged

Desktop Client using Electron #1163

Closed

Subscribe for mobile notifications from accounts to drive engagement #1122

Closed

Web push notifications #1039

Closed

sorin-davidoi commented Jun 24, 2017

View reviewed changes

beatrix-bitrot approved these changes Jul 11, 2017

View reviewed changes

akihikodaki suggested changes Jul 12, 2017

View reviewed changes

sorin-davidoi added 10 commits July 12, 2017 09:58

refactor: Pull request feedback

a847236

refactor: Remove logging statements

bf3397d

chore(yarn): Fix conflicts with master

0bf308a

chore(yarn): Copy latest from master

be5ea12

chore(yarn): Readd offline-plugin

bccb6e7

chore: Fix conflicts with master

44785e0

refactor: Use save! and update!

28a2c0f

refactor: Send notifications async

99a482b

fix: Allow retry when push fails

e3e79f3

fix: Save track for failed pushes

c4c0a7a

akihikodaki previously requested changes Jul 13, 2017

View reviewed changes

sorin-davidoi added 4 commits July 13, 2017 11:56

fix: Minify sw.js

580fea4

fix: Remove account_id from fabricator

5e6132d

fix: Fix conflicts with master

5067d70

fix: Conflicts with master

787279a

Gargron approved these changes Jul 13, 2017

View reviewed changes

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

ykzts mentioned this pull request Jul 14, 2017

Add Japanese translations for #3243 #4192

Merged

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

Add Japanese translations for #3243 (#4192)

87b96f8

ykzts mentioned this pull request Jul 14, 2017

Add Rake task for generate VAPID key #4195

Merged

sorin-davidoi mentioned this pull request Jul 14, 2017

Toot.Cafe Tusky app tootcafe/discussions#17

Closed

Sjors mentioned this pull request Jul 18, 2020

Safari push notifications #14352

Closed

		@@ -1,20 +1,47 @@
		# frozen_string_literal: true

		class Api::Web::SettingsController < ApiController


		web_subscription.save!

		active_session.web_push_subscription = web_subscription

		@@ -0,0 +1,6 @@
		Fabricator(:web_push_subscription) do
		account_id 1

Web Push Notifications #3243

Web Push Notifications #3243

Conversation

sorin-davidoi commented May 23, 2017 • edited

nolanlawson commented May 26, 2017

sorin-davidoi commented May 26, 2017

akihikodaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorin-davidoi commented May 28, 2017 • edited

akihikodaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorin-davidoi commented May 29, 2017

Gargron commented May 30, 2017

sorin-davidoi commented May 30, 2017

sorin-davidoi commented May 30, 2017 • edited

Gargron commented May 30, 2017

akihikodaki commented May 31, 2017

sorin-davidoi commented May 31, 2017 • edited

Gargron commented May 31, 2017

akihikodaki commented Jun 1, 2017

sorin-davidoi Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gargron commented Jun 1, 2017

Choose a reason for hiding this comment

sorin-davidoi commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorin-davidoi commented Jun 25, 2017

sorin-davidoi commented Jun 25, 2017 • edited

beatrix-bitrot left a comment

Choose a reason for hiding this comment

nightpool commented Jul 12, 2017 • edited

beatrix-bitrot commented Jul 12, 2017

akihikodaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightpool Jul 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akihikodaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorin-davidoi commented Jul 14, 2017

sorin-davidoi commented May 23, 2017 •

edited

sorin-davidoi commented May 28, 2017 •

edited

sorin-davidoi commented May 30, 2017 •

edited

sorin-davidoi commented May 31, 2017 •

edited

sorin-davidoi Jun 1, 2017 •

edited

sorin-davidoi commented Jun 25, 2017 •

edited

nightpool commented Jul 12, 2017 •

edited

nightpool Jul 12, 2017 •

edited