Page MenuHomePhabricator

[lib] Create notifs Olm session
ClosedPublic

Authored by tomek on Jan 17 2024, 8:16 AM.
Tags
None
Referenced Files
F3349302: D10667.diff
Fri, Nov 22, 5:36 PM
Unknown Object (File)
Fri, Nov 8, 7:06 AM
Unknown Object (File)
Tue, Nov 5, 10:03 PM
Unknown Object (File)
Tue, Nov 5, 9:27 PM
Unknown Object (File)
Tue, Nov 5, 7:21 PM
Unknown Object (File)
Oct 18 2024, 12:53 AM
Unknown Object (File)
Oct 18 2024, 12:53 AM
Unknown Object (File)
Oct 18 2024, 12:53 AM
Subscribers

Details

Summary

Use the keyserver keys to create the sessions.

Depends on D10666

Test Plan

Get access token, set it, and check if the session is created correctly... At this point it isn't because of invalid signature, probably due to prekey format - I'm investigating it.

Diff Detail

Repository
rCOMM Comm
Branch
handlers-2
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek held this revision as a draft.
tomek published this revision for review.Jan 17 2024, 8:54 AM
lib/components/keyserver-connection-handler.js
30–32 ↗(On Diff #35729)

Can we use cookieSelector here?

63–65 ↗(On Diff #35729)

Shouldn't this throw? Or at least give some kind of feedback?

lib/components/keyserver-connection-handler.js
30–32 ↗(On Diff #35729)

Yes, we should do that

63–65 ↗(On Diff #35729)

It's even better: after changes that are made earlier in the stack, the value is mandatory, so we don't need this check at all. But overall, the handling of missing keys should depend on the retry mechanism.

accepting but please address @inka's comments

This revision is now accepted and ready to land.Jan 18 2024, 10:28 AM

I am confused by this differential. I have two questions:

  1. What is the purpose of deleting and recreating notifications sessions at least every time app starts (session creation is not idempotent - it overwrites previous session)? Session is created on login and via socket if the keyserver happens to be missing session.
  2. In its current state this differential doesn't guarantee that session on the client is in sync with session on the keyserver - initialEncryptedMessage returned from session creator is not used. If this differential is intentionally breaking the app and you plan to fix it later in the stack then it should be explicitly stated in summary or in a comment.

I am confused by this differential. I have two questions:

  1. What is the purpose of deleting and recreating notifications sessions at least every time app starts (session creation is not idempotent - it overwrites previous session)? Session is created on login and via socket if the keyserver happens to be missing session.
  2. In its current state this differential doesn't guarantee that session on the client is in sync with session on the keyserver - initialEncryptedMessage returned from session creator is not used. If this differential is intentionally breaking the app and you plan to fix it later in the stack then it should be explicitly stated in summary or in a comment.

Currently, this code is gated behind usingCommServicesAccessToken flag, so it won't break the current logic.

  1. Ultimately, this code will execute as a part of login / retry logic, so it won't be the case that it is run on the app start. Thanks for the heads up about function not being idempotent!
  2. The session will be sent to a keyserver using a new auth endpoint - that will be implemented as the next part of https://linear.app/comm/project/implement-the-keyserver-connection-handlers-c95c61b19a15. This diff doesn't break the app, because this logic is hidden behind the flag.
This revision was automatically updated to reflect the committed changes.