[lib][native] Handle invalidSessionRecovery for keyserver auth
ClosedPublic
Actions

Authored by inka on Jan 10 2024, 5:18 AM.

Details

Reviewers

michal
kamil
varun
ginsu

Commits

rCOMM64434275ba45: [lib][native] Handle invalidSessionRecovery for keyserver auth

Summary

issue: ENG-6358
invalidSessionRecovery is supposed to prevent logout being overriden by old session recovery
See the original diff introducing invalidSessionRecovery: D183

Currently we cross reference the currentUserInfo in state and action payload. But in the new login flow, the currentUserInfo may not be included in the action - and in the future it will definitely not be included.
We will instead check if the current user info is the same as the one the action was called for. This is similar to how PreRequestUserState is used right above for logout, to check if the logout action is not trying to override a newer valid session

Test Plan

Tested that if keyserverAuthActionTypes.success is dispatched for socketAuthErrorResolutionAttempt with the same user id in action.payload.preRequestUserInfo as is currently in the store, the code proceeds to next reducers
Tested that if keyserverAuthActionTypes.success is dispatched for socketAuthErrorResolutionAttempt with a differrent user id in action.payload.preRequestUserInfo then is currently in the store, the current state is returned

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

inka created this revision.Jan 10 2024, 5:18 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 10 2024, 5:18 AM

inka added a parent revision: D10589: [lib] Refactor reduceMessageStore.Jan 10 2024, 5:21 AM

inka added reviewers: michal, kamil, varun, ginsu.

Harbormaster completed remote builds in B25652: Diff 35469.Jan 10 2024, 5:43 AM

inka requested review of this revision.Jan 10 2024, 5:43 AM

michal accepted this revision.Jan 11 2024, 8:13 AM

michal added inline comments.

lib/actions/user-actions.js
247 ↗	(On Diff #35469)	Can we get this in `useKeyserverAuth` so the caller doesn't have to handle it?

This revision is now accepted and ready to land.Jan 11 2024, 8:13 AM

Get preRequestUserInfo in useKeyserverAuth

inka added a child revision: D10484: [lib][web][native] Create defaultKeyserverInfo.Jan 15 2024, 1:43 AM

Harbormaster failed remote builds in B25749: Diff 35600!Jan 15 2024, 1:51 AM

When the user dispatches a login, they may not have a currentUserInfo. But if the login was a resolution attempt, they have to have it set

Harbormaster completed remote builds in B25754: Diff 35605.Jan 15 2024, 2:19 AM

Closed by commit rCOMM64434275ba45: [lib][native] Handle invalidSessionRecovery for keyserver auth (authored by inka). · Explain WhyJan 16 2024, 1:52 AM

This revision was automatically updated to reflect the committed changes.

inka added a commit: rCOMM64434275ba45: [lib][native] Handle invalidSessionRecovery for keyserver auth.

ashoat added inline comments.Jan 16 2024, 6:27 PM

lib/shared/session-utils.js
59–64	I'm confused how this `invariant` is safe to add. This diff was landed to `master`, but it appears that `COOKIE_INVALIDATION_RESOLUTION_ATTEMPT` and `SOCKET_AUTH_ERROR_RESOLUTION_ATTEMPT` logins are still being dispatched on `master` without going through `useKeyserverAuth`. Does that mean this `invariant` will be triggered?

inka added inline comments.Jan 17 2024, 5:05 AM

lib/shared/session-utils.js
59–64	Before, this action took `actionCurrentUserInfo: CurrentUserInfo`, so we were sure that `actionCurrentUserInfo` was truthy. So this invariant would never throw. In this diff I want to also use this function for `keyserverAuthActionTypes`, in which the `actionCurrentUserInfo` may be falsy, but only if the login is not a resolution attempt. I checked and this code doesn't throw for `SOCKET_AUTH_ERROR_RESOLUTION_ATTEMPT`, so I'm not sure what you mean