Page MenuHomePhabricator

[keyserver] Introduce `fetchUserIDForEthereumAddress` and use in `siweAuthResponder`
ClosedPublic

Authored by atul on Dec 27 2022, 11:19 PM.
Tags
None
Referenced Files
F3536514: D6060.diff
Wed, Dec 25, 6:35 PM
Unknown Object (File)
Wed, Dec 18, 5:34 PM
Unknown Object (File)
Wed, Dec 18, 5:34 PM
Unknown Object (File)
Wed, Dec 18, 5:34 PM
Unknown Object (File)
Wed, Dec 18, 5:33 PM
Unknown Object (File)
Wed, Dec 18, 5:33 PM
Unknown Object (File)
Wed, Dec 18, 5:33 PM
Unknown Object (File)
Wed, Dec 18, 5:33 PM
Subscribers
None

Details

Summary

Introduce fetchUserIDForEthereumAddress and use in siweAuthResponder to check if user exists and to complete login process (via call to successfulLogInQueries) if so.


Depends on D6059

Test Plan

Will be tested implicitly in future diffs. Will test thoroughly (lots of breakpoints to step through and ensure correctness) once account creation stuff is done.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.

tweak

atul published this revision for review.Dec 27 2022, 11:28 PM
ashoat added inline comments.
keyserver/src/responders/user-responders.js
323 ↗(On Diff #20206)
  1. This shouldn't be nullable – we should fail with a ServerError in any case where we can't generate it
  2. Is it okay that we're always returning a LogInResponse, even for cases where a user is registering for the first time? Does RegisterResponse have anything that LogInResponse doesn't?

On point 2, it seems like the intention is to handle new user registration later, so accepting to unblock

366 ↗(On Diff #20206)

We should probably still pass in a calendarQuery here... even though the client will always be resetting this to the default on log-in, we have no way to know the client's timezone here, so we might be off by a day.

Can you create a task to track passing the calendarQuery up from the client?

369 ↗(On Diff #20206)

Let's throw a ServerError for now if the account doesn't already exist, so we can keep the return type as not being nullable

This revision is now accepted and ready to land.Dec 28 2022, 6:20 AM
keyserver/src/responders/user-responders.js
323 ↗(On Diff #20206)
  1. It was just nullable temporarily before registration is integrated into siweAuthResponder, but I can make it required and throw instead.
  2. RegisterResponse has an id field which LogInResponse doesn't. LogInResponse has truncationStatuses, userInfos, rawEntryInfos?, and serverTime fields which RegisterResponse doesn't.

Initially I'll always return LogInResponse (calling createSIWEAccount before processSuccessfulLogin if necessary) to get login/registration working end-to-end asap, and then will break the registration case down after.

369 ↗(On Diff #20206)

Will update this diff

address feedback before landing

This revision was landed with ongoing or failed builds.Dec 28 2022, 3:16 PM
This revision was automatically updated to reflect the committed changes.

Having to revert this because of a stupid error, I should probably stop short-circuiting CI before landing...

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

throw new ServerError(...) instead of throw ServerError(...)

This revision was landed with ongoing or failed builds.Dec 28 2022, 3:50 PM
This revision was automatically updated to reflect the committed changes.