c.f. issue 1405 ... those 'if ldap.stuff()' are complete bullshit from the very beginning since they never return False : instead they trigger an exception which means the current error management is completely meaningless ... so this refactorize all the places if found those + add proper error messages

locales/en.json

@@ -164,9 +164,9 @@
     "domain_cannot_remove_main": "Cannot remove main domain. Set a new main domain first",
     "domain_cert_gen_failed": "Unable to generate certificate",
     "domain_created": "The domain has been created",
-    "domain_creation_failed": "Unable to create domain",
+    "domain_creation_failed": "Failed to create domain {domain}: {error}",
     "domain_deleted": "The domain has been deleted",
-    "domain_deletion_failed": "Unable to delete domain",
+    "domain_deletion_failed": "Failed to delete domain {domain}: {error}",
     "domain_dns_conf_is_just_a_recommendation": "This command shows you what is the *recommended* configuration. It does not actually set up the DNS configuration for you. It is your responsability to configure your DNS zone in your registrar according to this recommendation.",
     "domain_dyndns_already_subscribed": "You've already subscribed to a DynDNS domain",
     "domain_dyndns_dynette_is_unreachable": "Unable to reach YunoHost dynette, either your YunoHost is not correctly connected to the internet or the dynette server is down. Error: {error}",
@@ -229,14 +229,14 @@
     "group_already_exist": "Group {group} already exist",
     "group_already_exist_on_system": "Group {group} already exists in the system group",
     "group_created": "Group '{group}' successfully created",
-    "group_creation_failed": "Group creation failed for group '{group}'",
+    "group_creation_failed": "Failed to create group {group}: {error}",
     "group_cannot_be_edited": "The group {group} cannot be edited manually.",
     "group_cannot_be_deleted": "The group {group} cannot be deleted manually.",
     "group_deleted": "Group '{group}' deleted",
-    "group_deletion_failed": "Group '{group} 'deletion failed",
+    "group_deletion_failed": "Failed to delete group {group}: {error}",
     "group_unknown": "Group {group} unknown",
     "group_updated": "Group '{group}' updated",
-    "group_update_failed": "Group update failed for group '{group}'",
+    "group_update_failed": "Failed to update group {group}: {error}",
     "group_user_already_in_group": "User {user} is already in group {group}",
     "group_user_not_in_group": "User {user} is not in group {group}",
     "hook_exec_failed": "Script execution failed: {path:s}",
@@ -430,11 +430,11 @@
     "permission_already_exist": "Permission '{permission}' already exists",
     "permission_cannot_remove_main": "Removing a main permission is not allowed",
     "permission_created": "Permission '{permission}' created",
-    "permission_creation_failed": "Failed to create permission '{permission}'",
+    "permission_creation_failed": "Failed to create permission '{permission}': {error}",
     "permission_deleted": "Permission '{permission}' deleted",
-    "permission_deletion_failed": "Failed to delete permission '{permission}'",
+    "permission_deletion_failed": "Failed to delete permission '{permission}': {error}",
     "permission_not_found": "Permission '{permission}' does not seem to exist ?",
-    "permission_update_failed": "Failed to update permission '{permission}'",
+    "permission_update_failed": "Failed to update permission '{permission}' : {error}",
     "permission_updated": "Permission '{permission}' updated",
     "permission_update_nothing_to_do": "No permissions to update",
     "port_already_closed": "Port {port:d} is already closed for {ip_version:s} connections",
@@ -556,13 +556,13 @@
     "upnp_enabled": "UPnP has been enabled",
     "upnp_port_open_failed": "Unable to open UPnP ports",
     "user_created": "The user has been created",
-    "user_creation_failed": "Unable to create user",
+    "user_creation_failed": "Unable to create user {user}: {error}",
     "user_deleted": "The user has been deleted",
-    "user_deletion_failed": "Unable to delete user",
+    "user_deletion_failed": "Unable to delete user {user}: {error}",
     "user_home_creation_failed": "Unable to create user home folder",
     "user_info_failed": "Unable to retrieve user information",
     "user_unknown": "Unknown user: {user:s}",
-    "user_update_failed": "Unable to update user",
+    "user_update_failed": "Unable to update user {user}: {error}",
     "user_updated": "The user has been updated",
     "users_available": "Available users:",
     "yunohost_already_installed": "YunoHost is already installed",

src/yunohost/domain.py

@@ -112,8 +112,10 @@ def domain_add(operation_logger, domain, dyndns=False):
             'virtualdomain': domain,
         }
 
-        if not ldap.add('virtualdomain=%s,ou=domains' % domain, attr_dict):
+        try:
-            raise YunohostError('domain_creation_failed')
+            ldap.add('virtualdomain=%s,ou=domains' % domain, attr_dict)
+        except Exception as e:
+            raise YunohostError('domain_creation_failed', domain=domain, error=e)
 
         # Don't regen these conf if we're still in postinstall
         if os.path.exists('/etc/yunohost/installed'):
@@ -167,10 +169,12 @@ def domain_remove(operation_logger, domain, force=False):
 
     operation_logger.start()
     ldap = _get_ldap_interface()
-    if ldap.remove('virtualdomain=' + domain + ',ou=domains') or force:
+    try:
-        os.system('rm -rf /etc/yunohost/certs/%s' % domain)
+        ldap.remove('virtualdomain=' + domain + ',ou=domains')
-    else:
+    except Exception as e:
-        raise YunohostError('domain_deletion_failed')
+        raise YunohostError('domain_deletion_failed', domain=domain, error=e)
+
+    os.system('rm -rf /etc/yunohost/certs/%s' % domain)
 
     regen_conf(names=['nginx', 'metronome', 'dnsmasq', 'postfix'])
     app_ssowatconf()

src/yunohost/permission.py

@@ -153,36 +153,37 @@ def user_permission_update(operation_logger, permission, add=None, remove=None,
 
     operation_logger.start()
 
-    if ldap.update('cn=%s,ou=permission' % permission,
+    try:
-                     {'groupPermission': ['cn=' + g + ',ou=groups,dc=yunohost,dc=org' for g in new_allowed_groups]}):
+        ldap.update('cn=%s,ou=permission' % permission,
-        logger.debug(m18n.n('permission_updated', permission=permission))
+                    {'groupPermission': ['cn=' + g + ',ou=groups,dc=yunohost,dc=org' for g in new_allowed_groups]})
+    except Exception as e:
+        raise YunohostError('permission_update_failed', permission=permission, error=e)
 
-        # Trigger permission sync if asked
+    logger.debug(m18n.n('permission_updated', permission=permission))
 
-        if sync_perm:
+    # Trigger permission sync if asked
-            permission_sync_to_user()
 
-        new_permission = user_permission_list(full=True)["permissions"][permission]
+    if sync_perm:
+        permission_sync_to_user()
 
-        # Trigger app callbacks
+    new_permission = user_permission_list(full=True)["permissions"][permission]
 
-        app = permission.split(".")[0]
+    # Trigger app callbacks
 
-        old_allowed_users = set(existing_permission["corresponding_users"])
+    app = permission.split(".")[0]
-        new_allowed_users = set(new_permission["corresponding_users"])
 
-        effectively_added_users = new_allowed_users - old_allowed_users
+    old_allowed_users = set(existing_permission["corresponding_users"])
-        effectively_removed_users = old_allowed_users - new_allowed_users
+    new_allowed_users = set(new_permission["corresponding_users"])
 
-        if effectively_added_users:
+    effectively_added_users = new_allowed_users - old_allowed_users
-            hook_callback('post_app_addaccess', args=[app, ','.join(effectively_added_users)])
+    effectively_removed_users = old_allowed_users - new_allowed_users
-        if effectively_removed_users:
-            hook_callback('post_app_removeaccess', args=[app, ','.join(effectively_removed_users)])
 
-        return new_permission
+    if effectively_added_users:
+        hook_callback('post_app_addaccess', args=[app, ','.join(effectively_added_users)])
+    if effectively_removed_users:
+        hook_callback('post_app_removeaccess', args=[app, ','.join(effectively_removed_users)])
 
-    else:
+    return new_permission
-        raise YunohostError('permission_update_failed', permission=permission)
 
 
 @is_unit_operation()
@@ -209,10 +210,12 @@ def user_permission_reset(operation_logger, permission, sync_perm=True):
     operation_logger.start()
 
     default_permission = {'groupPermission': ['cn=all_users,ou=groups,dc=yunohost,dc=org']}
-    if ldap.update('cn=%s,ou=permission' % permission, default_permission):
+    try:
-        logger.debug(m18n.n('permission_updated', permission=permission))
+        ldap.update('cn=%s,ou=permission' % permission, default_permission)
-    else:
+    except Exception as e:
-        raise YunohostError('permission_update_failed', permission=permission)
+        raise YunohostError('permission_update_failed', permission=permission, error=e)
+
+    logger.debug(m18n.n('permission_updated', permission=permission))
 
     if sync_perm:
         permission_sync_to_user()
@@ -286,13 +289,17 @@ def permission_create(operation_logger, permission, urls=None, sync_perm=True):
 
     operation_logger.related_to.append(('app', permission.split(".")[0]))
     operation_logger.start()
-    if ldap.add('cn=%s,ou=permission' % permission, attr_dict):
+
-        if sync_perm:
+    try:
-            permission_sync_to_user()
+        ldap.add('cn=%s,ou=permission' % permission, attr_dict)
-        logger.debug(m18n.n('permission_created', permission=permission))
+    except Exception as e:
-        return user_permission_list(full=True)["permissions"][permission]
+        raise YunohostError('permission_creation_failed', permission=permission, error=e)
-    else:
+
-        raise YunohostError('permission_creation_failed')
+    if sync_perm:
+        permission_sync_to_user()
+
+    logger.debug(m18n.n('permission_created', permission=permission))
+    return user_permission_list(full=True)["permissions"][permission]
 
 
 @is_unit_operation()
@@ -336,13 +343,17 @@ def permission_urls(operation_logger, permission, add=None, remove=None, sync_pe
 
     operation_logger.related_to.append(('app', permission.split(".")[0]))
     operation_logger.start()
-    if ldap.update('cn=%s,ou=permission' % permission, {'URL': new_urls}):
+
-        if sync_perm:
+    try:
-            permission_sync_to_user()
+        ldap.update('cn=%s,ou=permission' % permission, {'URL': new_urls})
-        logger.debug(m18n.n('permission_updated', permission=permission))
+    except Exception as e:
-        return user_permission_list(full=True)["permissions"][permission]
+        raise YunohostError('permission_update_failed', permission=permission, error=e)
-    else:
+
-        raise YunohostError('permission_update_failed', permission=permission)
+    if sync_perm:
+        permission_sync_to_user()
+
+    logger.debug(m18n.n('permission_updated', permission=permission))
+    return user_permission_list(full=True)["permissions"][permission]
 
 
 @is_unit_operation()
@@ -370,12 +381,15 @@ def permission_delete(operation_logger, permission, force=False, sync_perm=True)
 
     operation_logger.related_to.append(('app', permission.split(".")[0]))
     operation_logger.start()
-    if ldap.remove('cn=%s,ou=permission' % permission):
+
-        if sync_perm:
+    try:
-            permission_sync_to_user()
+        ldap.remove('cn=%s,ou=permission' % permission)
-        logger.debug(m18n.n('permission_deleted', permission=permission))
+    except Exception as e:
-    else:
+        raise YunohostError('permission_deletion_failed', permission=permission, error=e)
-        raise YunohostError('permission_deletion_failed', permission=permission)
+
+    if sync_perm:
+        permission_sync_to_user()
+    logger.debug(m18n.n('permission_deleted', permission=permission))
 
 
 def permission_sync_to_user():
@@ -410,8 +424,10 @@ def permission_sync_to_user():
                                'memberUid': should_be_allowed_users}
 
         # Commit the change with the new inherited stuff
-        if not ldap.update('cn=%s,ou=permission' % permission_name, new_inherited_perms):
+        try:
-            raise YunohostError('permission_update_failed', permission=permission_name)
+            ldap.update('cn=%s,ou=permission' % permission_name, new_inherited_perms)
+        except Exception as e:
+            raise YunohostError('permission_update_failed', permission=permission_name, error=e)
 
     logger.debug("The permission database has been resynchronized")
 
@@ -421,6 +437,7 @@ def permission_sync_to_user():
     os.system('nscd --invalidate=passwd')
     os.system('nscd --invalidate=group')
 
+
 def _normalize_url(url):
     from yunohost.domain import _normalize_domain_path
     domain = url[:url.index('/')]

src/yunohost/user.py

@@ -205,32 +205,34 @@ def user_create(operation_logger, username, firstname, lastname, mail, password,
             except IOError as e:
                 raise YunohostError('ssowat_persistent_conf_write_error', error=e.strerror)
 
-    if ldap.add('uid=%s,ou=users' % username, attr_dict):
+    try:
-        # Invalidate passwd to take user creation into account
+        ldap.add('uid=%s,ou=users' % username, attr_dict)
-        subprocess.call(['nscd', '-i', 'passwd'])
+    except Exception as e:
+        raise YunohostError('user_creation_failed', user=username, error=e)
 
-        try:
+    # Invalidate passwd to take user creation into account
-            # Attempt to create user home folder
+    subprocess.call(['nscd', '-i', 'passwd'])
-            subprocess.check_call(
-                ['su', '-', username, '-c', "''"])
-        except subprocess.CalledProcessError:
-            if not os.path.isdir('/home/{0}'.format(username)):
-                logger.warning(m18n.n('user_home_creation_failed'),
-                               exc_info=1)
 
-        # Create group for user and add to group 'all_users'
+    try:
-        user_group_create(groupname=username, gid=uid, primary_group=True, sync_perm=False)
+        # Attempt to create user home folder
-        user_group_update(groupname='all_users', add=username, force=True, sync_perm=True)
+        subprocess.check_call(
+            ['su', '-', username, '-c', "''"])
+    except subprocess.CalledProcessError:
+        if not os.path.isdir('/home/{0}'.format(username)):
+            logger.warning(m18n.n('user_home_creation_failed'),
+                           exc_info=1)
 
-        # TODO: Send a welcome mail to user
+    # Create group for user and add to group 'all_users'
-        logger.success(m18n.n('user_created'))
+    user_group_create(groupname=username, gid=uid, primary_group=True, sync_perm=False)
+    user_group_update(groupname='all_users', add=username, force=True, sync_perm=True)
 
-        hook_callback('post_user_create',
+    # TODO: Send a welcome mail to user
-                      args=[username, mail, password, firstname, lastname])
+    logger.success(m18n.n('user_created'))
 
-        return {'fullname': fullname, 'username': username, 'mail': mail}
+    hook_callback('post_user_create',
+                  args=[username, mail, password, firstname, lastname])
 
-    raise YunohostError('user_creation_failed')
+    return {'fullname': fullname, 'username': username, 'mail': mail}
 
 
 @is_unit_operation([('username', 'user')])
@@ -258,15 +260,17 @@ def user_delete(operation_logger, username, purge=False):
     user_group_delete(username, force=True, sync_perm=True)
 
     ldap = _get_ldap_interface()
-    if ldap.remove('uid=%s,ou=users' % username):
+    try:
-        # Invalidate passwd to take user deletion into account
+        ldap.remove('uid=%s,ou=users' % username)
-        subprocess.call(['nscd', '-i', 'passwd'])
+    except Exception as e:
+        raise YunohostError('user_deletion_failed', user=username, error=e)
 
-        if purge:
+    # Invalidate passwd to take user deletion into account
-            subprocess.call(['rm', '-rf', '/home/{0}'.format(username)])
+    subprocess.call(['nscd', '-i', 'passwd'])
-            subprocess.call(['rm', '-rf', '/var/mail/{0}'.format(username)])
+
-    else:
+    if purge:
-        raise YunohostError('user_deletion_failed')
+        subprocess.call(['rm', '-rf', '/home/{0}'.format(username)])
+        subprocess.call(['rm', '-rf', '/var/mail/{0}'.format(username)])
 
     hook_callback('post_user_delete', args=[username, purge])
 
@@ -387,12 +391,14 @@ def user_update(operation_logger, username, firstname=None, lastname=None, mail=
 
     operation_logger.start()
 
-    if ldap.update('uid=%s,ou=users' % username, new_attr_dict):
+    try:
-        logger.success(m18n.n('user_updated'))
+        ldap.update('uid=%s,ou=users' % username, new_attr_dict)
-        app_ssowatconf()
+    except Exception as e:
-        return user_info(username)
+        raise YunohostError('user_update_failed', user=username, error=e)
-    else:
+
-        raise YunohostError('user_update_failed')
+    logger.success(m18n.n('user_updated'))
+    app_ssowatconf()
+    return user_info(username)
 
 
 def user_info(username):
@@ -476,10 +482,7 @@ def user_info(username):
             'use': storage_use
         }
 
-    if result:
+    return result_dict
-        return result_dict
-    else:
-        raise YunohostError('user_info_failed')
 
 
 #
@@ -569,13 +572,16 @@ def user_group_create(operation_logger, groupname, gid=None, primary_group=False
         attr_dict["member"] = ["uid=" + groupname + ",ou=users,dc=yunohost,dc=org"]
 
     operation_logger.start()
-    if ldap.add('cn=%s,ou=groups' % groupname, attr_dict):
+    try:
-        logger.success(m18n.n('group_created', group=groupname))
+        ldap.add('cn=%s,ou=groups' % groupname, attr_dict)
-        if sync_perm:
+    except Exception as e:
-            permission_sync_to_user()
+        raise YunohostError('group_creation_failed', group=groupname, error=e)
-        return {'name': groupname}
 
-    raise YunohostError('group_creation_failed', group=groupname)
+    if sync_perm:
+        permission_sync_to_user()
+
+    logger.success(m18n.n('group_created', group=groupname))
+    return {'name': groupname}
 
 
 @is_unit_operation([('groupname', 'group')])
@@ -601,13 +607,16 @@ def user_group_delete(operation_logger, groupname, force=False, sync_perm=True):
 
     operation_logger.start()
     ldap = _get_ldap_interface()
-    if not ldap.remove('cn=%s,ou=groups' % groupname):
+    try:
-        raise YunohostError('group_deletion_failed', group=groupname)
+        ldap.remove('cn=%s,ou=groups' % groupname)
+    except Exception as e:
+        raise YunohostError('group_deletion_failed', group=groupname, error=e)
 
-    logger.success(m18n.n('group_deleted', group=groupname))
     if sync_perm:
         permission_sync_to_user()
 
+    logger.success(m18n.n('group_deleted', group=groupname))
+
 
 @is_unit_operation([('groupname', 'group')])
 def user_group_update(operation_logger, groupname, add=None, remove=None, force=False, sync_perm=True):
@@ -668,8 +677,10 @@ def user_group_update(operation_logger, groupname, add=None, remove=None, force=
     if set(new_group) != set(current_group):
         operation_logger.start()
         ldap = _get_ldap_interface()
-        if not ldap.update('cn=%s,ou=groups' % groupname, {"member": set(new_group_dns), "memberUid": set(new_group)}):
+        try:
-            raise YunohostError('group_update_failed', group=groupname)
+            ldap.update('cn=%s,ou=groups' % groupname, {"member": set(new_group_dns), "memberUid": set(new_group)})
+        except Exception as e:
+            raise YunohostError('group_update_failed', group=groupname, error=e)
 
     logger.success(m18n.n('group_updated', group=groupname))
     if sync_perm: