Page MenuHomePhabricator

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

Authored by inka on Jan 10 2024, 5:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 4:15 PM
Unknown Object (File)
Tue, Nov 5, 4:05 PM
Unknown Object (File)
Tue, Nov 5, 1:25 PM
Unknown Object (File)
Thu, Oct 24, 10:43 AM
Unknown Object (File)
Thu, Oct 24, 7:20 AM
Unknown Object (File)
Thu, Oct 24, 6:34 AM
Unknown Object (File)
Thu, Oct 24, 6:23 AM
Unknown Object (File)
Thu, Oct 24, 6:21 AM
Subscribers

Details

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 requested review of this revision.Jan 10 2024, 5:43 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

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

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?

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

lib/shared/session-utils.js
59–64

Okay, thanks for explaining