Page MenuHomePhabricator

[lib] Fix updatesCurrentAsOf returned from siweAuth
ClosedPublic

Authored by inka on Dec 5 2023, 3:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jan 2, 7:17 AM
Unknown Object (File)
Thu, Jan 2, 7:17 AM
Unknown Object (File)
Thu, Jan 2, 7:17 AM
Unknown Object (File)
Thu, Jan 2, 7:17 AM
Unknown Object (File)
Thu, Dec 26, 3:20 PM
Unknown Object (File)
Sun, Dec 15, 4:41 AM
Unknown Object (File)
Sat, Dec 14, 3:46 PM
Unknown Object (File)
Nov 8 2024, 5:58 PM
Subscribers

Details

Summary

siweAuth returns LogInResult where updatesCurrentAsOf is of type {+[keyserverID: string]: number}. response.serverTime is a number. Flow didn't catch this because to it response is any. We use ashoatKeyserverID because this action was not yet refactored to work with multiple keyservers.

Test Plan

Checked that the types are now correct

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Dec 5 2023, 3:49 AM

Thanks! Would've been good to catch this in D10058

Do you know if this would have caused any issues on SIWE clients? Eg. would there be a crash loop because updatesCurrentAsOf gets set to 0 or undefined?

This revision is now accepted and ready to land.Dec 5 2023, 7:20 PM

Also, curious why password login (logIn) does a fanout to multiple keyservers, but SIWE login (siweAuth) only goes to one keyserver at a time. I guess siweAuth just hasn't been refactored yet to multi-keyserver... is there a task tracking this?

(I wonder if perhaps the approach in logIn will be replaced by ENG-5997, where we'll have a separate instance of a handler component for each keyserver, that will perhaps handle login individually for each keyserver, and will do it via a new keyserver endpoint that @varun introduces in ENG-3262. As such, not sure if it makes sense to do the above refactoring on siweAuth. cc @tomek)

Do you know if this would have caused any issues on SIWE clients? Eg. would there be a crash loop because updatesCurrentAsOf gets set to 0 or undefined?

This would result in full state sync, which I think can cause a crash loop, yes.

Also, curious why password login (logIn) does a fanout to multiple keyservers, but SIWE login (siweAuth) only goes to one keyserver at a time. I guess siweAuth just hasn't been refactored yet to multi-keyserver... is there a task tracking this?

As discussed here, SIWE actions are supposed to be deprecated by @varun's work. I think this is tracked in ENG-5926. Login was refactored by me to make it easier for @varun to add the new login action, that would use the new way of calling keyservers with useKeyserverCall, that I had implemented.

Thanks for explaining, @inka! I'll try to make a new native (and web) release after this is landed