Call identity service to update a user as of
the keyserver workflow.
Part of https://linear.app/comm/issue/ENG-2763
Depends on D6939
Details
Details
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
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jonringer/identity-updateuser-quick (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
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
| 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. |
| 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', ), ); |
Comment Actions
JS looks good, but identity build seems to have failed CI. Adding @varun as blocking for the Rust side of things
Comment Actions
Looks like this was because of issues with Buildkite Agent API, restarted the workflows.
Comment Actions
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 | This should be awaited, sorry I missed this initially | |
| 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? |
| 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. |
| 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 |
| 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 |
| 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. |