[lib] move olm utils from keyserver to lib
ClosedPublic
Actions

Authored by varun on Jan 29 2024, 8:13 PM.

Details

Reviewers

Commits

Summary

we can reuse these on web, too, so moving them out of keyserver

Test Plan

able to log in to staging identity service from my local keyserver after clearing the metadata table

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

lib/flow-typed/npm/@commapp/olm_vx.x.x.js
1 ↗	(On Diff #36343)	copied this file from `web`
lib/utils/olm-utils.js
64 ↗	(On Diff #36343)	Will update this comment
85 ↗	(On Diff #36343)	I think this is fine, but I changed this line from `throw new ServerError` to `throw new Error`. wanted to call it out since it might be hard to tell. this is the only line of code i modified

varun requested review of this revision.Jan 29 2024, 8:32 PM

marcin added inline comments.

keyserver/src/utils/olm-utils.js
23 ↗	(On Diff #36343)	This can be moved to lib as well.
71–107 ↗	(On Diff #36343)	I think that we could move to lib any function from this file that doesn't touch Rust/Database ops/networking. One thing that might block us is that a call to `olm.init()` isn't straightforward on web (see `web/olm-utils.js` file). @varun could you create a follow-up task to move all functions from this file that don't touch Rust/Database ops/networking to lib and seek for places in web that they could be used and link this task prior to landing?

This revision is now accepted and ready to land.Jan 30 2024, 9:25 AM

ashoat requested changes to this revision.Jan 30 2024, 11:37 AM

ashoat added inline comments.

keyserver/src/utils/olm-utils.js
23 ↗	(On Diff #36343)	Why does it need to be moved? It's only used in `keyserver`, it looks like
71–107 ↗	(On Diff #36343)	I'm not sure we need a task for this... we can move things to `lib` as needed, when we identify cases where code can be shared. I don't think we need to preemptively move things to `lib`... feels like overengineering, and could lead to eg. bloating our `native` bundle (I think some upcoming Metro release introduces tree shaking, but not sure it's there yet)
lib/utils/olm-utils.js
85 ↗	(On Diff #36343)	This is risky... `ServerError` is special-cased in keyserver code here and here Seeing as this function gets called from responders, I think it's important to make sure we throw `ServerError` in those contexts. I think we have two options: Revert to using `ServerError` here. It's weird since it's not technically a `ServerError` when it happens in `web`, but it would avoid any issues. Wrap the invocations of `retrieveAccountKeysSet` in `keyserver/src/user/login.js` with a `try-catch` that rethrows the error as a `ServerError`.

This revision now requires changes to proceed.Jan 30 2024, 11:37 AM

address feedback

lib/utils/olm-utils.js
85 ↗	(On Diff #36343)	Option 2 looked better but was a little awkward to implement since `retrieveAccountKeysSet` is a callback function. I updated `fetchCallUpdateOlmAccount` to re-throw any error from the callback function as a `ServerError`. Verified with this gist: https://gist.github.com/vdhanan/5949c0a4f0501d38a953774f81328916 I was able to see `ServerError: invalid_prekey` in the logs after applying this patch