remote config validation (#3628)

* remote config validation

Fixes #3552

* solving config test fail

* Validation problem in config merging

sloving validation problem in config merging from different levels.
Only those config in lower levels can affect the validation process.

* reduce Cognitive Complexity

* Two request changes

1. Duplicate load current level of config
2. rewrite test to pytest style

* black it

* config: don't use all_levels

Co-authored-by: karajan1001 <[email protected]>
Co-authored-by: Ruslan Kuprieiev <[email protected]>

dvc/config.py

@@ -278,10 +278,7 @@ class Config(dict):
         Raises:
             ConfigError: thrown if config has an invalid format.
         """
-        conf = {}
-        for level in self.LEVELS:
-            if level in self.files:
-                _merge(conf, self.load_one(level))
+        conf = self._load_config_to_level()
 
         if validate:
             conf = self.validate(conf)
@@ -333,6 +330,15 @@ class Config(dict):
         dirs_schema = {"cache": {"dir": func}, "remote": {str: {"url": func}}}
         return Schema(dirs_schema, extra=ALLOW_EXTRA)(conf)
 
+    def _load_config_to_level(self, level=None):
+        merged_conf = {}
+        for merge_level in self.LEVELS:
+            if merge_level == level:
+                break
+            if merge_level in self.files:
+                _merge(merged_conf, self.load_one(merge_level))
+        return merged_conf
+
     @contextmanager
     def edit(self, level="repo"):
         if level in {"repo", "local"} and self.dvc_dir is None:
@@ -342,6 +348,11 @@ class Config(dict):
         yield conf
 
         conf = self._save_paths(conf, self.files[level])
+
+        merged_conf = self._load_config_to_level(level)
+        _merge(merged_conf, conf)
+        self.validate(merged_conf)
+
         _save_config(self.files[level], conf)
         self.load()
 

tests/func/test_config.py

@@ -94,6 +94,12 @@ def test_merging_two_levels(dvc):
     with dvc.config.edit() as conf:
         conf["remote"]["test"] = {"url": "ssh://example.com"}
 
+    with pytest.raises(
+        ConfigError, match=r"expected 'url' for dictionary value"
+    ):
+        with dvc.config.edit("global") as conf:
+            conf["remote"]["test"] = {"password": "1"}
+
     with dvc.config.edit("local") as conf:
         conf["remote"]["test"] = {"password": "1"}
 

tests/func/test_remote.py

@@ -254,3 +254,19 @@ def test_push_order(tmp_dir, dvc, tmp_path_factory, mocker):
     dvc.push()
     # last uploaded file should be dir checksum
     assert mocked_upload.call_args[0][0].endswith(".dir")
+
+
+def test_remote_modify_validation(dvc):
+    remote_name = "drive"
+    unsupported_config = "unsupported_config"
+    assert (
+        main(["remote", "add", "-d", remote_name, "gdrive://test/test"]) == 0
+    )
+    assert (
+        main(
+            ["remote", "modify", remote_name, unsupported_config, "something"]
+        )
+        == 251
+    )
+    config = configobj.ConfigObj(dvc.config.files["repo"])
+    assert unsupported_config not in config['remote "{}"'.format(remote_name)]