Simplify permission_add/remove to just permission_update with --add and --remove, similar to what's done for groups

data/actionsmap/yunohost.yml

@@ -276,64 +276,31 @@ user:
         permission:
             subcategory_help: Manage permissions
             actions:
+
                 ### user_permission_list()
                 list:
                     action_help: List permissions and corresponding accesses
                     api: GET /users/permissions/<permission>
 
-                ### user_permission_add()
+                ### user_permission_update()
-                add:
+                update:
-                    action_help: Grant access right to users and group
+                    action_help: Grant / remove permissions to groups or users
-                    api: POST /users/permissions/<app>
+                    api: POST /users/permissions/<permission>
                     arguments:
-                        app:
+                        permission:
-                            help: Application to manage the permission
+                            help: Permission to manage (e.g. mail.main or wordpress.editors)
-                            nargs: "+"
+                        -a:
-                        -p:
+                            full: --add
-                            full: --permission
+                            help: Group or user names to add to this permission
-                            help: Name of permission (main by default)
                             nargs: "*"
-                            metavar: PERMISSION
+                            metavar: GROUP_OR_USER
-                        -u:
-                            full: --username
-                            help: Username
-                            nargs: "*"
-                            metavar: USER
                             extra:
                                 pattern: *pattern_username
-                        -g:
+                        -r:
-                            full: --group
+                            full: --remove
-                            help: Group name
+                            help: Group or user names to remove from this permission
                             nargs: "*"
-                            metavar: GROUP
+                            metavar: GROUP_OR_USER
-                            extra:
-                                pattern: *pattern_username
-
-                ### user_permission_remove()
-                remove:
-                    action_help: Revoke access right to users and group
-                    api: PUT /users/permissions/<app>
-                    arguments:
-                        app:
-                            help: Application to manage the permission
-                            nargs: "+"
-                        -p:
-                            full: --permission
-                            help: Name of permission (main by default)
-                            nargs: "*"
-                            metavar: PERMISSION
-                        -u:
-                            full: --username
-                            help: Username
-                            nargs: "*"
-                            metavar: USER
-                            extra:
-                                pattern: *pattern_username
-                        -g:
-                            full: --group
-                            help: Group name
-                            nargs: "*"
-                            metavar: GROUP
                             extra:
                                 pattern: *pattern_username
 

locales/en.json

@@ -196,7 +196,6 @@
     "dyndns_registration_failed": "Unable to register DynDNS domain: {error:s}",
     "dyndns_domain_not_provided": "Dyndns provider {provider:s} cannot provide domain {domain:s}.",
     "dyndns_unavailable": "Domain {domain:s} is not available.",
-    "edit_permission_with_group_all_users_not_allowed": "You are not allowed to edit permission for group 'all_users', use 'yunohost user permission clear APP' or 'yunohost user permission add APP -u USER' instead.",
     "error_when_removing_sftpuser_group": "Error when trying remove sftpusers group",
     "executing_command": "Executing command '{command:s}'…",
     "executing_script": "Executing script '{script:s}'…",
@@ -229,8 +228,8 @@
     "global_settings_unknown_type": "Unexpected situation, the setting {setting:s} appears to have the type {unknown_type:s} but it's not a type supported by the system.",
     "good_practices_about_admin_password": "You are now about to define a new administration password. The password should be at least 8 characters - though it is good practice to use longer password (i.e. a passphrase) and/or to use various kind of characters (uppercase, lowercase, digits and special characters).",
     "good_practices_about_user_password": "You are now about to define a new user password. The password should be at least 8 characters - though it is good practice to use longer password (i.e. a passphrase) and/or to use various kind of characters (uppercase, lowercase, digits and special characters).",
-    "group_already_allowed": "Group '{group:s}' already has permission '{permission:s}' enabled for app '{app:s}'",
+    "group_already_allowed": "Group '{group:s}' already has permission '{permission:s}' enabled'",
-    "group_already_disallowed": "Group '{group:s}' already has permissions '{permission:s}' disabled for app '{app:s}'",
+    "group_already_disallowed": "Group '{group:s}' already has permission '{permission:s}' disabled'",
     "group_name_already_exist": "Group {name:s} already exist",
     "group_created": "Group '{group}' successfully created",
     "group_creation_failed": "Group creation failed for group '{group}'",
@@ -397,7 +396,6 @@
     "mysql_db_creation_failed": "MySQL database creation failed",
     "mysql_db_init_failed": "MySQL database init failed",
     "mysql_db_initialized": "The MySQL database has been initialized",
-    "need_define_permission_before": "You need to redefine the permission using 'yunohost user permission add -u USER' before removing an allowed group",
     "network_check_mx_ko": "DNS MX record is not set",
     "network_check_smtp_ko": "Outbound mail (SMTP port 25) seems to be blocked by your network",
     "network_check_smtp_ok": "Outbound mail (SMTP port 25) is not blocked",

src/yunohost/permission.py

@@ -24,6 +24,7 @@
     Manage permissions
 """
 
+import copy
 import grp
 import random
 
@@ -45,7 +46,6 @@ logger = getActionLogger('yunohost.user')
 def user_permission_list():
     """
     List permissions and corresponding accesses
-
     """
 
     from yunohost.utils.ldap import _get_ldap_interface, _ldap_path_extract
@@ -70,146 +70,92 @@ def user_permission_list():
     return {'permissions': permissions}
 
 
-def user_permission_update(operation_logger, app=[], permission=None, add_username=None, add_group=None, del_username=None, del_group=None, sync_perm=True):
+def user_permission_update(operation_logger, permission, add=None, remove=None, sync_perm=True):
     """
     Allow or Disallow a user or group to a permission for a specific application
 
     Keyword argument:
-        app            -- an application OR sftp, xmpp (metronome), mail
+        permission     -- Name of the permission (e.g. mail.mail or wordpress.editors)
-        permission     -- name of the permission ("main" by default)
+        add            -- List of groups or usernames to add to this permission
-        add_username   -- Username to allow
+        remove         -- List of groups or usernames to remove from to this permission
-        add_group      -- Groupname to allow
-        del_username   -- Username to disallow
-        del_group      -- Groupname to disallow
-
     """
     from yunohost.hook import hook_callback
     from yunohost.user import user_group_list
-    from yunohost.utils.ldap import _get_ldap_interface
+    from yunohost.utils.ldap import _get_ldap_interface, _ldap_path_extract
     ldap = _get_ldap_interface()
 
-    if permission:
+    # Fetch currently allowed groups for this permission
-        if not isinstance(permission, list):
-            permission = [permission]
-    else:
-        permission = ["main"]
 
-    if add_group:
-        if not isinstance(add_group, list):
-            add_group = [add_group]
-    else:
-        add_group = []
-
-    if add_username:
-        if not isinstance(add_username, list):
-            add_username = [add_username]
-    else:
-        add_username = []
-
-    if del_group:
-        if not isinstance(del_group, list):
-            del_group = [del_group]
-    else:
-        del_group = []
-
-    if del_username:
-        if not isinstance(del_username, list):
-            del_username = [del_username]
-    else:
-        del_username = []
-
-    # Validate that the group exist
-    for g in add_group:
-        if g not in user_group_list()['groups']:
-            raise YunohostError('group_unknown', group=g)
-    for u in add_username:
-        if u not in user_list(['uid'])['users']:
-            raise YunohostError('user_unknown', user=u)
-    for g in del_group:
-        if g not in user_group_list()['groups']:
-            raise YunohostError('group_unknown', group=g)
-    for u in del_username:
-        if u not in user_list(['uid'])['users']:
-            raise YunohostError('user_unknown', user=u)
-
-    # Merge user and group (note that we consider all user as a group)
-    add_group.extend(add_username)
-    del_group.extend(del_username)
-
-    if 'all_users' in add_group or 'all_users' in del_group:
-        raise YunohostError('edit_permission_with_group_all_users_not_allowed')
-
-    # Populate permission informations
-    permission_attrs = [
-        'cn',
-        'groupPermission',
-    ]
     result = ldap.search('ou=permission,dc=yunohost,dc=org',
-                         '(objectclass=permissionYnh)', permission_attrs)
+                         '(objectclass=permissionYnh)',
+                         ["cn", "groupPermission"])
     result = {p['cn'][0]: p for p in result}
+    if permission not in result:
+        raise YunohostError('permission_not_found', permission=permission)
 
-    new_per_dict = {}
+    current_allowed_groups = [_ldap_path_extract(p, "cn") for p in result[permission].get("groupPermission", [])]
 
-    for a in app:
+    # Compute new allowed group list (and make sure what we're doing make sense)
-        for per in permission:
-            permission_name = per + '.' + a
-            if permission_name not in result:
-                raise YunohostError('permission_not_found', permission=per, app=a)
-            new_per_dict[permission_name] = set()
-            if 'groupPermission' in result[permission_name]:
-                new_per_dict[permission_name] = set(result[permission_name]['groupPermission'])
 
-            for g in del_group:
+    new_allowed_groups = copy.copy(current_allowed_groups)
-                if 'cn=all_users,ou=groups,dc=yunohost,dc=org' in new_per_dict[permission_name]:
-                    raise YunohostError('need_define_permission_before')
-                group_name = 'cn=' + g + ',ou=groups,dc=yunohost,dc=org'
-                if group_name not in new_per_dict[permission_name]:
-                    logger.warning(m18n.n('group_already_disallowed', permission=per, app=a, group=g))
-                else:
-                    new_per_dict[permission_name].remove(group_name)
 
-            if 'cn=all_users,ou=groups,dc=yunohost,dc=org' in new_per_dict[permission_name]:
+    if add:
-                new_per_dict[permission_name].remove('cn=all_users,ou=groups,dc=yunohost,dc=org')
+        existing_groups = user_group_list()['groups'].keys()
-            for g in add_group:
+        groups_to_add = [add] if not isinstance(add, list) else add
-                group_name = 'cn=' + g + ',ou=groups,dc=yunohost,dc=org'
+        for group in groups_to_add:
-                if group_name in new_per_dict[permission_name]:
+            if group not in existing_groups:
-                    logger.warning(m18n.n('group_already_allowed', permission=per, app=a, group=g))
+                raise YunohostError('group_unknown', group=group)
-                else:
+            if group in current_allowed_groups:
-                    new_per_dict[permission_name].add(group_name)
+                logger.warning(m18n.n('group_already_allowed', permission=permission, group=group))
+        new_allowed_groups += groups_to_add
+
+    if remove:
+        groups_to_remove = [remove] if not isinstance(remove, list) else remove
+        for group in groups_to_remove:
+            if group not in existing_groups:
+                raise YunohostError('group_unknown', group=group)
+            if group not in current_allowed_groups:
+                logger.warning(m18n.n('group_already_disallowed', permission=permission, group=group))
+
+        new_allowed_groups = [g for g in new_allowed_groups if g not in groups_to_remove]
+
+    # If we end up with something like allowed groups is ["all_users", "volunteers"]
+    # we shall warn the users that they should probably choose between one or the other,
+    # because the current situation is probably not what they expect / is temporary ?
+
+    if len(new_allowed_groups) > 1 and "all_users" in new_allowed_groups:
+        # FIXME : write a better explanation
+        logger.warning("This permission is currently enabled for all users in addition to other groups. You probably want to either remove the 'all_users' permission or remove the specific groups currently allowed.")
+
+    # Commit the new allowed group list
 
     operation_logger.start()
 
-    for per, val in new_per_dict.items():
     # Don't update LDAP if we update exactly the same values
-        if val == set(result[per]['groupPermission'] if 'groupPermission' in result[per] else []):
+    if set(new_allowed_groups) == set(current_allowed_groups):
-            continue
+        logger.warning("No change was applied because not relevant modification were found")
-        if ldap.update('cn=%s,ou=permission' % per, {'groupPermission': val}):
+    elif ldap.update('cn=%s,ou=permission' % permission,
-            p = per.split('.')
+                     {'groupPermission': ['cn=' + g + ',ou=groups,dc=yunohost,dc=org' for g in new_allowed_groups]}):
-            logger.debug(m18n.n('permission_updated', permission=p[0], app=p[1]))
+        logger.debug(m18n.n('permission_updated', permission=permission))
-        else:
+
-            raise YunohostError('permission_update_failed')
+        # Trigger permission sync if asked
 
         if sync_perm:
             permission_sync_to_user()
 
-    for a in app:
+        # Trigger app callbacks
-        allowed_users = set()
-        disallowed_users = set()
-        group_list = user_group_list()['groups']
 
-        for g in add_group:
+        # FIXME FIXME FIXME
-            allowed_users.union(group_list[g]['members'])
-        for g in del_group:
-            disallowed_users.union(group_list[g]['members'])
 
-        allowed_users = ','.join(allowed_users)
+        #if groups_to_add:
-        disallowed_users = ','.join(disallowed_users)
+        #    hook_callback('post_app_addaccess', args=[app, allowed_users])
-        if add_group:
+        #if groups_to_remove:
-            hook_callback('post_app_addaccess', args=[app, allowed_users])
+        #    hook_callback('post_app_removeaccess', args=[app, disallowed_users])
-        if del_group:
-            hook_callback('post_app_removeaccess', args=[app, disallowed_users])
 
-    return user_permission_list(app, permission)
+    else:
+        raise YunohostError('permission_update_failed')
+
+    return user_permission_list()["permissions"][permission]
 
 
 def user_permission_clear(operation_logger, app=[], permission=None, sync_perm=True):

src/yunohost/user.py

@@ -724,21 +724,11 @@ def user_permission_list():
     return yunohost.permission.user_permission_list()
 
 
-@is_unit_operation([('app', 'user')])
+@is_unit_operation([('permission', 'user')])
-def user_permission_add(operation_logger, app, permission="main", username=None, group=None, sync_perm=True):
+def user_permission_update(operation_logger, permission, add=None, remove=None, sync_perm=True):
     import yunohost.permission
-    return yunohost.permission.user_permission_update(operation_logger, app, permission=permission,
+    return yunohost.permission.user_permission_update(operation_logger, permission,
-                                                      add_username=username, add_group=group,
+                                                      add=add, remove=remove,
-                                                      del_username=None, del_group=None,
-                                                      sync_perm=sync_perm)
-
-
-@is_unit_operation([('app', 'user')])
-def user_permission_remove(operation_logger, app, permission="main", username=None, group=None, sync_perm=True):
-    import yunohost.permission
-    return yunohost.permission.user_permission_update(operation_logger, app, permission=permission,
-                                                      add_username=None, add_group=None,
-                                                      del_username=username, del_group=group,
                                                       sync_perm=sync_perm)