Details

Reviewers

ashoat
tomek

Commits

rCOMM32e3d236ca9a: [keyserver] Return generated `nonce` in `siweNonceResponder`

Summary

return nonce generated from siwe:generateNonce
input validation with tcomb
introduce SIWENonceRequest and SIWENonceResponse types (to appease flow)

Again nothing very interesting, just necessary scaffolding.

Depends on D5884

Test Plan

Send a malformed request and make sure the keyserver throws.
Send a correctly formed request and make sure the keyserver validates input:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Dec 16 2022, 11:09 AM

atul published this revision for review.Dec 16 2022, 11:09 AM

Harbormaster failed remote builds in B14206: Diff 19439!Dec 16 2022, 11:12 AM

resolve eslint issue

atul edited the summary of this revision. (Show Details)Dec 16 2022, 11:17 AM

atul edited the test plan for this revision. (Show Details)

atul retitled this revision from [keyserver] Introduce `siweNonceRequestInputValidator` to [keyserver] Return generated `nonce` in `siweNonceResponder`.

atul edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B14207: Diff 19440.Dec 16 2022, 11:28 AM

atul added a child revision: D5887: [keyserver] Introduce `createSIWENonceEntry` to persist nonce in DB.Dec 16 2022, 12:02 PM

Don't land until you connect the nonce to a specific session please

This revision is now accepted and ready to land.Dec 16 2022, 1:15 PM

remove walletAddress from endpoint input

Harbormaster failed remote builds in B14212: Diff 19446!Dec 17 2022, 3:03 PM

update

Harbormaster failed remote builds in B14213: Diff 19447!Dec 17 2022, 3:10 PM

Harbormaster completed remote builds in B14213: Diff 19447.Dec 17 2022, 4:01 PM

ashoat added inline comments.Dec 19 2022, 9:20 AM

keyserver/src/responders/siwe-nonce-responders.js
7–9 ↗	(On Diff #19447)	This looks weird for a `responder` given it doesn't take `viewer` or `input`, but I guess that's by design. Worth noting that the `validateInput` function has the side effect of calling `checkClientSupported`, which makes sure that we don't respond to requests from clients with ancient `codeVersion`s. Probably doesn't matter for this use case, but it worries me a little bit... what if we add some additional side effect to `validateInput` in the future, on the assumption that every responder calls it? Not sure

atul added inline comments.Dec 19 2022, 9:50 AM

keyserver/src/responders/siwe-nonce-responders.js
7–9 ↗	(On Diff #19447)	Probably doesn't matter for this use case, but it worries me a little bit... what if we add some additional side effect to validateInput in the future, on the assumption that every responder calls it? Not sure Might make sense to change the name of `validateInput` to something like `validateInputAndViewer` or `validateRequest` to make clear that there's more to it than checking `input` with the `tcomb` stuff.