Page MenuHomePhabricator

[lib] Conclude sessionRecoveryInProgress when user cookie is received or session is invalidated
ClosedPublic

Authored by ashoat on Feb 5 2024, 12:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 20 2025, 8:04 PM
Unknown Object (File)
Feb 3 2025, 7:49 PM
Unknown Object (File)
Jan 23 2025, 2:33 PM
Unknown Object (File)
Jan 22 2025, 10:45 PM
Unknown Object (File)
Jan 22 2025, 10:45 PM
Unknown Object (File)
Jan 22 2025, 10:45 PM
Unknown Object (File)
Jan 22 2025, 10:45 PM
Unknown Object (File)
Jan 22 2025, 10:45 PM
Subscribers
None

Details

Summary

If we ever receive a user cookie, we can consider the keyserver session to be recovered.

By flipping sessionRecoveryInProgress to false, we tell CallKeyserverEndpointProvider to flush all waitingCalls using the new cookie.

It doens't matter if the new cookie came from the keyserver session recovery or not. If the keyserver session recovery is still in progress, when it concludes it should be ignored by invalidSessionRecovery.

Additionally, if the session is invalidated, we can consider the recovery to be failed and concluded.

Depends on D10948

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

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 5 2024, 12:39 PM
Harbormaster failed remote builds in B26506: Diff 36641!
ashoat requested review of this revision.Feb 5 2024, 1:05 PM

By flipping sessionRecoveryInProgress to false, we tell CallKeyserverEndpointProvider to flush all waitingCalls using the new cookie.

We also flip it to false on recovery fail, doesn't that cause the flush too? Wouldn't that be a problem?

This revision is now accepted and ready to land.Feb 6 2024, 3:28 AM

We also flip it to false on recovery fail, doesn't that cause the flush too? Wouldn't that be a problem?

No, that's intentional. If the recovery fails, we want to "flush" all of the requests so that they fail.

No, that's intentional. If the recovery fails, we want to "flush" all of the requests so that they fail.

I see, thank you

Also conclude the recovery when the session is invalidated

ashoat retitled this revision from [lib] Conclude sessionRecoveryInProgress whenever user cookie is received to [lib] Conclude sessionRecoveryInProgress when user cookie is received or session is invalidated.Feb 6 2024, 11:58 AM
ashoat edited the summary of this revision. (Show Details)

The latest revision here reflects a change in the pattern of Redux actions for a session recovery failure.

If a session recovery fails, we will dispatch a SET_NEW_SESSION. Previously I was relying on a SET_SESSION_RECOVERY_IN_PROGRESS, but I decided to avoid having to dispatch two actions in a row, as this is considered bad Redux practice. Instead, we'll rely on just the single SET_NEW_SESSION action to flip sessionRecoveryInProgress to false.

I think this is safe, as we can always consider a session recovery to be concluded if the session is invalidated.