Rework function that validate and sanitize permission url

src/yunohost/app.py

@@ -317,7 +317,7 @@ 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.domain import _normalize_domain_path, _assert_no_conflicting_apps
 
     installed = _is_installed(app)
     if not installed:
@@ -337,17 +337,7 @@ def app_change_url(operation_logger, app, domain, path):
         raise YunohostError("app_change_url_identical_domains", domain=domain, path=path)
 
     # Check the url is available
-    conflicts = _get_conflicting_apps(domain, path, ignore_app=app)
+    _assert_no_conflicting_apps(domain, path, ignore_app=app)
-    if conflicts:
-        apps = []
-        for path, app_id, app_label in conflicts:
-            apps.append(" * {domain:s}{path:s} → {app_label:s} ({app_id:s})".format(
-                domain=domain,
-                path=path,
-                app_id=app_id,
-                app_label=app_label,
-            ))
-        raise YunohostError('app_location_unavailable', apps="\n".join(apps))
 
     manifest = _get_manifest_of_app(os.path.join(APPS_SETTING_PATH, app))
 
@@ -1275,7 +1265,7 @@ def app_register_url(app, domain, path):
 
     # This line can't be moved on top of file, otherwise it creates an infinite
     # loop of import with tools.py...
-    from .domain import _get_conflicting_apps, _normalize_domain_path
+    from .domain import _assert_no_conflicting_apps, _normalize_domain_path
 
     domain, path = _normalize_domain_path(domain, path)
 
@@ -1288,18 +1278,7 @@ def app_register_url(app, domain, path):
             raise YunohostError('app_already_installed_cant_change_url')
 
     # Check the url is available
-    conflicts = _get_conflicting_apps(domain, path)
+    _assert_no_conflicting_apps(domain, path)
-    if conflicts:
-        apps = []
-        for path, app_id, app_label in conflicts:
-            apps.append(" * {domain:s}{path:s} → {app_label:s} ({app_id:s})".format(
-                domain=domain,
-                path=path,
-                app_id=app_id,
-                app_label=app_label,
-            ))
-
-        raise YunohostError('app_location_unavailable', apps="\n".join(apps))
 
     app_setting(app, 'domain', value=domain)
     app_setting(app, 'path', value=path)
@@ -2655,7 +2634,7 @@ def _parse_args_in_yunohost_format(user_answers, argument_questions):
 
 def _validate_and_normalize_webpath(manifest, args_dict, app_folder):
 
-    from yunohost.domain import _get_conflicting_apps, _normalize_domain_path
+    from yunohost.domain import _assert_no_conflicting_apps, _normalize_domain_path
 
     # If there's only one "domain" and "path", validate that domain/path
     # is an available url and normalize the path.
@@ -2670,18 +2649,7 @@ def _validate_and_normalize_webpath(manifest, args_dict, app_folder):
         domain, path = _normalize_domain_path(domain, path)
 
         # Check the url is available
-        conflicts = _get_conflicting_apps(domain, path)
+        _assert_no_conflicting_apps(domain, path)
-        if conflicts:
-            apps = []
-            for path, app_id, app_label in conflicts:
-                apps.append(" * {domain:s}{path:s} → {app_label:s} ({app_id:s})".format(
-                    domain=domain,
-                    path=path,
-                    app_id=app_id,
-                    app_label=app_label,
-                ))
-
-            raise YunohostError('app_location_unavailable', apps="\n".join(apps))
 
         # (We save this normalized path so that the install script have a
         # standard path format to deal with no matter what the user inputted)
@@ -2704,8 +2672,7 @@ def _validate_and_normalize_webpath(manifest, args_dict, app_folder):
            and re.search(r"(ynh_webpath_register|yunohost app checkurl)", install_script_content):
 
             domain = domain_args[0][1]
-            if _get_conflicting_apps(domain, "/"):
+            _assert_no_conflicting_apps(domain, "/")
-                raise YunohostError('app_full_domain_unavailable', domain=domain)
 
 
 def _make_environment_dict(args_dict, prefix="APP_ARG_"):

src/yunohost/domain.py

@@ -388,6 +388,22 @@ def _get_conflicting_apps(domain, path, ignore_app=None):
 
     return conflicts
 
+def _assert_no_conflicting_apps(domain, path, ignore_app=None):
+
+    conflicts = _get_conflicting_apps(domain, path, ignore_app)
+
+    if conflicts:
+        apps = []
+        for path, app_id, app_label in conflicts:
+            apps.append(" * {domain:s}{path:s} → {app_label:s} ({app_id:s})".format(
+                domain=domain,
+                path=path,
+                app_id=app_id,
+                app_label=app_label,
+            ))
+
+        raise YunohostError('app_location_unavailable', apps="\n".join(apps))
+
 
 def domain_url_available(domain, path):
     """
@@ -430,98 +446,6 @@ def _normalize_domain_path(domain, path):
     return domain, path
 
 
-def _check_and_sanitize_permission_path(url, app_main_path, permission):
-    """
-    Check and normalize the urls passed for all permissions
-    Also check that the Regex is valid
-
-    As documented in the 'ynh_permission_create' helper:
-
-    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
-       domain.tld                    -> domain.tld
-
-    '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]*$
-    """
-
-    domains = domain_list()['domains']
-
-    # regex without domain
-    if url.startswith('re:/'):
-        regex = url[4:]
-        # check regex if it's a PCRE regex, if it's a lua regex just print a warning
-        # I don't how how to validate a regex
-        if '%' in regex:
-            logger.warning("/!\\ Packagers! You are probably using a lua regex. You should use a PCRE regex instead.")
-        else:
-            try:
-                re.compile(regex)
-            except Exception:
-                raise YunohostError('invalid_regex', regex=regex)
-        return url
-
-    # regex with domain
-    if url.startswith('re:'):
-        if '/' not in url:
-            raise YunohostError('regex_with_only_domain')
-        domain = url[3:].split('/')[0]
-        path = '/' + url[3:].split('/', 1)[1]
-
-        if domain.replace('%', '').replace('\\', '') not in domains:
-            raise YunohostError('domain_name_unknown', domain=domain)
-
-        if '%' in path:
-            logger.warning("/!\\ Packagers! You are probably using a lua regex. You should use a PCRE regex instead.")
-        else:
-            try:
-                re.compile(path)
-            except Exception:
-                raise YunohostError('invalid_regex', regex=path)
-
-        return 're:' + domain + path
-
-    # uris without domain
-    if url.startswith('/'):
-        sanitized_url = '/' + url.strip("/")
-        domain = app_main_path.split('/')[0]
-        path = ('/' + app_main_path.split('/')[1]) if '/' in app_main_path else '/'
-
-    # uris with domain
-    else:
-        domain = url.split('/')[0]
-        if domain not in domains:
-            raise YunohostError('domain_name_unknown', domain=domain)
-
-        if '/' in url:
-            path = '/' + url.split('/', 1)[1].rstrip('/')
-            sanitized_url = domain + path
-        else:
-            sanitized_url = domain
-            path = '/'
-
-    conflicts = _get_conflicting_apps(domain, path, ignore_app=permission.split('.')[0])
-
-    if conflicts:
-        apps = []
-        for path, app_id, app_label in conflicts:
-            apps.append(" * {domain:s}{path:s} → {app_label:s} ({app_id:s})".format(
-                domain=domain,
-                path=path,
-                app_id=app_id,
-                app_label=app_label,
-            ))
-
-        raise YunohostError('app_location_unavailable', apps="\n".join(apps))
-
-    return sanitized_url
-
-
 def _build_dns_conf(domain, ttl=3600, include_empty_AAAA_if_no_ipv6=False):
     """
     Internal function that will returns a data structure containing the needed

src/yunohost/permission.py

@@ -24,6 +24,7 @@
     Manage permissions
 """
 
+import re
 import copy
 import grp
 import random
@@ -368,15 +369,15 @@ def permission_url(operation_logger, permission,
     """
     from yunohost.app import app_setting
     from yunohost.utils.ldap import _get_ldap_interface
-    from yunohost.domain import _check_and_sanitize_permission_path
     ldap = _get_ldap_interface()
 
     # By default, manipulate main permission
     if "." not in permission:
         permission = permission + ".main"
 
-    if url or add_url:
     app = permission.split('.')[0]
+
+    if url or add_url:
         domain = app_setting(app, 'domain')
         path = app_setting(app, 'path')
         if domain is None or path is None:
@@ -395,7 +396,7 @@ def permission_url(operation_logger, permission,
     if url is None:
         url = existing_permission["url"]
     else:
-        url = _check_and_sanitize_permission_path(url, app_main_path, permission)
+        url = _validate_and_sanitize_permission_url(url, app_main_path, app)
 
         if url.startswith('re:') and existing_permission['show_tile']:
             logger.warning(m18n.n('regex_incompatible_with_tile', regex=url, permission=permission))
@@ -409,7 +410,7 @@ def permission_url(operation_logger, permission,
             if ur in current_additional_urls:
                 logger.warning(m18n.n('additional_urls_already_added', permission=permission, url=ur))
             else:
-                ur = _check_and_sanitize_permission_path(ur, app_main_path, permission)
+                ur = _validate_and_sanitize_permission_url(ur, app_main_path, app)
                 new_additional_urls += [ur]
 
     if remove_url:
@@ -645,3 +646,100 @@ def _get_absolute_url(url, base_path):
         return 're:' + base_path.replace('.', '\\.') + url[3:]
     else:
         return url
+
+
+def _validate_and_sanitize_permission_url(url, app_base_path, app):
+    """
+    Check and normalize the urls passed for all permissions
+    Also check that the Regex is valid
+
+    As documented in the 'ynh_permission_create' helper:
+
+    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
+       domain.tld                    -> domain.tld
+
+    '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]*$
+    """
+
+    from yunohost.domain import domain_list, _assert_no_conflicting_apps
+
+
+    domains = domain_list()['domains']
+
+    #
+    # Regexes
+    #
+
+    def validate_regex(regex):
+        if '%' in regex:
+            logger.warning("/!\\ Packagers! You are probably using a lua regex. You should use a PCRE regex instead.")
+            return
+
+        try:
+            re.compile(regex)
+        except Exception:
+            raise YunohostError('invalid_regex', regex=regex)
+
+    if url.startswith('re:'):
+
+        # regex without domain
+
+        if url.startswith('re:/'):
+            validate_regex(url[4:])
+            return url
+
+        # regex with domain
+
+        if '/' not in url:
+            raise YunohostError('regex_with_only_domain')
+        domain, path = url[3:].split('/', 1)
+        path = '/' + path
+
+        if domain.replace('%', '').replace('\\', '') not in domains:
+            raise YunohostError('domain_name_unknown', domain=domain)
+
+        validate_regex(path)
+
+        return 're:' + domain + path
+
+    #
+    # "Regular" URIs
+    #
+
+    def split_domain_path(url):
+        url = url.strip("/")
+        (domain, path) = url.split('/', 1) if "/" in url else (url, "/")
+        if path != "/":
+            path = "/" + path
+        return (domain, path)
+
+    # uris without domain
+    if url.startswith('/'):
+        # if url is for example /admin/
+        # we want sanitized_url to be: /admin
+        # and (domain, path) to be   : (domain.tld, /app/admin)
+        sanitized_url = "/" + url.strip("/")
+        domain, path = split_domain_path(app_base_path)
+        path = "/" + path.strip("/") + sanitized_url.strip("/")
+
+    # uris with domain
+    else:
+        # if url is for example domain.tld/wat/
+        # we want sanitized_url to be: domain.tld/wat
+        # and (domain, path) to be   : (domain.tld, /wat)
+        domain, path = split_domain_path(url)
+        sanitized_url = domain + path
+
+        if domain not in domains:
+            raise YunohostError('domain_name_unknown', domain=domain)
+
+    _assert_no_conflicting_apps(domain, path, ignore_app=app)
+
+    return sanitized_url

src/yunohost/tests/test_appurl.py

@@ -5,7 +5,8 @@ from conftest import get_test_apps_dir
 
 from yunohost.utils.error import YunohostError
 from yunohost.app import app_install, app_remove
-from yunohost.domain import _get_maindomain, domain_url_available, _normalize_domain_path, _check_and_sanitize_permission_path
+from yunohost.domain import _get_maindomain, domain_url_available, _normalize_domain_path
+from yunohost.permission import _validate_and_sanitize_permission_url
 
 # Get main domain
 maindomain = _get_maindomain()
@@ -66,43 +67,43 @@ def test_registerurl_baddomain():
 
 def test_normalize_permission_path():
     # Relative path
-    assert _check_and_sanitize_permission_path("/wiki/", maindomain + '/path', 'test_permission.main') == "/wiki"
+    assert _validate_and_sanitize_permission_url("/wiki/", maindomain + '/path', 'test_permission') == "/wiki"
-    assert _check_and_sanitize_permission_path("/", maindomain + '/path', 'test_permission.main') == "/"
+    assert _validate_and_sanitize_permission_url("/", maindomain + '/path', 'test_permission') == "/"
-    assert _check_and_sanitize_permission_path("//salut/", maindomain + '/path', 'test_permission.main') == "/salut"
+    assert _validate_and_sanitize_permission_url("//salut/", maindomain + '/path', 'test_permission') == "/salut"
 
     # Full path
-    assert _check_and_sanitize_permission_path(maindomain + "/hey/", maindomain + '/path', 'test_permission.main') == maindomain + "/hey"
+    assert _validate_and_sanitize_permission_url(maindomain + "/hey/", maindomain + '/path', 'test_permission') == maindomain + "/hey"
-    assert _check_and_sanitize_permission_path(maindomain + "//", maindomain + '/path', 'test_permission.main') == maindomain + "/"
+    assert _validate_and_sanitize_permission_url(maindomain + "//", maindomain + '/path', 'test_permission') == maindomain + "/"
-    assert _check_and_sanitize_permission_path(maindomain + "/", maindomain + '/path', 'test_permission.main') == maindomain + "/"
+    assert _validate_and_sanitize_permission_url(maindomain + "/", maindomain + '/path', 'test_permission') == maindomain + "/"
 
     # Relative Regex
-    assert _check_and_sanitize_permission_path("re:/yolo.*/", maindomain + '/path', 'test_permission.main') == "re:/yolo.*/"
+    assert _validate_and_sanitize_permission_url("re:/yolo.*/", maindomain + '/path', 'test_permission') == "re:/yolo.*/"
-    assert _check_and_sanitize_permission_path("re:/y.*o(o+)[a-z]*/bo\1y", maindomain + '/path', 'test_permission.main') == "re:/y.*o(o+)[a-z]*/bo\1y"
+    assert _validate_and_sanitize_permission_url("re:/y.*o(o+)[a-z]*/bo\1y", maindomain + '/path', 'test_permission') == "re:/y.*o(o+)[a-z]*/bo\1y"
 
     # Full Regex
-    assert _check_and_sanitize_permission_path("re:" + maindomain + "/yolo.*/", maindomain + '/path', 'test_permission.main') == "re:" + maindomain + "/yolo.*/"
+    assert _validate_and_sanitize_permission_url("re:" + maindomain + "/yolo.*/", maindomain + '/path', 'test_permission') == "re:" + maindomain + "/yolo.*/"
-    assert _check_and_sanitize_permission_path("re:" + maindomain + "/y.*o(o+)[a-z]*/bo\1y", maindomain + '/path', 'test_permission.main') == "re:" + maindomain + "/y.*o(o+)[a-z]*/bo\1y"
+    assert _validate_and_sanitize_permission_url("re:" + maindomain + "/y.*o(o+)[a-z]*/bo\1y", maindomain + '/path', 'test_permission') == "re:" + maindomain + "/y.*o(o+)[a-z]*/bo\1y"
 
 
 def test_normalize_permission_path_with_bad_regex():
     # Relative Regex
     with pytest.raises(YunohostError):
-        _check_and_sanitize_permission_path("re:/yolo.*[1-7]^?/", maindomain + '/path', 'test_permission.main')
+        _validate_and_sanitize_permission_url("re:/yolo.*[1-7]^?/", maindomain + '/path', 'test_permission')
     with pytest.raises(YunohostError):
-        _check_and_sanitize_permission_path("re:/yolo.*[1-7](]/", maindomain + '/path', 'test_permission.main')
+        _validate_and_sanitize_permission_url("re:/yolo.*[1-7](]/", maindomain + '/path', 'test_permission')
 
     # Full Regex
     with pytest.raises(YunohostError):
-        _check_and_sanitize_permission_path("re:" + maindomain + "/yolo?+/", maindomain + '/path', 'test_permission.main')
+        _validate_and_sanitize_permission_url("re:" + maindomain + "/yolo?+/", maindomain + '/path', 'test_permission')
     with pytest.raises(YunohostError):
-        _check_and_sanitize_permission_path("re:" + maindomain + "/yolo[1-9]**/", maindomain + '/path', 'test_permission.main')
+        _validate_and_sanitize_permission_url("re:" + maindomain + "/yolo[1-9]**/", maindomain + '/path', 'test_permission')
 
 
 def test_normalize_permission_path_with_unknown_domain():
     with pytest.raises(YunohostError):
-        _check_and_sanitize_permission_path("shouldntexist.tld/hey", maindomain + '/path', 'test_permission.main')
+        _validate_and_sanitize_permission_url("shouldntexist.tld/hey", maindomain + '/path', 'test_permission')
     with pytest.raises(YunohostError):
-        _check_and_sanitize_permission_path("re:shouldntexist.tld/hey.*", maindomain + '/path', 'test_permission.main')
+        _validate_and_sanitize_permission_url("re:shouldntexist.tld/hey.*", maindomain + '/path', 'test_permission')
 
 
 def test_normalize_permission_path_conflicting_path():
@@ -110,6 +111,6 @@ def test_normalize_permission_path_conflicting_path():
                 args="domain=%s&path=%s" % (maindomain, "/url/registerapp"), force=True)
 
     with pytest.raises(YunohostError):
-        _check_and_sanitize_permission_path("/registerapp", maindomain + '/url', 'test_permission.main')
+        _validate_and_sanitize_permission_url("/registerapp", maindomain + '/url', 'test_permission')
     with pytest.raises(YunohostError):
-        _check_and_sanitize_permission_path(maindomain + "/url/registerapp", maindomain + '/path', 'test_permission.main')
+        _validate_and_sanitize_permission_url(maindomain + "/url/registerapp", maindomain + '/path', 'test_permission')