Merge branch 'enh-config-panel-file' of github.com:YunoHost/yunohost into enh-config-panel-file

data/helpers.d/config

@@ -40,14 +40,14 @@ EOL
             old[$short_setting]="$($getter)"
             formats[${short_setting}]="yaml"
 
-        elif [[ "$bind" == "null" ]];
+        elif [[ "$bind" == "null" ]]
         then
             old[$short_setting]="YNH_NULL"
 
         # Get value from app settings or from another file
-        elif [[ "$type" == "file" ]];
+        elif [[ "$type" == "file" ]]
         then
-            if [[ "$bind" == "settings" ]];
+            if [[ "$bind" == "settings" ]]
             then
                 ynh_die "File '${short_setting}' can't be stored in settings"
             fi
@@ -55,12 +55,12 @@ EOL
             file_hash[$short_setting]="true"
 
         # Get multiline text from settings or from a full file
-        elif [[ "$type" == "text" ]];
+        elif [[ "$type" == "text" ]]
         then
-            if [[ "$bind" == "settings" ]];
+            if [[ "$bind" == "settings" ]]
             then
                 old[$short_setting]="$(ynh_app_setting_get $app $short_setting)"
-            elif [[ "$bind" == *":"* ]];
+            elif [[ "$bind" == *":"* ]]
             then
                 ynh_die "For technical reasons, multiline text '${short_setting}' can't be stored automatically  in a variable file, you have to create custom getter/setter"
             else
@@ -69,7 +69,7 @@ EOL
 
         # Get value from a kind of key/value file
         else
-            if [[ "$bind" == "settings" ]];
+            if [[ "$bind" == "settings" ]]
             then
                 bind=":/etc/yunohost/apps/$app/settings.yml"
             fi
@@ -90,26 +90,26 @@ _ynh_app_config_apply() {
         local setter="set__${short_setting}"
         local bind="${binds[$short_setting]}"
         local type="${types[$short_setting]}"
-        if [ "${changed[$short_setting]}" == "true" ];
+        if [ "${changed[$short_setting]}" == "true" ]
         then
             # Apply setter if exists
             if type -t $setter 2>/dev/null | grep -q '^function$' 2>/dev/null;
             then
                 $setter
 
-            elif [[ "$bind" == "null" ]];
+            elif [[ "$bind" == "null" ]]
             then
                 continue
 
             # Save in a file
-            elif [[ "$type" == "file" ]];
+            elif [[ "$type" == "file" ]]
             then
-                if [[ "$bind" == "settings" ]];
+                if [[ "$bind" == "settings" ]]
                 then
                     ynh_die "File '${short_setting}' can't be stored in settings"
                 fi
                 local bind_file="$(echo "$bind" | sed s@__FINALPATH__@$final_path@ | sed s/__APP__/$app/)"
-                if [[ "${!short_setting}" == "" ]];
+                if [[ "${!short_setting}" == "" ]]
                 then
                     ynh_backup_if_checksum_is_different --file="$bind_file"
                     rm -f "$bind_file"
@@ -123,15 +123,15 @@ _ynh_app_config_apply() {
                 fi
 
             # Save value in app settings
-            elif [[ "$bind" == "settings" ]];
+            elif [[ "$bind" == "settings" ]]
             then
                 ynh_app_setting_set $app $short_setting "${!short_setting}"
                 ynh_print_info "Configuration key '$short_setting' edited in app settings"
 
             # Save multiline text in a file
-            elif [[ "$type" == "text" ]];
+            elif [[ "$type" == "text" ]]
             then
-                if [[ "$bind" == *":"* ]];
+                if [[ "$bind" == *":"* ]]
                 then
                     ynh_die "For technical reasons, multiline text '${short_setting}' can't be stored automatically  in a variable file, you have to create custom getter/setter"
                 fi
@@ -169,9 +169,9 @@ _ynh_app_config_apply() {
 _ynh_app_config_show() {
     for short_setting in "${!old[@]}"
     do
-        if [[ "${old[$short_setting]}" != YNH_NULL ]];
+        if [[ "${old[$short_setting]}" != YNH_NULL ]]
         then
-            if [[ "${formats[$short_setting]}" == "yaml" ]];
+            if [[ "${formats[$short_setting]}" == "yaml" ]]
             then
                 ynh_return "${short_setting}:"
                 ynh_return "$(echo "${old[$short_setting]}" | sed 's/^/  /g')"
@@ -192,27 +192,27 @@ _ynh_app_config_validate() {
     for short_setting in "${!old[@]}"
     do
         changed[$short_setting]=false
-        if [ -z ${!short_setting+x} ];
+        if [ -z ${!short_setting+x} ]
         then
             # Assign the var with the old value in order to allows multiple
             # args validation
             declare "$short_setting"="${old[$short_setting]}"
             continue
         fi
-        if [ ! -z "${file_hash[${short_setting}]}" ];
+        if [ ! -z "${file_hash[${short_setting}]}" ]
         then
             file_hash[old__$short_setting]=""
             file_hash[new__$short_setting]=""
-            if [ -f "${old[$short_setting]}" ];
+            if [ -f "${old[$short_setting]}" ]
             then
                 file_hash[old__$short_setting]=$(sha256sum "${old[$short_setting]}" | cut -d' ' -f1)
-                if [ -z "${!short_setting}" ];
+                if [ -z "${!short_setting}" ]
                 then
                     changed[$short_setting]=true
                     nothing_changed=false
                 fi
             fi
-            if [ -f "${!short_setting}" ];
+            if [ -f "${!short_setting}" ]
             then
                 file_hash[new__$short_setting]=$(sha256sum "${!short_setting}" | cut -d' ' -f1)
                 if [[ "${file_hash[old__$short_setting]}" != "${file_hash[new__$short_setting]}" ]]
@@ -248,8 +248,20 @@ _ynh_app_config_validate() {
         fi
         if [ -n "$result" ]
         then
-            local key="YNH_ERROR_${short_setting}"
+            #
-            ynh_return "$key: \"$result\""
+            # Return a yaml such as:
+            #
+            # validation_errors:
+            #   some_key: "An error message"
+            #   some_other_key: "Another error message"
+            #
+            # We use changes_validated to know if this is
+            # the first validation error
+            if [[ "$changes_validated" == true ]]
+            then
+                ynh_return "validation_errors:"
+            fi
+            ynh_return "  ${short_setting}: \"$result\""
             changes_validated=false
         fi
     done

src/yunohost/app.py

@@ -1695,9 +1695,7 @@ def app_config_set(
 
     Question.operation_logger = operation_logger
 
-    result = config_.set(key, value, args, args_file, operation_logger=operation_logger)
+    return config_.set(key, value, args, args_file, operation_logger=operation_logger)
-
-    return result
 
 
 class AppConfigPanel(ConfigPanel):
@@ -1715,7 +1713,18 @@ class AppConfigPanel(ConfigPanel):
 
     def _apply(self):
         env = {key: str(value) for key, value in self.new_values.items()}
-        self.errors = self._call_config_script("apply", env=env)
+        return_content = self._call_config_script("apply", env=env)
+
+        # If the script returned validation error
+        # raise a ValidationError exception using
+        # the first key
+        if return_content:
+            for key, message in return_content.get("validation_errors").items():
+                raise YunohostValidationError(
+                    "app_argument_invalid",
+                    name=key,
+                    error=message,
+                )
 
     def _call_config_script(self, action, env={}):
         from yunohost.hook import hook_exec

src/yunohost/tests/test_app_config.py

@@ -5,8 +5,11 @@ import pytest
 
 from .conftest import get_test_apps_dir
 
+from moulinette.utils.filesystem import read_file
+
 from yunohost.domain import _get_maindomain
 from yunohost.app import (
+    app_setting,
     app_install,
     app_remove,
     _is_installed,
@@ -15,7 +18,7 @@ from yunohost.app import (
     app_ssowatconf,
 )
 
-from yunohost.utils.error import YunohostValidationError
+from yunohost.utils.error import YunohostError, YunohostValidationError
 
 
 def setup_function(function):
@@ -128,17 +131,68 @@ def test_app_config_get_nonexistentstuff(config_app):
         app_config_get(config_app, "main.components.nonexistent")
 
 
-def test_app_config_set_boolean(config_app):
+def test_app_config_regular_setting(config_app):
 
     assert app_config_get(config_app, "main.components.boolean") is None
 
     app_config_set(config_app, "main.components.boolean", "no")
 
     assert app_config_get(config_app, "main.components.boolean") == "0"
+    assert app_setting(config_app, "boolean") == "0"
 
     app_config_set(config_app, "main.components.boolean", "yes")
 
     assert app_config_get(config_app, "main.components.boolean") == "1"
+    assert app_setting(config_app, "boolean") == "1"
 
     with pytest.raises(YunohostValidationError):
         app_config_set(config_app, "main.components.boolean", "pwet")
+
+
+def test_app_config_bind_on_file(config_app):
+
+    # c.f. conf/test.php in the config app
+    assert '$arg5= "Arg5 value";' in read_file("/var/www/config_app/test.php")
+    assert app_config_get(config_app, "bind.variable.arg5") == "Arg5 value"
+    assert app_setting(config_app, "arg5") is None
+
+    app_config_set(config_app, "bind.variable.arg5", "Foo Bar")
+
+    assert '$arg5= "Foo Bar";' in read_file("/var/www/config_app/test.php")
+    assert app_config_get(config_app, "bind.variable.arg5") == "Foo Bar"
+    assert app_setting(config_app, "arg5") == "Foo Bar"
+
+
+def test_app_config_custom_get(config_app):
+
+    assert app_setting(config_app, "arg9") is None
+    assert "Files in /var/www" in app_config_get(config_app, "bind.function.arg9")["ask"]["en"]
+    assert app_setting(config_app, "arg9") is None
+
+
+def test_app_config_custom_validator(config_app):
+
+    # c.f. the config script
+    # arg8 is a password that must be at least 8 chars
+    assert not os.path.exists("/var/www/config_app/password")
+    assert app_setting(config_app, "arg8") is None
+
+    with pytest.raises(YunohostValidationError):
+        app_config_set(config_app, "bind.function.arg8", "pZo6i7u91h")
+
+    assert not os.path.exists("/var/www/config_app/password")
+    assert app_setting(config_app, "arg8") is None
+
+
+def test_app_config_custom_set(config_app):
+
+    assert not os.path.exists("/var/www/config_app/password")
+    assert app_setting(config_app, "arg8") is None
+
+    app_config_set(config_app, "bind.function.arg8", "OneSuperStrongPassword")
+
+    assert os.path.exists("/var/www/config_app/password")
+    content = read_file("/var/www/config_app/password")
+    assert "OneSuperStrongPassword" not in content
+    assert content.startswith("$6$saltsalt$")
+    assert app_setting(config_app, "arg8") is None

src/yunohost/utils/config.py

@@ -135,6 +135,8 @@ class ConfigPanel:
 
         try:
             self._apply()
+        except YunohostError:
+            raise
         # Script got manually interrupted ...
         # N.B. : KeyboardInterrupt does not inherit from Exception
         except (KeyboardInterrupt, EOFError):
@@ -152,16 +154,10 @@ class ConfigPanel:
             # Delete files uploaded from API
             FileQuestion.clean_upload_dirs()
 
-        if self.errors:
-            return {
-                "errors": self.errors,
-            }
-
         self._reload_services()
 
         logger.success("Config updated as expected")
         operation_logger.success()
-        return {}
 
     def _get_toml(self):
         return read_toml(self.config_path)