Details

Reviewers

tomek
inka

Commits

rCOMM477ec3ca967d: [lib] Dispatch SET_NEW_SESSION when session recovery fails

Summary

Prior to D10952, the code in call-single-keyserver-endpoint.js would handle dispatching SET_NEW_SESSION for a failed session recovery.

In that diff, I changed that behavior. The reason was that we can have multiple simultaneous callSingleKeyserverEndpoint calls. Even if they trigger only a single session recovery, when that session recovery fails it would result in multiple SET_NEW_SESSION actions, since they will be dispatched by multiple callSingleKeyserverEndpoint calls.

Now that callSingleKeyserverEndpoint isn't handling SET_NEW_SESSION for failed session recoveries, we need to handle it elsewhere. The KeyserverConnectionHandler is the right place, since it is where we handle the session recovery, and we are able to ensure that only a single session recovery is running for a particular keyserver at one time.

I considered leaving the setSessionRecoveryInProgressActionType calls there, but decided against it because it's bad Redux practice to dispatch two actions in a row, instead of having a single action handle everything. In order for the SET_NEW_SESSION call to handle flipping sessionRecoveryInProgress, I made some changes to D10949.

I decided to make this a separate diff (instead of including it in D10954) because there are some substantial changes here. I will annotate them inline.

Depends on D10954

Test Plan

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)
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
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 created this revision.Feb 6 2024, 12:09 PM

Cache value of preRequestUserState before calling resolveKeyserverSessionInvalidation

ashoat added inline comments.Feb 6 2024, 12:19 PM

lib/components/keyserver-connection-handler.js
219 ↗	(On Diff #36731)	We only need to do a "spot check" on this value below. To avoid regenerating `performRecovery` whenever it changes, I assign to a ref here By assigning to a ref, I avoid using the value of `preRequestUserState` at the time `performRecovery` is bound, and instead am able to use the value of `preRequestUserState` right before `resolveKeyserverSessionInvalidation` is called below
254–255 ↗	(On Diff #36731)	One difference in the code before my recent changes: previously, if the session recovery failed, we would commit the `sessionChange` that came in the initial failed request to Redux. After my changes, we are using this `genericCookieInvalidation` instead. This means we won't keep the `anonymous` cookie we are passed. But these `anonymous` cookies are useless, and are due to be deprecated in ENG-4653. Furthermore, trying to pass the `sessionChange` from the failed request would be tough... we would need to find some way to get it from the `CallKeyserverEndpointProvider` to the `KeyserverConnectionHandler`.
lib/shared/session-utils.js
65–68 ↗	(On Diff #36731)	Without this change, `invalidSessionRecovery` was filtering out the `SET_NEW_SESSION` call. I think that prior to my recent changes, `SET_NEW_SESSION` with `cookieInvalidated` would only be responsible for changing the `cookie`, and it would contain the same `currentUserInfo` that was currently in the Redux store. In my new system, `SET_NEW_SESSION` with `cookieInvalidated` is responsible for clearing the `currentUserInfo`, so it contains a `LoggedOutCurrentUserInfo` here. I think this change is safe. If it's a `LoggedOutCurrentUserInfo`, it should not count as a session recovery, since the session is being thrown away. Meanwhile, I think `invalidSessionDowngrade` should handle making sure that the `SET_NEW_SESSION` is ignored if something has changed in the meantime.
lib/types/session-types.js
65 ↗	(On Diff #36731)	This change was necessary because if we simply don't include `cookie`, the the code in `keyserver-reducer.js` won't clear it. In order to make sure the `cookie` is cleared, we need to specify `null`. This wasn't necessary before because the `cookie` would be set to a string (anonymous cookie).

ashoat added a child revision: D10972: [lib] Clear sessionRecoveryInProgress during auth / deauth.Feb 6 2024, 12:23 PM

Harbormaster completed remote builds in B26582: Diff 36730.Feb 6 2024, 12:26 PM

Harbormaster completed remote builds in B26583: Diff 36731.Feb 6 2024, 12:29 PM

ashoat requested review of this revision.Feb 6 2024, 12:29 PM

inka added inline comments.Feb 7 2024, 4:27 AM

lib/components/keyserver-connection-handler.js
204 ↗	(On Diff #36731)	Can we use `sessionIDSelector`?
lib/shared/session-utils.js
65–68 ↗	(On Diff #36731)	`actionCurrentUserInfo` can be null/undefined according to our types. Should we return in such case as well? We wanted to pass `preRequestUserState.currentUserInfo` for set new session action, instead of `sessionChange.currentUserInfo` ENG-6126 This was intended to fix the bug described in ENG-5648, in which a set new session was trying to downgrade the session, but would fail because of this functions `currentReduxState.currentUserInfo?.id !== actionCurrentUserInfo.id` check. But I think this code you introduce here is better - an action cannot be a recovery if it has `currentUserInfo = {anonymous: true}`. And this should fix the issue from ENG-5648

ashoat added inline comments.Feb 7 2024, 1:17 PM

lib/components/keyserver-connection-handler.js
204 ↗	(On Diff #36731)	Will make this change
lib/shared/session-utils.js
65–68 ↗	(On Diff #36731)	This was intended to fix the bug described in ENG-5648, in which a set new session was trying to downgrade the session, but would fail because of this functions `currentReduxState.currentUserInfo?.id !== actionCurrentUserInfo.id` check. But I think this code you introduce here is better - an action cannot be a recovery if it has `currentUserInfo = {anonymous: true}`. And this should fix the issue from ENG-5648 That makes sense, and happy to hear it will solve ENG-5648. `actionCurrentUserInfo` can be null/undefined according to our types. Should we return in such case as well? Are you sure we need to consider the case where `actionCurrentUserInfo` is `null`/`undefined`? There is an `invariant` directly above this line that would catch this case, and in fact there is a check in `native/redux/redux-setup.js` as well. We wanted to pass `preRequestUserState.currentUserInfo` for set new session action, instead of `sessionChange.currentUserInfo` ENG-6126 I think we still need to fix `invalidSessionRecovery`, as it does not currently do what it is supposed to (prevent a session recovery from overwriting a logout). I did some thinking about this and added a comment here.

sessionIDSelector

Harbormaster completed remote builds in B26633: Diff 36804.Feb 7 2024, 1:40 PM

inka added inline comments.Feb 8 2024, 1:59 AM

lib/shared/session-utils.js
65–68 ↗	(On Diff #36731)	Are you sure we need to consider the case where actionCurrentUserInfo is null/undefined? There is an invariant directly above this line that would catch this case, and in fact there is a check in native/redux/redux-setup.js as well. Ohh, that's right, sorry.

tomek accepted this revision.Feb 8 2024, 2:06 AM

tomek added inline comments.

lib/components/keyserver-connection-handler.js
219 ↗	(On Diff #36731)	Assigning to a ref in the render function might be confusing, and we should include a code comment explaining it. But we can also consider an alternative solution in which we replace `memo` with an `effect` that computes this value and assigns it to the ref.
lib/types/session-types.js
65 ↗	(On Diff #36731)	Can't we use `cookie?: ?string` instead?

This revision is now accepted and ready to land.Feb 8 2024, 2:06 AM

ashoat added inline comments.Feb 8 2024, 4:36 AM

lib/components/keyserver-connection-handler.js
219 ↗	(On Diff #36731)	I definitely think the effect approach is worse, as it forces us to wait on React to run the effect. I can add a code comment.
lib/types/session-types.js
65 ↗	(On Diff #36731)	Yes – that would additionally allow an explicit `cookie: undefined` to be specified. I'm not opposed to it, but I opted for this approach for consistency with the `sessionID` line directly above