[lib][web] Update condition for Redux state reset in SET_NEW_SESSION reducers
ClosedPublic
Actions

Authored by ashoat on Mar 3 2024, 9:06 PM.

Details

Reviewers

inka
tomek

Commits

rCOMM8801bf51e987: [lib][web] Update condition for Redux state reset in SET_NEW_SESSION reducers

Summary

We have some logic in reducers to special-case SET_NEW_SESSION when a cookie is invalidated.

This diff introduces three changes:

Now that we have authoritativeKeyserverID, I figured we should check it. The only time a SET_NEW_SESSION should lead to a state reset is when it's for the authoritative keyserver.
I changed it from checking usingCommServicesAccessToken to checking relyingOnAuthoritativeKeyserver. As long as we rely on an authoritative keyserver, we'll wait until we auth with it before logging the user in. As such, we should treat a deauth with the authoritative keyserver as a logout, and reset the state in that case.
I added a check for relyingOnAuthoritativeKeyserver to reduceCurrentUserInfo, which already had the authoritativeKeyserverID check.

Separately, something I'm wondering about... in some cases we check action.payload.sessionChange.cookieInvalidated, and in others we check action.payload.sessionChange.currentUserInfo?.anonymous. Is there any difference in these checks? Should we unify them? Wanted to ask @inka as they have touched this code recently and probably made the same observation.

Depends on D11228

Test Plan

I used this test plan for the whole stack: https://gist.github.com/Ashoat/75ab690d5c53cdd68a51b02e03e27c58

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Mar 3 2024, 9:06 PM

ashoat added a child revision: D11230: [lib][native] Introduce supportingMultipleKeyservers constant.

Harbormaster completed remote builds in B27285: Diff 37762.Mar 3 2024, 10:14 PM

ashoat requested review of this revision.Mar 3 2024, 10:14 PM

Separately, something I'm wondering about... in some cases we check action.payload.sessionChange.cookieInvalidated, and in others we check action.payload.sessionChange.currentUserInfo?.anonymous. Is there any difference in these checks? Should we unify them? Wanted to ask @inka as they have touched this code recently and probably made the same observation.

According to our types, if cookieInvalidated: true, then currentUserInfo is LoggedOutUserInfo.
It also looks like actually every time we send currentUserInfo: { anonymous: true }, then we also set cookieInvalidated: true, but this is less straightforward and connected to some logic on the keyserver side

I'm not sure if we want it to be possible to send one without the other. If so, I don't know why we would want to leave for example the enabled apps or the user store, if we logged the user out (dataLoaded is set to false if anonymous: true, so the logout screen is shown). This may need to be reviewed

ashoat edited the summary of this revision. (Show Details)Mar 4 2024, 5:13 PM

It sounds like it would be a good idea to make all of this consistent then.

Based on the ClientSessionChange Flow type, it seems like the action.payload.sessionChange.currentUserInfo.anonymous check is slightly more "generic" than the action.payload.sessionChange.cookieInvalidated... meaning if cookieInvalidated is true, then anonymous is definitely true, but it's possible (according to types) for anonymous to be true but cookieInvalidated to be false.

But on the other hand, I think the cookieInvalidated check is more readable. And based on @inka's investigation, it doesn't seem like the "cookieInvalidated is false but anonymous is true" case is possible on the keyserver side.

Based on that, I'll opt to unify everything to use the cookieInvalidated check.