Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review the Identities CLI and RPC Handlers #6

Open
emmacasolin opened this issue Feb 15, 2022 · 6 comments
Open

Review the Identities CLI and RPC Handlers #6

emmacasolin opened this issue Feb 15, 2022 · 6 comments
Assignees
Labels
design Requires design (architecture, protocol, specification and task list requires further work) discussion Requires discussion epic Big issue with multiple subissues r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy

Comments

@emmacasolin
Copy link

emmacasolin commented Feb 15, 2022

Specification

Our Identities CLI seems incomplete and needs to be reviewed. In particular:

  • The identitiesToken* rpc handlers have no corresponding CLI commands - something like pk identities token ... should be added
    • We need to consider what kind of access we want the user to have to this data (authenticating a provider already "puts" a token - need to consider the implications of allowing the user to put/get/delete their own tokens)
    • Our 'tokens' are data of the following type:
      type TokenData = {
        accessToken: string;
        refreshToken?: string;
        accessTokenExpiresIn?: number;
        refreshTokenExpiresIn?: number;
      };
  • The identitiesProvidersList rpc handler has no corresponding CLI command - possible command could be pk identities providers
    • As it's currently implemented, this just returns a list of the providers (provider ids) currently registered to the IdentitiesManager (and thus supported by Polykey)
  • The identitiesAuthenticatedGet rpc handler (which returns the user's own authenticated identities) currently uses the command pk identities authenticated - this name is very similar to pk identities authenticate and could get confusing
    • The naming of this command, as well as its relationship with the token manipulation commands (particularly identitiesTokenGet) should be reviewed
  • At the moment, the gestalts and identities commands are both grouped under the pk identities subcommand
    • We should consider separating these, especially as the functionality of our CLI grows and we need to be able to reuse command names (e.g. we may need both a gestalts list and an identities list command)
  • Many of the identities CLI commands receive data from the server in the form of a stream, however this data is outputted as a single array
    • This should be modified such that the data returned from the stream is streamed to stdout as we iterate through it (as opposed to after)
  • The identities trust command does not add a new node into the gestalt graph if it cannot find it in the node graph/via Kademlia

Additional context

Tasks

  1. Review what functionality should be exposed to the CLI and what should not
  2. Create CLI commands for functionality that is not yet exposed to the CLI
  3. Consider separating identities and gestalts commands in the CLI
  4. Ensure that data returned as a stream to the CLI is outputted to STDOUT as a stream
@CMCDragonkai
Copy link
Member

There's a problem with pk identities trust command.

From: MatrixAI/Polykey#326 (comment).

a. No output provided on pk identities trust? However, I understand that we've previously shied away from providing "success" output on every single one of our commands. It can be tricky to diagnose problems when this is the case though.
b. pk identities trust seemingly not adding a node to the gestalt graph when it doesn't already exist in GestaltGraph/NodeGraph - see MatrixAI/Polykey#326 (comment) and https://matrixai.slack.com/archives/CEAUUV5QX/p1645069875382019

  1. The seed node has no nodes in the NodeGraph
  2. The seed node isn't currently adding client nodes to its graph when they connect, this is a problem to be fixed
  3. Even then, the discovery queue may either A. Not be able to resolve the trusted node or B. Not have any node in the graph to help resolve the trusted node - which leads to A. But through a different exception.
  4. When a discovery queue cannot resolve a node, it should be put on the backburner TTL to be revisited later with low priority.

Solution to problem 3 and 4 should be designed along with MatrixAI/Polykey#329 and MatrixAI/Polykey#328.

@CMCDragonkai CMCDragonkai added design Requires design (architecture, protocol, specification and task list requires further work) and removed development Standard development labels Feb 18, 2022
@CMCDragonkai
Copy link
Member

This is a design issue, not yet for development. @emmacasolin should collate the comments here into the above PR description, and we'll schedule this to be tackled after testnet deployment is done.

@CMCDragonkai
Copy link
Member

Quoting MatrixAI/Polykey#493 (comment)

I'm starting to see that having a separate gestalts subcommands would be useful.

pk nodes add
pk nodes find
pk nodes ping
pk nodes getall
pk nodes connections
pk nodes claim         <- claim only node

pk identities authenticate
pk identities authenticated
pk identities search
pk identities claim    <- claim only identity

pk gestalts discover
pk gestalts get
pk gestalts list
pk gestalts claim      <- claim either identity/node
pk gestalts invite     <- invite node to a gestalt
pk gestalts uninvite   <- uninvite node to a gestalt
pk gestalts permissions
pk gestalts allow
pk gestalts disallow
pk gestalts trust      <- short-hand for allow for trust
pk gestalts untrust    <- short-hand for disallow for trust

There are a few ambiguities here.

  1. The claim subcommands can be referring to node or identity or both.
  2. The invite is like pk vaults share and uninvite is like pk vaults unshare. It's combination of setting a permission AND sending a notification.
  3. The trust and untrust is just a short-hand of allow and disallow

We can sort this out in the future issue.

I think the pk gestalts trust and pk gestalts untrust should be removed, it's a bit wishy washy and the generic allow is sufficient for all the kinds of permissions.

@CMCDragonkai
Copy link
Member

Moving to Polykey-CLI.

@CMCDragonkai CMCDragonkai transferred this issue from MatrixAI/Polykey Aug 11, 2023
@CMCDragonkai CMCDragonkai assigned addievo and unassigned tegefaulkes Oct 18, 2023
@CMCDragonkai CMCDragonkai assigned amydevs and unassigned addievo Nov 14, 2023
@CMCDragonkai
Copy link
Member

@amydevs any comments here based on your review in the context of #30 and MatrixAI/Polykey#626

@CMCDragonkai
Copy link
Member

To be part of the new docs too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Requires design (architecture, protocol, specification and task list requires further work) discussion Requires discussion epic Big issue with multiple subissues r&d:polykey:core activity 3 Peer to Peer Federated Hierarchy
Development

No branches or pull requests

5 participants