From cc59501b55b269747be983aacc392f9d99aa8522 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sun, 15 Sep 2019 16:59:34 +0200 Subject: [PATCH 01/26] Naive implementation of visitors group (without any relation to the ssowat conf yet) --- data/other/ldap_scheme.yml | 6 ++++++ locales/en.json | 3 +++ .../0011_setup_group_permission.py | 4 ++++ src/yunohost/permission.py | 13 +++++++++---- src/yunohost/user.py | 15 ++++++++++----- 5 files changed, 32 insertions(+), 9 deletions(-) diff --git a/data/other/ldap_scheme.yml b/data/other/ldap_scheme.yml index caa8fffb2..660d6fbb5 100644 --- a/data/other/ldap_scheme.yml +++ b/data/other/ldap_scheme.yml @@ -57,6 +57,12 @@ children: objectClass: - posixGroup - groupOfNamesYnh + cn=visitors,ou=groups: + cn: visitors + gidNumber: "4003" + objectClass: + - posixGroup + - groupOfNamesYnh depends_children: cn=mail.main,ou=permission: diff --git a/locales/en.json b/locales/en.json index ae349edf3..5df21b684 100644 --- a/locales/en.json +++ b/locales/en.json @@ -230,6 +230,9 @@ "group_already_exist_on_system": "Group {group} already exists in the system group", "group_created": "Group '{group}' successfully created", "group_creation_failed": "Failed to create group {group}: {error}", + "group_cannot_edit_all_users": "The group 'all_users' cannot be edited manually. It is a special group meant to contain all users registered in Yunohost", + "group_cannot_edit_visitors": "The group 'visitors' cannot be edited manually. It is a special group representing anonymous visitors", + "group_cannot_edit_primary_group": "The group '{group}' cannot be edited manually. It is the primary group meant to contain only one specific user.", "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", diff --git a/src/yunohost/data_migrations/0011_setup_group_permission.py b/src/yunohost/data_migrations/0011_setup_group_permission.py index 8949239e0..b3e11cb14 100644 --- a/src/yunohost/data_migrations/0011_setup_group_permission.py +++ b/src/yunohost/data_migrations/0011_setup_group_permission.py @@ -63,6 +63,7 @@ class MyMigration(Migration): self.remove_if_exists("cn=sftpusers,ou=groups") self.remove_if_exists("ou=permission") self.remove_if_exists('cn=all_users,ou=groups') + self.remove_if_exists('cn=visitors,ou=groups') attr_dict = ldap_map['parents']['ou=permission'] ldap.add('ou=permission', attr_dict) @@ -70,6 +71,9 @@ class MyMigration(Migration): attr_dict = ldap_map['children']['cn=all_users,ou=groups'] ldap.add('cn=all_users,ou=groups', attr_dict) + attr_dict = ldap_map['children']['cn=visitors,ou=groups'] + ldap.add('cn=visitors,ou=groups', attr_dict) + for rdn, attr_dict in ldap_map['depends_children'].items(): ldap.add(rdn, attr_dict) except Exception as e: diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 1472f4b88..dbfc6e6f5 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -142,10 +142,15 @@ def user_permission_update(operation_logger, permission, add=None, remove=None, # 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 : i18n - # 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.") + if len(new_allowed_groups) > 1: + if "all_users" in new_allowed_groups: + # FIXME : i18n + # 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 other groups currently allowed.") + if "visitors" in new_allowed_groups: + # FIXME : i18n + # FIXME : write a better explanation ? + logger.warning("This permission is currently enabled for visitors in addition to other groups. You probably want to either remove the 'visitors' permission or remove the other groups currently 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/user.py b/src/yunohost/user.py index c6413d7e1..581354f77 100644 --- a/src/yunohost/user.py +++ b/src/yunohost/user.py @@ -635,7 +635,7 @@ def user_group_delete(operation_logger, groupname, force=False, sync_perm=True): # # We also can't delete "all_users" because that's a special group... existing_users = user_list()['users'].keys() - undeletable_groups = existing_users + ["all_users", "admins"] + undeletable_groups = existing_users + ["all_users", "visitors"] if groupname in undeletable_groups and not force: raise YunohostError('group_cannot_be_deleted', group=groupname) @@ -670,13 +670,18 @@ def user_group_update(operation_logger, groupname, add=None, remove=None, force= from yunohost.permission import permission_sync_to_user from yunohost.utils.ldap import _get_ldap_interface + existing_users = user_list()['users'].keys() + # Refuse to edit a primary group of a user (e.g. group 'sam' related to user 'sam') # Those kind of group should only ever contain the user (e.g. sam) and only this one. # We also can't edit "all_users" without the force option because that's a special group... - existing_users = user_list()['users'].keys() - uneditable_groups = existing_users + ["all_users", "admins"] - if groupname in uneditable_groups and not force: - raise YunohostError('group_cannot_be_edited', group=groupname) + if not force: + if groupname == "all_users": + raise YunohostError('group_cannot_edit_all_users') + elif groupname == "all_users": + raise YunohostError('group_cannot_edit_visitors') + elif groupname in existing_users: + raise YunohostError('group_cannot_edit_primary_group', group=groupname) # We extract the uid for each member of the group to keep a simple flat list of members current_group = user_group_info(groupname)["members"] From 95a8dfa71c22103152a9241bc508de96f04cfe19 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sun, 15 Sep 2019 16:59:44 +0200 Subject: [PATCH 02/26] Simplify part of app_ssowatconf --- src/yunohost/app.py | 49 ++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index ab290cb4d..a9c91aaf5 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -41,7 +41,7 @@ from datetime import datetime from moulinette import msignals, m18n, msettings from moulinette.utils.log import getActionLogger -from moulinette.utils.filesystem import read_json, read_toml +from moulinette.utils.filesystem import read_json, read_toml, read_yaml from yunohost.service import service_log, service_status, _run_service_command from yunohost.utils import packages @@ -1366,34 +1366,29 @@ def app_ssowatconf(): return s.split(',') if s else [] for app in apps_list: - with open(APPS_SETTING_PATH + app['id'] + '/settings.yml') as f: - app_settings = yaml.load(f) - if 'no_sso' in app_settings: - continue + app_settings = read_yaml(APPS_SETTING_PATH + app['id'] + '/settings.yml') - for item in _get_setting(app_settings, 'skipped_uris'): - if item[-1:] == '/': - item = item[:-1] - skipped_urls.append(app_settings['domain'] + app_settings['path'].rstrip('/') + item) - for item in _get_setting(app_settings, 'skipped_regex'): - skipped_regex.append(item) - for item in _get_setting(app_settings, 'unprotected_uris'): - if item[-1:] == '/': - item = item[:-1] - unprotected_urls.append(app_settings['domain'] + app_settings['path'].rstrip('/') + item) - for item in _get_setting(app_settings, 'unprotected_regex'): - unprotected_regex.append(item) - for item in _get_setting(app_settings, 'protected_uris'): - if item[-1:] == '/': - item = item[:-1] - protected_urls.append(app_settings['domain'] + app_settings['path'].rstrip('/') + item) - for item in _get_setting(app_settings, 'protected_regex'): - protected_regex.append(item) - if 'redirected_urls' in app_settings: - redirected_urls.update(app_settings['redirected_urls']) - if 'redirected_regex' in app_settings: - redirected_regex.update(app_settings['redirected_regex']) + if 'no_sso' in app_settings: + continue + + app_root_webpath = app_settings['domain'] + app_settings['path'].rstrip('/') + + # Skipped + skipped_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'skipped_uris')] + skipped_regex += _get_setting(app_settings, 'skipped_regex') + + # Unprotected + unprotected_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'unprotected_uris')] + unprotected_regex += _get_setting(app_settings, 'unprotected_regex') + + # Protected + unprotected_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'protected_uris')] + unprotected_regex += _get_setting(app_settings, 'protected_regex') + + # Redirected + redirected_urls.update(app_settings.get('redirected_urls', {})) + redirected_regex.update(app_settings.get('redirected_regex', {})) for domain in domains: skipped_urls.extend([domain + '/yunohost/admin', domain + '/yunohost/api']) From 8abfd2a6e60bfd53d6fb398261e002e223a0c217 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sun, 15 Sep 2019 17:58:41 +0200 Subject: [PATCH 03/26] Naive implementation of protected/unprotected inplementation using the visitors group --- src/yunohost/app.py | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index a9c91aaf5..52371ff29 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -1345,6 +1345,7 @@ def app_ssowatconf(): main_domain = _get_maindomain() domains = domain_list()['domains'] + all_permissions = user_permission_list(full=True)['permissions'] skipped_urls = [] skipped_regex = [] @@ -1378,18 +1379,32 @@ def app_ssowatconf(): skipped_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'skipped_uris')] skipped_regex += _get_setting(app_settings, 'skipped_regex') - # Unprotected - unprotected_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'unprotected_uris')] - unprotected_regex += _get_setting(app_settings, 'unprotected_regex') - - # Protected - unprotected_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'protected_uris')] - unprotected_regex += _get_setting(app_settings, 'protected_regex') - # Redirected redirected_urls.update(app_settings.get('redirected_urls', {})) redirected_regex.update(app_settings.get('redirected_regex', {})) + # Legacy permission system using (un)protected_uris and _regex managed in app settings... + unprotected_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'unprotected_uris')] + unprotected_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'protected_uris')] + unprotected_regex += _get_setting(app_settings, 'unprotected_regex') + unprotected_regex += _get_setting(app_settings, 'protected_regex') + + # New permission system + this_app_perms = {name: info for name, info in all_permissions.items if name.startswith(app + ".")} + for perm_name, perm_info in this_app_perms: + urls = [url.rstrip("/") for url in perm_info["urls"]] + if "visitors" in perm_info["allowed"]: + unprotected_urls += urls + + # Legacy stuff : we remove now unprotected-urls that might have been declared as protected earlier... + protected_urls = [u for u in protected_urls if u not in urls] + else: + # TODO : small optimization to implement : we don't need to explictly add all the app roots + protected_urls += urls + + # Legacy stuff : we remove now unprotected-urls that might have been declared as protected earlier... + unprotected_urls = [u for u in unprotected_urls if u not in urls] + for domain in domains: skipped_urls.extend([domain + '/yunohost/admin', domain + '/yunohost/api']) @@ -1397,8 +1412,10 @@ def app_ssowatconf(): skipped_regex.append("^[^/]*/%.well%-known/acme%-challenge/.*$") skipped_regex.append("^[^/]*/%.well%-known/autoconfig/mail/config%-v1%.1%.xml.*$") + + permissions_per_url = {} - for permission_name, permission_infos in user_permission_list(full=True)['permissions'].items(): + for permission_name, permission_infos in all_permissions.items(): for url in permission_infos["urls"]: permissions_per_url[url] = permission_infos['corresponding_users'] From c4743398e687738a8b55bd500f219f7a69afe28e Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sun, 15 Sep 2019 18:10:58 +0200 Subject: [PATCH 04/26] Deprecate (un)protected_uris and _regex settings + more explicit deprecation warning for app_add/remove/clearaccess --- data/helpers.d/setting | 4 +++- src/yunohost/app.py | 8 ++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/data/helpers.d/setting b/data/helpers.d/setting index 502da1ed7..d083ed563 100644 --- a/data/helpers.d/setting +++ b/data/helpers.d/setting @@ -176,6 +176,8 @@ else: elif action == "set": if key in ['redirected_urls', 'redirected_regex']: value = yaml.load(value) + if key in ["unprotected_uris", "unprotected_regex", "protected_uris", "protected_regex"]: + logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please delete these legacy settings and use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage public/private access.") settings[key] = value else: raise ValueError("action should either be get, set or delete") @@ -249,7 +251,7 @@ ynh_permission_create() { # Remove a permission for the app (note that when the app is removed all permission is automatically removed) # -# usage: ynh_permission_remove --permission "permission" +# usage: ynh_permission_delete --permission "permission" # | arg: permission - the name for the permission (by default a permission named "main" is removed automatically when the app is removed) # # example: ynh_permission_delete --permission editors diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 52371ff29..8b0c99d46 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -1037,6 +1037,8 @@ def app_addaccess(apps, users=[]): """ from yunohost.permission import user_permission_update + logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage permissions.") + output = {} for app in apps: permission = user_permission_update(app+".main", add=users) @@ -1056,6 +1058,8 @@ def app_removeaccess(apps, users=[]): """ from yunohost.permission import user_permission_update + logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage permissions.") + output = {} for app in apps: permission = user_permission_update(app+".main", remove=users) @@ -1074,6 +1078,8 @@ def app_clearaccess(apps): """ from yunohost.permission import user_permission_reset + logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage permissions.") + output = {} for app in apps: permission = user_permission_reset(app+".main") @@ -1181,6 +1187,8 @@ def app_setting(app, key, value=None, delete=False): # FIXME: Allow multiple values for some keys? if key in ['redirected_urls', 'redirected_regex']: value = yaml.load(value) + if key in ["unprotected_uris", "unprotected_regex", "protected_uris", "protected_regex"]: + logger.warning("/!\ Packagers ! This app is using the legacy permission system. Please delete these legacy settings and use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage public/private access.") app_settings[key] = value _set_app_settings(app, app_settings) From b2a26a64a74d2f7289d857b7bb780ac2f91741ce Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sun, 15 Sep 2019 18:33:31 +0200 Subject: [PATCH 05/26] Naively migrate legacy and classical unprotected_uris = / that sets the app as public --- src/yunohost/app.py | 8 +++++++- .../data_migrations/0011_setup_group_permission.py | 6 ++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 8b0c99d46..537616e68 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -733,7 +733,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu from yunohost.hook import hook_add, hook_remove, hook_exec, hook_callback from yunohost.log import OperationLogger - from yunohost.permission import user_permission_list, permission_create, permission_urls, permission_delete, permission_sync_to_user + from yunohost.permission import user_permission_list, permission_create, permission_urls, permission_delete, permission_sync_to_user, user_permission_update # Fetch or extract sources if not os.path.exists(INSTALL_TMP): @@ -952,7 +952,13 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu domain = app_settings.get('domain', None) path = app_settings.get('path', None) if domain and path: + # FIXME : might want to move this to before running the install script because some app need to run install script during initialization etc (idk) ? permission_urls(app_instance_name+".main", add=[domain+path], sync_perm=False) + + # Migrate classic public app still using the legacy unprotected_uris + if app_settings.get("unprotected_uris", None) == "/": + user_permission_update(app_instance_name+".main", remove="all_users", add="visitors", sync_perm=False) + permission_sync_to_user() logger.success(m18n.n('installation_complete')) diff --git a/src/yunohost/data_migrations/0011_setup_group_permission.py b/src/yunohost/data_migrations/0011_setup_group_permission.py index b3e11cb14..c79d80e0c 100644 --- a/src/yunohost/data_migrations/0011_setup_group_permission.py +++ b/src/yunohost/data_migrations/0011_setup_group_permission.py @@ -113,6 +113,12 @@ class MyMigration(Migration): user_permission_update(app+".main", remove="all_users", add=allowed_group, sync_perm=False) app_setting(app, 'allowed_users', delete=True) + # Migrate classic public app still using the legacy unprotected_uris + if app_setting(app, "unprotected_uris") == "/": + user_permission_update(app+".main", remove="all_users", add="visitors", sync_perm=False) + + permission_sync_to_user() + def run(self): # FIXME : what do we really want to do here ... From 821a3ac4ff0f3180ff5b5884f020c02b3a982b34 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sun, 15 Sep 2019 18:53:25 +0200 Subject: [PATCH 06/26] Draft tests to check that permissions are actually propagated and effective on the SSO --- src/yunohost/tests/test_permission.py | 55 ++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index 94728505d..1c81e015f 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -333,7 +333,7 @@ def test_permission_remove_url_not_added(): def test_permission_app_install(): app_install("./tests/apps/permissions_app_ynh", - args="domain=%s&path=%s&admin=%s" % (maindomain, "/urlpermissionapp", "alice"), force=True) + args="domain=%s&path=%s&is_public=0&admin=%s" % (maindomain, "/urlpermissionapp", "alice"), force=True) res = user_permission_list(full=True)['permissions'] assert "permissions_app.main" in res @@ -361,7 +361,7 @@ def test_permission_app_install(): def test_permission_app_remove(): app_install("./tests/apps/permissions_app_ynh", - args="domain=%s&path=%s&admin=%s" % (maindomain, "/urlpermissionapp", "alice"), force=True) + args="domain=%s&path=%s&is_public=0&admin=%s" % (maindomain, "/urlpermissionapp", "alice"), force=True) app_remove("permissions_app") # Check all permissions for this app got deleted @@ -383,3 +383,54 @@ def test_permission_app_change_url(): assert res['permissions_app.main']['urls'] == [maindomain + "/newchangeurl"] assert res['permissions_app.admin']['urls'] == [maindomain + "/newchangeurl/admin"] assert res['permissions_app.dev']['urls'] == [maindomain + "/newchangeurl/dev"] + + +def test_permission_app_propagation_on_ssowat(): + + # TODO / FIXME : To be actually implemented later .... + raise NotImplementedError + + app_install("./tests/apps/permissions_app_ynh", + 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'] == ["all_users"] + + assert can_access(maindomain + "/urlpermissionapp", logged_as=None) + assert can_access(maindomain + "/urlpermissionapp", logged_as="alice") + + user_permission_update("permissions_app.main", remove="visitors", add="bob") + res = user_permission_list(full=True)['permissions'] + + assert cannot_access(maindomain + "/urlpermissionapp", logged_as=None) + assert cannot_access(maindomain + "/urlpermissionapp", logged_as="alice") + assert can_access(maindomain + "/urlpermissionapp", logged_as="bob") + + # Test admin access, as configured during install, only alice should be able to access it + + assert cannot_access(maindomain + "/urlpermissionapp/admin", logged_as=None) + assert cannot_access(maindomain + "/urlpermissionapp/admin", logged_as="alice") + assert can_access(maindomain + "/urlpermissionapp/admin", logged_as="bob") + +def test_permission_legacy_app_propagation_on_ssowat(): + + # TODO / FIXME : To be actually implemented later .... + raise NotImplementedError + + app_install("./tests/apps/legacy_app_ynh", + args="domain=%s&path=%s" % (maindomain, "/legacy"), force=True) + + # App is configured as public by default using the legacy unprotected_uri mechanics + # It should automatically be migrated during the install + assert res['permissions_app.main']['allowed'] == ["visitors"] + + assert can_access(maindomain + "/legacy", logged_as=None) + assert can_access(maindomain + "/legacy", 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") + res = user_permission_list(full=True)['permissions'] + + assert cannot_access(maindomain + "/legacy", logged_as=None) + assert cannot_access(maindomain + "/legacy", logged_as="alice") + assert can_access(maindomain + "/legacy", logged_as="bob") From 64e388fa7d952690c3f35d8b13f67d52869bb383 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 17 Sep 2019 23:38:17 +0200 Subject: [PATCH 07/26] Implement helper function to test if we're able to access a webpage being logged in (or not) as user --- src/yunohost/tests/test_permission.py | 56 ++++++++++++++++++++------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index 1c81e015f..51bf6a4c6 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -1,3 +1,4 @@ +import requests import pytest from yunohost.app import app_install, app_remove, app_change_url, app_list, app_map @@ -11,6 +12,7 @@ from yunohost.utils.error import YunohostError # Get main domain maindomain = _get_maindomain() +dummy_password = "test123Ynh" def clean_user_groups_permission(): for u in user_list()['users']: @@ -27,8 +29,8 @@ def clean_user_groups_permission(): def setup_function(function): clean_user_groups_permission() - user_create("alice", "Alice", "White", "alice@" + maindomain, "test123Ynh") - user_create("bob", "Bob", "Snow", "bob@" + maindomain, "test123Ynh") + user_create("alice", "Alice", "White", "alice@" + maindomain, dummy_password) + user_create("bob", "Bob", "Snow", "bob@" + maindomain, dummy_password) permission_create("wiki.main", urls=[maindomain + "/wiki"], sync_perm=False) permission_create("blog.main", sync_perm=False) user_permission_update("blog.main", remove="all_users", add="alice") @@ -156,6 +158,30 @@ def check_permission_for_apps(): assert installed_apps == app_perms_prefix + +def can_access_webpage(webpath, logged_as=None): + + webpath = webpath.rstrip("/") + webroot = webpath.rsplit("/", 1)[0] + sso_url = webroot+"/yunohost/sso" + + # Anonymous access + if not logged_as: + r = requests.get(webpath, verify=False) + # Login as a user using dummy password + else: + with requests.Session() as session: + session.post(sso_url, + data={"user": logged_as, + "password": dummy_password}, + headers={"Referer": sso_url, + "Content-Type": "application/x-www-form-urlencoded"}, + verify=False) + r = session.get(webpath, verify=False) + + # If we can't access it, we got redirected to the sso + return not r.url.startswith(sso_url) + # # List functions # @@ -396,21 +422,21 @@ def test_permission_app_propagation_on_ssowat(): res = user_permission_list(full=True)['permissions'] assert res['permissions_app.main']['allowed'] == ["all_users"] - assert can_access(maindomain + "/urlpermissionapp", logged_as=None) - assert can_access(maindomain + "/urlpermissionapp", logged_as="alice") + assert can_access_webpage(maindomain + "/urlpermissionapp", logged_as=None) + assert can_access_webpage(maindomain + "/urlpermissionapp", logged_as="alice") user_permission_update("permissions_app.main", remove="visitors", add="bob") res = user_permission_list(full=True)['permissions'] - assert cannot_access(maindomain + "/urlpermissionapp", logged_as=None) - assert cannot_access(maindomain + "/urlpermissionapp", logged_as="alice") - assert can_access(maindomain + "/urlpermissionapp", logged_as="bob") + assert not can_access_webpage(maindomain + "/urlpermissionapp", logged_as=None) + assert not can_access_webpage(maindomain + "/urlpermissionapp", logged_as="alice") + assert can_access_webpage(maindomain + "/urlpermissionapp", logged_as="bob") # Test admin access, as configured during install, only alice should be able to access it - assert cannot_access(maindomain + "/urlpermissionapp/admin", logged_as=None) - assert cannot_access(maindomain + "/urlpermissionapp/admin", logged_as="alice") - assert can_access(maindomain + "/urlpermissionapp/admin", logged_as="bob") + assert not can_access_webpage(maindomain + "/urlpermissionapp/admin", logged_as=None) + assert not can_access_webpage(maindomain + "/urlpermissionapp/admin", logged_as="alice") + assert can_access_webpage(maindomain + "/urlpermissionapp/admin", logged_as="bob") def test_permission_legacy_app_propagation_on_ssowat(): @@ -424,13 +450,13 @@ def test_permission_legacy_app_propagation_on_ssowat(): # It should automatically be migrated during the install assert res['permissions_app.main']['allowed'] == ["visitors"] - assert can_access(maindomain + "/legacy", logged_as=None) - assert can_access(maindomain + "/legacy", logged_as="alice") + assert can_access_webpage(maindomain + "/legacy", logged_as=None) + assert can_access_webpage(maindomain + "/legacy", 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") res = user_permission_list(full=True)['permissions'] - assert cannot_access(maindomain + "/legacy", logged_as=None) - assert cannot_access(maindomain + "/legacy", logged_as="alice") - assert can_access(maindomain + "/legacy", logged_as="bob") + assert not can_access_webpage(maindomain + "/legacy", logged_as=None) + assert not can_access_webpage(maindomain + "/legacy", logged_as="alice") + assert can_access_webpage(maindomain + "/legacy", logged_as="bob") From 00795a7a0156d5d45aeabb63819ab3d6511270e5 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 18 Sep 2019 18:38:47 +0200 Subject: [PATCH 08/26] Make migration re-run even more robust --- src/yunohost/data_migrations/0011_setup_group_permission.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/yunohost/data_migrations/0011_setup_group_permission.py b/src/yunohost/data_migrations/0011_setup_group_permission.py index c79d80e0c..a99dfb7c1 100644 --- a/src/yunohost/data_migrations/0011_setup_group_permission.py +++ b/src/yunohost/data_migrations/0011_setup_group_permission.py @@ -62,12 +62,14 @@ class MyMigration(Migration): try: self.remove_if_exists("cn=sftpusers,ou=groups") self.remove_if_exists("ou=permission") - self.remove_if_exists('cn=all_users,ou=groups') - self.remove_if_exists('cn=visitors,ou=groups') + self.remove_if_exists('ou=groups') attr_dict = ldap_map['parents']['ou=permission'] ldap.add('ou=permission', attr_dict) + attr_dict = ldap_map['parents']['ou=groups'] + ldap.add('ou=groups', attr_dict) + attr_dict = ldap_map['children']['cn=all_users,ou=groups'] ldap.add('cn=all_users,ou=groups', attr_dict) From 8d01a816f3cd0fd07f570cd2d9f290d01f100ed9 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 18 Sep 2019 18:39:05 +0200 Subject: [PATCH 09/26] Typo fixes following tests --- src/yunohost/app.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 537616e68..7938d6786 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -1404,8 +1404,8 @@ def app_ssowatconf(): unprotected_regex += _get_setting(app_settings, 'protected_regex') # New permission system - this_app_perms = {name: info for name, info in all_permissions.items if name.startswith(app + ".")} - for perm_name, perm_info in this_app_perms: + this_app_perms = {name: info for name, info in all_permissions.items() if name.startswith(app['id'] + ".")} + for perm_name, perm_info in this_app_perms.items(): urls = [url.rstrip("/") for url in perm_info["urls"]] if "visitors" in perm_info["allowed"]: unprotected_urls += urls @@ -1414,6 +1414,7 @@ def app_ssowatconf(): protected_urls = [u for u in protected_urls if u not in urls] else: # TODO : small optimization to implement : we don't need to explictly add all the app roots + protected_urls += urls # Legacy stuff : we remove now unprotected-urls that might have been declared as protected earlier... From 87050276b4a217acea21b7f45820bdfaead1194d Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 19 Sep 2019 19:26:41 +0200 Subject: [PATCH 10/26] Finish to implement first visitor test + fixes following test ... --- src/yunohost/app.py | 12 +++++------ src/yunohost/tests/test_permission.py | 29 +++++++++++++-------------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 7938d6786..bba5fb104 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -429,8 +429,10 @@ def app_map(app=None, raw=False, user=None): continue if 'no_sso' in app_settings: # I don't think we need to check for the value here continue - if user and user not in permissions[app_id + ".main"]["corresponding_users"]: - continue + if user: + main_perm = permissions[app_id + ".main"] + if user not in main_perm["corresponding_users"] and "visitors" not in main_perm["allowed"]: + continue domain = app_settings['domain'] path = app_settings['path'] @@ -2613,10 +2615,8 @@ def _parse_args_in_yunohost_format(args, action_args): if arg_value not in domain_list()['domains']: raise YunohostError('app_argument_invalid', name=arg_name, error=m18n.n('domain_unknown')) elif arg_type == 'user': - try: - user_info(arg_value) - except YunohostError as e: - raise YunohostError('app_argument_invalid', name=arg_name, error=e) + if not arg_value in user_list()["users"].keys(): + raise YunohostError('app_argument_invalid', name=arg_name, error=m18n.n('user_unknown', user=arg_value)) elif arg_type == 'app': if not _is_installed(arg_value): raise YunohostError('app_argument_invalid', name=arg_name, error=m18n.n('app_unknown')) diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index 51bf6a4c6..bef042be1 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -19,7 +19,7 @@ def clean_user_groups_permission(): user_delete(u) for g in user_group_list()['groups']: - if g != "all_users": + if g not in ["all_users", "visitors"]: user_group_delete(g) for p in user_permission_list()['permissions']: @@ -162,8 +162,7 @@ def check_permission_for_apps(): def can_access_webpage(webpath, logged_as=None): webpath = webpath.rstrip("/") - webroot = webpath.rsplit("/", 1)[0] - sso_url = webroot+"/yunohost/sso" + sso_url = "https://"+maindomain+"/yunohost/sso/" # Anonymous access if not logged_as: @@ -177,6 +176,8 @@ def can_access_webpage(webpath, logged_as=None): headers={"Referer": sso_url, "Content-Type": "application/x-www-form-urlencoded"}, verify=False) + # We should have some cookies related to authentication now + assert session.cookies r = session.get(webpath, verify=False) # If we can't access it, we got redirected to the sso @@ -413,30 +414,28 @@ def test_permission_app_change_url(): def test_permission_app_propagation_on_ssowat(): - # TODO / FIXME : To be actually implemented later .... - raise NotImplementedError - app_install("./tests/apps/permissions_app_ynh", 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'] == ["all_users"] + assert res['permissions_app.main']['allowed'] == ["visitors"] - assert can_access_webpage(maindomain + "/urlpermissionapp", logged_as=None) - assert can_access_webpage(maindomain + "/urlpermissionapp", logged_as="alice") + 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") res = user_permission_list(full=True)['permissions'] - assert not can_access_webpage(maindomain + "/urlpermissionapp", logged_as=None) - assert not can_access_webpage(maindomain + "/urlpermissionapp", logged_as="alice") - assert can_access_webpage(maindomain + "/urlpermissionapp", logged_as="bob") + assert not can_access_webpage(app_webroot, logged_as=None) + assert not can_access_webpage(app_webroot, logged_as="alice") + assert can_access_webpage(app_webroot, logged_as="bob") # Test admin access, as configured during install, only alice should be able to access it - assert not can_access_webpage(maindomain + "/urlpermissionapp/admin", logged_as=None) - assert not can_access_webpage(maindomain + "/urlpermissionapp/admin", logged_as="alice") - assert can_access_webpage(maindomain + "/urlpermissionapp/admin", logged_as="bob") + assert not can_access_webpage(app_webroot+"/admin", logged_as=None) + assert can_access_webpage(app_webroot+"/admin", logged_as="alice") + assert not can_access_webpage(app_webroot+"/admin", logged_as="bob") def test_permission_legacy_app_propagation_on_ssowat(): From ebf2fb9a141da65954d1855bc025f0cf362c6f99 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 20 Sep 2019 20:13:51 +0200 Subject: [PATCH 11/26] Use relative urls by default for permissions while still supporting absolute urls ... --- src/yunohost/app.py | 18 +++++------- .../0011_setup_group_permission.py | 2 +- src/yunohost/permission.py | 29 ++++++++++--------- 3 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index bba5fb104..231568439 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -553,8 +553,6 @@ def app_change_url(operation_logger, app, domain, path): app_setting(app, 'domain', value=domain) app_setting(app, 'path', value=path) - permission_urls(app+".main", add=[domain+path], remove=[old_domain+old_path], sync_perm=True) - # avoid common mistakes if _run_service_command("reload", "nginx") is False: # grab nginx errors @@ -868,10 +866,9 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu if os.path.exists(os.path.join(extracted_app_folder, file_to_copy)): os.system('cp -R %s/%s %s' % (extracted_app_folder, file_to_copy, app_setting_path)) - # Create permission before the install (useful if the install script redefine the permission) - # Note that sync_perm is disabled to avoid triggering a whole bunch of code and messages - # can't be sure that we don't have one case when it's needed - permission_create(app_instance_name+".main", sync_perm=False) + # Initialize the main permission for the app + # After the install, if apps don't have a domain and path defined, the default url '/' is removed from the permission + permission_create(app_instance_name+".main", urls=["/"]) # Execute the app install script install_retcode = 1 @@ -949,17 +946,16 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu os.system('chown -R root: %s' % app_setting_path) os.system('chown -R admin: %s/scripts' % app_setting_path) - # Add path in permission if it's defined in the app install script + # If an app doesn't have at least a domain and a path, assume it's not a webapp and remove the default "/" permission app_settings = _get_app_settings(app_instance_name) domain = app_settings.get('domain', None) path = app_settings.get('path', None) - if domain and path: - # FIXME : might want to move this to before running the install script because some app need to run install script during initialization etc (idk) ? - permission_urls(app_instance_name+".main", add=[domain+path], sync_perm=False) + if not (domain and path): + permission_urls(app_instance_name + ".main", remove=["/"], sync_perm=False) # Migrate classic public app still using the legacy unprotected_uris if app_settings.get("unprotected_uris", None) == "/": - user_permission_update(app_instance_name+".main", remove="all_users", add="visitors", sync_perm=False) + user_permission_update(app_instance_name + ".main", remove="all_users", add="visitors", sync_perm=False) permission_sync_to_user() diff --git a/src/yunohost/data_migrations/0011_setup_group_permission.py b/src/yunohost/data_migrations/0011_setup_group_permission.py index a99dfb7c1..dd5b3c274 100644 --- a/src/yunohost/data_migrations/0011_setup_group_permission.py +++ b/src/yunohost/data_migrations/0011_setup_group_permission.py @@ -108,7 +108,7 @@ class MyMigration(Migration): path = app_setting(app, 'path') domain = app_setting(app, 'domain') - urls = [domain + path] if domain and path else None + urls = "/" if domain and path else None permission_create(app+".main", urls=urls, sync_perm=False) if permission: allowed_group = permission.split(',') diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index dbfc6e6f5..5f9a88e11 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -268,7 +268,18 @@ def permission_create(operation_logger, permission, urls=None, sync_perm=True): Keyword argument: permission -- Name of the permission (e.g. mail or nextcloud or wordpress.editors) - urls -- list of urls to specify for the permission + urls -- list of urls to specify for the permission. + + Urls are assumed to be relative to the app domain/path if they start with '/'. + For example: + / -> domain.tld/app + /admin -> domain.tld/app/admin + domain.tld/app/api -> domain.tld/app/api + + Urls can be later treated as regexes when they start with "re:". + For example: + re:/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ + re:domain.tld/app/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ """ from yunohost.utils.ldap import _get_ldap_interface @@ -302,7 +313,7 @@ def permission_create(operation_logger, permission, urls=None, sync_perm=True): attr_dict['groupPermission'] = ['cn=all_users,ou=groups,dc=yunohost,dc=org'] if urls: - attr_dict['URL'] = [_normalize_url(url) for url in urls] + attr_dict['URL'] = urls operation_logger.related_to.append(('app', permission.split(".")[0])) operation_logger.start() @@ -326,8 +337,8 @@ def permission_urls(operation_logger, permission, add=None, remove=None, sync_pe Keyword argument: permission -- Name of the permission (e.g. mail or nextcloud or wordpress.editors) - add -- List of urls to add - remove -- List of urls to remove + add -- List of urls to add (c.f. permission_create for documentation about their format) + remove -- List of urls to remove (c.f. permission_create for documentation about their format) """ from yunohost.utils.ldap import _get_ldap_interface @@ -345,11 +356,9 @@ def permission_urls(operation_logger, permission, add=None, remove=None, sync_pe if add: urls_to_add = [add] if not isinstance(add, list) else add - urls_to_add = [_normalize_url(url) for url in urls_to_add] new_urls += urls_to_add if remove: urls_to_remove = [remove] if not isinstance(remove, list) else remove - urls_to_remove = [_normalize_url(url) for url in urls_to_remove] new_urls = [u for u in new_urls if u not in urls_to_remove] if set(new_urls) == set(existing_permission["urls"]): @@ -457,11 +466,3 @@ def permission_sync_to_user(): # Reload unscd, otherwise the group ain't propagated to the LDAP database 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('/')] - path = url[url.index('/'):] - domain, path = _normalize_domain_path(domain, path) - return domain + path From 2b51d247fb5a1a66c152c13a6ad83e97fdaceee3 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 20 Sep 2019 20:14:14 +0200 Subject: [PATCH 12/26] Propagate changes on app helpers + tests --- data/helpers.d/setting | 22 +++++++++--- src/yunohost/tests/test_backuprestore.py | 12 +++---- src/yunohost/tests/test_permission.py | 45 ++++++++++++------------ 3 files changed, 47 insertions(+), 32 deletions(-) diff --git a/data/helpers.d/setting b/data/helpers.d/setting index d083ed563..9deba2dc0 100644 --- a/data/helpers.d/setting +++ b/data/helpers.d/setting @@ -232,11 +232,24 @@ ynh_webpath_register () { # Create a new permission for the app # +# example: ynh_permission_create --permission admin --urls /admin +# # usage: ynh_permission_create --permission "permission" [--urls "url" ["url" ...]] # | arg: permission - the name for the permission (by default a permission named "main" already exist) # | arg: urls - (optional) a list of FULL urls for the permission (e.g. domain.tld/apps/admin) +# | arg: urls - (optional) a list of urls to specify for the permission. +# +# Urls are assumed to be relative to the app domain/path if they start with '/'. +# For example: +# / -> domain.tld/app +# /admin -> domain.tld/app/admin +# domain.tld/app/api -> domain.tld/app/api +# +# Urls can be treated as regexes when they start with "re:". +# For example: +# re:/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ +# re:domain.tld/app/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ # -# example: ynh_permission_create --permission admin --urls domain.tld/blog/admin ynh_permission_create() { declare -Ar args_array=( [p]=permission= [u]=urls= ) local permission @@ -251,10 +264,11 @@ ynh_permission_create() { # Remove a permission for the app (note that when the app is removed all permission is automatically removed) # +# example: ynh_permission_delete --permission editors +# # usage: ynh_permission_delete --permission "permission" # | arg: permission - the name for the permission (by default a permission named "main" is removed automatically when the app is removed) # -# example: ynh_permission_delete --permission editors ynh_permission_delete() { declare -Ar args_array=( [p]=permission= ) local permission @@ -267,8 +281,8 @@ ynh_permission_delete() { # # usage: ynh_permission_urls --permission "permission" --add "url" ["url" ...] --remove "url" ["url" ...] # | arg: permission - the name for the permission (by default a permission named "main" is removed automatically when the app is removed) -# | arg: add - (optional) a list of FULL urls to add to the permission (e.g. domain.tld/apps/admin) -# | arg: remove - (optional) a list of FULL urls to remove from the permission (e.g. other.tld/apps/admin) +# | arg: add - (optional) a list of urls to add to the permission (see permission_create for details regarding their format) +# | arg: remove - (optional) a list of urls to remove from the permission (see permission_create for details regarding their format) # ynh_permission_urls() { declare -Ar args_array=([p]=permission= [a]=add= [r]=remove=) diff --git a/src/yunohost/tests/test_backuprestore.py b/src/yunohost/tests/test_backuprestore.py index bdaf25299..cab98089b 100644 --- a/src/yunohost/tests/test_backuprestore.py +++ b/src/yunohost/tests/test_backuprestore.py @@ -529,9 +529,9 @@ def test_backup_and_restore_permission_app(): assert "permissions_app.main" in res assert "permissions_app.admin" in res assert "permissions_app.dev" in res - assert res['permissions_app.main']['urls'] == [maindomain + "/urlpermissionapp"] - assert res['permissions_app.admin']['urls'] == [maindomain + "/urlpermissionapp/admin"] - assert res['permissions_app.dev']['urls'] == [maindomain + "/urlpermissionapp/dev"] + assert res['permissions_app.main']['urls'] == ["/"] + assert res['permissions_app.admin']['urls'] == ["/admin"] + assert res['permissions_app.dev']['urls'] == ["/dev"] assert res['permissions_app.main']['allowed'] == ["all_users"] assert res['permissions_app.admin']['allowed'] == ["alice"] @@ -543,9 +543,9 @@ def test_backup_and_restore_permission_app(): assert "permissions_app.main" in res assert "permissions_app.admin" in res assert "permissions_app.dev" in res - assert res['permissions_app.main']['urls'] == [maindomain + "/urlpermissionapp"] - assert res['permissions_app.admin']['urls'] == [maindomain + "/urlpermissionapp/admin"] - assert res['permissions_app.dev']['urls'] == [maindomain + "/urlpermissionapp/dev"] + assert res['permissions_app.main']['urls'] == ["/"] + assert res['permissions_app.admin']['urls'] == ["/admin"] + assert res['permissions_app.dev']['urls'] == ["/dev"] assert res['permissions_app.main']['allowed'] == ["all_users"] assert res['permissions_app.admin']['allowed'] == ["alice"] diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index bef042be1..a8b6b8258 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -31,7 +31,7 @@ def setup_function(function): user_create("alice", "Alice", "White", "alice@" + maindomain, dummy_password) user_create("bob", "Bob", "Snow", "bob@" + maindomain, dummy_password) - permission_create("wiki.main", urls=[maindomain + "/wiki"], sync_perm=False) + permission_create("wiki.main", urls=["/"], sync_perm=False) permission_create("blog.main", sync_perm=False) user_permission_update("blog.main", remove="all_users", add="alice") @@ -198,7 +198,7 @@ def test_permission_list(): assert res['blog.main']['allowed'] == ["alice"] assert set(res['wiki.main']['corresponding_users']) == set(["alice", "bob"]) assert res['blog.main']['corresponding_users'] == ["alice"] - assert res['wiki.main']['urls'] == [maindomain + "/wiki"] + assert res['wiki.main']['urls'] == ["/"] # # Create - Remove functions @@ -322,37 +322,37 @@ def test_permission_update_permission_that_doesnt_exist(): # Permission url management def test_permission_add_url(): - permission_urls("blog.main", add=[maindomain + "/testA"]) + permission_urls("blog.main", add=["/testA"]) res = user_permission_list(full=True)['permissions'] - assert res["blog.main"]["urls"] == [maindomain + "/testA"] + assert res["blog.main"]["urls"] == ["/testA"] -def test_permission_add_second_url(): - permission_urls("wiki.main", add=[maindomain + "/testA"]) +def test_permission_add_another_url(): + permission_urls("wiki.main", add=["/testA"]) res = user_permission_list(full=True)['permissions'] - assert set(res["wiki.main"]["urls"]) == set([maindomain + "/testA", maindomain + "/wiki"]) + assert set(res["wiki.main"]["urls"]) == set(["/", "/testA"]) def test_permission_remove_url(): - permission_urls("wiki.main", remove=[maindomain + "/wiki"]) + permission_urls("wiki.main", remove=["/"]) res = user_permission_list(full=True)['permissions'] assert res["wiki.main"]["urls"] == [] def test_permission_add_url_already_added(): res = user_permission_list(full=True)['permissions'] - assert res["wiki.main"]["urls"] == [maindomain + "/wiki"] + assert res["wiki.main"]["urls"] == ["/"] - permission_urls("wiki.main", add=[maindomain + "/wiki"]) + permission_urls("wiki.main", add=["/"]) res = user_permission_list(full=True)['permissions'] - assert res["wiki.main"]["urls"] == [maindomain + "/wiki"] + assert res["wiki.main"]["urls"] == ["/"] def test_permission_remove_url_not_added(): - permission_urls("wiki.main", remove=[maindomain + "/doesnt_exist"]) + permission_urls("wiki.main", remove=["/doesnt_exist"]) res = user_permission_list(full=True)['permissions'] - assert res['wiki.main']['urls'] == [maindomain + "/wiki"] + assert res['wiki.main']['urls'] == ["/"] # # Application interaction @@ -366,9 +366,9 @@ def test_permission_app_install(): assert "permissions_app.main" in res assert "permissions_app.admin" in res assert "permissions_app.dev" in res - assert res['permissions_app.main']['urls'] == [maindomain + "/urlpermissionapp"] - assert res['permissions_app.admin']['urls'] == [maindomain + "/urlpermissionapp/admin"] - assert res['permissions_app.dev']['urls'] == [maindomain + "/urlpermissionapp/dev"] + assert res['permissions_app.main']['urls'] == ["/"] + assert res['permissions_app.admin']['urls'] == ["/admin"] + assert res['permissions_app.dev']['urls'] == ["/dev"] assert res['permissions_app.main']['allowed'] == ["all_users"] assert set(res['permissions_app.main']['corresponding_users']) == set(["alice", "bob"]) @@ -399,17 +399,18 @@ def test_permission_app_change_url(): app_install("./tests/apps/permissions_app_ynh", args="domain=%s&path=%s&admin=%s" % (maindomain, "/urlpermissionapp", "alice"), force=True) + # FIXME : should rework this test to look for differences in the generated app map / app tiles ... res = user_permission_list(full=True)['permissions'] - assert res['permissions_app.main']['urls'] == [maindomain + "/urlpermissionapp"] - assert res['permissions_app.admin']['urls'] == [maindomain + "/urlpermissionapp/admin"] - assert res['permissions_app.dev']['urls'] == [maindomain + "/urlpermissionapp/dev"] + assert res['permissions_app.main']['urls'] == ["/"] + assert res['permissions_app.admin']['urls'] == ["/admin"] + assert res['permissions_app.dev']['urls'] == ["/dev"] app_change_url("permissions_app", maindomain, "/newchangeurl") res = user_permission_list(full=True)['permissions'] - assert res['permissions_app.main']['urls'] == [maindomain + "/newchangeurl"] - assert res['permissions_app.admin']['urls'] == [maindomain + "/newchangeurl/admin"] - assert res['permissions_app.dev']['urls'] == [maindomain + "/newchangeurl/dev"] + assert res['permissions_app.main']['urls'] == ["/"] + assert res['permissions_app.admin']['urls'] == ["/admin"] + assert res['permissions_app.dev']['urls'] == ["/dev"] def test_permission_app_propagation_on_ssowat(): From 7102c5d0ca1ce36a3ff90e3fcb688d4a5be0d221 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 20 Sep 2019 20:14:44 +0200 Subject: [PATCH 13/26] Propagate the new relative url stuff to app_ssowatconf and actuall implement the whole permission system thing in app_map (related to ssowatconf) --- src/yunohost/app.py | 126 ++++++++++++++++++++++---- src/yunohost/tests/test_permission.py | 3 + 2 files changed, 113 insertions(+), 16 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 231568439..51030021f 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -427,25 +427,97 @@ def app_map(app=None, raw=False, user=None): if 'path' not in app_settings: # we assume that an app that doesn't have a path doesn't have an HTTP api continue + # This 'no_sso' settings sound redundant to not having $path defined .... + # At least from what I can see, all apps using it don't have a path defined ... if 'no_sso' in app_settings: # I don't think we need to check for the value here continue + # Users must at least have access to the main permission to have access to extra permissions if user: main_perm = permissions[app_id + ".main"] if user not in main_perm["corresponding_users"] and "visitors" not in main_perm["allowed"]: continue domain = app_settings['domain'] - path = app_settings['path'] + path = app_settings['path'].rstrip('/') + label = app_settings['label'] - if raw: - if domain not in result: - result[domain] = {} - result[domain][path] = { - 'label': app_settings['label'], - 'id': app_settings['id'] - } - else: - result[domain + path] = app_settings['label'] + def _sanitized_absolute_url(perm_url): + # Nominal case : url is relative to the app's path + if perm_url.startswith("/"): + perm_domain = domain + perm_path = path + perm_url.rstrip("/") + # Otherwise, the urls starts with a domain name, like domain.tld/foo/bar + # We want perm_domain = domain.tld and perm_path = "/foo/bar" + else: + perm_domain, perm_path = perm_url.split("/", 1) + perm_path = "/" + perm_path.rstrip("/") + + return perm_domain, perm_path + + this_app_perms = {p: i for p, i in permissions.items() if p.startswith(app_id + ".") and i["urls"]} + for perm_name, perm_info in this_app_perms.items(): + # If we're building the map for a specific user, check the user + # actually is allowed for this specific perm + if user and user not in perm_info["corresponding_users"] and "visitors" not in perm_info["allowed"]: + continue + if len(perm_info["urls"]) > 1 or perm_info["urls"][0].startswith("re:"): + # + # Here we have a big conceptual issue about the sso ... + # Let me take a sip of coffee and turn off the music... + # + # Let's say we have an app foo which created a permission + # 'foo.admin' and added as url "/admin" and "/api" This + # permission got defined somehow as only accessible for group + # "admins". So both "/admin" and "/api" are protected. Good! + # + # Now if we really want users in group "admins" to access those + # uris, then each users in group "admins" need to have these + # urls in the ssowat dict for this user. Which corresponds to a + # tile. To put it otherwise : in the current code of ssowat, a + # permission = a tile = a url ! + # + # We also have an issue if the url define is a regex, because + # the url we want to add to the dict is going to be turned into + # a clickable link (or analyzed by other parts of yunohost + # code...). To put it otherwise : in the current code of ssowat, + # you can't give access a user to a regex + # + # Instead, as drafted by Josue, we could rework the ssowat logic + # about how routes and their permissions are defined. So for example, + # have a dict of + # { "/route1": ["visitors", "user1", "user2", ...], # Public route + # "/route2_with_a_regex$": ["user1", "user2"], # Private route + # "/route3": None, # Skipped route idk + # } + # then each time a user try to request and url, we only keep the + # longest matching rule and check the user is allowed etc... + # + # The challenge with this is (beside actually implementing it) + # is that it creates a whole new mechanism that ultimately + # replace all the existing logic about + # protected/unprotected/skipped uris and regexes and we gotta + # handle / migrate all the legacy stuff somehow if we don't + # want to end up with a total mess in the future idk + logger.error("Permission %s can't be added to the SSOwat configuration because it uses multiple urls and/or uses a regex url" % perm_name) + continue + + perm_domain, perm_path = _sanitized_absolute_url(perm_info["urls"][0]) + + if perm_name.endswith(".main"): + perm_label = label + else: + # e.g. if perm_name is wordpress.admin, we want "Blog (Admin)" (where Blog is the label of this app) + perm_label = "%s (%s)" % (label, perm_name.rsplit(".")[-1].replace("_", " ").title()) + + if raw: + if domain not in result: + result[perm_domain] = {} + result[perm_domain][perm_path] = { + 'label': perm_label, + 'id': app_id + } + else: + result[perm_domain + perm_path] = perm_label return result @@ -1382,13 +1454,34 @@ def app_ssowatconf(): app_settings = read_yaml(APPS_SETTING_PATH + app['id'] + '/settings.yml') + if 'domain' not in app_settings: + continue + if 'path' not in app_settings: + continue + + # This 'no_sso' settings sound redundant to not having $path defined .... + # At least from what I can see, all apps using it don't have a path defined ... if 'no_sso' in app_settings: continue - app_root_webpath = app_settings['domain'] + app_settings['path'].rstrip('/') + domain = app_settings['domain'] + path = app_settings['path'].rstrip('/') + + def _sanitized_absolute_url(perm_url): + # Nominal case : url is relative to the app's path + if perm_url.startswith("/"): + perm_domain = domain + perm_path = path + perm_url.rstrip("/") + # Otherwise, the urls starts with a domain name, like domain.tld/foo/bar + # We want perm_domain = domain.tld and perm_path = "/foo/bar" + else: + perm_domain, perm_path = perm_url.split("/", 1) + perm_path = "/" + perm_path.rstrip("/") + + return perm_domain + perm_path # Skipped - skipped_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'skipped_uris')] + skipped_urls += [_sanitized_absolute_url(uri) for uri in _get_setting(app_settings, 'skipped_uris')] skipped_regex += _get_setting(app_settings, 'skipped_regex') # Redirected @@ -1396,15 +1489,16 @@ def app_ssowatconf(): redirected_regex.update(app_settings.get('redirected_regex', {})) # Legacy permission system using (un)protected_uris and _regex managed in app settings... - unprotected_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'unprotected_uris')] - unprotected_urls += [app_root_webpath + uri.rstrip("/") for uri in _get_setting(app_settings, 'protected_uris')] + unprotected_urls += [_sanitized_absolute_url(uri) for uri in _get_setting(app_settings, 'unprotected_uris')] + protected_urls += [_sanitized_absolute_url(uri) for uri in _get_setting(app_settings, 'protected_uris')] unprotected_regex += _get_setting(app_settings, 'unprotected_regex') - unprotected_regex += _get_setting(app_settings, 'protected_regex') + protected_regex += _get_setting(app_settings, 'protected_regex') # New permission system this_app_perms = {name: info for name, info in all_permissions.items() if name.startswith(app['id'] + ".")} for perm_name, perm_info in this_app_perms.items(): - urls = [url.rstrip("/") for url in perm_info["urls"]] + # FIXME : gotta handle regex-urls here... meh + urls = [_sanitized_absolute_url(url) for url in perm_info["urls"]] if "visitors" in perm_info["allowed"]: unprotected_urls += urls diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index a8b6b8258..4dc021496 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -434,6 +434,9 @@ def test_permission_app_propagation_on_ssowat(): # Test admin access, as configured during install, only alice should be able to access it + # alice gotta be allowed on the main permission to access the admin tho + user_permission_update("permissions_app.main", remove="bob", add="all_users") + assert not can_access_webpage(app_webroot+"/admin", logged_as=None) assert can_access_webpage(app_webroot+"/admin", logged_as="alice") assert not can_access_webpage(app_webroot+"/admin", logged_as="bob") From 2a5053b66bfc7bd48cd8f7668db1ee93b1103fe7 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 21 Sep 2019 13:32:40 +0200 Subject: [PATCH 14/26] Misc wording and orthotypography... MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Allan Nordhøy --- data/helpers.d/setting | 8 ++++---- locales/en.json | 2 +- src/yunohost/permission.py | 12 ++++++------ src/yunohost/tests/test_permission.py | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/data/helpers.d/setting b/data/helpers.d/setting index 9deba2dc0..b2a647993 100644 --- a/data/helpers.d/setting +++ b/data/helpers.d/setting @@ -177,7 +177,7 @@ else: if key in ['redirected_urls', 'redirected_regex']: value = yaml.load(value) if key in ["unprotected_uris", "unprotected_regex", "protected_uris", "protected_regex"]: - logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please delete these legacy settings and use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage public/private access.") + logger.warning("/!\\ Packagers! This app is using the legacy permission system. Please delete these legacy settings and use the new helpers 'ynh_permission_{create,urls,update,delete}' and the 'visitors' group to manage public/private access.") settings[key] = value else: raise ValueError("action should either be get, set or delete") @@ -237,15 +237,15 @@ ynh_webpath_register () { # usage: ynh_permission_create --permission "permission" [--urls "url" ["url" ...]] # | arg: permission - the name for the permission (by default a permission named "main" already exist) # | arg: urls - (optional) a list of FULL urls for the permission (e.g. domain.tld/apps/admin) -# | arg: urls - (optional) a list of urls to specify for the permission. +# | arg: urls - (optional) a list of URLs to specify for the permission. # -# Urls are assumed to be relative to the app domain/path if they start with '/'. +# URLs are assumed to be relative to the app domain/path if they start with '/'. # For example: # / -> domain.tld/app # /admin -> domain.tld/app/admin # domain.tld/app/api -> domain.tld/app/api # -# Urls can be treated as regexes when they start with "re:". +# URLs can be treated as regexes when they start with "re:". # For example: # re:/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ # re:domain.tld/app/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ diff --git a/locales/en.json b/locales/en.json index 5df21b684..70b9fa626 100644 --- a/locales/en.json +++ b/locales/en.json @@ -230,7 +230,7 @@ "group_already_exist_on_system": "Group {group} already exists in the system group", "group_created": "Group '{group}' successfully created", "group_creation_failed": "Failed to create group {group}: {error}", - "group_cannot_edit_all_users": "The group 'all_users' cannot be edited manually. It is a special group meant to contain all users registered in Yunohost", + "group_cannot_edit_all_users": "The group 'all_users' cannot be edited manually. It is a special group meant to contain all users registered in YunoHost", "group_cannot_edit_visitors": "The group 'visitors' cannot be edited manually. It is a special group representing anonymous visitors", "group_cannot_edit_primary_group": "The group '{group}' cannot be edited manually. It is the primary group meant to contain only one specific user.", "group_cannot_be_edited": "The group {group} cannot be edited manually.", diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 5f9a88e11..75e3f6037 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -146,11 +146,11 @@ def user_permission_update(operation_logger, permission, add=None, remove=None, if "all_users" in new_allowed_groups: # FIXME : i18n # 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 other groups currently allowed.") + logger.warning("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.") if "visitors" in new_allowed_groups: # FIXME : i18n # FIXME : write a better explanation ? - logger.warning("This permission is currently enabled for visitors in addition to other groups. You probably want to either remove the 'visitors' permission or remove the other groups currently allowed.") + logger.warning("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.") # Don't update LDAP if we update exactly the same values if set(new_allowed_groups) == set(current_allowed_groups): @@ -268,7 +268,7 @@ def permission_create(operation_logger, permission, urls=None, sync_perm=True): Keyword argument: permission -- Name of the permission (e.g. mail or nextcloud or wordpress.editors) - urls -- list of urls to specify for the permission. + urls -- list of URLs to specify for the permission. Urls are assumed to be relative to the app domain/path if they start with '/'. For example: @@ -276,7 +276,7 @@ def permission_create(operation_logger, permission, urls=None, sync_perm=True): /admin -> domain.tld/app/admin domain.tld/app/api -> domain.tld/app/api - Urls can be later treated as regexes when they start with "re:". + URLs can be later treated as regexes when they start with "re:". For example: re:/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ re:domain.tld/app/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ @@ -337,8 +337,8 @@ def permission_urls(operation_logger, permission, add=None, remove=None, sync_pe Keyword argument: permission -- Name of the permission (e.g. mail or nextcloud or wordpress.editors) - add -- List of urls to add (c.f. permission_create for documentation about their format) - remove -- List of urls to remove (c.f. permission_create for documentation about their format) + add -- List of URLs to add (c.f. permission_create for documentation about their format) + remove -- List of URLs to remove (c.f. permission_create for documentation about their format) """ from yunohost.utils.ldap import _get_ldap_interface diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index 4dc021496..f17313fa1 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -180,7 +180,7 @@ def can_access_webpage(webpath, logged_as=None): assert session.cookies r = session.get(webpath, verify=False) - # If we can't access it, we got redirected to the sso + # If we can't access it, we got redirected to the SSO return not r.url.startswith(sso_url) # From 35bfe97d50b1a6bb44f16ccddaf376723e304c57 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 25 Sep 2019 22:08:47 +0200 Subject: [PATCH 15/26] Copy pasta typo : all_users -> visitors Co-Authored-By: Josue-T --- src/yunohost/user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/yunohost/user.py b/src/yunohost/user.py index 581354f77..312b131b9 100644 --- a/src/yunohost/user.py +++ b/src/yunohost/user.py @@ -678,7 +678,7 @@ def user_group_update(operation_logger, groupname, add=None, remove=None, force= if not force: if groupname == "all_users": raise YunohostError('group_cannot_edit_all_users') - elif groupname == "all_users": + elif groupname == "visitors": raise YunohostError('group_cannot_edit_visitors') elif groupname in existing_users: raise YunohostError('group_cannot_edit_primary_group', group=groupname) From 4a14cbd6e0bc092c7bf3ed94f994d9330dc1c1a0 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 18:42:17 +0200 Subject: [PATCH 16/26] Fix / implement remaining test --- src/yunohost/tests/test_permission.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index f17313fa1..a9e16cfc6 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -41,6 +41,10 @@ def teardown_function(function): app_remove("permissions_app") except: pass + try: + app_remove("legacy_app") + except: + pass @pytest.fixture(autouse=True) def check_LDAP_db_integrity_call(): @@ -443,23 +447,22 @@ def test_permission_app_propagation_on_ssowat(): def test_permission_legacy_app_propagation_on_ssowat(): - # TODO / FIXME : To be actually implemented later .... - raise NotImplementedError - app_install("./tests/apps/legacy_app_ynh", args="domain=%s&path=%s" % (maindomain, "/legacy"), force=True) # App is configured as public by default using the legacy unprotected_uri mechanics # It should automatically be migrated during the install - assert res['permissions_app.main']['allowed'] == ["visitors"] + res = user_permission_list(full=True)['permissions'] + assert res['legacy_app.main']['allowed'] == ["visitors"] - assert can_access_webpage(maindomain + "/legacy", logged_as=None) - assert can_access_webpage(maindomain + "/legacy", logged_as="alice") + app_webroot = "https://%s/legacy" % maindomain + + assert can_access_webpage(app_webroot, logged_as=None) + 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") - res = user_permission_list(full=True)['permissions'] - assert not can_access_webpage(maindomain + "/legacy", logged_as=None) - assert not can_access_webpage(maindomain + "/legacy", logged_as="alice") - assert can_access_webpage(maindomain + "/legacy", logged_as="bob") + assert not can_access_webpage(app_webroot, logged_as=None) + assert not can_access_webpage(app_webroot, logged_as="alice") + assert can_access_webpage(app_webroot, logged_as="bob") From df49af0ad001b99086083798a2b6e888c9352a80 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 18:55:11 +0200 Subject: [PATCH 17/26] Redundant operation considering we're deleting all groups right after --- src/yunohost/data_migrations/0011_setup_group_permission.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/yunohost/data_migrations/0011_setup_group_permission.py b/src/yunohost/data_migrations/0011_setup_group_permission.py index dd5b3c274..ae5a8bfb9 100644 --- a/src/yunohost/data_migrations/0011_setup_group_permission.py +++ b/src/yunohost/data_migrations/0011_setup_group_permission.py @@ -60,7 +60,6 @@ class MyMigration(Migration): ldap_map = read_yaml('/usr/share/yunohost/yunohost-config/moulinette/ldap_scheme.yml') try: - self.remove_if_exists("cn=sftpusers,ou=groups") self.remove_if_exists("ou=permission") self.remove_if_exists('ou=groups') From 96bc95656c6ad29fa59ae337a0f9f4f04c097261 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 19:22:31 +0200 Subject: [PATCH 18/26] Allow the migration to proceed if slapd config was manually modified, warn the user about where the conf will be backuped --- locales/en.json | 2 +- src/yunohost/data_migrations/0011_setup_group_permission.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/locales/en.json b/locales/en.json index 5c3595782..3e1054069 100644 --- a/locales/en.json +++ b/locales/en.json @@ -346,7 +346,7 @@ "migration_0011_can_not_backup_before_migration": "The backup of the system before the migration failed. Migration failed. Error: {error:s}", "migration_0011_create_group": "Creating a group for each user…", "migration_0011_done": "Migration successful. You are now able to manage usergroups.", - "migration_0011_LDAP_config_dirty": "It look like that you customized your LDAP configuration. For this migration the LDAP configuration needs to be updated.\nYou need to save your current configuration, reintialize the original configuration by running 'yunohost tools regen-conf -f' and retry the migration", + "migration_0011_slapd_config_will_be_overwritten": "It looks like you manually edited the slapd configuration. For this critical migration, YunoHost needs to force the update of the slapd configuration. The original files will be backuped in {conf_backup_folder}.", "migration_0011_LDAP_update_failed": "Could not update LDAP. Error: {error:s}", "migration_0011_migrate_permission": "Migrating permissions from apps settings to LDAP…", "migration_0011_migration_failed_trying_to_rollback": "Migration failed… trying to roll back the system.", diff --git a/src/yunohost/data_migrations/0011_setup_group_permission.py b/src/yunohost/data_migrations/0011_setup_group_permission.py index ae5a8bfb9..de28a3ad7 100644 --- a/src/yunohost/data_migrations/0011_setup_group_permission.py +++ b/src/yunohost/data_migrations/0011_setup_group_permission.py @@ -9,7 +9,7 @@ from moulinette.utils.filesystem import read_yaml from yunohost.tools import Migration from yunohost.user import user_group_create, user_group_update from yunohost.app import app_setting, app_list -from yunohost.regenconf import regen_conf +from yunohost.regenconf import regen_conf, BACKUP_CONF_DIR from yunohost.permission import permission_create, user_permission_update, permission_sync_to_user logger = getActionLogger('yunohost.migration') @@ -130,7 +130,7 @@ class MyMigration(Migration): ldap_regen_conf_status = regen_conf(names=['slapd'], dry_run=True) # By this we check if the have been customized if ldap_regen_conf_status and ldap_regen_conf_status['slapd']['pending']: - raise YunohostError("migration_0011_LDAP_config_dirty") + logger.warning("migration_0011_slapd_config_will_be_overwritten", conf_backup_folder=BACKUP_CONF_DIR) # Backup LDAP and the apps settings before to do the migration logger.info(m18n.n("migration_0011_backup_before_migration")) From 9cecd71437d050696a5e98e676532c21e8396749 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 19:39:37 +0200 Subject: [PATCH 19/26] Fix permission_reset idempotency --- src/yunohost/permission.py | 4 ++++ src/yunohost/tests/test_permission.py | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 75e3f6037..97e5b4122 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -217,6 +217,10 @@ def user_permission_reset(operation_logger, permission, sync_perm=True): if existing_permission is None: raise YunohostError('permission_not_found', permission=permission) + if existing_permission["allowed"] == ["all_users"]: + logger.warning("The permission was not updated all addition/removal requests already match the current state.") + return + # Update permission with default (all_users) operation_logger.related_to.append(('app', permission.split(".")[0])) diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index a9e16cfc6..0ddda4cec 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -306,6 +306,17 @@ def test_permission_reset(): assert res['blog.main']['allowed'] == ["all_users"] assert set(res['blog.main']['corresponding_users']) == set(["alice", "bob"]) + +def test_permission_reset_idempotency(): + # Reset permission + user_permission_reset("blog.main") + user_permission_reset("blog.main") + + res = user_permission_list(full=True)['permissions'] + assert res['blog.main']['allowed'] == ["all_users"] + assert set(res['blog.main']['corresponding_users']) == set(["alice", "bob"]) + + # # Error on update function # From 88794805eba3ececc5bc04aac2d41b4d09241bf7 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 22:08:12 +0200 Subject: [PATCH 20/26] We probably don't need to have multiple urls per permissions ... --- data/helpers.d/setting | 62 ++++++++--------- locales/en.json | 2 +- src/yunohost/app.py | 68 ++++++++----------- src/yunohost/backup.py | 6 +- .../0011_setup_group_permission.py | 4 +- src/yunohost/permission.py | 38 ++++------- src/yunohost/tests/test_backuprestore.py | 16 ++--- src/yunohost/tests/test_permission.py | 56 +++++---------- 8 files changed, 105 insertions(+), 147 deletions(-) diff --git a/data/helpers.d/setting b/data/helpers.d/setting index b2a647993..c911c5811 100644 --- a/data/helpers.d/setting +++ b/data/helpers.d/setting @@ -232,34 +232,36 @@ ynh_webpath_register () { # Create a new permission for the app # -# example: ynh_permission_create --permission admin --urls /admin +# example: ynh_permission_create --permission admin --url /admin # -# usage: ynh_permission_create --permission "permission" [--urls "url" ["url" ...]] +# usage: ynh_permission_create --permission "permission" [--url "url"] # | arg: permission - the name for the permission (by default a permission named "main" already exist) -# | arg: urls - (optional) a list of FULL urls for the permission (e.g. domain.tld/apps/admin) -# | arg: urls - (optional) a list of URLs to specify for the permission. +# | arg: url - (optional) URL for which access will be allowed/forbidden # -# URLs are assumed to be relative to the app domain/path if they start with '/'. -# For example: -# / -> domain.tld/app -# /admin -> domain.tld/app/admin -# domain.tld/app/api -> domain.tld/app/api +# If provided, 'url' is assumed to be relative to the app domain/path if they +# start with '/'. For example: +# / -> domain.tld/app +# /admin -> domain.tld/app/admin +# domain.tld/app/api -> domain.tld/app/api # -# URLs can be treated as regexes when they start with "re:". -# For example: -# re:/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ -# re:domain.tld/app/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ +# 'url' can be later treated as a regex if it starts with "re:". +# For example: +# re:/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ +# re:domain.tld/app/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ # ynh_permission_create() { - declare -Ar args_array=( [p]=permission= [u]=urls= ) + declare -Ar args_array=( [p]=permission= [u]=url= ) local permission local urls ynh_handle_getopts_args "$@" - if [[ -n ${urls:-} ]]; then - urls=",urls=['${urls//';'/"','"}']" + if [[ -n ${url:-} ]]; then + url="'$url'" + else + url="None" fi - yunohost tools shell -c "from yunohost.permission import permission_create; permission_create('$app.$permission' ${urls:-}, sync_perm=False)" + + yunohost tools shell -c "from yunohost.permission import permission_create; permission_create('$app.$permission', url=$url, sync_perm=False)" } # Remove a permission for the app (note that when the app is removed all permission is automatically removed) @@ -277,30 +279,28 @@ ynh_permission_delete() { yunohost tools shell -c "from yunohost.permission import permission_delete; permission_delete('$app.$permission', sync_perm=False)" } -# Manage urls related to a permission +# Redefine the url associated to a permission # -# usage: ynh_permission_urls --permission "permission" --add "url" ["url" ...] --remove "url" ["url" ...] +# usage: ynh_permission_url --permission "permission" --url "url" # | arg: permission - the name for the permission (by default a permission named "main" is removed automatically when the app is removed) -# | arg: add - (optional) a list of urls to add to the permission (see permission_create for details regarding their format) -# | arg: remove - (optional) a list of urls to remove from the permission (see permission_create for details regarding their format) +# | arg: url - (optional) URL for which access will be allowed/forbidden # -ynh_permission_urls() { - declare -Ar args_array=([p]=permission= [a]=add= [r]=remove=) +ynh_permission_url() { + declare -Ar args_array=([p]=permission= [u]=url=) local permission - local add - local remove + local url ynh_handle_getopts_args "$@" - if [[ -n ${add:-} ]]; then - add=",add=['${add//';'/"','"}']" - fi - if [[ -n ${remove:-} ]]; then - remove=",remove=['${remove//';'/"','"}']" + if [[ -n ${url:-} ]]; then + url="'$url'" + else + url="None" fi - yunohost tools shell -c "from yunohost.permission import permission_urls; permission_urls('$app.$permission' ${add:-} ${remove:-})" + yunohost tools shell -c "from yunohost.permission import permission_url; permission_url('$app.$permission', url=$url)" } + # Update a permission for the app # # usage: ynh_permission_update --permission "permission" --add "group" ["group" ...] --remove "group" ["group" ...] diff --git a/locales/en.json b/locales/en.json index 3e1054069..e3911a334 100644 --- a/locales/en.json +++ b/locales/en.json @@ -268,7 +268,7 @@ "log_letsencrypt_cert_install": "Install a Let's encrypt certificate on '{}' domain", "log_permission_create": "Create permission '{}'", "log_permission_delete": "Delete permission '{}'", - "log_permission_urls": "Update urls related to permission '{}'", + "log_permission_url": "Update url related to permission '{}'", "log_selfsigned_cert_install": "Install self signed certificate on '{}' domain", "log_letsencrypt_cert_renew": "Renew '{}' Let's encrypt certificate", "log_regen_conf": "Regenerate system configurations '{}'", diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 4bda9ccf6..abb4387e5 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -454,33 +454,18 @@ def app_map(app=None, raw=False, user=None): return perm_domain, perm_path - this_app_perms = {p: i for p, i in permissions.items() if p.startswith(app_id + ".") and i["urls"]} + this_app_perms = {p: i for p, i in permissions.items() if p.startswith(app_id + ".") and i["url"]} for perm_name, perm_info in this_app_perms.items(): # If we're building the map for a specific user, check the user # actually is allowed for this specific perm if user and user not in perm_info["corresponding_users"] and "visitors" not in perm_info["allowed"]: continue - if len(perm_info["urls"]) > 1 or perm_info["urls"][0].startswith("re:"): - # - # Here we have a big conceptual issue about the sso ... - # Let me take a sip of coffee and turn off the music... - # - # Let's say we have an app foo which created a permission - # 'foo.admin' and added as url "/admin" and "/api" This - # permission got defined somehow as only accessible for group - # "admins". So both "/admin" and "/api" are protected. Good! - # - # Now if we really want users in group "admins" to access those - # uris, then each users in group "admins" need to have these - # urls in the ssowat dict for this user. Which corresponds to a - # tile. To put it otherwise : in the current code of ssowat, a - # permission = a tile = a url ! - # - # We also have an issue if the url define is a regex, because + if perm_info["url"].startswith("re:"): + # Here, we have an issue if the chosen url is a regex, because # the url we want to add to the dict is going to be turned into # a clickable link (or analyzed by other parts of yunohost # code...). To put it otherwise : in the current code of ssowat, - # you can't give access a user to a regex + # you can't give access a user to a regex. # # Instead, as drafted by Josue, we could rework the ssowat logic # about how routes and their permissions are defined. So for example, @@ -498,10 +483,10 @@ def app_map(app=None, raw=False, user=None): # protected/unprotected/skipped uris and regexes and we gotta # handle / migrate all the legacy stuff somehow if we don't # want to end up with a total mess in the future idk - logger.error("Permission %s can't be added to the SSOwat configuration because it uses multiple urls and/or uses a regex url" % perm_name) + logger.error("Permission %s can't be added to the SSOwat configuration because it doesn't support regexes so far..." % perm_name) continue - perm_domain, perm_path = _sanitized_absolute_url(perm_info["urls"][0]) + perm_domain, perm_path = _sanitized_absolute_url(perm_info["url"]) if perm_name.endswith(".main"): perm_label = label @@ -535,7 +520,6 @@ def app_change_url(operation_logger, app, domain, path): """ from yunohost.hook import hook_exec, hook_callback from yunohost.domain import _normalize_domain_path, _get_conflicting_apps - from yunohost.permission import permission_urls installed = _is_installed(app) if not installed: @@ -835,7 +819,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu from yunohost.hook import hook_add, hook_remove, hook_exec, hook_callback from yunohost.log import OperationLogger - from yunohost.permission import user_permission_list, permission_create, permission_urls, permission_delete, permission_sync_to_user, user_permission_update + from yunohost.permission import user_permission_list, permission_create, permission_url, permission_delete, permission_sync_to_user, user_permission_update # Fetch or extract sources if not os.path.exists(INSTALL_TMP): @@ -994,7 +978,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu # Initialize the main permission for the app # After the install, if apps don't have a domain and path defined, the default url '/' is removed from the permission - permission_create(app_instance_name+".main", urls=["/"]) + permission_create(app_instance_name+".main", url="/") # Execute the app install script install_retcode = 1 @@ -1088,7 +1072,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu domain = app_settings.get('domain', None) path = app_settings.get('path', None) if not (domain and path): - permission_urls(app_instance_name + ".main", remove=["/"], sync_perm=False) + permission_url(app_instance_name + ".main", url=None, sync_perm=False) # Migrate classic public app still using the legacy unprotected_uris if app_settings.get("unprotected_uris", None) == "/": @@ -1178,7 +1162,7 @@ def app_addaccess(apps, users=[]): """ from yunohost.permission import user_permission_update - logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage permissions.") + logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please use the new helpers ynh_permission_{create,url,update,delete} and the 'visitors' group to manage permissions.") output = {} for app in apps: @@ -1199,7 +1183,7 @@ def app_removeaccess(apps, users=[]): """ from yunohost.permission import user_permission_update - logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage permissions.") + logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please use the new helpers ynh_permission_{create,url,update,delete} and the 'visitors' group to manage permissions.") output = {} for app in apps: @@ -1219,7 +1203,7 @@ def app_clearaccess(apps): """ from yunohost.permission import user_permission_reset - logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage permissions.") + logger.warning("/!\\ Packagers ! This app is using the legacy permission system. Please use the new helpers ynh_permission_{create,url,update,delete} and the 'visitors' group to manage permissions.") output = {} for app in apps: @@ -1329,7 +1313,7 @@ def app_setting(app, key, value=None, delete=False): if key in ['redirected_urls', 'redirected_regex']: value = yaml.load(value) if key in ["unprotected_uris", "unprotected_regex", "protected_uris", "protected_regex"]: - logger.warning("/!\ Packagers ! This app is using the legacy permission system. Please delete these legacy settings and use the new helpers ynh_permission_{create,urls,update,delete} and the 'visitors' group to manage public/private access.") + logger.warning("/!\ Packagers ! This app is using the legacy permission system. Please delete these legacy settings and use the new helpers ynh_permission_{create,url,update,delete} and the 'visitors' group to manage public/private access.") app_settings[key] = value _set_app_settings(app, app_settings) @@ -1562,20 +1546,24 @@ def app_ssowatconf(): # New permission system this_app_perms = {name: info for name, info in all_permissions.items() if name.startswith(app['id'] + ".")} for perm_name, perm_info in this_app_perms.items(): + + # Ignore permissions for which there's no url defined + if not perm_info["url"]: + continue + # FIXME : gotta handle regex-urls here... meh - urls = [_sanitized_absolute_url(url) for url in perm_info["urls"]] + url = _sanitized_absolute_url(perm_info["url"]) if "visitors" in perm_info["allowed"]: - unprotected_urls += urls + unprotected_urls.append(url) # Legacy stuff : we remove now unprotected-urls that might have been declared as protected earlier... - protected_urls = [u for u in protected_urls if u not in urls] + protected_urls = [u for u in protected_urls if u != url] else: # TODO : small optimization to implement : we don't need to explictly add all the app roots - - protected_urls += urls + protected_urls.append(url) # Legacy stuff : we remove now unprotected-urls that might have been declared as protected earlier... - unprotected_urls = [u for u in unprotected_urls if u not in urls] + unprotected_urls = [u for u in unprotected_urls if u != url] for domain in domains: skipped_urls.extend([domain + '/yunohost/admin', domain + '/yunohost/api']) @@ -1585,11 +1573,13 @@ def app_ssowatconf(): skipped_regex.append("^[^/]*/%.well%-known/autoconfig/mail/config%-v1%.1%.xml.*$") - permissions_per_url = {} - for permission_name, permission_infos in all_permissions.items(): - for url in permission_infos["urls"]: - permissions_per_url[url] = permission_infos['corresponding_users'] + for perm_name, perm_info in all_permissions.items(): + # Ignore permissions for which there's no url defined + if not perm_info["url"]: + continue + permissions_per_url[perm_info["url"]] = perm_info['corresponding_users'] + conf_dict = { 'portal_domain': main_domain, diff --git a/src/yunohost/backup.py b/src/yunohost/backup.py index c28160342..dcdb1adec 100644 --- a/src/yunohost/backup.py +++ b/src/yunohost/backup.py @@ -1189,7 +1189,7 @@ class RestoreManager(): return from yunohost.user import user_group_list - from yunohost.permission import permission_create, permission_delete, user_permission_update, user_permission_list + from yunohost.permission import permission_create, permission_delete, user_permission_update, user_permission_list, permission_sync_to_user # Backup old permission for apps # We need to do that because in case of an app is installed we can't remove the permission for this app @@ -1251,7 +1251,7 @@ class RestoreManager(): for permission_name, permission_infos in old_apps_permission.items(): app_name = permission_name.split(".")[0] if _is_installed(app_name): - permission_create(permission_name, urls=permission_infos["urls"], sync_perm=False) + permission_create(permission_name, url=permission_infos["url"], sync_perm=False) user_permission_update(permission_name, remove="all_users", add=permission_infos["allowed"]) def _restore_apps(self): @@ -1362,7 +1362,7 @@ class RestoreManager(): for permission_name, permission_infos in permissions.items(): - permission_create(permission_name, urls=permission_infos.get("urls", [])) + permission_create(permission_name, url=permission_infos.get("url", None)) if "allowed" not in permission_infos: logger.warning("'allowed' key corresponding to allowed groups for permission %s not found when restoring app %s … You might have to reconfigure permissions yourself." % (permission_name, app_instance_name)) diff --git a/src/yunohost/data_migrations/0011_setup_group_permission.py b/src/yunohost/data_migrations/0011_setup_group_permission.py index de28a3ad7..880c5f54b 100644 --- a/src/yunohost/data_migrations/0011_setup_group_permission.py +++ b/src/yunohost/data_migrations/0011_setup_group_permission.py @@ -107,8 +107,8 @@ class MyMigration(Migration): path = app_setting(app, 'path') domain = app_setting(app, 'domain') - urls = "/" if domain and path else None - permission_create(app+".main", urls=urls, sync_perm=False) + url = "/" if domain and path else None + permission_create(app+".main", url=url, sync_perm=False) if permission: allowed_group = permission.split(',') user_permission_update(app+".main", remove="all_users", add=allowed_group, sync_perm=False) diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 97e5b4122..6f9d63d69 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -73,7 +73,7 @@ def user_permission_list(short=False, full=False, ignore_system_perms=False): if full: permissions[name]["corresponding_users"] = [_ldap_path_extract(p, "uid") for p in infos.get('inheritPermission', [])] - permissions[name]["urls"] = infos.get("URL", []) + permissions[name]["url"] = infos.get("URL", [None])[0] if short: permissions = permissions.keys() @@ -260,27 +260,27 @@ def user_permission_reset(operation_logger, permission, sync_perm=True): # # The followings methods are *not* directly exposed. # They are used to create/delete the permissions (e.g. during app install/remove) -# and by some app helpers to possibly add additional permissions and tweak the urls +# and by some app helpers to possibly add additional permissions # # @is_unit_operation() -def permission_create(operation_logger, permission, urls=None, sync_perm=True): +def permission_create(operation_logger, permission, url=None, sync_perm=True): """ Create a new permission for a specific application Keyword argument: permission -- Name of the permission (e.g. mail or nextcloud or wordpress.editors) - urls -- list of URLs to specify for the permission. + url -- (optional) URL for which access will be allowed/forbidden - Urls are assumed to be relative to the app domain/path if they start with '/'. - For example: + If provided, 'url' is assumed to be relative to the app domain/path if they + start with '/'. For example: / -> domain.tld/app /admin -> domain.tld/app/admin domain.tld/app/api -> domain.tld/app/api - URLs can be later treated as regexes when they start with "re:". + 'url' can be later treated as a regex if it starts with "re:". For example: re:/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ re:domain.tld/app/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ @@ -316,8 +316,8 @@ def permission_create(operation_logger, permission, urls=None, sync_perm=True): if permission.endswith(".main"): attr_dict['groupPermission'] = ['cn=all_users,ou=groups,dc=yunohost,dc=org'] - if urls: - attr_dict['URL'] = urls + if url: + attr_dict['URL'] = url operation_logger.related_to.append(('app', permission.split(".")[0])) operation_logger.start() @@ -335,15 +335,13 @@ def permission_create(operation_logger, permission, urls=None, sync_perm=True): @is_unit_operation() -def permission_urls(operation_logger, permission, add=None, remove=None, sync_perm=True): +def permission_url(operation_logger, permission, url=None, sync_perm=True): """ Update urls related to a permission for a specific application Keyword argument: permission -- Name of the permission (e.g. mail or nextcloud or wordpress.editors) - add -- List of URLs to add (c.f. permission_create for documentation about their format) - remove -- List of URLs to remove (c.f. permission_create for documentation about their format) - + url -- (optional) URL for which access will be allowed/forbidden """ from yunohost.utils.ldap import _get_ldap_interface ldap = _get_ldap_interface() @@ -355,17 +353,9 @@ def permission_urls(operation_logger, permission, add=None, remove=None, sync_pe raise YunohostError('permission_not_found', permission=permission) # Compute new url list + old_url = existing_permission["url"] - new_urls = copy.copy(existing_permission["urls"]) - - if add: - urls_to_add = [add] if not isinstance(add, list) else add - new_urls += urls_to_add - if remove: - urls_to_remove = [remove] if not isinstance(remove, list) else remove - new_urls = [u for u in new_urls if u not in urls_to_remove] - - if set(new_urls) == set(existing_permission["urls"]): + if old_url == url: logger.warning(m18n.n('permission_update_nothing_to_do')) return existing_permission @@ -375,7 +365,7 @@ def permission_urls(operation_logger, permission, add=None, remove=None, sync_pe operation_logger.start() try: - ldap.update('cn=%s,ou=permission' % permission, {'URL': new_urls}) + ldap.update('cn=%s,ou=permission' % permission, {'URL': [url]}) except Exception as e: raise YunohostError('permission_update_failed', permission=permission, error=e) diff --git a/src/yunohost/tests/test_backuprestore.py b/src/yunohost/tests/test_backuprestore.py index cab98089b..82d3da660 100644 --- a/src/yunohost/tests/test_backuprestore.py +++ b/src/yunohost/tests/test_backuprestore.py @@ -529,11 +529,11 @@ def test_backup_and_restore_permission_app(): assert "permissions_app.main" in res assert "permissions_app.admin" in res assert "permissions_app.dev" in res - assert res['permissions_app.main']['urls'] == ["/"] - assert res['permissions_app.admin']['urls'] == ["/admin"] - assert res['permissions_app.dev']['urls'] == ["/dev"] + assert res['permissions_app.main']['url'] == "/" + assert res['permissions_app.admin']['url'] == "/admin" + assert res['permissions_app.dev']['url'] == "/dev" - assert res['permissions_app.main']['allowed'] == ["all_users"] + assert res['permissions_app.main']['allowed'] == ["visitors"] assert res['permissions_app.admin']['allowed'] == ["alice"] assert res['permissions_app.dev']['allowed'] == [] @@ -543,11 +543,11 @@ def test_backup_and_restore_permission_app(): assert "permissions_app.main" in res assert "permissions_app.admin" in res assert "permissions_app.dev" in res - assert res['permissions_app.main']['urls'] == ["/"] - assert res['permissions_app.admin']['urls'] == ["/admin"] - assert res['permissions_app.dev']['urls'] == ["/dev"] + assert res['permissions_app.main']['url'] == "/" + assert res['permissions_app.admin']['url'] == "/admin" + assert res['permissions_app.dev']['url'] == "/dev" - assert res['permissions_app.main']['allowed'] == ["all_users"] + assert res['permissions_app.main']['allowed'] == ["visitors"] 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 0ddda4cec..8e536ec9e 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -6,7 +6,7 @@ from yunohost.app import app_install, app_remove, app_change_url, app_list, app_ from yunohost.user import user_list, user_info, user_create, user_delete, user_update, \ user_group_list, user_group_create, user_group_delete, user_group_update, user_group_info from yunohost.permission import user_permission_update, user_permission_list, user_permission_reset, \ - permission_create, permission_urls, permission_delete + permission_create, permission_delete, permission_url from yunohost.domain import _get_maindomain from yunohost.utils.error import YunohostError @@ -31,7 +31,7 @@ def setup_function(function): user_create("alice", "Alice", "White", "alice@" + maindomain, dummy_password) user_create("bob", "Bob", "Snow", "bob@" + maindomain, dummy_password) - permission_create("wiki.main", urls=["/"], sync_perm=False) + permission_create("wiki.main", url="/", sync_perm=False) permission_create("blog.main", sync_perm=False) user_permission_update("blog.main", remove="all_users", add="alice") @@ -202,7 +202,7 @@ def test_permission_list(): assert res['blog.main']['allowed'] == ["alice"] assert set(res['wiki.main']['corresponding_users']) == set(["alice", "bob"]) assert res['blog.main']['corresponding_users'] == ["alice"] - assert res['wiki.main']['urls'] == ["/"] + assert res['wiki.main']['url'] == "/" # # Create - Remove functions @@ -333,41 +333,19 @@ def test_permission_update_permission_that_doesnt_exist(): with pytest.raises(YunohostError): user_permission_update("doesnt.exist", add="alice") - # Permission url management -def test_permission_add_url(): - permission_urls("blog.main", add=["/testA"]) +def test_permission_redefine_url(): + permission_url("blog.main", url="/pwet") res = user_permission_list(full=True)['permissions'] - assert res["blog.main"]["urls"] == ["/testA"] - -def test_permission_add_another_url(): - permission_urls("wiki.main", add=["/testA"]) - - res = user_permission_list(full=True)['permissions'] - assert set(res["wiki.main"]["urls"]) == set(["/", "/testA"]) + assert res["blog.main"]["url"] == "/pwet" def test_permission_remove_url(): - permission_urls("wiki.main", remove=["/"]) + permission_url("blog.main", url=None) res = user_permission_list(full=True)['permissions'] - assert res["wiki.main"]["urls"] == [] - -def test_permission_add_url_already_added(): - res = user_permission_list(full=True)['permissions'] - assert res["wiki.main"]["urls"] == ["/"] - - permission_urls("wiki.main", add=["/"]) - - res = user_permission_list(full=True)['permissions'] - assert res["wiki.main"]["urls"] == ["/"] - -def test_permission_remove_url_not_added(): - permission_urls("wiki.main", remove=["/doesnt_exist"]) - - res = user_permission_list(full=True)['permissions'] - assert res['wiki.main']['urls'] == ["/"] + assert res["blog.main"]["url"] is None # # Application interaction @@ -381,9 +359,9 @@ def test_permission_app_install(): assert "permissions_app.main" in res assert "permissions_app.admin" in res assert "permissions_app.dev" in res - assert res['permissions_app.main']['urls'] == ["/"] - assert res['permissions_app.admin']['urls'] == ["/admin"] - assert res['permissions_app.dev']['urls'] == ["/dev"] + assert res['permissions_app.main']['url'] == "/" + assert res['permissions_app.admin']['url'] == "/admin" + assert res['permissions_app.dev']['url'] == "/dev" assert res['permissions_app.main']['allowed'] == ["all_users"] assert set(res['permissions_app.main']['corresponding_users']) == set(["alice", "bob"]) @@ -416,16 +394,16 @@ def test_permission_app_change_url(): # FIXME : should rework this test to look for differences in the generated app map / app tiles ... res = user_permission_list(full=True)['permissions'] - assert res['permissions_app.main']['urls'] == ["/"] - assert res['permissions_app.admin']['urls'] == ["/admin"] - assert res['permissions_app.dev']['urls'] == ["/dev"] + assert res['permissions_app.main']['url'] == "/" + assert res['permissions_app.admin']['url'] == "/admin" + assert res['permissions_app.dev']['url'] == "/dev" app_change_url("permissions_app", maindomain, "/newchangeurl") res = user_permission_list(full=True)['permissions'] - assert res['permissions_app.main']['urls'] == ["/"] - assert res['permissions_app.admin']['urls'] == ["/admin"] - assert res['permissions_app.dev']['urls'] == ["/dev"] + assert res['permissions_app.main']['url'] == "/" + assert res['permissions_app.admin']['url'] == "/admin" + assert res['permissions_app.dev']['url'] == "/dev" def test_permission_app_propagation_on_ssowat(): From 2617fd2487d8940309b7a19cb3011985eff3a327 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 22:11:19 +0200 Subject: [PATCH 21/26] Fix issues related to regerating ssowat conf while hacking permissions... --- src/yunohost/backup.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/yunohost/backup.py b/src/yunohost/backup.py index dcdb1adec..90f795ea5 100644 --- a/src/yunohost/backup.py +++ b/src/yunohost/backup.py @@ -1245,14 +1245,17 @@ class RestoreManager(): # Remove all permission for all app which is still in the LDAP for permission_name in user_permission_list(ignore_system_perms=True)["permissions"].keys(): - permission_delete(permission_name, force=True) + permission_delete(permission_name, force=True, sync_perm=False) # Restore permission for the app which is installed for permission_name, permission_infos in old_apps_permission.items(): app_name = permission_name.split(".")[0] if _is_installed(app_name): permission_create(permission_name, url=permission_infos["url"], sync_perm=False) - user_permission_update(permission_name, remove="all_users", add=permission_infos["allowed"]) + user_permission_update(permission_name, remove="all_users", add=permission_infos["allowed"], sync_perm=False) + + permission_sync_to_user() + def _restore_apps(self): """Restore all apps targeted""" @@ -1290,7 +1293,7 @@ class RestoreManager(): restore_app_failed -- Raised if the restore bash script failed """ from yunohost.user import user_group_list - from yunohost.permission import permission_create, permission_delete, user_permission_list, user_permission_update + from yunohost.permission import permission_create, permission_delete, user_permission_list, user_permission_update, permission_sync_to_user def copytree(src, dst, symlinks=False, ignore=None): for item in os.listdir(src): @@ -1362,7 +1365,7 @@ class RestoreManager(): for permission_name, permission_infos in permissions.items(): - permission_create(permission_name, url=permission_infos.get("url", None)) + permission_create(permission_name, url=permission_infos.get("url", None), sync_perm=False) if "allowed" not in permission_infos: logger.warning("'allowed' key corresponding to allowed groups for permission %s not found when restoring app %s … You might have to reconfigure permissions yourself." % (permission_name, app_instance_name)) @@ -1370,7 +1373,9 @@ class RestoreManager(): should_be_allowed = [g for g in permission_infos["allowed"] if g in existing_groups] current_allowed = user_permission_list()["permissions"][permission_name]["allowed"] if should_be_allowed != current_allowed: - user_permission_update(permission_name, remove=current_allowed, add=should_be_allowed) + user_permission_update(permission_name, remove=current_allowed, add=should_be_allowed, sync_perm=False) + + permission_sync_to_user() os.remove('%s/permissions.yml' % app_settings_new_path) else: From c315df926999376fd79b81dd32072d8d3f06e4a9 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 22:48:47 +0200 Subject: [PATCH 22/26] Wokay, getting tired of breaking the entire permission/group ecosystem because of bugs when developing. --- src/yunohost/app.py | 3 +++ src/yunohost/user.py | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index abb4387e5..75bf12f3d 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -433,6 +433,9 @@ def app_map(app=None, raw=False, user=None): continue # Users must at least have access to the main permission to have access to extra permissions if user: + if not app_id + ".main" in permissions: + logger.warning("Uhoh, no main permission was found for app %s ... sounds like an app was only partially removed due to another bug :/" % app_id) + continue main_perm = permissions[app_id + ".main"] if user not in main_perm["corresponding_users"] and "visitors" not in main_perm["allowed"]: continue diff --git a/src/yunohost/user.py b/src/yunohost/user.py index f4e550230..72aa36184 100644 --- a/src/yunohost/user.py +++ b/src/yunohost/user.py @@ -268,7 +268,12 @@ def user_delete(operation_logger, username, purge=False): # remove the member from the group if username != group and username in infos["members"]: user_group_update(group, remove=username, sync_perm=False) - user_group_delete(username, force=True, sync_perm=True) + + # Delete primary group if it exists (why wouldnt it exists ? because some + # epic bug happened somewhere else and only a partial removal was + # performed...) + if username in user_group_list()['groups'].keys(): + user_group_delete(username, force=True, sync_perm=True) ldap = _get_ldap_interface() try: From e7d1cc5f9449f77bf575c82f9faae5f2f2168b41 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 22:55:06 +0200 Subject: [PATCH 23/26] Allow to specify right away what groups to allow for a permission when creating it --- data/helpers.d/setting | 16 +++++++++++----- src/yunohost/app.py | 2 +- src/yunohost/backup.py | 11 ++++------- .../0011_setup_group_permission.py | 8 +++++--- src/yunohost/permission.py | 16 ++++++++++++++-- src/yunohost/tests/test_permission.py | 9 +++++++++ 6 files changed, 44 insertions(+), 18 deletions(-) diff --git a/data/helpers.d/setting b/data/helpers.d/setting index c911c5811..a8d2919a4 100644 --- a/data/helpers.d/setting +++ b/data/helpers.d/setting @@ -232,11 +232,12 @@ ynh_webpath_register () { # Create a new permission for the app # -# example: ynh_permission_create --permission admin --url /admin +# example: ynh_permission_create --permission admin --url /admin --allowed alice bob # -# usage: ynh_permission_create --permission "permission" [--url "url"] +# usage: ynh_permission_create --permission "permission" [--url "url"] [--allowed group1 group2] # | arg: permission - the name for the permission (by default a permission named "main" already exist) # | arg: url - (optional) URL for which access will be allowed/forbidden +# | arg: allowed - (optional) A list of group/user to allow for the permission # # If provided, 'url' is assumed to be relative to the app domain/path if they # start with '/'. For example: @@ -250,9 +251,10 @@ ynh_webpath_register () { # re:domain.tld/app/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ # ynh_permission_create() { - declare -Ar args_array=( [p]=permission= [u]=url= ) + declare -Ar args_array=( [p]=permission= [u]=url= [a]=allowed= ) local permission - local urls + local url + local allowed ynh_handle_getopts_args "$@" if [[ -n ${url:-} ]]; then @@ -261,7 +263,11 @@ ynh_permission_create() { url="None" fi - yunohost tools shell -c "from yunohost.permission import permission_create; permission_create('$app.$permission', url=$url, sync_perm=False)" + if [[ -n ${allowed:-} ]]; then + allowed=",allowed=['${allowed//';'/"','"}']" + fi + + yunohost tools shell -c "from yunohost.permission import permission_create; permission_create('$app.$permission', url=$url ${allowed:-} , sync_perm=False)" } # Remove a permission for the app (note that when the app is removed all permission is automatically removed) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 75bf12f3d..7235535cd 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -981,7 +981,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu # Initialize the main permission for the app # After the install, if apps don't have a domain and path defined, the default url '/' is removed from the permission - permission_create(app_instance_name+".main", url="/") + permission_create(app_instance_name+".main", url="/", allowed=["all_users"]) # Execute the app install script install_retcode = 1 diff --git a/src/yunohost/backup.py b/src/yunohost/backup.py index 90f795ea5..c57ab6685 100644 --- a/src/yunohost/backup.py +++ b/src/yunohost/backup.py @@ -1251,8 +1251,7 @@ class RestoreManager(): for permission_name, permission_infos in old_apps_permission.items(): app_name = permission_name.split(".")[0] if _is_installed(app_name): - permission_create(permission_name, url=permission_infos["url"], sync_perm=False) - user_permission_update(permission_name, remove="all_users", add=permission_infos["allowed"], sync_perm=False) + permission_create(permission_name, url=permission_infos["url"], allowed=permission_infos["allowed"], sync_perm=False) permission_sync_to_user() @@ -1365,15 +1364,13 @@ class RestoreManager(): for permission_name, permission_infos in permissions.items(): - permission_create(permission_name, url=permission_infos.get("url", None), sync_perm=False) - if "allowed" not in permission_infos: logger.warning("'allowed' key corresponding to allowed groups for permission %s not found when restoring app %s … You might have to reconfigure permissions yourself." % (permission_name, app_instance_name)) + should_be_allowed = ["all_users"] else: should_be_allowed = [g for g in permission_infos["allowed"] if g in existing_groups] - current_allowed = user_permission_list()["permissions"][permission_name]["allowed"] - if should_be_allowed != current_allowed: - user_permission_update(permission_name, remove=current_allowed, add=should_be_allowed, sync_perm=False) + + permission_create(permission_name, url=permission_infos.get("url", None), allowed=should_be_allowed, sync_perm=False) permission_sync_to_user() diff --git a/src/yunohost/data_migrations/0011_setup_group_permission.py b/src/yunohost/data_migrations/0011_setup_group_permission.py index 880c5f54b..3114817b9 100644 --- a/src/yunohost/data_migrations/0011_setup_group_permission.py +++ b/src/yunohost/data_migrations/0011_setup_group_permission.py @@ -108,10 +108,12 @@ class MyMigration(Migration): domain = app_setting(app, 'domain') url = "/" if domain and path else None - permission_create(app+".main", url=url, sync_perm=False) if permission: - allowed_group = permission.split(',') - user_permission_update(app+".main", remove="all_users", add=allowed_group, sync_perm=False) + allowed_groups = permission.split(',') + else: + allowed_groups = ["all_users"] + permission_create(app+".main", url=url, allowed=allowed_groups, sync_perm=False) + app_setting(app, 'allowed_users', delete=True) # Migrate classic public app still using the legacy unprotected_uris diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 6f9d63d69..426ecd10f 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -266,13 +266,14 @@ def user_permission_reset(operation_logger, permission, sync_perm=True): @is_unit_operation() -def permission_create(operation_logger, permission, url=None, sync_perm=True): +def permission_create(operation_logger, permission, url=None, allowed=None, sync_perm=True): """ Create a new permission for a specific application Keyword argument: permission -- Name of the permission (e.g. mail or nextcloud or wordpress.editors) url -- (optional) URL for which access will be allowed/forbidden + allowed -- (optional) A list of group/user to allow for the permission If provided, 'url' is assumed to be relative to the app domain/path if they start with '/'. For example: @@ -286,6 +287,7 @@ def permission_create(operation_logger, permission, url=None, sync_perm=True): re:domain.tld/app/api/[A-Z]*$ -> domain.tld/app/api/[A-Z]*$ """ + from yunohost.user import user_group_list from yunohost.utils.ldap import _get_ldap_interface ldap = _get_ldap_interface() @@ -312,8 +314,18 @@ def permission_create(operation_logger, permission, url=None, sync_perm=True): 'gidNumber': gid, } + # If who should be allowed is explicitly provided, use this info + if allowed: + if not isinstance(allowed, list): + allowed = [allowed] + # (though first we validate that the targets actually exist) + all_existing_groups = user_group_list()['groups'].keys() + for g in allowed: + if g not in all_existing_groups: + raise YunohostError('group_unknown', group=g) + attr_dict['groupPermission'] = ['cn=%s,ou=groups,dc=yunohost,dc=org' % g for g in allowed] # For main permission, we add all users by default - if permission.endswith(".main"): + elif permission.endswith(".main"): attr_dict['groupPermission'] = ['cn=all_users,ou=groups,dc=yunohost,dc=org'] if url: diff --git a/src/yunohost/tests/test_permission.py b/src/yunohost/tests/test_permission.py index 8e536ec9e..5e1246793 100644 --- a/src/yunohost/tests/test_permission.py +++ b/src/yunohost/tests/test_permission.py @@ -226,6 +226,15 @@ def test_permission_create_extra(): assert "all_users" not in res['site.test']['allowed'] assert res['site.test']['corresponding_users'] == [] + +def test_permission_create_with_allowed(): + permission_create("site.test", allowed=["alice"]) + + res = user_permission_list(full=True)['permissions'] + assert "site.test" in res + assert res['site.test']['allowed'] == ["alice"] + + def test_permission_delete(): permission_delete("wiki.main", force=True) From 4bdcfb4373bfc0ce44931df209fb5d949a3d3768 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 23:16:07 +0200 Subject: [PATCH 24/26] Implement / fix i18n strings --- locales/en.json | 3 +++ .../data_migrations/0011_setup_group_permission.py | 2 +- src/yunohost/permission.py | 13 ++++--------- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/locales/en.json b/locales/en.json index e3911a334..c6a6a440e 100644 --- a/locales/en.json +++ b/locales/en.json @@ -415,9 +415,12 @@ "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_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_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}", "permission_not_found": "Permission '{permission:s}' not found", diff --git a/src/yunohost/data_migrations/0011_setup_group_permission.py b/src/yunohost/data_migrations/0011_setup_group_permission.py index 3114817b9..9ba2268d9 100644 --- a/src/yunohost/data_migrations/0011_setup_group_permission.py +++ b/src/yunohost/data_migrations/0011_setup_group_permission.py @@ -132,7 +132,7 @@ class MyMigration(Migration): ldap_regen_conf_status = regen_conf(names=['slapd'], dry_run=True) # By this we check if the have been customized if ldap_regen_conf_status and ldap_regen_conf_status['slapd']['pending']: - logger.warning("migration_0011_slapd_config_will_be_overwritten", conf_backup_folder=BACKUP_CONF_DIR) + logger.warning(m18n.n("migration_0011_slapd_config_will_be_overwritten", conf_backup_folder=BACKUP_CONF_DIR)) # Backup LDAP and the apps settings before to do the migration logger.info(m18n.n("migration_0011_backup_before_migration")) diff --git a/src/yunohost/permission.py b/src/yunohost/permission.py index 426ecd10f..4cfbc214f 100644 --- a/src/yunohost/permission.py +++ b/src/yunohost/permission.py @@ -144,18 +144,13 @@ def user_permission_update(operation_logger, permission, add=None, remove=None, if len(new_allowed_groups) > 1: if "all_users" in new_allowed_groups: - # FIXME : i18n - # FIXME : write a better explanation ? - logger.warning("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.") + logger.warning(m18n.n("permission_currently_allowed_for_all_users")) if "visitors" in new_allowed_groups: - # FIXME : i18n - # FIXME : write a better explanation ? - logger.warning("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.") + logger.warning(m18n.n("permission_currently_allowed_for_visitors")) # Don't update LDAP if we update exactly the same values if set(new_allowed_groups) == set(current_allowed_groups): - # FIXME : i18n - logger.warning("The permission was not updated all addition/removal requests already match the current state.") + logger.warning("permission_already_up_to_date") return # Commit the new allowed group list @@ -218,7 +213,7 @@ def user_permission_reset(operation_logger, permission, sync_perm=True): raise YunohostError('permission_not_found', permission=permission) if existing_permission["allowed"] == ["all_users"]: - logger.warning("The permission was not updated all addition/removal requests already match the current state.") + logger.warning(m18n.n("permission_already_up_to_date")) return # Update permission with default (all_users) From e4163136bbe6c7eff26102767400c0a99cb702c0 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 9 Oct 2019 23:40:50 +0200 Subject: [PATCH 25/26] Don't attempt to delete the 'visitors' group during user/group tests --- src/yunohost/tests/test_user-group.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/yunohost/tests/test_user-group.py b/src/yunohost/tests/test_user-group.py index 30bdeb017..53fded94c 100644 --- a/src/yunohost/tests/test_user-group.py +++ b/src/yunohost/tests/test_user-group.py @@ -14,7 +14,7 @@ def clean_user_groups(): user_delete(u) for g in user_group_list()['groups']: - if g != "all_users": + if g not in ["all_users", "visitors"]: user_group_delete(g) def setup_function(function): From e48036a0829ed2754d2c39e033c78dd8b6d07c84 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 10 Oct 2019 00:05:20 +0200 Subject: [PATCH 26/26] Fix test about private app installs --- src/yunohost/tests/test_apps.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/yunohost/tests/test_apps.py b/src/yunohost/tests/test_apps.py index fc44ef105..fb2f13c3f 100644 --- a/src/yunohost/tests/test_apps.py +++ b/src/yunohost/tests/test_apps.py @@ -119,10 +119,10 @@ def app_is_exposed_on_http(domain, path, message_in_page): return False -def install_legacy_app(domain, path): +def install_legacy_app(domain, path, public=True): app_install("./tests/apps/legacy_app_ynh", - args="domain=%s&path=%s" % (domain, path), + args="domain=%s&path=%s&is_public=%s" % (domain, path, 1 if public else 0), force=True) @@ -180,13 +180,7 @@ def test_legacy_app_install_secondary_domain_on_root(secondary_domain): def test_legacy_app_install_private(secondary_domain): - install_legacy_app(secondary_domain, "/legacy") - - settings = open("/etc/yunohost/apps/legacy_app/settings.yml", "r").read() - new_settings = settings.replace("\nunprotected_uris: /", "") - assert new_settings != settings - open("/etc/yunohost/apps/legacy_app/settings.yml", "w").write(new_settings) - app_ssowatconf() + install_legacy_app(secondary_domain, "/legacy", public=False) assert app_is_installed(secondary_domain, "legacy_app") assert not app_is_exposed_on_http(secondary_domain, "/legacy", "This is a dummy app")