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.

Sign up for GitHub

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

Jump to bottom

Fix permissions on share workflows #914

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Fix permissions on share workflows #914

merged 3 commits into from
Dec 13, 2023

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Dec 13, 2023 •
edited
Loading

Feature or Bugfix

  • Bugfix

Detail

There has been a mismatch on the permissions-granting in the shares. There are 2 types of permissions: on the share_object and preview permissions for requesters in the shared_tabled

In this PR:

  • delete share object permissions from RDS when the share object is deleted
  • attach table permissions to requesters for approved tables in approve_share_object
  • delete table permissions to requesters for revoked tables in revoke_items_share_object
  • delete table permissions to requesters for revoked tables in remove_shared_item if the removed item is a table and was in Share_Failed
  • remove the deletion of permissions on the dataset level (which should not have been there in the first place)

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

  • Does this PR introduce or modify any input fields or queries - this includes
    fetching data from storage outside the application (e.g. a database, an S3 bucket)?
    • Is the input sanitized?
    • What precautions are you taking before deserializing the data you consume?
    • Is injection prevented by parametrizing queries?
    • Have you ensured no eval or similar functions are used?
  • Does this PR introduce any functionality or component that requires authorization?
    • How have you ensured it respects the existing AuthN/AuthZ mechanisms?
    • Are you logging failed auth attempts?
  • Are you using or adding any cryptographic features?
    • Do you use a standard proven implementations?
    • Are the used keys controlled by the customer? Where are they stored?
  • Are you introducing any new policies/roles/users?
    • Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@itsmo-amzn
Copy link
Contributor

@dlpzx Have you deployed and tested the changes yet?

itsmo-amzn
itsmo-amzn reviewed Dec 13, 2023
View reviewed changes
backend/dataall/modules/dataset_sharing/db/share_object_repositories.py
@@ -849,17 +849,20 @@ def _find_all_share_item(session, share, status, share_type_model, share_type_ur
)

@staticmethod
def find_all_share_items(session, share_uri, share_type):
return (
def find_all_share_items(session, share_uri, share_type, status=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we pass this status parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To check the share item status, now we need to filter by that field. For example for "delete table permissions to requesters for revoked tables" we need to find all share items that are tables and are in Revoke_Approved

backend/dataall/modules/dataset_sharing/services/share_object_service.py
permissions=DATASET_TABLE_READ,
resource_uri=table.itemUri,
resource_type=DatasetTable.__name__,
if share.groupUri != dataset.SamlAdminGroupName:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we add this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update the description as well. If a table is shared, the requesters will be given permissions to read the table (to preview it). The dataset owner already has those permissions, because it is the owner and those permissions where granted when the table was created. There is a possibility that the requester team is also the dataset owner. In such case those permissions would be duplicated

backend/dataall/modules/dataset_sharing/services/share_item_service.py
group=share.groupUri,
resource_uri=dataset.datasetUri,
)
if share.groupUri != dataset.SamlAdminGroupName:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we add this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@dlpzx
Copy link
Contributor Author

dlpzx commented Dec 13, 2023 •
edited
Loading

I have just deployed to AWS. I have tested:

  • CICD successfully completes
  • existing share requests is deleted without issues

Share request without where Dataset Owner Team != request Team

  • Share-object permissions created in RDS
  • share request without tables completes without issues
  • share request with tables completes without issues
  • table permissions created in RDS for requester
  • revoke items completes without issues
  • table permissions deleted in RDS for requester
  • I got lucky and one table failed share, when it is deleted, the table permissions are also deleted in RDS
  • dataset owner can see dataset and its tables (no missing permissions)
  • Delete share object, Share-object permissions deleted in RDS

Share request without where Dataset Owner Team == request Team (with a consumption role)

  • Share-object permissions created in RDS
  • share request with tables completes without issues
  • dataset owner can see dataset and its tables (no missing permissions)
  • revoke items completes without issues
  • table permissions NOT deleted in RDS for requester
  • dataset owner can see dataset and its tables (no missing permissions)
  • Delete share object, Share-object permissions deleted in RDS

Dataset owner has permissions in both share requests when they are created:

image

Requester has permissions in the first share request:
image

table permissions created in RDS for requester (the resourceUris match the table uris)
image
if we get the sid and permissionUri for the particular permissions for Preview we can check if the permission is created:
image

After the first share is deleted, the dataset owner still has all the PREVIEW permissions on all 3 tables of the dataset. We get the same result after we revoke all permissions in the second share request.
image

NickCorbett
NickCorbett approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@NickCorbett NickCorbett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved following review by @dbalintx and @itsmo-amzn

@dlpzx dlpzx merged commit 823d642 into main Dec 13, 2023
8 checks passed
@dlpzx dlpzx deleted the fix/check-datasetowner-revokes-shares branch December 18, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsmo-amzn itsmo-amzn itsmo-amzn approved these changes

@dbalintx dbalintx dbalintx approved these changes

@NickCorbett NickCorbett NickCorbett approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dlpzx @itsmo-amzn @NickCorbett @dbalintx