From 32b6c2eccfdecc16269c32cb4fc916fbcf5d4b52 Mon Sep 17 00:00:00 2001 From: Kay0u Date: Fri, 29 Nov 2019 20:39:18 +0900 Subject: [PATCH 1/5] Visitors permission needs All Users --- data/helpers.d/setting | 2 +- locales/en.json | 2 +- src/yunohost/app.py | 6 +++--- src/yunohost/permission.py | 18 +++++++++++++----- src/yunohost/tests/test_backuprestore.py | 4 ++-- src/yunohost/tests/test_permission.py | 8 ++++---- 6 files changed, 24 insertions(+), 16 deletions(-) diff --git a/data/helpers.d/setting b/data/helpers.d/setting index f0963444a..93386c9dd 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 b3e79ca92..71e17a7c8 100644 --- a/locales/en.json +++ b/locales/en.json @@ -471,7 +471,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 0e4a473b4..5ccca1394 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -959,12 +959,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() @@ -1194,7 +1194,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_register_url(app, domain, path): diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 0b88254ce..90f1bb9f0 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 48fe7f5d8..d6bc1bb31 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(mocker): 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(mocker): 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") From 9697ca8e4b730b3c9ad1187d8fdce8b0f03d14ee Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 29 Nov 2019 15:51:43 +0100 Subject: [PATCH 2/5] Let's convert this in list in all cases (+ simplify later core) --- src/yunohost/permission.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 90f1bb9f0..6603d6bdc 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -140,11 +140,11 @@ 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 "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 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') @@ -261,15 +261,14 @@ def permission_create(operation_logger, permission, url=None, allowed=None, sync attr_dict['URL'] = url if allowed is not None: + if not isinstance(allowed, list): + allowed = [allowed] 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 - for group in allowed_: + for group in allowed or []: if group not in all_existing_groups: raise YunohostError('group_unknown', group=group) From 7247c9f7aa7c3870e41e9954f93e52284322010c Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 29 Nov 2019 16:06:26 +0100 Subject: [PATCH 3/5] Let's keep one thread per line for readability + easier to indentify the issue --- src/yunohost/tests/test_backuprestore.py | 6 ++++-- src/yunohost/tests/test_permission.py | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/yunohost/tests/test_backuprestore.py b/src/yunohost/tests/test_backuprestore.py index d6bc1bb31..bcba21bb6 100644 --- a/src/yunohost/tests/test_backuprestore.py +++ b/src/yunohost/tests/test_backuprestore.py @@ -510,7 +510,8 @@ def test_backup_and_restore_permission_app(mocker): assert res['permissions_app.admin']['url'] == "/admin" assert res['permissions_app.dev']['url'] == "/dev" - assert "visitors" in res['permissions_app.main']['allowed'] and "all_users" in res['permissions_app.main']['allowed'] + assert "visitors" in res['permissions_app.main']['allowed'] + assert "all_users" in res['permissions_app.main']['allowed'] assert res['permissions_app.admin']['allowed'] == ["alice"] assert res['permissions_app.dev']['allowed'] == [] @@ -524,7 +525,8 @@ def test_backup_and_restore_permission_app(mocker): assert res['permissions_app.admin']['url'] == "/admin" assert res['permissions_app.dev']['url'] == "/dev" - assert "visitors" in res['permissions_app.main']['allowed'] and "all_users" in res['permissions_app.main']['allowed'] + assert "visitors" in res['permissions_app.main']['allowed'] + assert "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 f11d215f1..a4bd570e5 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -460,7 +460,8 @@ 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 "visitors" in res['permissions_app.main']['allowed'] and "all_users" in res['permissions_app.main']['allowed'] + assert "visitors" in res['permissions_app.main']['allowed'] + assert "all_users" in res['permissions_app.main']['allowed'] app_webroot = "https://%s/urlpermissionapp" % maindomain assert can_access_webpage(app_webroot, logged_as=None) @@ -491,7 +492,8 @@ 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 "visitors" in res['legacy_app.main']['allowed'] and "all_users" in res['legacy_app.main']['allowed'] + assert "visitors" in res['legacy_app.main']['allowed'] + assert "all_users" in res['legacy_app.main']['allowed'] app_webroot = "https://%s/legacy" % maindomain From 54a5ecebd763c8999f038c756e6ff915e110b80c Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 29 Nov 2019 16:41:50 +0100 Subject: [PATCH 4/5] Try to improve readability for these conditions --- src/yunohost/permission.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 6603d6bdc..75a00d6c5 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -138,11 +138,12 @@ def user_permission_update(operation_logger, permission, add=None, remove=None, 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 "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: + # 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 ? Note that it's fine to have ["all_users", "visitors"] + # though, but it's not fine to have ["all_users", "visitors", "volunteers"] + if "all_users" in new_allowed_groups and len(new_allowed_groups) >= 2: + 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. From 442cec170d68f7d068816695b45663d1b612185d Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Mon, 2 Dec 2019 19:04:07 +0100 Subject: [PATCH 5/5] Implicitly add all_users when adding visitors group --- locales/en.json | 3 ++- src/yunohost/permission.py | 12 +++++++++--- src/yunohost/tests/test_permission.py | 21 +++++++++++++++++++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/locales/en.json b/locales/en.json index 71e17a7c8..08fc6b74e 100644 --- a/locales/en.json +++ b/locales/en.json @@ -464,14 +464,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}", diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 75a00d6c5..5fe9f327f 100644 --- a/src/yunohost/permission.py +++ b/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): diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index a4bd570e5..2e53f13a7 100644 --- a/src/yunohost/tests/test_permission.py +++ b/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")