Page MenuHomePhabricator
Differential D11141

[IGNORE] Client hacks for testing multiple keyservers
AbandonedPublic
Actions

Authored by inka on Feb 22 2024, 4:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 25, 7:48 AM
Unknown Object (File)
Thu, Apr 11, 12:15 AM
Unknown Object (File)
Wed, Apr 10, 5:18 PM
Unknown Object (File)
Tue, Apr 9, 9:13 PM
Unknown Object (File)
Tue, Apr 9, 2:53 PM
Unknown Object (File)
Tue, Apr 9, 9:38 AM
Unknown Object (File)
Mon, Apr 8, 9:03 PM
Unknown Object (File)
Mon, Apr 8, 8:05 AM
View All 35 Files
Subscribers
ashoat
tomek

Details

Reviewers
None
Summary

issue: ENG-6576
This diff contains hacks that should allow adding a second keyserver form a client

Test Plan

This is hard to test because I'm having trouble loging in using the identity flow, but I did thest this in the following way:

  1. I was logged in with the old flow
  2. I checked that in the keyservers screen, the authoritative keyserver does show up
  3. I tried adding a second keyserver (which I had running in Docker) - checked that it showed up on the list of keyservers as well. I didn't get logged in to, because I didn't have the CSAT

For the identity changes:

  1. I was logged out
  2. I typed in crenetials of a user that was registered with identity
  3. I checked that CSAT is being saved

Diff Detail

Repository
rCOMM Comm
Branch
inka/testing_multikeyservers
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka created this revision.Feb 22 2024, 4:06 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptFeb 22 2024, 4:06 AM
Harbormaster completed remote builds in B27056: Diff 37434.Feb 22 2024, 4:21 AM
inka requested review of this revision.Feb 22 2024, 4:21 AM
inka updated this revision to Diff 37442.Feb 22 2024, 6:53 AM

Work around keys being in different shapes on ks and clients

Harbormaster completed remote builds in B27064: Diff 37442.Feb 22 2024, 7:09 AM
inka added inline comments.Feb 22 2024, 7:49 AM
lib/selectors/keyserver-selectors.js
129–130 ↗(On Diff #37442)

We might not have the keyservers admin in userStore

137 ↗(On Diff #37442)

Admin id is the same as keyserver id

lib/utils/services-utils.js
7 ↗(On Diff #37442)

This flag lets us actually use the identity code

native/account/log-in-panel.react.js
335–340 ↗(On Diff #37442)

We need to save the access token to commCoreModule, so that comm services auth metadata emitter can emit it back to js code that listens for it

Handling this correctly is tracked in ENG-6606

web/account/account-hooks.js
337–340 ↗(On Diff #37442)

Clients and keyservers use a different format of the prey key. This is a workaround for now. Fixing this is for now tracked in ENG-6902

web/redux/action-types.js
48–59 ↗(On Diff #37442)

This is not needed if we are actually connecting to the keyservers we add to our store

inka added a parent revision: D11146: [lib] Update getDefaultAvatar and threadMembersWithoutAddedAshoat.Feb 22 2024, 8:50 AM
inka mentioned this in D11238: [lib] Set CSAT on identity login / register.Mar 4 2024, 9:06 AM
inka mentioned this in rCOMM220e66c5898b: [lib] Set CSAT on identity login / register.Mar 7 2024, 12:43 AM
inka mentioned this in D11234: Implement notifications sessions storage on top of MMKV. Migrate existing notifs account to SQLite.Mar 7 2024, 8:24 AM
inka updated this revision to Diff 37946.Mar 8 2024, 2:57 AM

Rebase

Harbormaster completed remote builds in B27402: Diff 37946.Mar 8 2024, 3:13 AM
inka mentioned this in D11283: [lib][web][native] Use real admin username or id when adding a ks.Mar 8 2024, 3:54 AM
inka updated this revision to Diff 37951.Mar 8 2024, 7:09 AM

Use the real user id on native as well

Harbormaster completed remote builds in B27405: Diff 37951.Mar 8 2024, 7:25 AM
inka updated this revision to Diff 37995.Mar 11 2024, 8:29 AM

Rebase over D11283

inka mentioned this in rCOMMa4f77daafee9: [lib][web][native] Use real admin username or id when adding a ks.Mar 11 2024, 8:32 AM
Harbormaster completed remote builds in B27447: Diff 37995.Mar 11 2024, 8:45 AM
inka updated this revision to Diff 38093.Mar 15 2024, 3:00 AM

Rebase

Harbormaster completed remote builds in B27525: Diff 38093.Mar 15 2024, 3:16 AM
ashoat added inline comments.Mar 19 2024, 9:08 AM
web/account/account-hooks.js
9 ↗(On Diff #38093)

Now that D11319 has landed, I don't think the changes to this file are necessary anymore

inka updated this revision to Diff 38184.Mar 19 2024, 9:11 AM

Rebase

Harbormaster completed remote builds in B27594: Diff 38184.Mar 19 2024, 9:26 AM
ashoat added a comment.Mar 19 2024, 5:53 PM

That was fast!!

inka updated this revision to Diff 38438.Mar 28 2024, 6:21 AM

Rebase

Harbormaster completed remote builds in B27759: Diff 38438.Mar 28 2024, 6:37 AM
inka updated this revision to Diff 38698.Wed, Apr 3, 1:21 AM

Rebase

Harbormaster completed remote builds in B27926: Diff 38698.Wed, Apr 3, 1:36 AM
inka updated this revision to Diff 38981.Wed, Apr 10, 4:07 AM

Rebase

Harbormaster completed remote builds in B28124: Diff 38981.Wed, Apr 10, 4:23 AM
ashoat added a comment.Wed, Apr 10, 8:37 AM

Love to see more hacks getting removed here!

lib/reducers/services-access-token-reducer.js
7 ↗(On Diff #38981)

Nit: this change can be removed

ashoat added inline comments.Wed, Apr 10, 11:48 AM
native/account/log-in-panel.react.js
329–334 ↗(On Diff #38981)

Same as below; should be covered by IdentityServiceContextProvider.logInPasswordUser

native/account/registration/registration-server-call.js
101–106 ↗(On Diff #38981)

Wondering – why is this necessary? It seems like there is a setCommServicesAuthMetadata call inside IdentityServiceContextProvider.registerPasswordUser, which was introduced in D11021

It looks like that change was landed shortly after this diff was first introduced, so I suspect you had it here initially because it was necessary, but it likely is not necessary anymore

ashoat added a comment.Mon, Apr 15, 6:14 AM

Ping for the above comments!

inka added a comment.Thu, Apr 18, 4:41 AM

I tested this and I was indeed able to login and register without these lines!

inka updated this revision to Diff 39214.Thu, Apr 18, 4:42 AM

Remove unnecessary lines

ashoat added a comment.Thu, Apr 18, 4:51 AM

Exciting that there's only one hack left here!

Harbormaster completed remote builds in B28290: Diff 39214.Thu, Apr 18, 4:58 AM
inka mentioned this in D11701: [web] Remove authoritativeKeyserverID check from getInitialReduxState.Fri, Apr 19, 8:53 AM
inka mentioned this in rCOMMfaa0659b1a25: [web] Remove authoritativeKeyserverID check from getInitialReduxState.Mon, Apr 22, 4:35 AM
ashoat added inline comments.Mon, Apr 22, 4:46 AM
web/redux/action-types.js
48–59 ↗(On Diff #39214)

Now that you've removed this last hack in D11701, should this diff be abandoned, and the Notion doc updated to just direct devs to set usingCommServicesAccessToken to true?

inka updated this revision to Diff 39332.Mon, Apr 22, 4:50 AM

Rebase

Harbormaster completed remote builds in B28349: Diff 39332.Mon, Apr 22, 5:05 AM
ashoat added inline comments.Thu, Apr 25, 8:11 AM
web/redux/action-types.js
48–59 ↗(On Diff #39214)

Ping on this!

inka abandoned this revision.Thu, Apr 25, 8:32 AM
inka added inline comments.
web/redux/action-types.js
48–59 ↗(On Diff #39214)

Yes, I think we can do that!

Revision Contents
Changeset List

PathSize
lib/
utils/
2 lines

Diff 39332

View Options

lib/utils/services-utils.js

Loading...