Object.values has a bug in Flow. This bug is fixed in Flow 0.196.0, but we're still on 0.182.0. The Flow update is blocked on the next React Native upgrade, which should happen in the coming months (hopefully)

For now, we work around this with a values utility in lib/utils/objects.js, which uses $FlowFixMe (basically an any-cast) to silence the error

validateAccountPrekey appears to be an async function. From my review of D8400:

This is actually very risky because if it throws an exception, nothing will catch it. 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.

In other words, in Node.js code we should never call a function that returns a Promise (such as an async function) WITHOUT either awaiting it OR wrapping it in handleAsyncPromise.

In this case, however, it doesn't appear that there is any good reason for validateAccountPrekey to be an async function, as it does not use the await keyword. As such, we should probably just turn validateAccountPrekey into a normal function.

It looks like it would introduce a Flow error currently to do that, but that should be solvable by updating fetchCallUpdateOlmAccount so that it accepts any callback that returns T | Promise<T> (rather than requiring Promise<T>).

Here's what I would suggest:

const [prekey] = Object.values(prekeyMap);

You might need to use the aforementioned values utility if you see Flow errors.

I think this name is more clear. Your original name seems like it's validating a login action, or the result of a login action

Always early exit please. Simple case should go first to reduce mental RAM

This appears to be copy-pasted from getOlmSessionInitializationDataResponder. Instead of copy-pasting, ideally we could factor out the shared code.

If it's possible to factor out, can you please introduce a preceding diff to this one that does it? If not, can you explain why it can't be factored out?

one thing to note, is that marcin's work in keys-responder also marks the prekey and one time keys as published, introducing a potential race condition.

Can you explain the race condition in a bit more detail? fetchCallUpdateOlmAccount should prevent races

You should never leave a dangling Promise like this in Node.js code. You should either await it or wrap in handleAsyncPromise. See comment above for more details

You should never leave a dangling Promise like this in Node.js code. You should either await it or wrap in handleAsyncPromise. See comment above for more details

From my last review:

it doesn't appear that there is any good reason for validateAccountPrekey to be an async function

You appear to have made some related changes, but validateAccountPrekey is still an async function. Can you make it a normal function?

Not sure this was responded to?

You didn't actually address the feedback – promise is still dangling.

Please try "digging deeper" in understanding promises / async / await. It feels like you're currently taking a "guess and check" approach. You should make sure you actually understand this stuff...

Please re-review my feedback, and potentially look for some resources online to augment your understanding of things like Promise.all, the "unhandled promise rejection" error from Node, etc.

Same here

I think we decided to move this to happen before the fork, in order to avoid races with the retrieveAccountKeysSet in keyserver/src/responders/keys-responders.js

Nit

See here. How about invalid_prekey?

Delete trailing semicolon

Nit

There's no await keyword, so this should not be an async function. You're creating a new Promise unnecessarily

Combine these awaits with Promise.all

Let's get rid of this console.log. It's unnecessarily "scary" for a first-time registration, where it's unexpected

Let's get rid of this console.log

When you concatenate these, you're assuming err is a string or can be coerced into one. We shouldn't make that assumption

Also better to use console.warn

1. See here. How about identity_auth_failed?
2. Why do you use ServerError above, but Error here?

It can be factored, but the code overlap should be removed in https://linear.app/comm/issue/ENG-4290/replace-usage-of-keyserver-x3dh-endpoint-with-identity-service.

Since it uses scoping to introduce quite a few items, the refactored method would be quite a doozy. But maybe it's worthwhile to refactor it.

Fair enough, that makes sense to me

Between lines 152 and 168 (or 186), it might be possible for someone to request the outgoing X3DH key details from the keys-responder. In which case the endpoint will return 11 one-time-keys, and then immediately mark them all as published.

In our 1:1, we decided to address this by awaiting verifyUserLoggedIn before we fork Express servers