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.