Repository: radiocosmology/alpenhorn

Issue:
Deal with no config in CLI
Right now all we see is:
```
Error: No configuration files available.
```
which is true, but not a helpful thing to say to the user.

The problem is: without a config file, we don't know how to connect to the database.

Discussion:

Solution patch:
diff --git a/alpenhorn/cli/acq/create.py b/alpenhorn/cli/acq/create.py
index 7e49b7777..87efb9202 100644
--- a/alpenhorn/cli/acq/create.py
+++ b/alpenhorn/cli/acq/create.py
@@ -3,6 +3,7 @@
 import click
 import peewee as pw
 
+from ...common.util import invalid_import_path
 from ...db import database_proxy, ArchiveAcq
 from ..options import cli_option
 from ..cli import echo
@@ -17,6 +18,11 @@ def create(name):
     another existing Acquisition.
     """
 
+    # Validate
+    rejection_reason = invalid_import_path(name)
+    if rejection_reason:
+        raise click.ClickException(f"invalid name: {rejection_reason}")
+
     with database_proxy.atomic():
         try:
             ArchiveAcq.get(name=name)
diff --git a/alpenhorn/cli/entry.py b/alpenhorn/cli/entry.py
index 20abc72b8..2232cf1a0 100644
--- a/alpenhorn/cli/entry.py
+++ b/alpenhorn/cli/entry.py
@@ -5,7 +5,7 @@
 import click
 
 from .. import db
-from ..common.util import start_alpenhorn, version_option
+from ..common.util import help_config_option, start_alpenhorn, version_option
 
 from . import acq, db, file, group, node
 from .options import not_both
@@ -65,6 +65,7 @@ def _verbosity_from_cli(verbose: int, debug: bool, quiet: int) -> int:
     show_default=False,
     default=False,
 )
+@help_config_option
 def entry(conf, quiet, verbose, debug):
     """Alpenhorn data management system.
 
diff --git a/alpenhorn/cli/file/create.py b/alpenhorn/cli/file/create.py
index d091c76f7..a54ee6435 100644
--- a/alpenhorn/cli/file/create.py
+++ b/alpenhorn/cli/file/create.py
@@ -4,7 +4,7 @@
 import pathlib
 import peewee as pw
 
-from ...common.util import md5sum_file
+from ...common.util import md5sum_file, invalid_import_path
 from ...db import (
     ArchiveAcq,
     ArchiveFile,
@@ -91,6 +91,11 @@ def create(name, acq_name, from_file, md5, prefix, size):
     if size is not None and size < 0:
         raise click.ClickException(f"negative file size.")
 
+    # Validate
+    rejection_reason = invalid_import_path(name)
+    if rejection_reason:
+        raise click.ClickException(f"invalid name: {rejection_reason}")
+
     validate_md5(md5)
 
     # Scan a file, if requested
diff --git a/alpenhorn/common/config.py b/alpenhorn/common/config.py
index 4adf4ce99..e5c6f05c9 100644
--- a/alpenhorn/common/config.py
+++ b/alpenhorn/common/config.py
@@ -182,7 +182,7 @@ def load_config(cli_conf: os.PathLike, cli: bool) -> None:
     if cli_conf:
         config_files.append(cli_conf)
 
-    any_exist = False
+    no_config = True
 
     for cfile in config_files:
         # Expand the configuration file path
@@ -198,7 +198,7 @@ def load_config(cli_conf: os.PathLike, cli: bool) -> None:
                 )
             continue
 
-        any_exist = True
+        no_config = False
 
         log.info("Loading config file %s", cfile)
 
@@ -208,13 +208,10 @@ def load_config(cli_conf: os.PathLike, cli: bool) -> None:
         if conf is not None:
             config = merge_dict_tree(config, conf)
 
-    if not any_exist:
-        if cli:
-            exc = ClickException
-        else:
-            exc = RuntimeError
-
-        raise exc("No configuration files available.")
+    if no_config:
+        raise ClickException(
+            "No configuration files available.  See --help-config for more details."
+        )
 
 
 def merge_dict_tree(a: dict, b: dict) -> dict:
diff --git a/alpenhorn/common/util.py b/alpenhorn/common/util.py
index 905b75285..625719dee 100644
--- a/alpenhorn/common/util.py
+++ b/alpenhorn/common/util.py
@@ -14,6 +14,62 @@
 log = logging.getLogger(__name__)
 
 
+def help_config_option(func):
+    """Click --help-config option"""
+
+    # This is the callback
+    def _help_config(ctx, param, value):
+        if not value or ctx.resilient_parsing:
+            return
+
+        click.echo(
+            """
+In order to operate, alpenhorn needs to be configured to point it to the
+SQL database containing its Data Index.  This is done through one or more
+alpenhorn config files.
+
+Alpenhorn searches for config files in the following order:
+\b
+  * /etc/alpenhorn/alpenhorn.conf
+  * /etc/xdg/alpenhorn/alpenhorn.conf
+  * ~/.config/alpenhorn/alpenhorn.conf
+  * the value of the "ALPENHORN_CONFIG_FILE" environment variable
+  * the path passed via "-c" or "--conf" on the command line
+
+If multiple config files from this list are found, all will be read,
+with config from later files overriding earlier ones.  The config files
+are YAML.
+
+Typically, to configure the database connection, the config should define
+a database URL, with a YAML config like this:
+\b
+database:
+	url: mysql://user:passwd@hostname:port/my_database
+
+However, an alternate way to provide database configuration to alpenhorn
+is through a database extension module.  If you use such an extension, you
+should declare it in the config, instead, so alpenhorn can load it:
+\b
+extensions:
+	- my_extensions.alpenhorn_db_extension
+
+The database connection is the only thing that can be configured for the
+alpenhorn CLI.  But further configuration of the daemon is possible.
+Consult the alpenhorn documentation for more information.
+"""
+        )
+        ctx.exit(0)
+
+    return click.option(
+        "--help-config",
+        is_flag=True,
+        callback=_help_config,
+        expose_value=False,
+        is_eager=True,
+        help="Help on configuring alpenhorn.",
+    )(func)
+
+
 def version_option(func):
     """Click --version option"""
     return click.option(
@@ -313,3 +369,47 @@ def pretty_deltat(seconds: float) -> str:
 
     # For short durations, include tenths of a second
     return f"{seconds:.1f}s"
+
+
+def invalid_import_path(name: str) -> str | None:
+    """Is `name` invalid as an import path?
+
+    i.e., can `name` be used as an ArchiveAcq or
+    ArchiveFile name (or both, combined).
+
+    Returns
+    -------
+    rejection_reason: str or None
+        A string describing why `name` was rejected,
+        or None, if the name was valid.
+    """
+
+    # Can't be the null string
+    if name == "":
+        return "empty path"
+
+    # Can't simply be "." or ".."
+    if name == "." or name == "..":
+        return "invalid path"
+
+    # Can't start with "/" or "./" or "../"
+    if name.startswith("/") or name.startswith("./") or name.startswith("../"):
+        return "invalid start"
+
+    # Can't end with "/" or "/." or "/.."
+    if name.endswith("/") or name.endswith("/.") or name.endswith("/.."):
+        return "invalid end"
+
+    # Can't have multiple "/" in a row
+    if "//" in name:
+        return "repeated /"
+
+    # Can't have internal "/./"
+    if "/./" in name:
+        return 'invalid path element "."'
+
+    # Can't have internal "/../"
+    if "/../" in name:
+        return 'invalid path element ".."'
+
+    return None
diff --git a/alpenhorn/daemon/auto_import.py b/alpenhorn/daemon/auto_import.py
index a25852d1f..224132424 100644
--- a/alpenhorn/daemon/auto_import.py
+++ b/alpenhorn/daemon/auto_import.py
@@ -15,6 +15,7 @@
 
 from .. import db
 from ..common import config, extensions
+from ..common.util import invalid_import_path
 from ..db import (
     ArchiveAcq,
     ArchiveFile,
@@ -173,6 +174,15 @@ def _import_file(
             req.complete()
         return
 
+    # Vet acq_name from extension
+    rejection_reason = invalid_import_path(str(acq_name))
+    if rejection_reason:
+        log.warning(f'Rejecting invalid acq path "{acq_name}": {rejection_reason}')
+        if req:
+            log.info(f"Completed import request #{req.id}.")
+            req.complete()
+        return
+
     file_name = path.relative_to(acq_name)
 
     # If a copy already exists, we're done
diff --git a/alpenhorn/daemon/entry.py b/alpenhorn/daemon/entry.py
index 5910c93cd..f46b24665 100755
--- a/alpenhorn/daemon/entry.py
+++ b/alpenhorn/daemon/entry.py
@@ -6,7 +6,7 @@
 
 from .. import db
 from ..common import config
-from ..common.util import start_alpenhorn, version_option
+from ..common.util import help_config_option, start_alpenhorn, version_option
 from ..scheduler import FairMultiFIFOQueue, pool
 from . import auto_import, update
 
@@ -33,6 +33,7 @@ def log_exception(*args):
     metavar="FILE",
 )
 @version_option
+@help_config_option
 def entry(conf):
     """Alpenhorn data management daemon.
 
diff --git a/alpenhorn/daemon/update.py b/alpenhorn/daemon/update.py
index 08c2aede5..9d71d6bc9 100644
--- a/alpenhorn/daemon/update.py
+++ b/alpenhorn/daemon/update.py
@@ -491,7 +491,29 @@ def update_import(self) -> None:
                 continue
 
             if req.recurse:
-                # If recursion was requested, run a scan on the path
+                # Ensure the base directory we want to scan is in-tree
+                fullpath = pathlib.Path(self.db.root, path)
+                try:
+                    fullpath = fullpath.resolve(strict=True)
+                except OSError as e:
+                    log.warning(
+                        f"Ignoring import request of unresolvable scan path: {fullpath}"
+                    )
+                    req.complete()
+                    continue
+
+                # Recompute the relative path after resolution, or skip scan if we're
+                # now out-of-tree
+                try:
+                    path = fullpath.relative_to(self.db.root)
+                except ValueError:
+                    log.warning(
+                        f"Ignoring import request of out-of-tree scan path: {fullpath}"
+                    )
+                    req.complete()
+                    continue
+
+                # Run scan
                 Task(
                     func=auto_import.scan,
                     queue=self._queue,
@@ -500,7 +522,17 @@ def update_import(self) -> None:
                     name=f'Scan "{path}" on {self.name}',
                 )
             else:
-                # Otherwise, try to directly import the path
+                # Check that the import path is valid
+                rejection_reason = util.invalid_import_path(req.path)
+                if rejection_reason:
+                    log.warning(
+                        f'Ignoring request for import of invalid path "{req.path}": '
+                        + rejection_reason
+                    )
+                    req.complete()
+                    continue
+
+                # Try to directly import the path
                 auto_import.import_file(self, self._queue, req.path, req.register, req)
 
     def update(self) -> None:


Test patch:
diff --git a/tests/cli/acq/test_create.py b/tests/cli/acq/test_create.py
index 3794d594d..ad50a09f6 100644
--- a/tests/cli/acq/test_create.py
+++ b/tests/cli/acq/test_create.py
@@ -14,6 +14,15 @@ def test_create(clidb, cli):
     ArchiveAcq.get(name="TEST")
 
 
+def test_bad_name(clidb, cli):
+    """Test rejection of invalid name."""
+
+    cli(1, ["acq", "create", "/test/"])
+
+    # No acq was created
+    assert ArchiveAcq.select().count() == 0
+
+
 def test_create_existing(clidb, cli):
     """Test creating an acq that already exists."""
 
diff --git a/tests/cli/file/test_create.py b/tests/cli/file/test_create.py
index cc179bf33..e62576479 100644
--- a/tests/cli/file/test_create.py
+++ b/tests/cli/file/test_create.py
@@ -16,6 +16,24 @@ def test_no_data(clidb, cli):
     cli(2, ["file", "create", "name", "Acq", "--size=3"])
 
 
+def test_bad_name(clidb, cli):
+    """Test an invalid file name."""
+
+    ArchiveAcq.create(name="Acq")
+
+    cli(
+        1,
+        [
+            "file",
+            "create",
+            "name/../name",
+            "Acq",
+            "--md5=0123456789ABCDEF0123456789ABCDEF",
+            "--size=3",
+        ],
+    )
+
+
 def test_negative_size(clidb, cli):
     """Test a negative size."""
 
diff --git a/tests/common/test_config.py b/tests/common/test_config.py
index b3a5f2095..d145c0610 100644
--- a/tests/common/test_config.py
+++ b/tests/common/test_config.py
@@ -1,6 +1,8 @@
-import os
+"""Test common.config"""
 
+import os
 import pytest
+from click import ClickException
 
 from alpenhorn.common import config
 
@@ -15,7 +17,7 @@ def merge_dict(a, b):
 def test_no_config():
     # Check that alpenhorn fails if it has no appropriate configuration
 
-    with pytest.raises(RuntimeError) as excinfo:
+    with pytest.raises(ClickException) as excinfo:
         config.load_config(None, False)
 
     assert "No configuration" in str(excinfo.value)
diff --git a/tests/common/test_util.py b/tests/common/test_util.py
index 77f3817ca..697ad62a9 100644
--- a/tests/common/test_util.py
+++ b/tests/common/test_util.py
@@ -127,3 +127,33 @@ def test_pretty_deltat():
     assert util.pretty_deltat(0.01) == "0.0s"
     assert util.pretty_deltat(0) == "0.0s"
     assert util.pretty_deltat(-1) == "-1.0s"
+
+
+def test_invalid_import_path():
+    """Test invalid_import_path"""
+
+    # Explicitly forbidden names
+    assert util.invalid_import_path("") is not None
+    assert util.invalid_import_path(".") is not None
+    assert util.invalid_import_path("..") is not None
+
+    # Forbidden starts
+    assert util.invalid_import_path("/name") is not None
+    assert util.invalid_import_path("./name") is not None
+    assert util.invalid_import_path("../name") is not None
+
+    # Forbidden ends
+    assert util.invalid_import_path("name/") is not None
+    assert util.invalid_import_path("name/.") is not None
+    assert util.invalid_import_path("name/..") is not None
+
+    # Forbidden middles
+    assert util.invalid_import_path("name//name") is not None
+    assert util.invalid_import_path("name///name") is not None
+    assert util.invalid_import_path("name/./name") is not None
+    assert util.invalid_import_path("name/../name") is not None
+
+    # These are fine
+    assert util.invalid_import_path("name") is None
+    assert util.invalid_import_path("name/name") is None
+    assert util.invalid_import_path("name/.../name") is None
diff --git a/tests/daemon/test_auto_import.py b/tests/daemon/test_auto_import.py
index 9e2a158e6..676e9d01c 100644
--- a/tests/daemon/test_auto_import.py
+++ b/tests/daemon/test_auto_import.py
@@ -104,6 +104,24 @@ def test_import_file_no_detect(dbtables, unode):
         ArchiveAcq.get(name="acq")
 
 
+def test_import_file_invalid_acqname(dbtables, unode):
+    """Test invalid acq_name from import_detect."""
+
+    with patch(
+        "alpenhorn.common.extensions._id_ext", [lambda path, node: ("acq/", None)]
+    ):
+        with pytest.raises(StopIteration):
+            next(
+                auto_import._import_file(
+                    None, unode, pathlib.PurePath("acq/file"), True, None
+                )
+            )
+
+    # No acq has been added
+    with pytest.raises(pw.DoesNotExist):
+        ArchiveAcq.get(name="acq")
+
+
 def test_import_file_locked(xfs, dbtables, unode):
     """Test bad file in _import_file()"""
 
diff --git a/tests/daemon/test_update_node.py b/tests/daemon/test_update_node.py
index 7c0704d8a..2cd3db15f 100644
--- a/tests/daemon/test_update_node.py
+++ b/tests/daemon/test_update_node.py
@@ -514,6 +514,52 @@ def test_update_import_absolute(unode, queue, archivefileimportrequest):
     assert ArchiveFileCopy.select().count() == 0
 
 
+def test_update_import_invalid_path(unode, queue, archivefileimportrequest):
+    """update_import() should ignore invalid paths."""
+
+    afir = archivefileimportrequest(path="./path/..", node=unode.db)
+
+    unode.update_import()
+
+    # Nothing queued
+    assert queue.qsize == 0
+
+    # Request is completed
+    assert ArchiveFileImportRequest.get(id=afir.id).completed == 1
+
+
+def test_update_import_outtree_scan(unode, xfs, queue, archivefileimportrequest):
+    """Test rejection of out-of-tree scan paths from import requests"""
+
+    xfs.create_dir("/node/import")
+
+    afir = archivefileimportrequest(path="..", node=unode.db, recurse=True)
+
+    unode.update_import()
+
+    # Nothing queued
+    assert queue.qsize == 0
+
+    # Request is completed
+    assert ArchiveFileImportRequest.get(id=afir.id).completed == 1
+
+
+def test_update_import_unresolve_scan(unode, xfs, queue, archivefileimportrequest):
+    """Test rejection of unresolvable scan paths from import requests"""
+
+    xfs.create_dir("/node/import")
+
+    afir = archivefileimportrequest(path="path/subpath/..", node=unode.db, recurse=True)
+
+    unode.update_import()
+
+    # Nothing queued
+    assert queue.qsize == 0
+
+    # Request is completed
+    assert ArchiveFileImportRequest.get(id=afir.id).completed == 1
+
+
 def test_update_import_no_recurse(unode, queue, archivefileimportrequest):
     """Test update_import() with a non-recursive import request."""
 
@@ -532,9 +578,11 @@ def test_update_import_no_recurse(unode, queue, archivefileimportrequest):
     mock.assert_called_with(unode, queue, "import/path", True, afir)
 
 
-def test_update_import_scan(unode, queue, archivefileimportrequest):
+def test_update_import_scan(unode, xfs, queue, archivefileimportrequest):
     """Test update_import() with a recursive import request."""
 
+    xfs.create_dir("/node/import/path")
+
     afir = archivefileimportrequest(
         path="import/path", node=unode.db, recurse=True, register=True
     )