Page MenuHomePhabricator

[lib] Disable keyserver session recovery when paramOverride is specified
ClosedPublic

Authored by ashoat on Feb 5 2024, 12:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Apr 9 2024, 3:34 AM
Unknown Object (File)
Mar 1 2024, 12:13 PM
Unknown Object (File)
Feb 28 2024, 10:13 PM
Unknown Object (File)
Feb 24 2024, 9:53 AM
Unknown Object (File)
Feb 24 2024, 9:53 AM
Unknown Object (File)
Feb 24 2024, 9:52 AM
Unknown Object (File)
Feb 24 2024, 8:20 AM
Unknown Object (File)
Feb 19 2024, 1:38 PM
Subscribers
None

Details

Summary

We currently use paramOverride in three places in the codebase:

  1. UsernameSelection on native, where we call useLegacyAshoatKeyserverCall with paramOverride set to override urlPrefix for exactSearch.
  2. ConnectEthereum on native, where we call useLegacyAshoatKeyserverCall with paramOverride set to override urlPrefix for both exactSearch and getSIWENonce.
  3. useIsKeyserverURLValid, where we call useKeyserverCall with paramOverride set to override keyserverInfos as follows. (D10290 does something similar for useVerifyInviteLink.)
keyserverInfos: {
  [keyserverURL]: {
    urlPrefix: keyserverURL,
  },
},

In all of these cases, we're calling a keyserver before we've authenticated to it. Since keyserver session recovery functionality is only relevant when a session invalidation occurs, it's not needed in these cases.

The reason I want to disable it broadly for cases of paramOverride being set is that paramOverride can be set to override something more important, such as the cookie. We keep just a single set of waitingCalls for each keyserver... as such, you could imagine somebody setting paramOverride to an incorrect cookie, and triggering a session recovery when the cookie in Redux is actually correct.

Depends on D10950

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

ashoat requested review of this revision.Feb 5 2024, 1:09 PM
inka added 1 blocking reviewer(s): tomek.

This code is quite complex and hard to analyse. Moreover, instead of setting canRecoverSession to false, we use a default undefined, and only set to true when we want to allow it. I followed all places where getCallSingleKeyserverEndpoint is used (which is the only place where we set canRecoverSession to true), and it looks like it is indeed never used where paramOverride is used.
It also looks like canRecoverSession i being set to true in all other cases.
I would still love for someone else to review this though

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