Removing share item (folder) does not work

[degoldner]

Describe the bug

After having successfully shared folders cross account the removal of certain folders from the share object is not working as expected. The permission is not revoked immediately and also after subsequent submission the permission does not get revoked correctly, though it is not shown in the data.all UI anymore.

How to Reproduce

WHEN

1. Share a dataset with two folders with another environment (cross account)
2. Approve share
3. Owner of dataset removes one of the folders, share object turns into DRAFT mode
4. Requester submits share request again with only the one remaining folder
5. Owner approves share request again

THEN
After owner removes folder from share object the permission is NOT revoked and the owner has no possibility to revoke the whole share object any more.

After approving share object with only one remaining folder the revoking of removed of folders fails and thus no permission to the removed folder is revoked. The requester still has access to the folder.

We identified that the issue resides in the removal of folders in the S3ShareApproval.py class.
When removing a share item from a share object it is deleted from the share object in the database.
Later during approval of the share object we look for the removed folders as a share item and get a None exception:

Sharing task failed due to: 'NoneType' object has no attribute 'status'

With the following code being the root cause:

Expected behavior

When the owner of a dataset edits items in the share object he/she should trigger an immediate action which is reflected also in the AWS permission layer or get better feedback about the status of the share item. Removing a share item currently is not equal to actually revoking permissions.

Your project

No response

Screenshots

No response

OS

AWS deployment

Python version

3.7

AWS data.all version

v1.2.0

Additional context

No response

[dlpzx]

Hi @degoldner I just tested it and you are right, the sharing stays. I did not get the error Sharing task failed due to: 'NoneType' object has no attribute 'status' but I was able to list the objects in a folder previously shared that I removed from the share request.

[degoldner]

@dlpzx , the error happens only when the share request is submitted and approved again after a successfully shared folder has been removed. Then only the share manager task will try to revoke the access for the removed folder as part of the anew approval for the share object.

[degoldner]

That said I think our issue here is not how we handle the removal of folders for a share request which has been approved anew but rather we have to think how we handle the removal of share items in the first place and the state associated to them.

I could imagine to only directly remove/delete share items in certain states (DRAFT e.g.) and introduce new logic for revoking permissions on item level for share items which are in status Share_Succeeded.

[dlpzx]

Hi @degoldner, I agree, I have revisited the sharing state machines a few times as well. For a while, I thought about introducing a DRAFT_REMOVE status for items that are being removed to have visibility on the removal process. Is this to what you are referring to?

I will post here any ideas that come up to improve the design and avoid this kind of errors

[degoldner]

@dlpzx, our team will also revisit the current state machine for the share request and its share items and then provide a proposal.

[dlpzx]

Hi @degoldner! Here is a draft from ideas that we have considered. There are 2 options, option 2 is a little simplified but it is mostly the same. The top diagram is the share items state machine and the bottom one the share request state machine. Submission affects only the request and there is a better handling of states in which under-the-hood there is a share (all that are colored in blue)

[dlpzx]

I have modified it a bit adding the revokeALL:

[degoldner]

I like the more detailed view on the share items state machine! One remark here, I think we should differentiate between removing and revoking a share item. The removing of a share item being the deletion from the share request and the revoking the technical permission removal on the share item.

Another point I would like to mention is that the state machine for the share request should be extended as well.
Especially when the requester adds new items to a share request which has already approved items the status DRAFT is not correct here, maybe we could introduce something like EDIT instead?

[dlpzx]

Hi @degoldner, I have been working on this today. I think you are right on differentiating between the two kinds of actions (removeItem vs revokeItem). I have adjusted the SM accordingly. Thank you :)

As for the Share Request being in Edit, I see why you are proposing it, but I think with the above design without having items in draft, you always know which items are being requested and what their actual state is.

I am working in this branch https://github.com/awslabs/aws-dataall/tree/review-sharing-workflow-folders

[degoldner]

Hi @dlpzx, thank you very much for picking this issue up so fast! I agree that inside the share object the item status is clear now. The intention of introducing the EDIT status for the share request was to clearly differentiate between a share request which has already approved items and one which does not. Imagine a user wants to check share requests with approved share items and sees the request in status DRAFT, that might be confusing.

Regarding the implementation for the SM of the share items we might want to discuss whether an approval for a share item revoke action is required or it happens automatically? The status of the share request after an item has been revoked could be DRAFT or EDIT, and bringing another point in, what would happen if the share revoke fails, this would not be reflected on the current SM of the share request.

Also I think when we revoke an item we should do this not as part of the approval of the share request but immediately calling the revoke handler but for a single items only.

What I do not understand is the NO_ITEM status of a share item, where would that state be saved if the share item is deleted or non existing?

[dlpzx]

Hi @degoldner, After some iterations, here is a more advanced design:

• I have tried to clarify the status of the Share Object by renaming the pending to SUBMITTED and adding the COMPLETED state.
• For the case of failure of items in the State machine, it is true that it does not reflect on the SM of the share object, I was thinking of introducing this information. Something like...
  
• I renamed the NO_ITEM to DELETED, you were totally right, it was confusing. It is just a pre-state to delete the item

Last, the most important part: do we handle revoke and granting together or separated? As advantage, I see the point of revoking items without the need to going through submission-approval process and secondly we simplify the SM by removing the PENDINGREVOKE state. I think those are the main advantages (please add any point that I am missing)
As drawbacks, my concern is the batch nature of the ECS task, if we remove one by one we need to start new task instances. I think it would be great if we re-architect the task and send that one to the Worker Lambda, however, then we will also face difficulties for the cleanup of resources. To alleviate this issue I have added the possibility to REVOKE_ALL for both requesters and approvers. This way they can skip the submit-approve process. From a user point of view, users revoke one single item if they made a mistake and actually want to request access to another one, that is why I did not think that it was a problem to make them part of the same approval process.

I am going to take some time to reflect on this and discuss internally. Thank you for your feedback, it is gold :)