diff --git a/data/helpers.d/setting b/data/helpers.d/setting index 8046dfab4..9dbbe93fa 100644 --- a/data/helpers.d/setting +++ b/data/helpers.d/setting @@ -189,7 +189,7 @@ EOF # We need this because app temporarily set the app as unprotected to configure it with curl... if [[ "$3" =~ ^(unprotected|skipped)_ ]] && [[ "${4:-}" == "/" ]] then - ynh_permission_update --permission "main" --remove "all_users" --add "visitors" + ynh_permission_update --permission "main" --add "visitors" fi } diff --git a/locales/en.json b/locales/en.json index 6c7d0b42d..d93d2b5f4 100644 --- a/locales/en.json +++ b/locales/en.json @@ -422,7 +422,7 @@ "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_currently_allowed_for_visitors": "This permission is currently granted to visitors in addition to other groups. You probably want to either remove the 'visitors' permission or remove the other groups it is currently granted to.", + "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}", diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 2d32fc0cc..063d0f97d 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -1169,12 +1169,12 @@ def _migrate_legacy_permissions(app): # If the current permission says app is protected, but there are legacy rules saying it should be public... if app_perm_currently_allowed == ["all_users"] and settings_say_it_should_be_public: # Make it public - user_permission_update(app + ".main", remove="all_users", add="visitors", sync_perm=False) + user_permission_update(app + ".main", add="visitors", sync_perm=False) # If the current permission says app is public, but there are no setting saying it should be public... if app_perm_currently_allowed == ["visitors"] and not settings_say_it_should_be_public: # Make is private - user_permission_update(app + ".main", remove="visitors", add="all_users", sync_perm=False) + user_permission_update(app + ".main", remove="visitors", sync_perm=False) @is_unit_operation() @@ -1423,7 +1423,7 @@ def app_setting(app, key, value=None, delete=False): # We need this because app temporarily set the app as unprotected to configure it with curl... if key.startswith("unprotected_") or key.startswith("skipped_") and value == "/": from permission import user_permission_update - user_permission_update(app + ".main", remove="all_users", add="visitors") + user_permission_update(app + ".main", add="visitors") def app_checkport(port): diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index ad06c0487..011e8265d 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -140,12 +140,14 @@ def user_permission_update(operation_logger, permission, add=None, remove=None, # 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: - if "all_users" in new_allowed_groups: + + if "all_users" in new_allowed_groups: + if len(new_allowed_groups) > 1 and "visitors" not in new_allowed_groups or len(new_allowed_groups) > 2: logger.warning(m18n.n("permission_currently_allowed_for_all_users")) - if "visitors" in new_allowed_groups: - logger.warning(m18n.n("permission_currently_allowed_for_visitors")) + + # 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') # Don't update LDAP if we update exactly the same values if set(new_allowed_groups) == set(current_allowed_groups): @@ -258,6 +260,12 @@ def permission_create(operation_logger, permission, url=None, allowed=None, sync if url: attr_dict['URL'] = url + if allowed is not None: + if "visitors" in allowed and "all_users" not in allowed: + if not isinstance(allowed, list): + allowed = [allowed] + allowed.append("all_users") + # Validate that the groups to add actually exist all_existing_groups = user_group_list()['groups'].keys() allowed_ = [] if allowed is None else [allowed] if not isinstance(allowed, list) else allowed diff --git a/src/yunohost/tests/test_backuprestore.py b/src/yunohost/tests/test_backuprestore.py index d67ccfe37..3662226e1 100644 --- a/src/yunohost/tests/test_backuprestore.py +++ b/src/yunohost/tests/test_backuprestore.py @@ -510,7 +510,7 @@ def test_backup_and_restore_permission_app(): assert res['permissions_app.admin']['url'] == "/admin" assert res['permissions_app.dev']['url'] == "/dev" - assert res['permissions_app.main']['allowed'] == ["visitors"] + assert "visitors" in res['permissions_app.main']['allowed'] and "all_users" in res['permissions_app.main']['allowed'] assert res['permissions_app.admin']['allowed'] == ["alice"] assert res['permissions_app.dev']['allowed'] == [] @@ -524,7 +524,7 @@ def test_backup_and_restore_permission_app(): assert res['permissions_app.admin']['url'] == "/admin" assert res['permissions_app.dev']['url'] == "/dev" - assert res['permissions_app.main']['allowed'] == ["visitors"] + assert "visitors" in res['permissions_app.main']['allowed'] and "all_users" in res['permissions_app.main']['allowed'] assert res['permissions_app.admin']['allowed'] == ["alice"] assert res['permissions_app.dev']['allowed'] == [] diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index b3fa9fefb..f11d215f1 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -460,13 +460,13 @@ def test_permission_app_propagation_on_ssowat(): args="domain=%s&path=%s&is_public=1&admin=%s" % (maindomain, "/urlpermissionapp", "alice"), force=True) res = user_permission_list(full=True)['permissions'] - assert res['permissions_app.main']['allowed'] == ["visitors"] + assert "visitors" in res['permissions_app.main']['allowed'] and "all_users" in res['permissions_app.main']['allowed'] app_webroot = "https://%s/urlpermissionapp" % maindomain assert can_access_webpage(app_webroot, logged_as=None) assert can_access_webpage(app_webroot, logged_as="alice") - user_permission_update("permissions_app.main", remove="visitors", add="bob") + user_permission_update("permissions_app.main", remove=["visitors", "all_users"], add="bob") res = user_permission_list(full=True)['permissions'] assert not can_access_webpage(app_webroot, logged_as=None) @@ -491,7 +491,7 @@ def test_permission_legacy_app_propagation_on_ssowat(): # App is configured as public by default using the legacy unprotected_uri mechanics # It should automatically be migrated during the install res = user_permission_list(full=True)['permissions'] - assert res['legacy_app.main']['allowed'] == ["visitors"] + assert "visitors" in res['legacy_app.main']['allowed'] and "all_users" in res['legacy_app.main']['allowed'] app_webroot = "https://%s/legacy" % maindomain @@ -499,7 +499,7 @@ def test_permission_legacy_app_propagation_on_ssowat(): assert can_access_webpage(app_webroot, logged_as="alice") # Try to update the permission and check that permissions are still consistent - user_permission_update("legacy_app.main", remove="visitors", add="bob") + user_permission_update("legacy_app.main", remove=["visitors", "all_users"], add="bob") assert not can_access_webpage(app_webroot, logged_as=None) assert not can_access_webpage(app_webroot, logged_as="alice")