Page MenuHomePhabricator

[lib] Run the keyserver auth only when it isn't running and a user isn't already authenticated
ClosedPublic

Authored by tomek on Jan 26 2024, 5:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 20 2024, 4:02 PM
Unknown Object (File)
Feb 20 2024, 2:35 PM
Unknown Object (File)
Feb 20 2024, 11:39 AM
Unknown Object (File)
Feb 20 2024, 7:00 AM
Unknown Object (File)
Feb 19 2024, 2:37 AM
Unknown Object (File)
Feb 14 2024, 12:26 AM
Unknown Object (File)
Feb 9 2024, 11:19 AM
Unknown Object (File)
Feb 8 2024, 4:39 AM
Subscribers

Details

Summary

Check if a user is logged in. Keep track of running auth and don't start it if it's already running.

Depends on D10830

Test Plan

Add a sleep to the auth effect and some logs to its start and finish. Also make sure that store contains a keyserver to which we aren't authenticated.

  1. Check if the auth doesn't start for authenticated keyserver - ashoatKeyserverID
  2. Check if the auth starts for unauth keyserver
  3. Modify e.g. calendar query (a dependency of an effect) and check if it doesn't start a new auth before the previous is finished
  4. Modify it again after the timeout and check if auth restarts (if the previous failed)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.Jan 26 2024, 5:20 AM
This revision is now accepted and ready to land.Jan 26 2024, 5:49 AM
lib/components/keyserver-connection-handler.js
88 ↗(On Diff #36151)

This seems risky. If some important dependency changes during auth, the change can be ignored... the effect update will be skipped here, and the old login will be left unchanged. Am I missing something?

99 ↗(On Diff #36151)

How does this get used? I'm a little confused... it seems like the old cookie is used as input to get a new cookie. Will contentSession still be valid after the cookie changes?

lib/components/keyserver-connection-handler.js
88 ↗(On Diff #36151)

It looks like this is addressed in D10844. We convert this ref to state, when means that when it's flipped, the effect is run again

marcin added inline comments.
lib/components/keyserver-connection-handler.js
99 ↗(On Diff #36151)

When it comes to notifs session then we are safe. When the keyserver creates new cookie for user then it will see that olm session table in MariaDB is missing notifs session associated with this particular cookie and will ask the client via socket to create new notifs session.

As far as I am concerned there is no similar mechanism for content session. I don't have enough context on content session to tell whether there exists an alternative mechanism to recover from such scenario though.

lib/components/keyserver-connection-handler.js
88 ↗(On Diff #36151)

Good point. D10844 helps a bit, but it also introduces a timeout, but login after important prop changes should happen immediately.

lib/components/keyserver-connection-handler.js
99 ↗(On Diff #36151)

This comment doesn't seem to answer my question, and it only results in more questions...

Let me start by rephrasing my question.

I'm seeing the old cookie passed to notificationsSessionCreator. I'm trying to understand, why is this passed in? The cookie is about to be deleted. What purpose does passing in the soon-to-be-deleted cookie serve?

Next, the new questions posed by your comment.

  1. You say in your comment we are "safe". What are we safe from?
  2. It sounds like the keyserver will need a new notifs session anyways. Is there any point to this notificationsSessionCreator call, then? Does it immediately get thrown away?
  3. You mention the content session, and how there is no "similar mechanism". What mechanism are you referring to? Is there a risk here regarding the content session?
lib/components/keyserver-connection-handler.js
88 ↗(On Diff #36151)

It would also be helpful if we had some way to trigger a login when recoveryInProgress flips to true. I'll think about the best way to do this, and handle it in a separate diff... but figure it makes sense to bring it up here as it may influence your design.

One option would be to lift the login code to a useCallback, and then introduce a separate useEffect that calls it when recoveryInProgress flips to true

lib/components/keyserver-connection-handler.js
88 ↗(On Diff #36151)

In an ideal world, the callback itself would handle "cancelling" the previous invocation of the callback. One way to implement cancellation is to have a function that returns a tuple of [Promise, cancellationFunc]:

let cancelled = false;
const cancellationFunc = () => {
  cancelled = true;
};
const promise = (async () => {
  const someData = await fetchSomeData();
  if (cancelled) {
    throw new Error('cancelled');
  }
  const someOtherData = await fetchSomeOtherData();
  if (cancelled) {
    throw new Error('cancelled');
  }
  ...
})();
return [cancellationFunc, promise];

For dispatchActionPromise, we'd likely have to replace the promise we pass in with a promise that checks cancelled at the very end before returning.

lib/components/keyserver-connection-handler.js
99 ↗(On Diff #36151)

How does this get used? I'm a little confused... it seems like the old cookie is used as input to get a new cookie.

The cookie is used only to store it on the web client. It was implemented in D10245. We added this parameter to the native API as well, just to make it convenient to use in lib, but the value is ignored.

Will contentSession still be valid after the cookie changes?

None of the notif and content sessions depend on the cookie. It is possible that we need a similar mechanism for content sessions, but maybe https://linear.app/comm/issue/ENG-6462/browser-tab-synchronization will fix the issue.

On the server side, we're storing OLM sessions indexed by a cookie D7631. So after a cookie changes, we should either create a new session or rekey an existing one.

I'm seeing the old cookie passed to notificationsSessionCreator. I'm trying to understand, why is this passed in? The cookie is about to be deleted. What purpose does passing in the soon-to-be-deleted cookie serve?

It's used only for storage on the web client side. But if the cookie changes, the stored session will no longer be available, right @marcin? Are we able to fetch this session after the cookie change?

You say in your comment we are "safe". What are we safe from?

Even if the OLM session is lost, the client and the keyserver would communicate over the socket and create a new session. So we're safe from a scenario where cookie change would result in a missing OLM session.

It sounds like the keyserver will need a new notifs session anyways. Is there any point to this notificationsSessionCreator call, then? Does it immediately get thrown away?

We probably can avoid it and rely on creating a session using the socket. But it would be better for both a keyserver and the client to be able to use this session. For the content session, it is even more important because we don't have any code that would fix it.

You mention the content session, and how there is no "similar mechanism". What mechanism are you referring to? Is there a risk here regarding the content session?

The mechanism is about creating a new session using the socket. For the content session, we don't have such a mechanism, so the session won't get recreated. We definitely should find a solution to this issue.

lib/components/keyserver-connection-handler.js
99 ↗(On Diff #36151)

Thanks for explaining. It seems like we have two issues:

  1. The keyserver require an initialNotificationsEncryptedMessage for client auth, but the client immediately throws away its notif session after it receives a new cookie, so the entire exchange is wasted.
  2. We have no way for a keyserver and client to set up a content session after auth.

We should prioritize resolving both of these.

lib/components/keyserver-connection-handler.js
99 ↗(On Diff #36151)

I agree with everything that @tomek said except for one statement:

But if the cookie changes, the stored session will no longer be available, right @marcin? Are we able to fetch this session after the cookie change?

This isn't true. The session is still stored in the IndexedDB and the client can fetch it. In fact everytime we receive a notif we:

  • get all session for all cookies
  • sort by cookie id
  • for decryption we use the newest cookie id.
  • remaining session are deleted

So the session for cookie that was replaced is still available and can be and will be fetched from the db. But it will be deleted soon,

tomek requested review of this revision.Jan 31 2024, 7:51 AM

Decided that it is a lot more efficient to introduce a new diff which introduces canceling the auth D10884.

The keyserver require an initialNotificationsEncryptedMessage for client auth, but the client immediately throws away its notif session after it receives a new cookie, so the entire exchange is wasted.

According to @marcin's comment, it seems like we should be able to use an older session, but that might require some work - created https://linear.app/comm/issue/ENG-6637/make-sure-that-we-dont-immediately-throw-away-a-notif-session-during to track

We have no way for a keyserver and client to set up a content session after auth.

Created https://linear.app/comm/issue/ENG-6638/introduce-content-session-creation-using-socket to track

This revision is now accepted and ready to land.Jan 31 2024, 7:51 AM