[lib] Don't reduce KEYSERVER_AUTH_SUCCESS in reduceCurrentUserInfo
ClosedPublic
Actions

Authored by ashoat on Apr 18 2024, 12:52 PM.

Details

Reviewers

inka
tomek

Commits

rCOMM98dbbf1b5f85: [lib] Don't reduce KEYSERVER_AUTH_SUCCESS in reduceCurrentUserInfo

Summary

As discussed on Linear here, the changes in D11140 are not working as desired. The issue is that SET_NEW_SESSION is actually setting the currentUserInfo during keyserver auth, prior to KEYSERVER_AUTH_SUCCESS.

I figure that perhaps the simplest solution here is to remove the code for reducing KEYSERVER_AUTH_SUCCESS from reduceCurrentUserInfo, and to move the error checking to SET_NEW_SESSION.

Test Plan

Make sure that I can still register an account using the new flow with multi-keyserver

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Apr 18 2024, 12:52 PM

Harbormaster completed remote builds in B28302: Diff 39241.Apr 18 2024, 1:11 PM

ashoat requested review of this revision.Apr 18 2024, 1:11 PM

ashoat added inline comments.Apr 21 2024, 5:23 PM

lib/reducers/user-reducer.js
88 ↗	(On Diff #39241)	I'm seeing this get printed when keyserver auth fails and triggers `KeyserverConnectionHandler` to log out. The `SET_NEW_SESSION` from the logout is printing this error Not sure if that's behavior we want to keep. If we only want to print this error when the `SET_NEW_SESSION` has a user cookie or a non-anonymous `currentUserInfo`, I can make that change

inka added inline comments.Apr 22 2024, 7:57 AM

lib/reducers/user-reducer.js
88 ↗	(On Diff #39241)	This looks like an information about an inconsistency in the app behaviour, so I would rather it didn't print if everything is fine. I think it makes sense to check if `actionUserInfo.id` is present, and if not, silently disregard this action in this reducer

ashoat added inline comments.Apr 22 2024, 7:00 PM

lib/reducers/user-reducer.js
88 ↗	(On Diff #39241)	Hmm... wouldn't silently disregarding the action in that case break session invalidations? I agree with hiding the `console.log`, but I think we still need to process the action. I propose that we treat `LoggedOutUserInfo` the same as `null`/`undefined`: we just return `actionUserInfo`. Only if `actionUserInfo` is a `LoggedInUserInfo` will we print the notice and set `avatar`/`settings`.