[SYNPY-1244] Add delete permissions/acl functionality #1200

BryanFauble · 2025-05-06T20:41:44Z

https://sagebionetworks.jira.com/browse/SYNPY-1244

Problem:

When sharing subfolders or specific files with collaborators in private projects, permissions create a complex ACL (Access Control List) structure where:
- Sharing settings can be hidden deep in folder hierarchies
- Files don't automatically inherit the parent container's permissions
- When releasing data, these individual ACLs need to be removed so files properly inherit parent container sharing settings
There was no mechanism available to delete ACLs in the OOP model, nor was there a way to recursively delete the ACL

Solution:

Adds new functions to recursively reset permissions on entities within a container
Implements functionality to determine if an entity inherits ACLs from its parent
Works with nested folder structures to ensure proper permission inheritance/removal

See this in the API Reference docs:

Testing:

Automated tests were added to the codebase to:
- Verify that permission inheritance detection works correctly
- Ensure recursive permission setting functions properly traverse entity hierarchies
- Confirm that entity permissions are properly modified by the new functions
- Test error handling and edge cases

- Refactor existing integration tests in `test_permissions.py` to improve clarity and structure. - Introduce a new class `TestDeletePermissions` to cover various scenarios for deleting permissions across entities. - Implement unit tests for asynchronous permission deletion in `unit_test_permissions_async.py`, ensuring proper handling of errors and entity types. - Create a new unit test file `unit_test_permissions.py` to test synchronous permission deletion methods. - Ensure comprehensive coverage for edge cases, including invalid entity types and recursive deletion behavior.

Copilot

Pull Request Overview

This PR implements new functionality to delete and reset ACLs (permissions) on Synapse entities, including support for recursive deletion and filtering by entity type. Key changes include:

New synchronous and asynchronous methods for deleting entity permissions/ACLs added to the access control protocol and mixin.
Updates to unit and integration tests (both synchronous and asynchronous) to cover deletion behaviors and edge cases.
Enhancements to client API methods (e.g. get_acl) to support additional parameters such as check_benefactor.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/unit/synapseclient/models/unit_test_permissions.py	Added async unit tests for deletion of permissions.
tests/unit/synapseclient/models/async/unit_test_permissions_async.py	Added corresponding async unit tests.
tests/integration/synapseclient/models/synchronous/test_permissions.py	New integration tests for ACL deletion with various options.
tests/integration/synapseclient/models/async/test_permissions_async.py	Async integration tests for ACL deletion.
synapseclient/models/protocols/access_control_protocol.py	Added new delete_permissions method with comprehensive documentation.
synapseclient/models/mixins/access_control.py	Introduced delete_permissions_async and related helper methods for recursive deletion logic.
synapseclient/client.py	Modified ACL retrieval (_getACL) to support check_benefactor parameter.
synapseclient/api/entity_services.py & init.py	Added async delete_entity_acl function and updated API exports.

Comments suppressed due to low confidence (1)

synapseclient/client.py:2798

[nitpick] Although the new 'check_benefactor' parameter is documented, consider adding a concrete example in the docstring to clarify its usage and effect for consumers of the API.

def _getACL(self, entity: Union[Entity, str], check_benefactor: bool = True) -> Dict[str, Union[str, list]]:

synapseclient/models/mixins/access_control.py

BryanFauble · 2025-05-06T20:43:39Z

synapseclient/client.py

+            if check_benefactor:
+                # Get the ACL from the benefactor (which may be the entity itself)
+                benefactor = self._getBenefactor(entity)
+                trace.get_current_span().set_attributes(
+                    {"synapse.id": benefactor["id"]}
+                )
+                uri = "/entity/%s/acl" % (benefactor["id"])
+                return self.restGET(uri)
+            else:
+                synid, _ = utils.get_synid_and_version(entity)
+                trace.get_current_span().set_attributes({"synapse.id": synid})
+                uri = "/entity/%s/acl" % (synid)
+                try:
+                    return self.restGET(uri)
+                except SynapseHTTPError as e:
+                    if (
+                        "The requested ACL does not exist. This entity inherits its permissions from:"
+                        in str(e)
+                    ):
+                        # If the entity does not have an ACL, return an empty ACL
+                        return {"resourceAccess": []}
+                    raise e


I needed to add this set of changes into the client.py because the access_control.py mixin internally still uses it. The purpose of this changes is to allow us to get the ACL for a specific entity without the code automatically getting the benefactor. This is crucial during testing to verify that a given is or is not overwriting the folders ACL that it's contained within.

This is a nice addition to grab the ACL of an entity - it's confusing because technically the ACL is inherited on child entities from a benefactor but it's nice to not have it return [] when there is clearly an ACL

… new projects

thomasyu888 · 2025-05-07T03:47:26Z

synapseclient/models/protocols/access_control_protocol.py

+            syn = Synapse()
+            syn.login()
+
+            File(id="syn123").delete_permissions()


All entities across synapse like Tables,Views, and etc can have permissions deleted as well. Does this function only work for files and folders? (we can add it in later if it's in a future PR once people ask for it.

This function will work for all entity types. What the specific logic in the function is for is dealing with traversing the tree of files and folders within a container (Project/Folder). Users will still be able to delete the permissions for other entities like Tables, Views, ect..

See this script and console log output for an example:
https://gist.github.com/BryanFauble/0b8c0ab8644751241dc7a31e8e779158

thomasyu888

🔥 LGTM! I'll defer to someone on the team for final review, but thanks for this work!

BWMac

🚀

BryanFauble requested a review from a team as a code owner May 6, 2025 20:41

BryanFauble requested review from Copilot and removed request for a team May 6, 2025 20:41

Copilot AI reviewed May 6, 2025

View reviewed changes

synapseclient/models/mixins/access_control.py Show resolved Hide resolved

synapseclient/models/mixins/access_control.py Show resolved Hide resolved

BryanFauble commented May 6, 2025

View reviewed changes

Enhance project ACL handling and add tests for permission deletion on…

9472ac5

… new projects

BryanFauble requested review from a team and allaway May 6, 2025 21:03

Add to API reference docs

e5a86d3

thomasyu888 reviewed May 7, 2025

View reviewed changes

thomasyu888 requested a review from a team May 7, 2025 04:00

BryanFauble added 2 commits May 8, 2025 09:31

Patching for failed tests

3558bb4

Patch sync test

d8d148f

thomasyu888 reviewed May 9, 2025

View reviewed changes

thomasyu888 requested a review from a team May 9, 2025 08:25

BWMac approved these changes May 9, 2025

View reviewed changes

BryanFauble merged commit f9f0c63 into develop May 9, 2025
28 checks passed

BryanFauble deleted the synpy-1244-delete-acl-ability branch May 9, 2025 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYNPY-1244] Add delete permissions/acl functionality #1200

[SYNPY-1244] Add delete permissions/acl functionality #1200

Uh oh!

BryanFauble commented May 6, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

BryanFauble May 6, 2025

Uh oh!

thomasyu888 May 7, 2025

Uh oh!

thomasyu888 May 7, 2025 •

edited

Loading

Uh oh!

BryanFauble May 8, 2025

Uh oh!

thomasyu888 left a comment

Uh oh!

BWMac left a comment

Uh oh!

Uh oh!

Uh oh!

[SYNPY-1244] Add delete permissions/acl functionality #1200

[SYNPY-1244] Add delete permissions/acl functionality #1200

Uh oh!

Conversation

BryanFauble commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem:

Solution:

Testing:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

BryanFauble May 6, 2025

Choose a reason for hiding this comment

Uh oh!

thomasyu888 May 7, 2025

Choose a reason for hiding this comment

Uh oh!

thomasyu888 May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanFauble May 8, 2025

Choose a reason for hiding this comment

Uh oh!

thomasyu888 left a comment

Choose a reason for hiding this comment

Uh oh!

BWMac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BryanFauble commented May 6, 2025 •

edited

Loading

thomasyu888 May 7, 2025 •

edited

Loading