Page MenuHomePhabricator

[keyserver][lib][native] Refactor getVersion action
ClosedPublic

Authored by inka on Oct 30 2023, 11:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 2:04 AM
Unknown Object (File)
Sat, Nov 23, 8:51 AM
Unknown Object (File)
Thu, Nov 21, 3:55 PM
Unknown Object (File)
Wed, Nov 20, 1:25 PM
Unknown Object (File)
Oct 27 2024, 9:48 PM
Unknown Object (File)
Oct 27 2024, 9:48 PM
Unknown Object (File)
Oct 27 2024, 9:48 PM
Unknown Object (File)
Oct 27 2024, 9:48 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-5282/refactor-getversion-action
We need to get from the keyserver the username and id of the owner.
getVersion action is called before we have ever talked to the keyserver - we don't know its id, we don't have a cookie or session set, we don't have an entry for it in the keyserver store. To use the useKeyserverCall we create a temporary entry. We could probably do this better, but I'm low on time and I don't think it's worth doing now. This solution won't make it harder to upgrade later.
This mirors how it was done before in useServerCall, but there we were also sending the cookie and any other values that were already in our store, nevermind it was for a different keyserver potentially. This was actually a security breach

Test Plan

Passed parramOverride to useKeyserverCall:

{
      keyserverInfos: {
        [TEMPORARY_KS_ID]: {
          urlPrefix: 'http://localhost:3000/comm',
        },
      },
}

and called the action. Checked that the correct values are returned from the keyserver.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Oct 30 2023, 11:37 AM
keyserver/src/responders/version-responders.js
21–26 ↗(On Diff #32507)

Why are these two Promises being run sequentially? Does one depend on the other?

I forgot to create hooks, I'll fix that

Address review, add hook

michal added inline comments.
lib/actions/device-actions.js
71 ↗(On Diff #32642)

I think not shortening keyserver to KS would be clearer but up to you

This revision is now accepted and ready to land.Nov 3 2023, 7:45 AM
lib/actions/device-actions.js
76–78 ↗(On Diff #32642)

Does that mean that we can try to connect to only one keyserver at a time?

lib/actions/device-actions.js
76–78 ↗(On Diff #32642)

According to the designs and the discussion in them, we can be only adding one keyserver at a time.

inka planned changes to this revision.Nov 3 2023, 9:30 AM
inka added inline comments.
lib/actions/device-actions.js
76–78 ↗(On Diff #32642)

After discussion with @tomek we came to the conclusion that we should use the url of the keyserver as the key, to avoid any potential problems that could be caused by having only one temporary entry

Key the paramOverride keyserverInfos by urlPrefix

This revision is now accepted and ready to land.Nov 3 2023, 10:05 AM
inka planned changes to this revision.Nov 3 2023, 10:11 AM

Actually I will just make it possible to call many keyservers with this action

Make possible to call multiple keyservers

This revision is now accepted and ready to land.Nov 3 2023, 10:22 AM
lib/actions/device-actions.js
81 ↗(On Diff #32681)

Heads-up to @ginsu, who also has a diff up on this code (if I recall correctly)