Push Notifications (Chrome & Firefox) #795

micahalcorn · 2018-10-22T07:06:03Z

Checklist:

Test your work and double-check to confirm that you didn't break anything
Ensure all new and existing tests pass
~~Format code to be consistent with the project~~
Wrap any new text/strings for translation
Map any new environment variables with a default value in the Webpack config
~~Update any relevant READMEs and docs~~

Description:

This is a proof-of-concept for implementing browser-based push notifications. Here is an overview of what is included and what may or may not need to be handled in the near/distant future:

Notifications Server

A new subdirectory (origin-notifications) is created. It contains an Express server with a PostgreSQL connection mapped using Sequelize. There is only one model, the PushSubscription, which includes the following attributes:

id: default, automatically-incremented integer
createdAt: default, automatically-generated timestamp
updatedAt: default, automatically-generated timestamp
endpoint: URL string generated when a subscription is created in the browser, determined by the push service provider (Google FCM or Mozilla), with a unique token appended to the path
keys: a pair of values (auth and p256dh) generated when toJson is called on a subscription
expirationTime: currently left blank
account: the subscriber's then-current ETH address

Since the subscription object is unique to the client device, it may be relevant to more than one ETH account. For this reason, neither the endpoint nor the account values are separately unique in the PushSubscription table. However, no two records should contain both the same endpoint and the same account.

The only events currently resulting in notifications are those involving an offer. New messages and listings could be candidates for future notifications.

Three environment variables are necessary. An email address should be provided in case the push service provider needs to contact the sender of the push events. And a VAPID key pair can be generated by the web-push library.

VAPID_EMAIL_ADDRESS=
VAPID_PRIVATE_KEY=
VAPID_PUBLIC_KEY=

Routes

GET /: returns all PushSubscription records
POST /: creates a new PushSubscription if one does not exist with both a matching endpoint and a matching account
POST /events: conditionally converts a blockchain event into a push notification, which is sent to any subscription endpoints generated by an ETH account matching the buyer or seller

To Consider

appropriate messages to send
hiding the current GET results at /
storing and queueing events
CORS rules
safeguarding against duplicates
localization of messages
connecting to a production database, perhaps the same one that is used by origin-discovery and/or the upcoming origin-mobile
indexing
including API keys if required to send notifications at scale
integration with Discord webhook or other webhook subscribers
replacing integer IDs with UUIDs or something else
formatting code to match origin-js

To Test

createdb notifications
psql notifications -c 'CREATE EXTENSION hstore;'
npm install
npm install -g sequelize-cli
sequelize db:migrate
node app.js
Run origin-discovery listener with node listener/listener.js --continue-file=continue --webhook=http://localhost:3456/events

Service Worker

The service worker is installed in the browser when the DApp is loaded. It allows for the creation of PushSubscriptions and handles push events from the server and user interactions with notifications. The code runs in a separate thread outside of the DOM context and is usually executed when the DApp is not currently open in the browser. It is scoped to the origin, and thus, registrations and notifications will be specific to the dev/staging/prod environment as long as we continue to use separate subdomains.

To Consider

analytics
consolidating multiple active notifications
refraining from rendering notifications when relevant URL is already the focused tab
providing various actions for the user to take besides clicking to open or closing
the consequences of updates to the JS file
bundling the JS file with React vs deploying it in the public directory
caching (unrelated to notifications)

To Test

Run origin-dapp

DApp Changes

The most critical UX precaution is to ask the user to enable notifications at the most compelling time before actually initiating the browser's native permission request prompt. Once a user denies permission, she is unlikely to ever grant it since doing so would require changing the browser's settings. This first implementation prompts the user to enable notifications immediately after creating a listing or making an offer. If the modal is dismissed, a stronger prompt is rendered explaining the importance of push notifications in a decentralized application. Permission is only requested at the API level after the user affirms her interest in enabling notifications. A PushSubscription is created after this permission is granted. Additional PushSubscriptions are created each time the user creates a listing or makes an offer using a different ETH account using a browser in which permission was already granted.

Two environment variables are required: the public VAPID key and the notification server's URL.

NOTIFICATIONS_KEY=
NOTIFICATIONS_URL=

To Consider

not rendering the call-to-action modals on top of existing modals
prompting for notifications in later offer lifecycle stages if not previously enabled
analytics
scoping subscriptions to ETH networks
displaying non-push notifications
blocking/obscuring the UI while the native permission dialog is displayed
supporting notifications configuration in a settings page

To Test

Create a listing
Enable notifications
Make an offer from a different account

franckc

Awesome. A few comments inline. Let's discuss to resolve them...

origin-notifications/app.js

franckc · 2018-10-22T18:14:23Z

origin-notifications/app.js

+  res.sendStatus(201)
+})
+
+app.post('/events', async (req, res) => {


Need to add authentication otherwise user could get spammed by an external service calling this webhook.
We could have a secret that the listener passes as a header value in the request and that the notifier service checks. Another possibility would be to rely on internal queues rather than webhooks: listener would enqueue events and notifier would dequeue and process. Let's discuss...

This doesn't need to be exposed to the outside world right? The simple solution here is I don't expose this endpoint outside the Kubernetes cluster. The listener can just call it from inside the cluster.

Correct, @tomlinton 👌

Excellent idea @tomlinton !

franckc · 2018-10-22T18:16:03Z

origin-notifications/app.js

+    try {
+      // should safeguard against duplicates
+      await webpush.sendNotification(s, JSON.stringify({
+        title: log.eventName,


We'll probably want to localize this string. Add a TODO

franckc · 2018-10-22T18:22:14Z

origin-notifications/app.js

+  }).forEach(async s => {
+    try {
+      // should safeguard against duplicates
+      await webpush.sendNotification(s, JSON.stringify({


We'll probably want to have some retry in case for some reason that server is temporarily down.
Ok to just add a TODO for now.

Note: there is a retry function implemented in origin-js/src/utils/retries.js
We need this in quite a few places so perhaps we should just pull it out of origin-js and put it in a util package or equivalent ?

origin-notifications/migrations/20181019004743-create-push-subscription.js

franckc · 2018-10-22T18:34:43Z

origin-notifications/src/config/config.json

@@ -0,0 +1,9 @@
+{


Add a TODO. For productionizing this we'll need to use env variables (DATA_USERNAME, DATABASE_PASSWORD, etc...)

franckc · 2018-10-22T18:37:13Z

origin-notifications/migrations/20181019004743-create-push-subscription.js

+  up: (queryInterface, Sequelize) => {
+    return queryInterface.createTable('PushSubscriptions', {
+      id: {
+        allowNull: false,


Unecessary since this is a primary key and therefore it can't be null.

franckc · 2018-10-22T19:26:19Z

Also when we decide this is no longer a POC but a real feature, let's add some unit tests... :)

micahalcorn · 2018-10-22T23:18:59Z

Based on my conversation with @franckc this morning, here are some additional considerations:

authentication for write endpoints
server-side preferences
retries

Before merging this PR, I will add a compound index for the endpoints and accounts along with rate limiting for subscription creation. The Boss will add add a shared secret to this new server and the discovery server.

franckc · 2018-10-24T03:41:01Z

origin-notifications/app.js

@@ -50,8 +50,13 @@ app.use(bodyParser.json())
 // currently showing all subscriptions in an unauthenticated index view - maybe not a good idea in production?
 app.get('/', async (req, res) => {
  const subs = await PushSubscription.findAll()


I'd recommend moving the DB query within the if (app.get('env') === 'development') statement.

franckc

I have been notified by my supervisor that I should approve this request prompto.

This piece of software engineering looks very solid to me. I dutifully approve merging it into the Origin Protocol code repository.

Faithfully,
FC

DanielVF · 2018-11-16T00:58:38Z

Vapid keygen, for reference:
After installing web-push, from a node command, run:

const webpush = require('web-push');
const vapidKeys = webpush.generateVAPIDKeys()

vapidKeys.publicKey
// gives 'BDO0P...eoH'

vapidKeys.privateKey
// gives 'sjfiejfwief_wefwfwe'

micahalcorn requested review from crazybuster, franckc, wanderingstan and DanielVF October 22, 2018 13:10

franckc reviewed Oct 22, 2018

View reviewed changes

Notifications server with push subscriptions and service worker

fb5e1f4

micahalcorn force-pushed the micah/notifications branch from 5250352 to fb5e1f4 Compare October 22, 2018 23:10

Add index and remove unnecessary attribute constraint

24a7cf3

micahalcorn mentioned this pull request Oct 23, 2018

Notifications: Browser Push #806

Closed

micahalcorn added 4 commits October 23, 2018 17:16

Add rate limiting

d63f6a8

Condition rendering of indexed data and stub out production config

d4e84c8

Add README for new subdirectory

384e5c7

Merge branch 'master' into micah/notifications

d0f0536

franckc reviewed Oct 24, 2018

View reviewed changes

micahalcorn added 2 commits October 23, 2018 22:43

Refrain from sending notifications to transacting party

be0d17a

Update comments

edbf643

franckc approved these changes Oct 24, 2018

View reviewed changes

Merge branch 'master' into micah/notifications

16b03b9

franckc merged commit 5b95727 into master Oct 24, 2018

franckc deleted the micah/notifications branch October 24, 2018 06:12

micahalcorn removed request for DanielVF, wanderingstan and crazybuster October 24, 2018 06:19

micahalcorn mentioned this pull request Oct 24, 2018

Notifications Modal #766

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push Notifications (Chrome & Firefox) #795

Push Notifications (Chrome & Firefox) #795

micahalcorn commented Oct 22, 2018

franckc left a comment

franckc Oct 22, 2018

tomlinton Oct 25, 2018

micahalcorn Oct 25, 2018

franckc Oct 25, 2018

franckc Oct 22, 2018

franckc Oct 22, 2018

franckc Oct 22, 2018

franckc Oct 22, 2018

franckc commented Oct 22, 2018

micahalcorn commented Oct 22, 2018

franckc Oct 24, 2018 •

edited

Loading

franckc left a comment •

edited

Loading

DanielVF commented Nov 16, 2018 •

edited

Loading

Push Notifications (Chrome & Firefox) #795

Push Notifications (Chrome & Firefox) #795

Conversation

micahalcorn commented Oct 22, 2018

Checklist:

Description:

Notifications Server

Service Worker

DApp Changes

franckc 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

franckc commented Oct 22, 2018

micahalcorn commented Oct 22, 2018

franckc Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

franckc left a comment • edited Loading

Choose a reason for hiding this comment

DanielVF commented Nov 16, 2018 • edited Loading

franckc Oct 24, 2018 •

edited

Loading

franckc left a comment •

edited

Loading

DanielVF commented Nov 16, 2018 •

edited

Loading