Details

Reviewers

ashoat
tomek

Commits

rCOMM6d42130d4255: [keyserver] Introduce `createSIWEAccount` and use in `siweAuthResponder`

Summary

Introduce createSIWEAccount(...) and use in siweAuthResponder to create an account for an address if it doesn't already exist.

NOTE: This differs with how we do things currently for "password login" where the register and login endpoints both send back SUCCESS payloads with the necessary data to "log the user in" on the client. For SIWE the createSIWEAccount will only do account creation if necessary and will leave fetching to processSuccessfulLogin which will construct the SUCCESS payload (LogInResponse) and send it back to the client. This simplifies things on the client because we only have to handle one type of response from the siwe_auth endpoint. We should just be able to add | action.type === siweAuthActionTypes.success to all the reducers on the client where we check logInActionTypes.success and we should be good to go. Please let me know if there's somethign I'm missing that requires us to send back separate payloads for registration/login.

Depends on D6076

Test Plan

Will do thorough testing after I've put up rest of the stack + before landing.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Dec 28 2022, 5:31 PM

add redundant userID precondition check in createSIWEAccount to be safe.

atul published this revision for review.Dec 28 2022, 5:42 PM

atul added inline comments.

keyserver/src/creators/account-creator.js
200–215 ↗	(On Diff #20272)	We can remove this block of code because We won't have a `password` in the request for SIWE We check whether a username exists using `fetchUserIDForEthereumAddress` instead.
221–231 ↗	(On Diff #20272)	We can remove this block since we're using `fetchUserIDForEthereumAddress(...)` to check for existing accounts instead. We also have no equivalent of `reservedUsernamesSet` for ethereum addresses, but maybe one day we have a blocklist?
233 ↗	(On Diff #20272)	Irrelevant, we don't store a `hash`.
308–333 ↗	(On Diff #20272)	We want to limit `createSIWEAccount` solely to account creation. The fetching of `rawMessageInfos`, `currentUserInfo`, etc will be handled in `siweAuthResponder` by `processSuccessfulLogin` instead to avoid redundant queries and to make the `siweAuthResponder` interface cleaner (Only ever return `LogInResponse` even if registration is involved).

atul added inline comments.Dec 28 2022, 5:44 PM

keyserver/src/creators/account-creator.js
214–217 ↗	(On Diff #20272)	Yes, this is redundant with the check in `siweAuthResponder(...)` before this function is called... but I thought it was a good idea to include the check here anyways since we don't know how `createSIWEAccount(...)` may be used in the future?
235–238 ↗	(On Diff #20272)	Additional columns of the `cookies` table will be populated later in the stack.

Harbormaster completed remote builds in B14862: Diff 20271.Dec 28 2022, 5:44 PM

Harbormaster completed remote builds in B14863: Diff 20272.Dec 28 2022, 5:51 PM

Accepting with comment

keyserver/src/creators/account-creator.js
214–217 ↗	(On Diff #20272)	I don't think we should check this twice. I also think we should avoid implying to the reader that this function handling checking the validity of the request... I'd rather do all the checks here or none of them. Maybe we can rename the function to more clearly indicate that it doesn't do checks? Eg. `processSIWEAccountCreation`?

This revision is now accepted and ready to land.Dec 28 2022, 6:25 PM

atul added inline comments.Dec 28 2022, 6:26 PM

keyserver/src/creators/account-creator.js
214–217 ↗	(On Diff #20272)	Maybe we can rename the function to more clearly indicate that it doesn't do checks? Eg. `processSIWEAccountCreation`? Makes perfect sense, I'll rename and leave a comment saying validation should happen at callsite before the function is called.