Page MenuHomePhabricator

[lib, native,keyserver] Add KeyserverStore
ClosedPublic

Authored by inka on Jul 18 2023, 12:31 PM.
Tags
None
Referenced Files
F2157352: D8532.id28921.diff
Mon, Jul 1, 1:41 AM
F2157351: D8532.id28850.diff
Mon, Jul 1, 1:41 AM
F2157350: D8532.id28783.diff
Mon, Jul 1, 1:41 AM
F2157349: D8532.id28784.diff
Mon, Jul 1, 1:41 AM
F2157306: D8532.diff
Mon, Jul 1, 1:37 AM
F2153310: D8532.id28921.diff
Sun, Jun 30, 4:52 PM
F2150280: D8532.id28783.diff
Sun, Jun 30, 9:47 AM
F2149067: D8532.id28850.diff
Sun, Jun 30, 7:04 AM
Subscribers

Details

Summary

https://linear.app/comm/issue/ENG-4265/refactor-cookie-redux-field-for-multiple-keyservers
Adding keyserverStore field to redux that will hold keyserver-specific information. Currently it only has a cookie field.
We plan to remove the cookie field from redux, and have the cookie field in keyserverStore replace it. But in the interest of keeping the diffs small I will remove the cookie field later.
Previously we had a cookie?: void field on web, because we were not using it, but we needed it to exist for flow. Native had cookie: ?string. We plan to start using the cookie field on web in the future, and this stack is preparation
for it, so we do want the type on web to be changed. BUT for now we have not yet implemented the logic for using this field on web, so even though the type is changed, we want to keep the old functionality. So the important thing is
that we need the cookie on web to be undefined, and on native to be null or string, as they were before. The cookie field is defined as cookie?: ?string to allow this, but should be changed to cookie: ?string once ENG-4347 is done.

In this first iteration we are hardcoding the id of our keyserver

Test Plan

ran yarn flow-all. Checked using git grep that in all placed the cookie field is being set, I am also setting the keyserverStrore.keyserverInfos[].cookie

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Jul 18 2023, 12:51 PM
michal requested changes to this revision.Jul 19 2023, 6:06 AM

Seems like we are using keyserverPrefixID not as a prefix, but just as the server id. It would be more descriptive if it was named something like ashoatKeyserverID. Could make a task/ implement it quickly in a separate diff?

lib/reducers/keyserver-reducer.js
15–27 ↗(On Diff #28784)

Should resetUserStateActionType handle cookies for all keyservers?

28 ↗(On Diff #28784)

Not sure if this is in scope of this task/ diff but would it make sense to update setNewSessionAction so it also contains the keyserverID that we set the session for?

lib/types/keyserver-types.js
20 ↗(On Diff #28784)

This should be t.maybe(t.string)

lib/utils/sanitization.js
308 ↗(On Diff #28784)

Is this spread needed? Will there be more fields in keyserverStore in the future?

This revision now requires changes to proceed.Jul 19 2023, 6:06 AM

Seems like we are using keyserverPrefixID not as a prefix, but just as the server id. It would be more descriptive if it was named something like ashoatKeyserverID. Could make a task/ implement it quickly in a separate diff?

I've put up D8569

lib/reducers/keyserver-reducer.js
15–27 ↗(On Diff #28784)

I think so. This action "resets user state". It is called in onInitialAppLoad when the app was killed and restoring state fails. It results in currentUserInfo being set to null, which triggers LogInHandler rerender and a dispatch of { type: (logOutActionType: 'LOG_OUT') }. And if we log out, we should remove all cookies

The anonymous cookie doesn't make sense in the multikeyserver world, but we have to keep it for now though. To make this code at least somewhat logical I will always change cookies of "other kesyervers" to null, and the cookie to our keyserver to the value of the cookie variable here

28 ↗(On Diff #28784)

I think this will be done in the second iteration of redux refactor, when we will be changing the hardcoded keyserver id to some logic that decides which id to use. I want to do it iteratively.

lib/utils/sanitization.js
308 ↗(On Diff #28784)

There may be. ThreadStore also only has threadInfos but we spread it in persist.js:375, persist.js:385, redux-setup.js:284, and some other places. This shouldn't cause any issues, and helps avoid bugs if we did add a field to keyserverStore.

This revision is now accepted and ready to land.Jul 20 2023, 2:06 AM