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
Branch
inka/2_set_new_session
Lint
No Lint Coverage
Unit
No Test Coverage

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