Page MenuHomePhabricator

[lib] Move CallKeyserverEndpoint session recovery to KeyserverConnectionHandler
ClosedPublic

Authored by ashoat on Feb 5 2024, 12:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Apr 14 2024, 9:28 AM
Unknown Object (File)
Apr 13 2024, 12:32 PM
Unknown Object (File)
Apr 11 2024, 8:01 PM
Unknown Object (File)
Mar 28 2024, 8:31 AM
Unknown Object (File)
Mar 16 2024, 1:42 AM
Unknown Object (File)
Mar 12 2024, 2:48 AM
Unknown Object (File)
Mar 12 2024, 1:51 AM
Unknown Object (File)
Mar 10 2024, 7:37 PM
Subscribers
None

Details

Summary

This is part of ENG-6586.

We want the session recovery to be handled in one place, so that we can avoid multiple simultaneous recovery attempts.

This diff starts the process of moving all resolveKeyserverSessionInvalidation callsites to KeyserverConnectionHandler. In this diff we move the callsite in CallKeyserverEndpointProvider.

Depends on D10953

Test Plan
  1. Test successful session invalidation in single keyserver world
    1. I prevented the Socket from rendering by adding a return null line before the other returns in KeyserverConnectionHandler. This avoided the Socket triggering session recovery.
    2. I started the iOS simulator and logged in using a test user.
    3. I opened the Redux Dev Tools
    4. I deleted the test user's cookie from the MariaDB database: DELETE FROM cookies WHERE user = 6390578 AND platform = 'ios'
    5. I sent a message as the test user
    6. I confirmed that session recovery was triggered in the Redux dev tools (and through some console logs)
    7. I repeated the process above several times to make sure it consistently worked multiple times in a single run
    8. I confirmed that the message was delivered "transparently" (without any visible issues, or evidence of session invalidation)
  2. Test failed session invalidation in single keyserver world
    1. I ran through the above test, but I hacked legacy-recover-keyserver-session.js to use the wrong password so the session recovery would fail
    2. I confirmed that I was logged out, and that an alert appeared explaining that my session was invalidated
  3. Test logging out during session recovery
    1. I triggered an infinite loop of session recoveries by running through the above test, but swallowing the SET_NEW_SESSION
    2. I logged out of the app
    3. I confirmed that the session recovery loop stopped, and that I was logged out successfully

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/components/keyserver-connection-handler.js
229–230 ↗(On Diff #36646)

This will be handled in a later diff, where I will take apart resolveKeyserverSessionInvalidation

I won't land it without this piece

lib/keyserver-conn/call-keyserver-endpoint-provider.react.js
340 ↗(On Diff #36646)

It might be best (and more consistent with prior behavior) to call this with null if the session recovery failed. This would prevent a retry that will almost certainly fail

To detect if the session recovery failed, I could check if there is a user cookie

Setting @tomek as blocking since this touches KeyserverConnectionHandler

Rebase

lib/components/keyserver-connection-handler.js
229–230 ↗(On Diff #36654)

This will be handled in a later diff, where I will take apart resolveKeyserverSessionInvalidation

I won't land it without this piece

lib/keyserver-conn/call-keyserver-endpoint-provider.react.js
340 ↗(On Diff #36654)

It might be best (and more consistent with prior behavior) to call this with null if the session recovery failed. This would prevent a retry that will almost certainly fail

To detect if the session recovery failed, I could check if there is a user cookie

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 5 2024, 1:05 PM
Harbormaster failed remote builds in B26519: Diff 36654!
ashoat requested review of this revision.Feb 5 2024, 1:17 PM
  1. Pass null callSingleKeyserverEndpoint to waitingCall when the recovery fails
  2. Make sure to flip sessionRecoveryInProgress back to false when the recovery fails
lib/components/keyserver-connection-handler.js
229–230 ↗(On Diff #36696)

This will be handled in a later diff, where I will take apart resolveKeyserverSessionInvalidation

I won't land it without this piece

I've updated the Test Plan to cover testing both a successful and failed keyserver session recovery in the single-keyserver world

lib/components/keyserver-connection-handler.js
229–230 ↗(On Diff #36732)

This will be handled in a later diff, where I will take apart resolveKeyserverSessionInvalidation

I won't land it without this piece

tomek added inline comments.
lib/components/keyserver-connection-handler.js
246 ↗(On Diff #36732)

In this case, we won't set sessionRecoveryInProgress to false. Is it intentional?

This revision is now accepted and ready to land.Feb 7 2024, 6:27 AM
lib/components/keyserver-connection-handler.js
246 ↗(On Diff #36732)

Yes. If the session recovery is cancelled, there are two cases to consider:

  1. It was cancelled due to the code on line 281. In this case, we want the recovery to be tried again, so we don't want to flip sessionRecoveryInProgress to false.
  2. It was cancelled due to the code on line 294. In this case, either sessionRecoveryInProgress is false or isUserAuthenticated is false.
    • If sessionRecoveryInProgress is already false, then there is no reason to set it to false.
    • If isUserAuthenticated is false, then we no longer have a user cookie for this keyserver. We should rely on keyserver-reducer.js to make sure that when the cookie is removed (or set to an anonymous cookie), we also set sessionRecoveryInProgress to false. D10972 is responsible for this, but I will give it another pass to make sure all cases are considered.
lib/components/keyserver-connection-handler.js
203 ↗(On Diff #37628)

I can use urlPrefixSelector here