Page MenuHomePhabricator

[lib][web][native] Stop removing some state on keyserver logout
ClosedPublic

Authored by inka on Jan 16 2024, 2:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 29, 7:04 PM
Unknown Object (File)
Fri, Nov 29, 6:25 PM
Unknown Object (File)
Tue, Nov 26, 8:43 AM
Unknown Object (File)
Tue, Nov 26, 6:08 AM
Unknown Object (File)
Nov 22 2024, 9:34 PM
Unknown Object (File)
Nov 22 2024, 8:57 PM
Unknown Object (File)
Nov 22 2024, 8:17 PM
Unknown Object (File)
Nov 14 2024, 11:55 PM
Subscribers

Details

Summary

issue: ENG-5824
enabled apps, CSAT, theme, backup enabled, crypto store, current user info - are all values that shouldn't be removed when user disconnects from a keyserver.
In some cases change also affects set new session action. This is fince, this data shouldn't also be removed when a keyserver sets a new session. The rest of set new session will be refactored in ENG-5767

Test Plan

Tested that those values still get removed if the user is not using CSAT (didn't test if CSAT is removed because we don't have it, but the code is pretty straightforward)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 16 2024, 2:34 AM
Harbormaster failed remote builds in B25788: Diff 35656!
inka requested review of this revision.Jan 16 2024, 6:03 AM

On Linear earlier today I proposed that we continue to use logOutActionTypes.success for whole-app logout (including keyserver and identity logout). If we take that approach, should we undo some of the changes here for logOutActionTypes.success?

inka planned changes to this revision.Jan 17 2024, 8:18 AM

We decided to have a single action for logging out of keyservers and identity.

lib/reducers/enabled-apps-reducer.js
33 ↗(On Diff #35774)

Same question as here

lib/reducers/services-access-token-reducer.js
29–32 ↗(On Diff #35774)

Same question as here

lib/reducers/theme-reducer.js
42–45 ↗(On Diff #35774)

Same question as here

lib/reducers/user-reducer.js
65–71 ↗(On Diff #35774)

Same question as here

native/redux/redux-setup.js
205–213 ↗(On Diff #35774)

Same question as here

web/redux/crypto-store-reducer.js
21 ↗(On Diff #35774)

Same question as here

Merge identity delete account with keyserver delete account

lib/reducers/enabled-apps-reducer.js
39 ↗(On Diff #35936)

Same here

web/redux/crypto-store-reducer.js
27 ↗(On Diff #35936)

This can be "lifted" into the above condition– nested conditionals aren't needed

michal requested changes to this revision.Jan 25 2024, 6:20 AM

Requesting changes for some explanations so I can understand the diff better. Feel free to re-request.

lib/reducers/user-reducer.js
67 ↗(On Diff #36025)

Don't we also need to update these actions?

native/redux/redux-setup.js
126–132 ↗(On Diff #36025)

Not related to this diff, but will this code, and the ... === keyserverAuthActionTypes.success && invalidSessionRecovery( ... code below, be removed after we completely migrate to identity service?

This revision now requires changes to proceed.Jan 25 2024, 6:20 AM
inka requested review of this revision.Jan 26 2024, 6:13 AM
inka added inline comments.
lib/reducers/user-reducer.js
67 ↗(On Diff #36025)

This is done in D10795

native/redux/redux-setup.js
126–132 ↗(On Diff #36025)

No. The user may still disconnect from a keyserver and connect to it again before the action succeeds
The user may also logout while a recovery keyserver auth is running, so keyserverAuthActionTypes.success may be an invalid session recovery

This revision is now accepted and ready to land.Jan 29 2024, 5:33 AM