Merge share and unshare in the same request

Instead of having two different requests for adding or removing users from a shared resource now a simple list of users is sent in the request.
The resource will be shared with new users and unshared with users that are no more on that list.

lib/galaxy/managers/histories.py

@@ -306,10 +306,10 @@ class HistoryManager(sharable.SharableModelManager, deletable.PurgableManagerMix
         total_dataset_count = len(datasets)
         for user in users:
             if self.is_history_shared_with(history, user):
-                errors.add(f"History ({history.name}) already shared with user ({user.email})")
                 continue
 
             user_roles = user.all_roles()
+            # TODO: Handle this is a more performant way
             # Only deal with datasets that have not been purged
             for hda in datasets:
                 if trans.app.security_agent.can_access_dataset(user_roles, hda.dataset):

lib/galaxy/managers/sharable.py

@@ -50,6 +50,8 @@ from galaxy.util import ready_name_for_url
 
 log = logging.getLogger(__name__)
 
+UserIdentifier = Union[EncodedDatabaseIdField, str]
+
 
 class SharingOptions(str, Enum):
     """Options for sharing resources that may have restricted access to all or part of their contents."""
@@ -70,11 +72,12 @@ class ShareWithExtra(BaseModel):
 
 
 class ShareWithPayload(BaseModel):
-    user_ids: List[Union[EncodedDatabaseIdField, str]] = Field(
+    user_ids: List[UserIdentifier] = Field(
         ...,
         title="User Identifiers",
         description=(
-            "A collection of encoded IDs (or email addresses) of users."
+            "A collection of encoded IDs (or email addresses) of users "
+            "that this resource will be shared with."
         ),
     )
     share_option: Optional[SharingOptions] = Field(
@@ -368,6 +371,24 @@ class SharableModelManager(base.ModelManager, secured.OwnableManagerMixin, secur
         to provide the extra information, otherwise, it will be None by default."""
         return None
 
+    def update_current_shares(self, item, new_users_shared_with: Set[User], flush=True):
+        """Updates the currently list of users this item is shared with by adding new
+        users and removing missing ones."""
+        current_shares = self.get_share_assocs(item)
+        currently_shared_with = {share.user for share in current_shares}
+
+        needs_adding = new_users_shared_with - currently_shared_with
+        for user in needs_adding:
+            current_shares.append(self.share_with(item, user, flush=False))
+
+        needs_removing = currently_shared_with - new_users_shared_with
+        for user in needs_removing:
+            current_shares.remove(self.unshare_with(item, user, flush=False))
+
+        if flush:
+            self.session().flush()
+        return current_shares
+
     # .... slugs
     # slugs are human readable strings often used to link to sharable resources (replacing ids)
     # TODO: as validator, deserializer, etc. (maybe another object entirely?)
@@ -577,18 +598,7 @@ class SharableModelDeserializer(base.ModelDeserializer,
         """
         unencoded_ids = [self.app.security.decode_id(id_) for id_ in val]
         new_users_shared_with = set(self.manager.user_manager.by_ids(unencoded_ids))
-        current_shares = self.manager.get_share_assocs(item)
-        currently_shared_with = {share.user for share in current_shares}
-
-        needs_adding = new_users_shared_with - currently_shared_with
-        for user in needs_adding:
-            current_shares.append(self.manager.share_with(item, user, flush=False))
-
-        needs_removing = currently_shared_with - new_users_shared_with
-        for user in needs_removing:
-            current_shares.remove(self.manager.unshare_with(item, user, flush=False))
-
-        self.manager.session().flush()
+        current_shares = self.manager.update_current_shares(item, new_users_shared_with)
         # TODO: or should this return the list of ids?
         return current_shares
 
@@ -692,7 +702,7 @@ class ShareableService:
         self.manager.unpublish(item)
         return self._get_sharing_status(trans, item)
 
-    def share_with(self, trans, id: EncodedDatabaseIdField, payload: ShareWithPayload) -> ShareWithStatus:
+    def set_sharing_with_users(self, trans, id: EncodedDatabaseIdField, payload: ShareWithPayload) -> ShareWithStatus:
         item = self._get_item_by_id(trans, id)
         users, errors = self._get_users(trans, payload.user_ids)
         extra = self._share_with_options(trans, item, users, errors, payload.share_option)
@@ -702,21 +712,17 @@ class ShareableService:
         status.errors.extend(errors)
         return status
 
-    def unshare_with(self, trans, id: EncodedDatabaseIdField, payload: ShareWithPayload) -> ShareWithStatus:
-        item = self._get_item_by_id(trans, id)
-        users, errors = self._get_users(trans, payload.user_ids)
-        self.manager.unshare_with(item, list(users))
-        base_status = self._get_sharing_status(trans, item)
-        status = ShareWithStatus.parse_obj(base_status)
-        status.errors.extend(errors)
-        return status
-
-    def _share_with_options(self, trans, item, users: Set[User], errors: Set[str], share_option: Optional[SharingOptions] = None):
-        if not users:
-            return None
+    def _share_with_options(
+        self,
+        trans,
+        item,
+        users: Set[User],
+        errors: Set[str],
+        share_option: Optional[SharingOptions] = None,
+    ):
         extra = self.manager.get_sharing_extra_information(trans, item, users, errors, share_option)
         if not extra or extra.can_share:
-            self.manager.share_with(item, list(users))
+            self.manager.update_current_shares(item, users)
             extra = None
         return extra
 

lib/galaxy/webapps/galaxy/api/histories.py

@@ -122,30 +122,17 @@ class FastAPIHistories:
         return self.service.shareable_service.unpublish(trans, id)
 
     @router.put(
-        '/api/histories/{id}/share_with',
+        '/api/histories/{id}/set_sharing_with_users',
         summary="Share this item with specific users.",
     )
-    def share_with(
+    def set_sharing_with_users(
         self,
         trans: ProvidesUserContext = DependsOnTrans,
         id: EncodedDatabaseIdField = HistoryIdPathParam,
         payload: sharable.ShareWithPayload = Body(...)
     ) -> sharable.ShareWithStatus:
         """Shares this item with specific users and return the current sharing status."""
-        return self.service.shareable_service.share_with(trans, id, payload)
-
-    @router.put(
-        '/api/histories/{id}/unshare_with',
-        summary="Stop sharing this item with specific users.",
-    )
-    def unshare_with(
-        self,
-        trans: ProvidesUserContext = DependsOnTrans,
-        id: EncodedDatabaseIdField = HistoryIdPathParam,
-        payload: sharable.ShareWithPayload = Body(...)
-    ) -> sharable.ShareWithStatus:
-        """Stops sharing this item with specific users and return the current sharing status."""
-        return self.service.shareable_service.unshare_with(trans, id, payload)
+        return self.service.shareable_service.set_sharing_with_users(trans, id, payload)
 
     @router.put(
         '/api/histories/{id}/slug',
@@ -530,20 +517,12 @@ class HistoriesController(BaseGalaxyAPIController):
         return self.service.shareable_service.unpublish(trans, id)
 
     @expose_api
-    def share_with(self, trans, id, payload, **kwd):
+    def set_sharing_with_users(self, trans, id, payload, **kwd):
         """
-        * PUT /api/histories/{id}/share_with
+        * PUT /api/histories/{id}/set_sharing_with_users
         """
         payload = sharable.ShareWithPayload(**payload)
-        return self.service.shareable_service.share_with(trans, id, payload)
-
-    @expose_api
-    def unshare_with(self, trans, id, payload, **kwd):
-        """
-        * PUT /api/histories/{id}/unshare_with
-        """
-        payload = sharable.ShareWithPayload(**payload)
-        return self.service.shareable_service.unshare_with(trans, id, payload)
+        return self.service.shareable_service.set_sharing_with_users(trans, id, payload)
 
     @expose_api
     def set_slug(self, trans, id, payload, **kwd):

lib/galaxy/webapps/galaxy/api/pages.py

@@ -199,30 +199,17 @@ class FastAPIPages:
         return self.service.shareable_service.unpublish(trans, id)
 
     @router.put(
-        '/api/pages/{id}/share_with',
+        '/api/pages/{id}/set_sharing_with_users',
         summary="Share this item with specific users.",
     )
-    def share_with(
+    def set_sharing_with_users(
         self,
         trans: ProvidesUserContext = DependsOnTrans,
         id: EncodedDatabaseIdField = PageIdPathParam,
         payload: ShareWithPayload = Body(...)
     ) -> ShareWithStatus:
         """Shares this item with specific users and return the current sharing status."""
-        return self.service.shareable_service.share_with(trans, id, payload)
-
-    @router.put(
-        '/api/pages/{id}/unshare_with',
-        summary="Stop sharing this item with specific users.",
-    )
-    def unshare_with(
-        self,
-        trans: ProvidesUserContext = DependsOnTrans,
-        id: EncodedDatabaseIdField = PageIdPathParam,
-        payload: ShareWithPayload = Body(...)
-    ) -> ShareWithStatus:
-        """Stops sharing this item with specific users and return the current sharing status."""
-        return self.service.shareable_service.unshare_with(trans, id, payload)
+        return self.service.shareable_service.set_sharing_with_users(trans, id, payload)
 
     @router.put(
         '/api/pages/{id}/slug',
@@ -357,20 +344,12 @@ class PagesController(BaseGalaxyAPIController):
         return self.service.shareable_service.unpublish(trans, id)
 
     @expose_api
-    def share_with(self, trans, id, payload, **kwd):
+    def set_sharing_with_users(self, trans, id, payload, **kwd):
         """
-        * PUT /api/pages/{id}/share_with
+        * PUT /api/pages/{id}/set_sharing_with_users
         """
         payload = ShareWithPayload(**payload)
-        return self.service.shareable_service.share_with(trans, id, payload)
-
-    @expose_api
-    def unshare_with(self, trans, id, payload, **kwd):
-        """
-        * PUT /api/pages/{id}/unshare_with
-        """
-        payload = ShareWithPayload(**payload)
-        return self.service.shareable_service.unshare_with(trans, id, payload)
+        return self.service.shareable_service.set_sharing_with_users(trans, id, payload)
 
     @expose_api
     def set_slug(self, trans, id, payload, **kwd):

lib/galaxy/webapps/galaxy/api/visualizations.py

@@ -119,27 +119,14 @@ class FastAPIVisualizations:
         '/api/visualizations/{id}/share_with',
         summary="Share this item with specific users.",
     )
-    def share_with(
+    def set_sharing_with_users(
         self,
         trans: ProvidesUserContext = DependsOnTrans,
         id: EncodedDatabaseIdField = VisualizationIdPathParam,
         payload: ShareWithPayload = Body(...)
     ) -> ShareWithStatus:
         """Shares this item with specific users and return the current sharing status."""
-        return self.service.shareable_service.share_with(trans, id, payload)
-
-    @router.put(
-        '/api/visualizations/{id}/unshare_with',
-        summary="Stop sharing this item with specific users.",
-    )
-    def unshare_with(
-        self,
-        trans: ProvidesUserContext = DependsOnTrans,
-        id: EncodedDatabaseIdField = VisualizationIdPathParam,
-        payload: ShareWithPayload = Body(...)
-    ) -> ShareWithStatus:
-        """Stops sharing this item with specific users and return the current sharing status."""
-        return self.service.shareable_service.unshare_with(trans, id, payload)
+        return self.service.shareable_service.set_sharing_with_users(trans, id, payload)
 
     @router.put(
         '/api/visualizations/{id}/slug',
@@ -317,20 +304,12 @@ class VisualizationsController(BaseGalaxyAPIController, UsesVisualizationMixin,
         return self.service.shareable_service.unpublish(trans, id)
 
     @expose_api
-    def share_with(self, trans, id, payload, **kwd):
+    def set_sharing_with_users(self, trans, id, payload, **kwd):
         """
-        * PUT /api/visualizations/{id}/share_with
+        * PUT /api/visualizations/{id}/set_sharing_with_users
         """
         payload = ShareWithPayload(**payload)
-        return self.service.shareable_service.share_with(trans, id, payload)
-
-    @expose_api
-    def unshare_with(self, trans, id, payload, **kwd):
-        """
-        * PUT /api/visualizations/{id}/unshare_with
-        """
-        payload = ShareWithPayload(**payload)
-        return self.service.shareable_service.unshare_with(trans, id, payload)
+        return self.service.shareable_service.set_sharing_with_users(trans, id, payload)
 
     @expose_api
     def set_slug(self, trans, id, payload, **kwd):

lib/galaxy_test/api/sharable.py

@@ -50,6 +50,16 @@ class SharingApiTests(UsesApiTestCaseMixin):
         assert sharing_response["importable"] is True
         assert sharing_response["published"] is False
 
+    def test_sharing_without_user(self):
+        resource_id = self.create("resource-to-share-with-empty")
+
+        sharing_response = self._get_resource_sharing_status(resource_id)
+        assert not sharing_response["users_shared_with"]
+
+        payload = {"user_ids": []}
+        sharing_response = self._set_resource_sharing(resource_id, action="set_sharing_with_users", payload=payload)
+        assert not sharing_response["users_shared_with"]
+
     def test_sharing_with_user_id(self):
         target_user = self._setup_user("target@user.com")
         target_user_id = target_user["id"]
@@ -60,12 +70,12 @@ class SharingApiTests(UsesApiTestCaseMixin):
         assert not sharing_response["users_shared_with"]
 
         payload = {"user_ids": [target_user_id]}
-        sharing_response = self._set_resource_sharing(resource_id, action="share_with", payload=payload)
+        sharing_response = self._set_resource_sharing(resource_id, action="set_sharing_with_users", payload=payload)
         assert sharing_response["users_shared_with"]
         assert sharing_response["users_shared_with"][0]["id"] == target_user_id
 
-        payload = {"user_ids": [target_user_id]}
-        sharing_response = self._set_resource_sharing(resource_id, action="unshare_with", payload=payload)
+        payload = {"user_ids": []}
+        sharing_response = self._set_resource_sharing(resource_id, action="set_sharing_with_users", payload=payload)
         assert not sharing_response["users_shared_with"]
 
     def test_sharing_with_user_email(self):
@@ -78,14 +88,34 @@ class SharingApiTests(UsesApiTestCaseMixin):
         assert not sharing_response["users_shared_with"]
 
         payload = {"user_ids": [target_user_email]}
-        sharing_response = self._set_resource_sharing(resource_id, action="share_with", payload=payload)
+        sharing_response = self._set_resource_sharing(resource_id, action="set_sharing_with_users", payload=payload)
         assert sharing_response["users_shared_with"]
         assert sharing_response["users_shared_with"][0]["email"] == target_user_email
 
-        payload = {"user_ids": [target_user_email]}
-        sharing_response = self._set_resource_sharing(resource_id, action="unshare_with", payload=payload)
+        payload = {"user_ids": []}
+        sharing_response = self._set_resource_sharing(resource_id, action="set_sharing_with_users", payload=payload)
         assert not sharing_response["users_shared_with"]
 
+    def test_update_sharing_with_users(self):
+        target_user_list = ["target01@user.com", "target02@user.com"]
+        additional_user_list = ["add01@user.com", "add02@user.com"]
+        all_user_emails = target_user_list + additional_user_list
+        for email in all_user_emails:
+            self._setup_user(email)
+
+        resource_id = self.create("resource-to-share-user-email")
+
+        payload = {"user_ids": all_user_emails}
+        sharing_response = self._set_resource_sharing(resource_id, action="set_sharing_with_users", payload=payload)
+        assert sharing_response["users_shared_with"]
+        assert len(sharing_response["users_shared_with"]) == len(all_user_emails)
+
+        # We just keep target users so additional users should be removed
+        payload = {"user_ids": target_user_list}
+        sharing_response = self._set_resource_sharing(resource_id, action="set_sharing_with_users", payload=payload)
+        assert len(sharing_response["users_shared_with"]) == len(target_user_list)
+        assert additional_user_list not in sharing_response["users_shared_with"]
+
     def test_sharing_with_invalid_user(self):
         invalid_user_email = "unknown@user.com"
 
@@ -95,7 +125,7 @@ class SharingApiTests(UsesApiTestCaseMixin):
         assert not sharing_response["users_shared_with"]
 
         payload = {"user_ids": [invalid_user_email]}
-        sharing_response = self._set_resource_sharing(resource_id, action="share_with", payload=payload)
+        sharing_response = self._set_resource_sharing(resource_id, action="set_sharing_with_users", payload=payload)
         assert not sharing_response["users_shared_with"]
         assert sharing_response["errors"]
         assert invalid_user_email in sharing_response["errors"][0]

lib/galaxy_test/api/test_histories.py

@@ -547,7 +547,7 @@ class SharingHistoryTestCase(ApiTestCase, BaseHistories, SharingApiTests):
         assert "already shared" in sharing_response["errors"][0]
 
     def _share_history_with_payload(self, history_id, payload):
-        sharing_response = self._put(f"histories/{history_id}/share_with", data=payload, json=True)
+        sharing_response = self._put(f"histories/{history_id}/set_sharing_with_users", data=payload, json=True)
         self._assert_status_code_is(sharing_response, 200)
         return sharing_response.json()