Implicitly add all_users when adding visitors group

locales/en.json

@@ -415,14 +415,15 @@
     "pattern_positive_number": "Must be a positive number",
     "pattern_username": "Must be lower-case alphanumeric and underscore characters only",
     "pattern_password_app": "Sorry, passwords can not contain the following characters: {forbidden_chars}",
+    "permission_all_users_implicitly_added": "The permission was also implicitly granted to 'all_users' because it is required to allow the special group 'visitors'",
     "permission_already_allowed": "Group '{group}' already has permission '{permission}' enabled",
     "permission_already_disallowed": "Group '{group}' already has permission '{permission}' disabled'",
     "permission_already_exist": "Permission '{permission}' already exists",
     "permission_already_up_to_date": "The permission was not updated because the addition/removal requests already match the current state.",
+    "permission_cannot_remove_all_users_while_visitors_allowed": "You can't remove this permission for 'all_users' while it is still allowed for 'visitors'",
     "permission_cannot_remove_main": "Removing a main permission is not allowed",
     "permission_created": "Permission '{permission:s}' created",
     "permission_creation_failed": "Could not create permission '{permission}': {error}",
-    "permission_allowed_for_visitors_but_not_for_all_users": "Visitors can't be granted if all users is not already granted.",
     "permission_currently_allowed_for_all_users": "This permission is currently granted to all users in addition to other groups. You probably want to either remove the 'all_users' permission or remove the other groups it is currently granted to.",
     "permission_deleted": "Permission '{permission:s}' deleted",
     "permission_deletion_failed": "Could not delete permission '{permission}': {error}",

src/yunohost/permission.py

@@ -146,9 +146,15 @@ def user_permission_update(operation_logger, permission, add=None, remove=None,
         if "visitors" not in new_allowed_groups or len(new_allowed_groups) >= 3:
             logger.warning(m18n.n("permission_currently_allowed_for_all_users"))
 
-    # If visitors are allowed, but not all users, it can break some applications, so we prohibit it.
-    if "visitors" in new_allowed_groups and "all_users" not in new_allowed_groups:
-        raise YunohostError('permission_allowed_for_visitors_but_not_for_all_users')
+    # If visitors are to be added, we shall make sure that "all_users" are also allowed
+    # (e.g. if visitors are allowed to visit nextcloud, you still want to allow people to log in ...)
+    if add and "visitors" in groups_to_add and "all_users" not in new_allowed_groups:
+        new_allowed_groups.append("all_users")
+        logger.warning(m18n.n("permission_all_users_implicitly_added"))
+    # If all_users are to be added, yet visitors are still to allowed, then we
+    # refuse it (c.f. previous comment...)
+    if remove and "all_users" in groups_to_remove and "visitors" in new_allowed_groups:
+        raise YunohostError('permission_cannot_remove_all_users_while_visitors_allowed')
 
     # Don't update LDAP if we update exactly the same values
     if set(new_allowed_groups) == set(current_allowed_groups):

src/yunohost/tests/test_permission.py

@@ -313,6 +313,27 @@ def test_permission_add_and_remove_group(mocker):
     assert res['wiki.main']['corresponding_users'] == ["alice"]
 
 
+def test_permission_adding_visitors_implicitly_add_all_users(mocker):
+
+    res = user_permission_list(full=True)['permissions']
+    assert res['blog.main']['allowed'] == ["alice"]
+
+    with message(mocker, "permission_updated", permission="blog.main"):
+        user_permission_update("blog.main", add="visitors")
+
+    res = user_permission_list(full=True)['permissions']
+    assert set(res['blog.main']['allowed']) == set(["alice", "visitors", "all_users"])
+
+
+def test_permission_cant_remove_all_users_if_visitors_allowed(mocker):
+
+    with message(mocker, "permission_updated", permission="blog.main"):
+        user_permission_update("blog.main", add=["visitors", "all_users"])
+
+    with raiseYunohostError(mocker, 'permission_cannot_remove_all_users_while_visitors_allowed'):
+        user_permission_update("blog.main", remove="all_users")
+
+
 def test_permission_add_group_already_allowed(mocker):
     with message(mocker, "permission_already_allowed", permission="blog.main", group="alice"):
         user_permission_update("blog.main", add="alice")