Page MenuHomePhabricator

[lib] Clear sessionRecoveryInProgress during auth / deauth
ClosedPublic

Authored by ashoat on Feb 6 2024, 12:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 8 2024, 3:26 AM
Unknown Object (File)
Mar 8 2024, 3:25 AM
Unknown Object (File)
Mar 8 2024, 3:25 AM
Unknown Object (File)
Mar 8 2024, 3:25 AM
Unknown Object (File)
Mar 8 2024, 3:25 AM
Unknown Object (File)
Mar 2 2024, 1:39 PM
Unknown Object (File)
Feb 29 2024, 11:24 AM
Unknown Object (File)
Feb 21 2024, 1:35 AM
Subscribers
None

Details

Summary

This is basically the login/logout case. When a keyserver is authorized or deauthorized, we should clear any active session recovery. These are the same cases when we clear the connectionIssue.

Depends on D10970

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

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

After receiving a comment from @tomek here, I reviewed this diff in more detail. I think there's one edge case I missed, and one case that should arguably be simplified

lib/reducers/keyserver-reducer.js
110 ↗(On Diff #36734)

Since this results in isUserAuthenticated becoming false (see point 2 here), we should also set sessionRecoveryInProgress to false

166 ↗(On Diff #36734)

This should cover all cases where isUserAuthenticated becomes false (see point 2 here). That said, it might be worth simplifying this case to instead just check if sessionChange.cookie !== undefined

tomek added inline comments.
lib/reducers/keyserver-reducer.js
146–174 ↗(On Diff #36805)

This isn't related to this diff but having this flag makes this code fragile and can be easily avoided.