Details

Reviewers

kamil
tomek

Commits

rCOMMd91f5d96ab3e: [keyserver] Get WNS authorization tokens

Summary

We need to get WNS (windows notification service) access token from our WNS secrets. This access token is required to send the notifications. This is just a simple POST request taken from the quickstart tutorial.

Test Plan

Run the new function and check if the acces token is properly returned. Modify the returned 'expires_in' value to a smaller one and check if the token is requested again.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.Mar 31 2023, 5:19 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptMar 31 2023, 5:19 AM

I can't plan changes before the CI passes but I forgot to switch to node-fetch so I'm removing the reviewers for now.

Harbormaster completed remote builds in B17889: Diff 24504.Mar 31 2023, 5:36 AM

michal requested review of this revision.Mar 31 2023, 5:36 AM

Update to node-fetch

michal edited the summary of this revision. (Show Details)Apr 3 2023, 3:39 AM

michal added reviewers: kamil, tomek.Apr 3 2023, 3:45 AM

michal added inline comments.

keyserver/src/push/providers.js
130 ↗	(On Diff #24561)	This is to handle an edge case when the token is near expiring and we preemptively refresh it in case of longer network latency etc. Inspired by the readme in this library (I didn't add it as a dependency because it didn't seem worth just to handle this one case of oauth).

michal added a parent revision: D7282: [keyserver] Add node-fetch package.Apr 3 2023, 3:45 AM

Harbormaster completed remote builds in B17931: Diff 24561.Apr 3 2023, 3:49 AM

michal added a child revision: D7284: [keyserver] Send WNS notifications.Apr 3 2023, 3:59 AM

tomek accepted this revision.Apr 4 2023, 3:37 AM

tomek added inline comments.

keyserver/src/push/providers.js
130 ↗	(On Diff #24561)	For how long a new token lives? Should we make this duration as long as a request timeout?
135 ↗	(On Diff #24561)	When are we going to extend this token lifetime? I guess if we return a token that has just 301ms left we don't have too much time to act.
138–141 ↗	(On Diff #24561)	Should we include a note about this in our docs? Do we store this file in 1Password?
160 ↗	(On Diff #24561)	Can we add a little more explanation to this error?

This revision is now accepted and ready to land.Apr 4 2023, 3:37 AM

tomek added inline comments.Apr 4 2023, 3:39 AM

keyserver/src/push/providers.js
154–157 ↗	(On Diff #24561)	In the guide `Content-Type` header is set - is there a reason to not do that?

Extended the duration of the token to 10s so we have more time to act. All of the tokens I had gotten from WNS previously, had 1h expiry time so it's still a small portion of the expiry time. Added better error explanation.

keyserver/src/push/providers.js
138–141 ↗	(On Diff #24561)	I didn't add any new docs when adding web notifs, but if we have any official place for these I can definitely added them. There's a bit more info in the ENG-3562. The values themselves are stored in the 1Password.
154–157 ↗	(On Diff #24561)	It should be set automatically because we are using `URLSearchParams` docs