Page MenuHomePhabricator

[lib][web] Fix set_new_session action corrupting keyserverStore
ClosedPublic

Authored by inka on Jan 3 2024, 1:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 1, 4:22 PM
Unknown Object (File)
Thu, Oct 31, 1:23 AM
Unknown Object (File)
Oct 16 2024, 2:09 AM
Unknown Object (File)
Oct 16 2024, 2:09 AM
Unknown Object (File)
Oct 16 2024, 2:09 AM
Unknown Object (File)
Oct 16 2024, 2:09 AM
Unknown Object (File)
Oct 16 2024, 2:09 AM
Unknown Object (File)
Oct 14 2024, 7:05 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-6097/set-new-session-action-for-new-keyserver-corrupts-keyserverstore
When the client calls the version or verifyInviteLink they may not yet be authenticated with that keyserver, and not have it in the store. The keyserver would send them a session change, resulting in setNewSessionActionType being dispatched, and the reducer would add the keyserver to the store but with incomplete data.
Because this action only sets a cookie and/or removes queuedActivityUpdates, we don't really need to do anything for a keyserver that is not in our store - the cookie sent by a keyserver the client is not connected to is an anonymous cookie, and we plan to remove anonymous cookies anyway, because they don't really do anything (ENG-4653).

A second change was needed in web, because the reducer in web is run before the reducers in lib, and it would also add the keyserver while trying to set a sessionID (which is just null in this case, so we also don't need to remeber it for anything).

Test Plan

Tested that adding a keyserver on web and native doesn't result in an error anymore, and that the setNewSessionActionType doesn't add the keyserver anymore
Tested that registration flow is not broken anymore
I tested all of those by reverting D10441 and connecting to my local keyserver as the second keyserver

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Jan 3 2024, 1:58 AM
ashoat requested changes to this revision.Jan 3 2024, 6:13 AM

There is no reason to ever break the Redux state and crash the app. We don't expect to ever get a SET_NEW_SESSION with a user cookie when calling version or verify_invite_link, but if we do for some reason, we should print a warning instead of breaking the Redux state and crashing the app...

lib/reducers/keyserver-reducer.js
79–84 ↗(On Diff #35149)

I think it's pretty much always a bad idea to set cookie when !state.keyserverInfos[keyserverID]. Here's what I would do instead

web/redux/redux-setup.js
168–173 ↗(On Diff #35149)

Same thing here – if we get a user cookie, we should still avoid breaking the state. We can do a console.log here in that case

This revision now requires changes to proceed.Jan 3 2024, 6:13 AM
This revision is now accepted and ready to land.Jan 3 2024, 6:27 AM