Page MenuHomePhabricator

[keyserver] Replace ashoatKeyserverID with thisKeyserverID()
ClosedPublic

Authored by inka on Feb 13 2024, 2:22 AM.
Tags
None
Referenced Files
F3348723: D11050.id37389.diff
Fri, Nov 22, 3:50 PM
F3348702: D11050.id37070.diff
Fri, Nov 22, 3:46 PM
F3348252: D11050.id37506.diff
Fri, Nov 22, 2:14 PM
F3346758: D11050.diff
Fri, Nov 22, 9:45 AM
Unknown Object (File)
Wed, Nov 13, 10:49 PM
Unknown Object (File)
Tue, Nov 12, 10:31 PM
Unknown Object (File)
Thu, Nov 7, 1:52 AM
Unknown Object (File)
Wed, Nov 6, 1:13 PM
Subscribers

Details

Summary

issue: ENG-6633
Replacing ashoatKeyserverID with calls to fetchIdentityInfo - because this is async I had to make a couple of functions that need it be async as well.

Test Plan

Tested that state sync still works by logging infosHash in checkState and compoaring with values returned by the previous code, on a client in with the same state.
Tested that the id is fetched correctly in getInitialReduxStateResponder and validateOutput (by checking that ids are prefixed correctly)

Diff Detail

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

Event Timeline

inka requested review of this revision.Feb 13 2024, 2:37 AM

Go back to initial approach.

Some devs already have identity info in their dbs, this code will result in the id from the db being used instead of ashoatKeyserverID. This is not good, because the clients have ids prefixed with 256, so input validation will fail, and data sent by the server would be prefixed incorrectly, until the clients configure theis authoritativeKeyserverID. I need to handle this case

Check if the keyserver can use its id when communicating with clients

I decided to use a config file and fetch it with getCommConfig. If it is not present, then we use 256. otherwise, if present and contains "usingIdentityCredentials:: true, then we use value returned by identity.

This way, my script that:

  • sets keyserver/secrets/user_credentials.json
  • logs in with identity using data from keyserver/secrets/user_credentials.json, and obtains user id
  • sets this user id as authoritativeKeyserverID for clients

Can also create this config, allowing the keyserver to use its id when communicating with clients

In here @ashoat suggested putting this flag in user_credentials.json

In the keyserver we:

  • need a file with admins id (ENG-6614)
  • need a file with admins username and password - user_credentials.json
  • wanted to add a flag to user_credentials.json that would tell the keyserver if it should use its id, or 256

I think we can merge these and add id to user_credentials.json, and if present, use it.

The username, password and id will be written to user_credentials.json by my script after registering / logging in the user. In the intermediate solution, the dev will have to type the id in manually along with the username and password.

keyserver/src/user/login.js
111–113 ↗(On Diff #37070)

If we were able to log in to Identity, and the returned ID is inconsistent, we probably won't be able to successfully run the app - there should be some issues. So I guess it's better to kill the keyserver and inform the dev that a config should be fixed.

We decided to have one source of truth and just fetch the data from the db.

tomek requested changes to this revision.Feb 19 2024, 3:49 AM
tomek added inline comments.
keyserver/src/shared/state-sync/current-user-state-sync-spec.js
25 ↗(On Diff #37319)

We don't need this await - if a promise is returned from an async function its result gets automatically unwrapped.

keyserver/src/shared/state-sync/threads-state-sync-spec.js
45 ↗(On Diff #37319)

getServerInfoHash calls fetchIdentityInfo which means that for each thread we're fetching a value from the DB. Can we avoid this?

Can we modify getServerInfoHash function to accept a keyserverID?
Or, even better, maybe we can cache thisKeyserverID value. If this value is set only when a keyserver starts, it might be possible.

This revision now requires changes to proceed.Feb 19 2024, 3:49 AM
keyserver/src/shared/state-sync/current-user-state-sync-spec.js
25 ↗(On Diff #37319)

Is there an upside to skipping it?
I think the code is more readable if the awaits are explicit

Address review - I forced a state sync and checked that a cached value is used

tomek added inline comments.
keyserver/src/shared/state-sync/current-user-state-sync-spec.js
25 ↗(On Diff #37319)

I guess there might be an insignificant performance difference. But up to you - I don't think it matters too much.

This revision is now accepted and ready to land.Feb 21 2024, 4:23 AM