Complete shareVault/unshareVault implementation

[joshuakarp]

Specification

The commands Vault share vault unshare and vault permissions needs to act as porcelan commands while using GRPC methods as the plumbing. The Bin command themselves are largely unchanged. To this end we need to create GRPC methods called;

vaultsPermissionGet.ts
vaultsPermissionSet.ts
vaultsPermissionUnset.ts

Set and unset will take a permissions message that provides (NodeId, VaultIdOrName, VaultActions[]). Get will use the same message as a stream but it will be unnecessary to provide the VaultId in that case.

The Permission type needs to be updated to work with the ID changes.

type Permission = {
  gestalt: GestaltActions;
  vaults: Record<VaultIdString, VaultActions>;
};

Additional context

Relevant threads from vaultsrefactoring:

• "Vault Sharing Notes": https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_706100635
• ACL prototype for permission setting from me: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_708465644
• discussion of prototype from me, and how it can be integrated into shareVault: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_708467722
• latest TODO thread: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_708655162

Tasks

• change Permission vaults recvord to use VaultIdString as key.
• Replace grpc methods commandShare and commandUnshare with plumbing methods commandPermissionSet, commandPermissionUnset and commandPermissionGet.
• tests
  • vaultsPermissionsSet test in client/service.
  • vaultsPermissionsUnset test in client/service.
  • vaultsPermissionsGet test in client/service.

[scottmmorris]

This should also include the checking of permissions when pulling and cloning.

Because there was no way to set or unset permissions, I didn't include any checks for if a node has the correct permissions to clone/pull. This will require the get git info or pack method to also accept a node Id to check the permissions of but will be relatively straightforward apart from that. Also it would need to send over whether it was pulling or cloning.

I'm adding this into the tasks for this issue.

[scottmmorris]

While doing the permissions for vaults, I looked into the handling of errors over grpc and isomorphic git. Throwing a permissions error to the stream could be picked up on the client side using an event listener. However, the problem comes when we try and throw the error at the client side. Because the code to handle the error is only used within isomophic git, the thrown error will first be handled by iso git.

However I have found that errors thrown here have always resulted in them being generalise to this Smarthttperror here https://github.com/isomorphic-git/isomorphic-git/blob/76d0d69b9044bb6b38d247ba34c3a9a88b70036c/src/managers/GitRemoteHTTP.js#L154. So for example, if in agentService we threw an error for denying permissions and an error for a vault not existing, they would both result in a smarthttperror which has no properties to differentiate the two potential errors that we have thrown.

Another note is, where is the scan vaults function being implemented? Previously before this big vaults refactoring it was going to be implemented on node manager which still makes sense to me as it doesn't require any vaults stuff. I can implement it on #266 but I think it would be better off in its own PR. (E: It does exist on nodeManager but will need to be changed)

Also should there be a command to get the permissions of a vault on vault Manager? My thoughts are no. If anything this would be in gestalts or ACL so I am removing it from the client service.

[scottmmorris]

For the error handling in isomoprhic git the solution is to have an error flag within the context of cloneVault function (having it as a property of the vault manager would cause concurrency issues). Inside the request object we then catch any errors from the GRPC stream (permissions, vault undefined, etc.) and set the error flag to something which identifies the error that we caught. Then we throw the error inside the request object and let isomorphic git catch it and do any clean up that it needs to do. Here we shouldn't need to destroy the stream manually as we are using the promisified stream call and this should handle any errors and subsequent shutdown automatically. Then eventually we catch the error at the git.clone call and check that it is a SmartHttpError. If it is then we can check our error flag for the descriptive error and then throw the error that we expect.

[CMCDragonkai]

Because this code is a bit tricky, I believe you should have inline comments for when you are doing this so you are telling the dev what those variables are for.

…

On 10/26/21 7:14 PM, Scott wrote: For the error handling in isomoprhic git the solution is to have an error flag within the context of |cloneVault| function (having it as a property of the vault manager would cause concurrency issues). Inside the request object we then catch any errors from the GRPC stream (permissions, vault undefined, etc.) and set the error flag to something which identifies the error that we caught. Then we throw the error inside the request object and let isomorphic git catch it and do any clean up that it needs to do. Here we shouldn't need to destroy the stream manually as we are using the promisified stream call and this should handle any errors and subsequent shutdown automatically. Then eventually we catch the error at the |git.clone| call and check that it is a |SmartHttpError|. If it is then we can check our error flag for the descriptive error and then throw the error that we expect. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#259 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHNXWNZV6F47UWQZWPTUIZPNJANCNFSM5GNCN6UA>.

[joshuakarp]

Another note is, where is the scan vaults function being implemented? Previously before this big vaults refactoring it was going to be implemented on node manager which still makes sense to me as it doesn't require any vaults stuff.

I agree with this. If it's just performing a GRPC request to another agent, then it should follow the same structure of the other commands from NodeConnection (whereby, they expose some wrapper function that internally gets/creates a NodeConnection to the node and performs the required work). See requestChainData for what I'm talking about (ignore the chain verification logic).

Also should there be a command to get the permissions of a vault on vault Manager? My thoughts are no. If anything this would be in gestalts or ACL so I am removing it from the client service.

I agree with this. If you're finding that you're continually having to retrieve the permissions for some vault, then it could be worthwhile to wrap this as a helper function in VaultManager (or even ACL), but from memory, I think it's pretty straightforward to do so directly from the ACL.

[joshuakarp]

Converting this to an epic to place it on the roadmap. Setting start date as Oct 26th (according to first PR comments), and will set end date as Thursday 2nd December. @scottmmorris has said this should be done by end of Tuesday, so that gives time for reviewing on Wednesday 1st.

[CMCDragonkai]

1. I believe vaultsPermissionsCheck that is in agent service is being removed since it's superseded by vaultsScan. Agents don't generally need to know the permissions of a single vault.
2. The vaultsPermissionsGet and vaultsPermissionsSet and vaultsPermissionsUnset should still be available on the vaults API, sharing/unsharing may be the initial set of permissions, but there may be more. I think share/unshare can be higher level API keywords like at the CLI or GUI, but the API is a lower level, which is more flexible, but this needs to be considered in light of the other permission/acl calls too.

[CMCDragonkai]

Right now #266 has removed vaultsPermissionsSet and vaultsPermissionsUnset but they may be making a comeback.

[tegefaulkes]

Spec has been updated.

[tegefaulkes]

As part of the changes here we expect to convert the GRPC methods vaultsShare, vaultsUnshare ect from porcelain commands to plumbing commands vaultsPermissionsGet, vaultsPermissionsSet and vaultsPermissionsUnset`.

VaultsShare does the following steps

          await this.gestaltGraph.setGestaltActionByNode(nodeId, 'scan');
          await this.acl.setVaultAction(vaultId, nodeId, 'pull');
          await this.acl.setVaultAction(vaultId, nodeId, 'clone');
          await this.notificationsManager.sendNotification(nodeId, {
            type: 'VaultShare',
            vaultId: vaultsUtils.encodeVaultId(vaultId),
            vaultName: vaultMeta.vaultName,
            actions: {
              clone: null,
              pull: null,
            },
          });

It seems we can do the same thing within vaultsPermissionsSet but do we want to send the notification within the that plumbing command or in a separate plumbing command? Do we want the scan permission automatically set as well?

For vaultsUnshare we have

        await this.gestaltGraph.unsetGestaltActionByNode(nodeId, 'scan');
        await this.acl.unsetVaultAction(vaultId, nodeId, 'pull');
        await this.acl.unsetVaultAction(vaultId, nodeId, 'clone');

This seems broken. We can have multiple shared vaults so we don't want to remove the scan permission if we unshare one of them. maybe gestaltActionUnset should be a separate plumbing command? If not then we should only remove the scan permission of not more shared vaults are left. Is it valid to not have scan permission but shared vaults? Like do we want to disallow someone from scaning while retaining the vault permissions for later?

Also the locking here is a little jank. This seems like a good place to use the withF context with multiple resources. But I think updating the GestaltGraph and ACL to use WithF instead of the transaction is out of scope for this PR so this may need to be a later Issue. Also locking the vault itself seems unneeded here since we're just modifying permissions.

await withF([this.getWriteLock(vaultId)], async () => {
      await this.gestaltGraph._transaction(async () => {
        await this.acl._transaction(async () => {
          //...
        });
      });
    });

[tegefaulkes]

Based on a previous discussion the logic of the handlers of the plumbing functions and not inside a VaultManager method.

[tegefaulkes]

Saving old spec here for reference

Specification

Sharing and unsharing involves setting and unsetting vault permissions. A vault permission is based on vaultActions.

const vaultActions = ['clone', 'pull'] as const;

A permission is:

type Permission = {
  gestalt: GestaltActions;
  vaults: Record<VaultId, VaultActions>;
};

These Permission POJOs are assigned to PermissionId, which are stored in the ACL domain.

The PermissionId is assigned to a NodeId. Which means every NodeId can have their own PermissionId.

Multiple NodeId may point to the same PermissionId and in fact this is how we expect gestalts to work. For example the below is a sketch of 3 gestalts all composed of NodeIds. All NodeIds in the same gestalt point to the same PermissionId.

NodeId  PermissionId
[
  1       1
  2       1
]
[
  3       2
  4       2
]
[
  5       3
]

Note that vault permissions is identified by the VaultId. Right now the type is:

  vaults: Record<VaultId | string, VaultActions>;

It should be changed to:

  vaults: Record<VaultIdString, VaultActions>;

This is because VaultId objects will need be converted to binary string form before they are stored in the DB. Note that we could use VaultIdEncoded as well if this POJO is likely to be seen by the end user. However there's an advantage to using VaultIdString, and it is that there's only 1 canonical form of VaultIdString, but there may be multiple forms of VaultIdEncoded, and thus it is future proofed.

This means each vault may have their own set of permissions.

We represent each set of permissions as VaultActions:

type VaultActions = Partial<Record<VaultAction, null>>;

This enables us to set and unset just by setting the key in the POJO. Note that the Partial may not be needed here. I'm not sure, this requires prototyping.

On the GRPC service, we need to have client service handlers that allow the setting of vault permissions. These handlers should be created:

src/client/service
  vaultsPermissionGet.ts
  vaultsPermissionSet.ts
  vaultsPermissionUnset.ts

These handlers should accept:

1. NodeId
2. VaultId/VaultName
3. VaultAction

vaultPermissionSet(nodeId, vaultId/vaultName, 'clone')

On the CLI level, we abstract these into porcelain commands:

# note that this will apply to all nodes that share the same gestalt
pk vaults share nodeId vaultId/vaultName # sets both clone and pull permissions
pk vaults unshare nodeId vaultId/vaultName # unsets both clone and pull permissions

When you share a vault, it does not mean you are "pushing" the vault to the other user. It just means you are allowing the other user to "pull" your vault. When you unshare, you are just cancelling this allowance.

One issue that was recently discovered was that there's no way to set permissions on identities. Even though gestalts can be formed by a single identity. This is because of the aclNodesDb which forces us to identify PermissionId by the NodeId. If this were to be generalised, so that permissions can instead be indexed by GestaltKey, this would enable us to generalise our sharing functions, so that you can give a NodeId or an ProviderId:IdentityId to identify a gestalt where you want to share permissions. However this can be a separate issue to solve.

The CLI may also expose plumbing commands in case we want to really control the permissions:

pk acl ... # hasn't been specced out yet
pk vaults permissions # looks up all permissions
pk identities permissions # looks up all permissions

However this has not been specified, so we'll leave that to a separate issue. So pk vaults share/unshare falls in the same level of abstraction as pk identities trust/untrust.

This means we need these files to be created:

src/bin/vaults/
  CommandShare.ts
  CommandUnshare.ts
  CommandPermissions.ts

Which will need to translate to calling

src/client/service
  vaultsPermissionGet.ts
  vaultsPermissionSet.ts
  vaultsPermissionUnset.ts

These handlers may end up calling VaultManager methods. We have some design decisions here to make:

VaultManager.setVaultAction
VaultManager.unsetVaultAction
VaultManager.shareVault
VaultManager.unshareVault

If VaultManager were to have shareVault and unshareVault, that means these are not porcelain concepts, but instead low level plumbing concepts. And the GRPC commands should be updated accordingly. However if setting vault actions and unsetting vault actions/permissions is the the job of VaultManager, then share and unshare are porcelain commands. I'm leaning to having low level concepts in our VaultManager, and leave the "sharing" and "unsharing" to the high level commands. These methods would end up fundamentally calling the ACL and setting those actions/permissions.

So in terms of abstraction hierarchy:

VaultManager methods -> client/service/vaultsHandlers.ts -> bin/vaults/CommandX.ts

If the meat of the logic is in the VaultManager methods, then things should be come higher level to the right.

Alternatively composition can occur in the service handlers, and that could mean that VaultManager can be more "focused" and require less dependencies like ACL.

Make sure that tests ensure sharing/unsharing applies to the whole gestalt and see the comment: #266 (comment)

[tegefaulkes]

After the changes here, the ACL and GestaltGraph can be removed from the VaultManager. handleScanVaults will need to take the acl as a parameter if we want to fully remove it from the VaultManager.

[CMCDragonkai]

Don't you need it to set permissions for sharing vaults?

[tegefaulkes]

Yes, But that is part of the ACL. The logic is not needed inside the VaultManager. It's handled via the service handler logic now.

[tegefaulkes]

Since Polykey needs to be useable as a library we need to keep the share/unshare/get functions inside the VaultManager. But I'm going to make them align better with the plumbing level methods of the GRPC. this just you'll be able to specify the specific permissions you want to add or remove.