From c5496870e55246f507994d63d4dc7a5e91e67fcc Mon Sep 17 00:00:00 2001
From: Ruslan Kuprieiev <kupruser@gmail.com>
Date: Mon, 14 Dec 2020 12:25:46 +0200
Subject: [PATCH] cli: config: check name format (#5090)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently we don't handle bad name well:

```
(dvc-3.8.3) ➜  dvc git:(config-bug) ✗ dvc config section
ERROR: unexpected error - not enough values to unpack (expected 2, got 1)
```

With this PR, we check name to conform to either `remote.name.option` or
`section.option`, so we'll give a more friendly:

```
(dvc-3.8.3) ➜  dvc git:(config-bug) ✗ dvc config section
ERROR: argument name: name argument should look like remote.name.option or section.option
usage: dvc config [-h] [--global | --system | --local] [-q | -v] [-u] [-l] [name] [value]

Get or set config options.
Documentation: <https://man.dvc.org/config>

positional arguments:
  name           Option name (remote.name.option or section.option).
  value          Option value.

optional arguments:
  -h, --help     show this help message and exit
  --global       Use global config.
  --system       Use system config.
  --local        Use local config.
  -q, --quiet    Be quiet.
  -v, --verbose  Be verbose.
  -u, --unset    Unset option.
  -l, --list     List all defined config values.
```
---
 dvc/command/config.py             | 49 ++++++++++++++++++++++++-------
 tests/func/test_cli.py            |  4 +--
 tests/func/test_config.py         | 16 ++++++++++
 tests/unit/command/test_config.py | 11 +++++++
 4 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/dvc/command/config.py b/dvc/command/config.py
index ab2ecafdf..d615aa742 100644
--- a/dvc/command/config.py
+++ b/dvc/command/config.py
@@ -7,6 +7,25 @@ from dvc.utils.flatten import flatten
 
 logger = logging.getLogger(__name__)
 
+NAME_REGEX = r"^(?P<remote>remote\.)?(?P<section>[^\.]*)\.(?P<option>[^\.]*)$"
+
+
+def _name_type(value):
+    import re
+
+    match = re.match(NAME_REGEX, value)
+    if not match:
+        raise argparse.ArgumentTypeError(
+            "name argument should look like "
+            "remote.name.option or "
+            "section.option"
+        )
+    return (
+        bool(match.group("remote")),
+        match.group("section").lower(),
+        match.group("option").lower(),
+    )
+
 
 class CmdConfig(CmdBaseNoRepo):
     def __init__(self, args):
@@ -31,20 +50,24 @@ class CmdConfig(CmdBaseNoRepo):
             logger.error("name argument is required")
             return 1
 
-        section, opt = self.args.name.lower().strip().split(".", 1)
+        remote, section, opt = self.args.name
 
         if self.args.value is None and not self.args.unset:
             conf = self.config.load_one(self.args.level)
-            self._check(conf, section, opt)
+            if remote:
+                conf = conf["remote"]
+            self._check(conf, remote, section, opt)
             logger.info(conf[section][opt])
             return 0
 
         with self.config.edit(self.args.level) as conf:
+            if remote:
+                conf = conf["remote"]
             if self.args.unset:
-                self._check(conf, section, opt)
+                self._check(conf, remote, section, opt)
                 del conf[section][opt]
             else:
-                self._check(conf, section)
+                self._check(conf, remote, section)
                 conf[section][opt] = self.args.value
 
         if self.args.name == "cache.type":
@@ -56,14 +79,15 @@ class CmdConfig(CmdBaseNoRepo):
 
         return 0
 
-    def _check(self, conf, section, opt=None):
+    def _check(self, conf, remote, section, opt=None):
+        name = "remote" if remote else "section"
         if section not in conf:
-            msg = "section {} doesn't exist"
-            raise ConfigError(msg.format(self.args.name))
+            raise ConfigError(f"{name} '{section}' doesn't exist")
 
         if opt and opt not in conf[section]:
-            msg = "option {} doesn't exist"
-            raise ConfigError(msg.format(self.args.name))
+            raise ConfigError(
+                f"option '{opt}' doesn't exist in {name} '{section}'"
+            )
 
     @staticmethod
     def _format_config(config):
@@ -114,7 +138,12 @@ def add_parser(subparsers, parent_parser):
         action="store_true",
         help="Unset option.",
     )
-    config_parser.add_argument("name", nargs="?", help="Option name.")
+    config_parser.add_argument(
+        "name",
+        nargs="?",
+        type=_name_type,
+        help="Option name (remote.name.option or section.option).",
+    )
     config_parser.add_argument("value", nargs="?", help="Option value.")
     config_parser.add_argument(
         "-l",
diff --git a/tests/func/test_cli.py b/tests/func/test_cli.py
index 8dc46bc26..93c45926d 100644
--- a/tests/func/test_cli.py
+++ b/tests/func/test_cli.py
@@ -141,14 +141,14 @@ class TestGCMultipleDvcRepos(TestDvc):
 
 class TestConfig(TestDvc):
     def test(self):
-        name = "param"
+        name = "section.option"
         value = "1"
 
         args = parse_args(["config", "-u", "--unset", name, value])
 
         self.assertIsInstance(args.func(args), CmdConfig)
         self.assertEqual(args.unset, True)
-        self.assertEqual(args.name, name)
+        self.assertEqual(args.name, (False, "section", "option"))
         self.assertEqual(args.value, value)
 
     def test_config_list(self):
diff --git a/tests/func/test_config.py b/tests/func/test_config.py
index 91e44643d..0e0d6bb01 100644
--- a/tests/func/test_config.py
+++ b/tests/func/test_config.py
@@ -158,3 +158,19 @@ def test_load_relative_paths(dvc, field, remote_url):
     assert cfg["remote"]["test"][field] == os.path.join(
         dvc_dir, "..", "file.txt"
     )
+
+
+def test_config_remote(tmp_dir, dvc, caplog):
+    (tmp_dir / ".dvc" / "config").write_text(
+        "['remote \"myremote\"']\n"
+        "  url = s3://bucket/path\n"
+        "  region = myregion\n"
+    )
+
+    caplog.clear()
+    assert main(["config", "remote.myremote.url"]) == 0
+    assert "s3://bucket/path" in caplog.text
+
+    caplog.clear()
+    assert main(["config", "remote.myremote.region"]) == 0
+    assert "myregion" in caplog.text
diff --git a/tests/unit/command/test_config.py b/tests/unit/command/test_config.py
index 94f3538d9..43f6ce78e 100644
--- a/tests/unit/command/test_config.py
+++ b/tests/unit/command/test_config.py
@@ -1,3 +1,6 @@
+import pytest
+
+from dvc.cli import DvcParserError, parse_args
 from dvc.command.config import CmdConfig
 
 
@@ -18,3 +21,11 @@ def test_config_formatter():
         "section_foo2.option_bar2.option_baz2=True",
         "section_foo2.option_baz3.option_baz4=False",
     )
+
+
+@pytest.mark.parametrize(
+    "name", ["way.too.long", "no_option", "remote.way.too.long"],
+)
+def test_config_bad_name(name):
+    with pytest.raises(DvcParserError):
+        parse_args(["config", name])