Repository: plone/plone.restapi (language: python)

Issue:
Site Administrator cannot add user to group
In #1712, the ability for the Site Administrator to manage users was added. But I noticed that he is unable to add a user to a group. He receives an `Unauthorized` error.

The groups service uses the addMember method:

https://github.com/plone/plone.restapi/blob/16325f20b927f28ff03762687862dcaea9944da5/src/plone/restapi/services/groups/update.py#L125

This method checks if the user has the `Manage Users` permission:

https://github.com/plone/Products.PlonePAS/blob/d148e7eb50e52cddf954406c06026fb5e60f3da7/src/Products/PlonePAS/tools/groupdata.py#L223

But he doesn't.

@davisagli  informed me that it is not a good idea to give `Manager Users` to the Site Administrator. See:

https://github.com/plone/plone.restapi/pull/1712#discussion_r1377057545

So the ideal is to do the same thing that was done in the other PR and do it with Plone Classic, correct?

Plone Classic use `addPrincipalToGroup`:

https://github.com/plone/Products.CMFPlone/blob/f3096371d80c3ca8a5829c12976bd0ab2d2c08a4/Products/CMFPlone/controlpanel/browser/usergroups_groupmembership.py#L44

So we should use it here too.


Discussion / hints:

Fix patch:
diff --git a/news/1750.bugfix b/news/1750.bugfix
new file mode 100644
index 0000000000..9d584f24da
--- /dev/null
+++ b/news/1750.bugfix
@@ -0,0 +1,1 @@
+Fixed the permission check for adding users to groups and removing users from groups, so that it is allowed for users with the Site Administrator role. @wesleybl
diff --git a/src/plone/restapi/services/groups/add.py b/src/plone/restapi/services/groups/add.py
index e9b5ab5d11..43f1d830d2 100644
--- a/src/plone/restapi/services/groups/add.py
+++ b/src/plone/restapi/services/groups/add.py
@@ -71,7 +71,7 @@ def reply(self):
         # Add members
         group = gtool.getGroupById(groupname)
         for userid in users:
-            group.addMember(userid)
+            gtool.addPrincipalToGroup(userid, groupname)
 
         self.request.response.setStatus(201)
         self.request.response.setHeader(
diff --git a/src/plone/restapi/services/groups/update.py b/src/plone/restapi/services/groups/update.py
index 5cd3212aa9..68fd56502e 100644
--- a/src/plone/restapi/services/groups/update.py
+++ b/src/plone/restapi/services/groups/update.py
@@ -112,9 +112,9 @@ def reply(self):
         )
 
         properties = {}
-        for id, property in group.propertyItems():
-            if data.get(id, False):
-                properties[id] = data[id]
+        for _id, _property in group.propertyItems():
+            if data.get(_id, False):
+                properties[_id] = data[_id]
         group.setGroupProperties(properties)
 
         # Add/remove members
@@ -122,8 +122,8 @@ def reply(self):
         for userid, allow in users.items():
             if allow:
                 if userid not in memberids:
-                    group.addMember(userid)
+                    portal_groups.addPrincipalToGroup(userid, group.id)
             else:
                 if userid in memberids:
-                    group.removeMember(userid)
+                    portal_groups.removePrincipalFromGroup(userid, group.id)
         return self.reply_no_content()


Test patch:
diff --git a/src/plone/restapi/tests/test_services_groups.py b/src/plone/restapi/tests/test_services_groups.py
index 428069d845..44ac510657 100644
--- a/src/plone/restapi/tests/test_services_groups.py
+++ b/src/plone/restapi/tests/test_services_groups.py
@@ -188,6 +188,17 @@ def test_delete_non_existing_group(self):
 
         self.assertEqual(response.status_code, 404)
 
+    def test_siteadm_add_user_to_group(self):
+        self.set_siteadm()
+        payload = {
+            "users": {TEST_USER_ID: True, SITE_OWNER_NAME: False},
+        }
+        self.api_session.patch("/@groups/Reviewers", json=payload)
+        transaction.commit()
+
+        reviewers = self.gtool.getGroupById("Reviewers")
+        self.assertIn(TEST_USER_ID, reviewers.getGroupMemberIds())
+
     def test_siteadm_not_add_user_to_group_with_manager_role(self):
         self.set_siteadm()
         payload = {