Simplify group info and group update interface and code

data/actionsmap/yunohost.yml

@@ -249,15 +249,15 @@ user:
                             extra:
                                 pattern: *pattern_groupname
                         -a:
-                            full: --add-user
+                            full: --add
-                            help: User to add in group
+                            help: User(s) to add in the group
                             nargs: "*"
                             metavar: USERNAME
                             extra:
                                 pattern: *pattern_username
                         -r:
-                            full: --remove-user
+                            full: --remove
-                            help: User to remove in group
+                            help: User(s) to remove in the group
                             nargs: "*"
                             metavar: USERNAME
                             extra:

src/yunohost/data_migrations/0011_setup_group_permission.py

@@ -66,8 +66,8 @@ class MyMigration(Migration):
             ldap.update('uid=%s,ou=users' % username,
                         {'objectClass': ['mailAccount', 'inetOrgPerson', 'posixAccount', 'userPermissionYnh']})
             user_group_create(username, gid=user_info['uidNumber'][0], sync_perm=False)
-            user_group_update(groupname=username, add_user=username, force=True, sync_perm=False)
+            user_group_update(groupname=username, add=username, force=True, sync_perm=False)
-            user_group_update(groupname='all_users', add_user=username, force=True, sync_perm=False)
+            user_group_update(groupname='all_users', add=username, force=True, sync_perm=False)
 
 
     def migrate_app_permission(self, app=None):

src/yunohost/tests/test_user-group.py

@@ -26,8 +26,8 @@ def setup_function(function):
 
     user_group_create("dev")
     user_group_create("apps")
-    user_group_update("dev", add_user=["alice"])
+    user_group_update("dev", add=["alice"])
-    user_group_update("apps", add_user=["bob"])
+    user_group_update("apps", add=["bob"])
 
 def teardown_function(function):
     clean_user_groups()
@@ -151,28 +151,28 @@ def test_update_user_1():
     assert "NewLast" == info['lastname']
 
 def test_update_group_1():
-    user_group_update("dev", add_user=["bob"])
+    user_group_update("dev", add=["bob"])
 
     group_res = user_group_list()['groups']
     assert set(["alice", "bob"]) == set(group_res['dev']['members'])
 
 def test_update_group_2():
     # Try to add a user in a group when the user is already in
-    user_group_update("apps", add_user=["bob"])
+    user_group_update("apps", add=["bob"])
 
     group_res = user_group_list()['groups']
     assert ["bob"] == group_res['apps']['members']
 
 def test_update_group_3():
     # Try to remove a user in a group
-    user_group_update("apps", remove_user=["bob"])
+    user_group_update("apps", remove=["bob"])
 
     group_res = user_group_list()['groups']
     assert "members" not in group_res['apps']
 
 def test_update_group_4():
     # Try to remove a user in a group when it is not already in
-    user_group_update("apps", remove_user=["jack"])
+    user_group_update("apps", remove=["jack"])
 
     group_res = user_group_list()['groups']
     assert ["bob"] == group_res['apps']['members']
@@ -190,21 +190,21 @@ def test_bad_update_user_1():
 def bad_update_group_1():
     # Check groups not found
     with pytest.raises(YunohostError):
-        user_group_update("not_exit", add_user=["alice"])
+        user_group_update("not_exit", add=["alice"])
 
 def test_bad_update_group_2():
     # Check remove user in groups "all_users" not allowed
     with pytest.raises(YunohostError):
-        user_group_update("all_users", remove_user=["alice"])
+        user_group_update("all_users", remove=["alice"])
 
 def test_bad_update_group_3():
     # Check remove user in it own group not allowed
     with pytest.raises(YunohostError):
-        user_group_update("alice", remove_user=["alice"])
+        user_group_update("alice", remove=["alice"])
 
 def test_bad_update_group_1():
     # Check add bad user in group
     with pytest.raises(YunohostError):
-        user_group_update("dev", add_user=["not_exist"])
+        user_group_update("dev", add=["not_exist"])
 
     assert "not_exist" not in user_group_list()["groups"]["dev"]

src/yunohost/user.py

@@ -32,6 +32,7 @@ import crypt
 import random
 import string
 import subprocess
+import copy
 
 from moulinette import m18n
 from yunohost.utils.error import YunohostError
@@ -219,8 +220,8 @@ def user_create(operation_logger, username, firstname, lastname, mail, password,
 
         # Create group for user and add to group 'all_users'
         user_group_create(groupname=username, gid=uid, sync_perm=False)
-        user_group_update(groupname=username, add_user=username, force=True, sync_perm=False)
+        user_group_update(groupname=username, add=username, force=True, sync_perm=False)
-        user_group_update(groupname='all_users', add_user=username, force=True, sync_perm=True)
+        user_group_update(groupname='all_users', add=username, force=True, sync_perm=True)
 
         # TODO: Send a welcome mail to user
         logger.success(m18n.n('user_created'))
@@ -610,84 +611,67 @@ def user_group_delete(operation_logger, groupname, force=False, sync_perm=True):
 
 
 @is_unit_operation([('groupname', 'user')])
-def user_group_update(operation_logger, groupname, add_user=None, remove_user=None, force=False, sync_perm=True):
+def user_group_update(operation_logger, groupname, add=None, remove=None, force=False, sync_perm=True):
     """
     Update user informations
 
     Keyword argument:
         groupname -- Groupname to update
-        add_user -- User to add in group
+        add -- User(s) to add in group
-        remove_user -- User to remove in group
+        remove -- User(s) to remove in group
 
     """
 
     from yunohost.permission import permission_sync_to_user
     from yunohost.utils.ldap import _get_ldap_interface
 
+    # FIXME : we should also refuse to edit the main group of a user (e.g. group 'sam' related to user 'sam')
+
     if (groupname == 'all_users' or groupname == 'admins') and not force:
         raise YunohostError('edit_group_not_allowed', group=groupname)
 
     ldap = _get_ldap_interface()
 
-    # Populate group informations
+    # We extract the uid for each member of the group to keep a simple flat list of members
-    attrs_to_fetch = ['member']
+    current_group = user_group_info(groupname)["members"]
-    result = ldap.search(base='ou=groups,dc=yunohost,dc=org',
+    new_group = copy.copy(current_group)
-                         filter='cn=' + groupname, attrs=attrs_to_fetch)
-    if not result:
-        raise YunohostError('group_unknown', group=groupname)
-    group = result[0]
 
-    new_group_list = {'member': set(), 'memberUid': set()}
+    existing_users = user_list()['users'].keys()
-    if 'member' in group:
-        new_group_list['member'] = set(group['member'])
-    else:
-        group['member'] = []
 
-    existing_users = user_list(fields=['uid'])['users'].keys()
+    if add:
+        users_to_add = [add] if not isinstance(add, list) else add
 
-    if add_user:
+        for user in users_to_add:
-        if not isinstance(add_user, list):
-            add_user = [add_user]
-
-        for user in add_user:
             if user not in existing_users:
                 raise YunohostError('user_unknown', user=user)
 
-        for user in add_user:
+            if user in current_group:
-            userDN = "uid=" + user + ",ou=users,dc=yunohost,dc=org"
-            if userDN in group['member']:
                 logger.warning(m18n.n('user_already_in_group', user=user, group=groupname))
-            new_group_list['member'].add(userDN)
 
-    if remove_user:
+        new_group += users_to_add
-        if not isinstance(remove_user, list):
-            remove_user = [remove_user]
 
-        for user in remove_user:
+    if remove:
+        users_to_remove = [remove] if not isinstance(remove, list) else remove
+
+        for user in users_to_remove:
             if user == groupname:
+                # FIXME : well if the user equals the group, why pass the two info...
+                # anyway we should just forbid this from the very beginning ... (editing a user-related group)
                 raise YunohostError('remove_user_of_group_not_allowed', user=user, group=groupname)
 
-        for user in remove_user:
+            if user not in current_group:
-            userDN = "uid=" + user + ",ou=users,dc=yunohost,dc=org"
-            if 'member' in group and userDN in group['member']:
-                new_group_list['member'].remove(userDN)
-            else:
                 logger.warning(m18n.n('user_not_in_group', user=user, group=groupname))
 
-    # Sychronise memberUid with member (to keep the posix group structure)
+        # Remove users_to_remove from new_group
-    # In posixgroup the main group of each user is only written in the gid number of the user
+        # Kinda like a new_group -= users_to_remove
-    for member in new_group_list['member']:
+        new_group = [u for u in new_group if u not in users_to_remove]
-        member_Uid = member.split("=")[1].split(",")[0]
+
-        # Don't add main user in the group.
+    new_group_dns = ["uid=" + user + ",ou=users,dc=yunohost,dc=org" for user in new_group]
-        # Note that in the Unix system the main user of the group is linked by the gid in the user attribute.
-        # So the main user need to be not in the memberUid list of his own group.
-        if member_Uid != groupname:
-            new_group_list['memberUid'].add(member_Uid)
 
     operation_logger.start()
 
-    if new_group_list['member'] != set(group['member']):
+    if set(new_group) != set(current_group):
-        if not ldap.update('cn=%s,ou=groups' % groupname, new_group_list):
+        if not ldap.update('cn=%s,ou=groups' % groupname, {"member": set(new_group_dns), "memberUid": set(new_group)}):
             raise YunohostError('group_update_failed', group=groupname)
 
     logger.success(m18n.n('group_updated', group=groupname))
@@ -705,26 +689,25 @@ def user_group_info(groupname):
 
     """
 
-    from yunohost.utils.ldap import _get_ldap_interface
+    from yunohost.utils.ldap import _get_ldap_interface, _ldap_path_extract
     ldap = _get_ldap_interface()
 
-    group_attrs = [
+    # Fetch info for this group
-        'cn', 'member', 'permission'
+    result = ldap.search('ou=groups,dc=yunohost,dc=org',
-    ]
+                         "cn=" + groupname,
-    result = ldap.search('ou=groups,dc=yunohost,dc=org', "cn=" + groupname, group_attrs)
+                         ["cn", "member", "permission"])
 
     if not result:
         raise YunohostError('group_unknown', group=groupname)
 
-    group = result[0]
+    infos = result[0]
 
-    result_dict = {
+    # Format data
-        'groupname': group['cn'][0],
+
-        'member': None
+    return {
+        'members': [_ldap_path_extract(p, "uid") for p in infos.get("member", [])],
+        'permissions': [_ldap_path_extract(p, "cn") for p in infos.get("permission", [])]
     }
-    if 'member' in group:
-        result_dict['member'] = {m.split("=")[1].split(",")[0] for m in group['member']}
-    return result_dict
 
 
 #