Page MenuHomePhabricator

[lib] introduce stateless `olm` API
ClosedPublic

Authored by kamil on Feb 19 2024, 9:00 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 10:47 AM
Unknown Object (File)
Fri, Nov 22, 10:46 AM
Unknown Object (File)
Fri, Nov 22, 4:53 AM
Unknown Object (File)
Fri, Nov 8, 5:12 PM
Unknown Object (File)
Oct 23 2024, 7:24 PM
Unknown Object (File)
Oct 23 2024, 7:24 PM
Unknown Object (File)
Oct 23 2024, 7:24 PM
Unknown Object (File)
Oct 23 2024, 7:21 PM

Details

Summary

The point of this code is to introduce stateless olm interface shared across all platforms:

  • on native we have CryptoModule which takes care of each action and persists state
  • on web there is ongoing @michal's work to achieve the same (project).

This will allow us to:

  • use olm API from /lib (we can also expand it to /keyserver)
  • move code like peerToPeerMessageHandler to /lib
  • completely move taking care of state/transactions/atomicity to a different place and from a business logic perspective only call methods

Right now, on web it will cause some code duplication with OlmSessionCreatorContext, but after ENG-6462 this context won't have any state and we can remove it and just use this API.

This also helps @michal to implement moving responsibility to worker - right now app still works with context and when everything is ready we can just "flip-the-switch" to use this API.

This approach was discussed with @michal, @tomek, and @marcin.

Depends on D11110

Test Plan

Tested encrypt/decrypt methods on web with properly generated accounts/sessions.

Diff Detail

Repository
rCOMM Comm
Branch
land-olm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
lib/utils/config.js
30 ↗(On Diff #37331)

wondering if this is the best approach - but can't think of any alternative

web/crypto/olm-api.js
14–24 ↗(On Diff #37331)

we already have this API here but first we need to implement ENG-6462.

31–34 ↗(On Diff #37331)

After ENG-6462 here will be just something like suggested.

@michal going to just move this code to worker side.

kamil published this revision for review.Feb 19 2024, 9:33 AM

I think this approach makes sense

lib/utils/config.js
30 ↗(On Diff #37331)

If we only need the API from within React components, we could consider a React context with a provider

If we do choose to use this approach, keep in mind that it might be difficult to implement without import cycles. This might be easier to handle if you replace this with a () => OlmAPI. @inka has been dealing with something similar in ENG-6872

web/crypto/olm-api.js
28 ↗(On Diff #37331)

I think on native this function does more. Is there a plan to add that functionality later?

tomek added inline comments.
native/crypto/olm-api.js
8–16 ↗(On Diff #37331)

It seems that we can simplify this

lib/types/crypto-types.js
126 ↗(On Diff #37331)

Is this content account? What about a notifications account? Is it going to be included too?

lib/types/crypto-types.js
126 ↗(On Diff #37331)

As discussed in the office: Methods for notif account are already implemented in OlmSessionCreatorContext so I am not including them here to avoid code duplication - but those could be moved here while working on moving everything to worker (ENG-6657).

lib/utils/config.js
30 ↗(On Diff #37331)

If we only need the API from within React components, we could consider a React context with a provider

I think in the future we can use it from different places, and this also allows us to implement the same interface and have more shared code with keyserver.

If we do choose to use this approach, keep in mind that it might be difficult to implement without import cycles. This might be easier to handle if you replace this with a () => OlmAPI. @inka has been dealing with something similar in ENG-6872

Maybe I am missing something but not sure if () => OlmAPI fixes this - OlmAPI is just an object with methods and doesn't require additional computation. I think we should only avoid using getConfig() on the level. (like I did in D11120 - going to update this).

web/crypto/olm-api.js
28 ↗(On Diff #37331)

yes, but this is tracked in ENG-6647. Current logic is implemented in GetOrCreateCryptoStoreProvider

This revision is now accepted and ready to land.Feb 23 2024, 3:23 AM
This revision was landed with ongoing or failed builds.Feb 26 2024, 5:46 AM
This revision was automatically updated to reflect the committed changes.