Page MenuHomePhabricator

[keyserver/web] Pass push notif API public key to web app
ClosedPublic

Authored by michal on Feb 13 2023, 7:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 2:06 PM
Unknown Object (File)
Mon, Mar 25, 2:06 PM
Unknown Object (File)
Mon, Mar 25, 2:06 PM
Unknown Object (File)
Mon, Mar 25, 2:06 PM
Unknown Object (File)
Mon, Mar 25, 2:06 PM
Unknown Object (File)
Tue, Mar 5, 4:01 PM
Unknown Object (File)
Feb 21 2024, 1:26 PM
Unknown Object (File)
Feb 21 2024, 1:26 PM
Subscribers

Details

Summary

ENG-2825
Keyserver needs to authenticate with the push service and to do that we need to generate a key pair and send the public one to the web app, which it will use during notification subscription. The keys were generated using web-push package in a json format. This diff injects the push key into redux.

Test Plan

Check if the key is available in redux in the web app.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat requested changes to this revision.Feb 13 2023, 3:44 PM
ashoat added inline comments.
keyserver/src/responders/website-responders.js
104 ↗(On Diff #22464)

It looks like we're introducing one use case right now where we need to open this file to access the publicKey parameter. Will we need to access other parameters in the future? Should this code be factored out into another file that can be used for accessing this config?

110 ↗(On Diff #22464)

By circumventing the importJSON framework that we use for all other imports, you're making this impossible to configure on prod. Whenever you are doing something that has been done before, please spend some time researching how it's done and why

113 ↗(On Diff #22464)

Should we be printing some error?

This revision now requires changes to proceed.Feb 13 2023, 3:44 PM

Moved the code to the push/providers.js and use importJSON. I'm no longer caching the config myself because importJSON does the caching. Added logging and invariant.

ashoat requested changes to this revision.Feb 17 2023, 10:34 AM
ashoat added inline comments.
keyserver/src/responders/website-responders.js
309–311 ↗(On Diff #22670)

I don't think we need to print this in development... it's normal for a developer to not have this configured.

On the other hand, I think we should print something in production (instead of the invariant below)

314–317 ↗(On Diff #22670)

This is a dangerous invariant, and I'm not sure what it accomplishes vs. just printing a warning

This revision now requires changes to proceed.Feb 17 2023, 10:34 AM

Don't print in development, print warning instead of invariant in production (the previous code was inspired by our apn handling of secrets)

Please rebase on master, re-run yarn cleaninstall, update this diff, and confirm all CI gates are passing before landing!

Rebase, move the web-push dependency from future diff to this one, and init it during config loading.

This revision is now accepted and ready to land.Feb 21 2023, 3:22 AM

Not familiar with this library, but it seems reasonable. Unfortunately the author doesn't seem to keep an up-to-date changelog / GitHub releases, but it seems like a popular library so it probably isn't completely broken

This revision is now accepted and ready to land.Feb 21 2023, 6:44 AM