Page MenuHomePhabricator

[keyserver] add loginUserPake function to rust-node-addon, call it from loginResponder in js
ClosedPublic

Authored by varun on Feb 28 2023, 2:12 PM.
Tags
None
Referenced Files
F3517694: D6914.id23269.diff
Sun, Dec 22, 6:05 PM
F3517603: D6914.id23385.diff
Sun, Dec 22, 5:53 PM
F3517448: D6914.id23270.diff
Sun, Dec 22, 5:31 PM
F3517391: D6914.id23423.diff
Sun, Dec 22, 5:14 PM
F3516241: D6914.diff
Sun, Dec 22, 1:00 PM
Unknown Object (File)
Tue, Dec 17, 8:45 PM
Unknown Object (File)
Tue, Dec 17, 8:45 PM
Unknown Object (File)
Tue, Dec 17, 8:45 PM
Subscribers

Details

Summary

Added loginUserPake based on the existing, equivalent function in our native_rust_library. The only major
difference is the error types.

we call it in loginResponder here. will add it to siweAuthResponder the diff after the next

Test Plan

this is tested along with registerUser, see the test plan in the subsequent diff

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun held this revision as a draft.
keyserver/src/responders/user-responders.js
416–419 ↗(On Diff #23269)

Why don't we just pass signedIdentityKeysBlob directly?

lib/types/rust-binding-types.js
28 ↗(On Diff #23269)

Is this SignedIdentityKeysBlob? We should avoid Object

type sessionInitializationInfo field using SignedIdentityKeysBlob type

address other piece of feedback

keyserver/src/responders/user-responders.js
410 ↗(On Diff #23271)

Can't we just check signedIdentityKeysBlob?

keyserver/src/responders/user-responders.js
416–419 ↗(On Diff #23269)

I was thinking what if SignedIdentityKeysBlob changes, but it makes sense to then change what we're sending to the Identity service in lockstep

varun published this revision for review.Mar 2 2023, 6:56 PM
varun edited the summary of this revision. (Show Details)
varun edited the test plan for this revision. (Show Details)
varun added reviewers: jon, bartek.
varun added a parent revision: D6940: [identity] some small fixes.
keyserver/addons/rust-node-addon/src/identity_client/login_user.rs
46–65 ↗(On Diff #23385)

really not a fan of these deeply nested constructions with ? expressions in the middle

107–112 ↗(On Diff #23385)

refactor above?

Feel like it's the rust's equivalent of "constructors shouldn't through exceptions"

Just one nit. Realizing my comment from last time probably doesn't make sense to Flow, and passing in the primary ed25519 twice (in signingPublicKey and sessionInitializationInfo) lets Rust avoid having to re-parse the JSON

keyserver/src/responders/user-responders.js
428 ↗(On Diff #23385)

I guess Flow is forcing us to check both of these?

432 ↗(On Diff #23385)

I guess it's easier to parse the JSON in JS rather than just passing in signedIdentityKeysBlob and asking Rust to re-parse the JSON

lib/types/rust-binding-types.js
28 ↗(On Diff #23385)

I don't think we're being specific enough here or in the .proto. When we refer to the primary ed25519 key, I think it would good to be more specific, so that the reader understands that this is one of the keys inside the blob, as opposed to any of the other keys in the blob, or even some other random key

This revision is now accepted and ready to land.Mar 3 2023, 8:51 AM
varun marked an inline comment as done.

address feedback

Realizing my comment from last time probably doesn't make sense to Flow

sorry, not recalling this, which comment?

keyserver/addons/rust-node-addon/src/identity_client/login_user.rs
107–112 ↗(On Diff #23385)

done

keyserver/src/responders/user-responders.js
428 ↗(On Diff #23385)

that's right

432 ↗(On Diff #23385)

yeah i'd rather not do the JSON serialization/deserialization stuff on the rust side

lib/types/rust-binding-types.js
28 ↗(On Diff #23385)

I created a linear task to change all instances of "signingPublicKey" to a more descriptive name

https://linear.app/comm/issue/ENG-3224/change-signingpublickey-to-something-more-descriptive