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

[SYNPY-1302] add method to implement get entity permission api #1026

Conversation

danlu1
Copy link
Contributor

@danlu1 danlu1 commented Dec 15, 2023

Problem:
The original getPermission method uses /acl endpoint only that shows inconsistent behavior in the case of publicly downloadable content from Synapse.

Solution:
create a get_permissions method that implements GET /entity/{id}/permissions API. These API factors in the entity's ACL and the user's group membership when determining the caller's permission to the entity.

Example:
Screenshot 2023-12-15 at 9 31 28 AM

Testing:
Unit tests and integration tests are included in the PR.

@danlu1 danlu1 requested a review from a team as a code owner December 15, 2023 17:28
@pep8speaks
Copy link

Hello @danlu1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 5283:89: E501 line too long (95 > 88 characters)
Line 5285:89: E501 line too long (119 > 88 characters)
Line 5286:89: E501 line too long (141 > 88 characters)
Line 5287:89: E501 line too long (106 > 88 characters)
Line 5288:89: E501 line too long (93 > 88 characters)
Line 5291:89: E501 line too long (115 > 88 characters)
Line 5292:89: E501 line too long (148 > 88 characters)
Line 5293:89: E501 line too long (150 > 88 characters)
Line 5294:89: E501 line too long (120 > 88 characters)
Line 5296:89: E501 line too long (133 > 88 characters)
Line 5297:89: E501 line too long (128 > 88 characters)
Line 5298:89: E501 line too long (171 > 88 characters)
Line 5312:89: E501 line too long (103 > 88 characters)
Line 5322:89: E501 line too long (93 > 88 characters)
Line 5324:89: E501 line too long (128 > 88 characters)
Line 5326:89: E501 line too long (118 > 88 characters)
Line 5328:89: E501 line too long (92 > 88 characters)
Line 5332:89: E501 line too long (111 > 88 characters)
Line 5334:89: E501 line too long (93 > 88 characters)
Line 5336:89: E501 line too long (142 > 88 characters)
Line 5365:89: E501 line too long (96 > 88 characters)
Line 5381:89: E501 line too long (118 > 88 characters)
Line 5382:89: E501 line too long (125 > 88 characters)

Line 882:89: E501 line too long (90 > 88 characters)
Line 911:89: E501 line too long (114 > 88 characters)
Line 934:89: E501 line too long (174 > 88 characters)
Line 940:89: E501 line too long (112 > 88 characters)
Line 959:89: E501 line too long (95 > 88 characters)
Line 963:89: E501 line too long (123 > 88 characters)
Line 967:89: E501 line too long (103 > 88 characters)
Line 1005:89: E501 line too long (122 > 88 characters)
Line 1024:89: E501 line too long (99 > 88 characters)
Line 1033:89: E501 line too long (99 > 88 characters)
Line 1037:89: E501 line too long (123 > 88 characters)
Line 1042:89: E501 line too long (105 > 88 characters)
Line 1057:89: E501 line too long (92 > 88 characters)
Line 1088:89: E501 line too long (126 > 88 characters)
Line 1151:89: E501 line too long (107 > 88 characters)

Line 75:50: F541 f-string is missing placeholders
Line 83:50: F541 f-string is missing placeholders
Line 91:50: F541 f-string is missing placeholders

@danlu1 danlu1 changed the title add method to implement get entity permission api [SYNPY-1302] add method to implement get entity permission api Dec 15, 2023
is_entity_open_data: bool = None
"""(Read Only) Returns true if the Entity's DateType equals 'OPEN_DATA', indicating that the data is safe to be released to the public."""

def __init__(self, data: dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is how we should create an instance of this class. The reason is that if you are overriding the init dunder method than it prevent you from creating a Permissions instance like:

my_other_permissions = Permissions(can_view=True)
print(my_other_permissions)

This might make more sense:

    @classmethod
    def from_dict(cls, data: dict) -> "Permissions":
        """
            Convert a data dictory to an instance of this dataclass.
        """
        return cls(
            can_view=data["canView"],
            can_edit=data["canEdit"],
            can_move=data["canMove"],
            can_add_child=data["canAddChild"],
            can_certified_user_edit=data["canCertifiedUserEdit"],
            can_certified_user_add_child=data["canCertifiedUserAddChild"],
            is_certified_user=data["isCertifiedUser"],
            can_change_permissions=data["canChangePermissions"],
            can_change_settings=data["canChangeSettings"],
            can_delete=data["canDelete"],
            can_download=data["canDownload"],
            can_upload=data["canUpload"],
            can_enable_inheritance=data["canEnableInheritance"],
            owner_principal_id=data["ownerPrincipalId"],
            can_public_read=data["canPublicRead"],
            can_moderate=data["canModerate"],
            is_certification_required=data["isCertificationRequired"],
            is_entity_open_data=data["isEntityOpenData"],
        )

In the get_permissions function:
return Permissions.from_dict(data)

Thoughts?

Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

24 New issues
0 Security Hotspots
No data about Coverage
27.8% Duplication on New Code

See analysis details on SonarCloud

@BryanFauble
Copy link
Contributor

@danlu1 Something for when you are back in the office after the holidays.

I had missed this originally, but since this is running on the fork of the main Sage-Bionetworks repo you are missing a few key secrets for 1) Integration tests to run and 2) The newly added logic for SonarCloud scans to run in the CI pipeline.

Please raise a PR from within the main repo instead of a fork. I'll be working to update our documentation to mention these changes. This does not change external collaborators who are still required to work from a fork.

@thomasyu888
Copy link
Member

What do you all think about completing these changes here first due to all the comments that have been made?

Then create a feature branch in SYNPY to merge this branch into which can then be merged into develop.

@BryanFauble
Copy link
Contributor

What do you all think about completing these changes here first due to all the comments that have been made?
Then create a feature branch in SYNPY to merge this branch into which can then be merged into develop.

Great plan! Let's keep the changes and discussion here and move it over when it's ready so we can see all the pre-merge checks.

@danlu1 danlu1 marked this pull request as draft December 26, 2023 22:54
@thomasyu888 thomasyu888 changed the base branch from develop to SYNPY-1302-add_get_permissions December 27, 2023 21:26
@danlu1 danlu1 closed this Dec 27, 2023
@danlu1
Copy link
Contributor Author

danlu1 commented Dec 27, 2023

Closed in favor of new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants