Page MenuHomePhabricator

[Keyserver/identity] Call identity service update_user
ClosedPublic

Authored by jon on Mar 2 2023, 11:30 PM.
Tags
None
Referenced Files
F1439382: D6944.diff
Thu, Mar 28, 7:25 PM
Unknown Object (File)
Wed, Mar 20, 12:33 PM
Unknown Object (File)
Sun, Mar 17, 11:03 AM
Unknown Object (File)
Thu, Mar 14, 11:26 PM
Unknown Object (File)
Wed, Mar 6, 3:30 AM
Unknown Object (File)
Feb 27 2024, 11:49 PM
Unknown Object (File)
Feb 27 2024, 11:49 PM
Unknown Object (File)
Feb 27 2024, 11:49 PM
Subscribers

Details

Summary

Call identity service to update a user as of
the keyserver workflow.

Part of https://linear.app/comm/issue/ENG-2763

Depends on D6939

Test Plan
nix develop

comm-dev services start # start localstack

# Configure "aws" resources on localstack
(cd services/terraform && ./run.sh)

# start identity service and run mobile workflow
(cd services/identity && cargo run -- server &)
(cd keyserver && yarn dev &)
(cd native && yarn dev &)

Run either the iOS or Android client:

  • Create an account
  • Use TablePlus or other tool to find userID of your user
  • Seed a user:
    • aws dynamodb put-item --table-name identity-users --item '{"userID": {"S": "<USERID>"}}'
  • Verify user exists:
    • aws dynamodb get-item --table-name identity-users --key '{ "userID": {"S": "<USERID>"} }'
  • Change password on account in App
  • Verify user has registration updated with valid entry:
    • aws dynamodb get-item --table-name identity-users --key '{ "userID": {"S": "<USERID>"} }'

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jon published this revision for review.Mar 2 2023, 11:33 PM

I'm still working on one remaining issue with message ordering, but wanted the diff to be up so varun had more time to review

ashoat requested changes to this revision.Mar 3 2023, 10:34 AM
ashoat added inline comments.
keyserver/src/updaters/account-updaters.js
51–60 ↗(On Diff #23393)

We don't need to wait on getRustAPI before calling dbQuery. Here's how I would rewrite this (including my other two pieces of feedback)

52 ↗(On Diff #23393)

Previously, the code here would wait until the dbQuery completed before returning. Your updates here make it so we no longer wait for dbQuery, which could lead to serious issues since the state of the database does not reflect the updates anymore.

60 ↗(On Diff #23393)

We should never initiate a promise like this in Node.js without wrapping it in handleAsyncPromise.

The issue is that if the promise rejects without being caught, Node.js treats this a bit like an exception being thrown and not caught.

You'll see an "unhandled promise rejection" warning in our current version of Node. But in more recent versions of Node, this would crash the whole app.

This revision now requires changes to proceed.Mar 3 2023, 10:34 AM
keyserver/src/updaters/account-updaters.js
51–60 ↗(On Diff #23393)

Actually we might want to wait for the dbQuery to finish before calling updateUser. Not sure if that's what we're currently doing elsewhere (@varun?), but it so we could consider something like this instead:

const rustAPIPromise = getRustAPI(); // this could be moved to the top
const dbPromise = await dbQuery(saveQuery);
handleAsyncPromise(
  rustApi.updateUser(
      viewer.userID,
      'placeholder-device-id',
      'placeholder-name',
      newPassword,
      'placeholder-public-key',
    ),
);

Serialize messages in correct order

Just got to the point of a successful update, still need to address feedback

jon added inline comments.
keyserver/src/updaters/account-updaters.js
51–60 ↗(On Diff #23393)

Sounds good to me

52 ↗(On Diff #23393)

Made dbQuery finish before attempting to run the identity code

60 ↗(On Diff #23393)

Fair enough, was really tired when I threw this up.

jon marked 3 inline comments as done.

Handle promises more gracefully

ashoat added 1 blocking reviewer(s): varun.

JS looks good, but identity build seems to have failed CI. Adding @varun as blocking for the Rust side of things

but identity build seems to have failed CI

Looks like this was because of issues with Buildkite Agent API, restarted the workflows.

In D6944#206752, @atul wrote:

but identity build seems to have failed CI

Looks like this was because of issues with Buildkite Agent API, restarted the workflows.

Should happen less frequently going forward: https://linear.app/comm/issue/ENG-3226/add-retries-to-buildkite-agent-pipeline-upload-command

keyserver/src/updaters/account-updaters.js
56 ↗(On Diff #23421)

This should be awaited, sorry I missed this initially

varun requested changes to this revision.Mar 3 2023, 2:32 PM
varun added inline comments.
keyserver/addons/rust-node-addon/src/identity_client/update_user.rs
255 ↗(On Diff #23435)

handle

255 ↗(On Diff #23435)

Some of the nomenclature here needs to be cleared up and made consistent, but not hi pri

keyserver/src/updaters/account-updaters.js
56–61 ↗(On Diff #23435)

this needs to be updated to remove device ID. args should match registerUser's

lib/types/rust-binding-types.js
41–47 ↗(On Diff #23435)

we no longer have the concept of deviceId, this needs to be updated to match registerUser above

also the rust update_user fn signature returns a String, how is this working?

This revision now requires changes to proceed.Mar 3 2023, 2:32 PM

Explicitly await update user action

varun requested changes to this revision.Mar 3 2023, 2:49 PM

back to your queue to address feedback on previous revision

This revision now requires changes to proceed.Mar 3 2023, 2:49 PM
jon added inline comments.
keyserver/addons/rust-node-addon/src/identity_client/update_user.rs
255 ↗(On Diff #23435)

I copied this from the registration code.

In https://linear.app/comm/issue/ENG-2733 we can revisit how the code is structured and make it more consistent

keyserver/src/updaters/account-updaters.js
56–61 ↗(On Diff #23435)

Funny how things change like that...

lib/types/rust-binding-types.js
41–47 ↗(On Diff #23435)

Nothing is being done with the result...

My assumption would be that the string is being coerced into a falsey value.

The method currently returns with the access token... but keyserver shouldn't have anything to do with the token.

jon marked 3 inline comments as done.

Address varuns feedback

varun requested changes to this revision.Mar 3 2023, 3:13 PM
varun added inline comments.
keyserver/addons/rust-node-addon/src/identity_client/update_user.rs
255 ↗(On Diff #23443)

can we fix this typo before landing?

255 ↗(On Diff #23435)

in the registration code this is called handle_registration_credential_response. In the login workflow we have handle_login_credential_response. In this context, credential is the indicator that we're doing PAKE login, and the prefix informs which RPC workflow we're in. Admittedly this is very unclear and we should spend some time rethinking these names later. Just wanted to explain my original comment :)

keyserver/src/updaters/account-updaters.js
56–62 ↗(On Diff #23443)

this still needs to be fixed

This revision now requires changes to proceed.Mar 3 2023, 3:13 PM
jon marked 2 inline comments as done.

Move unneeded parameters to protobuf message creation

jon added inline comments.
keyserver/src/updaters/account-updaters.js
56–62 ↗(On Diff #23443)

Reduced JS bindings to just what was relevant.

Didn't know how to construct a phony blob

This revision is now accepted and ready to land.Mar 3 2023, 3:53 PM

Please wait for CI before landing

jon added inline comments.
keyserver/addons/rust-node-addon/src/identity_client/update_user.rs
255 ↗(On Diff #23435)

handle_registration_credential_response does both a register.finish() and a login_start. Which is confusing when you're looking at just subsets of code and don't have the full context. Either way, I think this will be addressed with some refactoring.

keyserver/addons/rust-node-addon/src/identity_client/update_user.rs
255 ↗(On Diff #23435)

ClientRegister::finish() and ClientLogin::start() is what I meant to say. Realized that we have similarly named methods, which is part of the confusion when they don't correspond one to one.