Page MenuHomePhabricator

[lib] Don't reduce KEYSERVER_AUTH_SUCCESS in reduceCurrentUserInfo
ClosedPublic

Authored by ashoat on Apr 18 2024, 12:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 2:19 AM
Unknown Object (File)
Fri, Oct 18, 2:05 AM
Unknown Object (File)
Fri, Oct 18, 1:56 AM
Unknown Object (File)
Thu, Oct 17, 3:16 PM
Unknown Object (File)
Tue, Oct 15, 6:20 PM
Unknown Object (File)
Tue, Oct 15, 6:20 PM
Unknown Object (File)
Tue, Oct 15, 6:20 PM
Unknown Object (File)
Tue, Oct 15, 6:19 PM
Subscribers
None

Details

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

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

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

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.

Treat LoggedOutUserInfo the same as null/undefined

This revision is now accepted and ready to land.Apr 23 2024, 1:20 AM