Disallow nested run directories

CHANGES.md

@@ -100,6 +100,10 @@ task outputs/message triggers are now validated.
 [#3614](https://github.com/cylc/cylc-flow/pull/3795) - Fix error when running
 `cylc ping --verbose $SUITE`.
 
+[#3852](https://github.com/cylc/cylc-flow/pull/3852) - Prevents registering a
+workflow in a sub-directory of a run directory (as `cylc scan` would not be
+able to find it).
+
 -------------------------------------------------------------------------------
 ## __cylc-8.0a2 (2020-07-03)__
 

cylc/flow/network/scan.py

@@ -67,7 +67,8 @@
     ContactFileFields,
     SuiteFiles,
     get_suite_title,
-    load_contact_file_async
+    load_contact_file_async,
+    MAX_SCAN_DEPTH
 )
 
 
@@ -103,7 +104,7 @@ def dir_is_flow(listing):
 
 
 @pipe
-async def scan(run_dir=None, scan_dir=None, max_depth=4):
+async def scan(run_dir=None, scan_dir=None, max_depth=MAX_SCAN_DEPTH):
     """List flows installed on the filesystem.
 
     Args:

cylc/flow/suite_files.py

@@ -223,6 +223,8 @@ class ContactFileFields:
 
 PS_OPTS = '-wopid,args'
 
+MAX_SCAN_DEPTH = 4  # How many subdir levels down to look for valid run dirs
+
 CONTACT_FILE_EXISTS_MSG = r"""suite contact file exists: %(fname)s
 
 Suite "%(suite)s" is already running, and listening at "%(host)s:%(port)s".
@@ -345,8 +347,7 @@ def get_contact_file(reg):
 def get_flow_file(reg, suite_owner=None):
     """Return the path of a suite's flow.cylc file."""
     return os.path.join(
-        get_suite_source_dir(reg, suite_owner),
-        SuiteFiles.FLOW_FILE)
+        get_suite_source_dir(reg, suite_owner), SuiteFiles.FLOW_FILE)
 
 
 def get_suite_source_dir(reg, suite_owner=None):
@@ -481,19 +482,20 @@ def register(reg=None, source=None, redirect=False, rundir=None):
             No flow.cylc file found in source location.
             Illegal name (can look like a relative path, but not absolute).
             Another suite already has this name (unless --redirect).
+            Trying to register a suite nested inside of another.
     """
     if reg is None:
         reg = os.path.basename(os.getcwd())
 
     is_valid, message = SuiteNameValidator.validate(reg)
     if not is_valid:
-        raise SuiteServiceFileError(
-            f'invalid suite name - {message}'
-        )
+        raise SuiteServiceFileError(f'invalid suite name - {message}')
 
     if os.path.isabs(reg):
         raise SuiteServiceFileError(
-            "suite name cannot be an absolute path: %s" % reg)
+            f'suite name cannot be an absolute path: {reg}')
+
+    check_nested_run_dirs(reg)
 
     if source is not None:
         if os.path.basename(source) == SuiteFiles.FLOW_FILE:
@@ -553,14 +555,12 @@ def register(reg=None, source=None, redirect=False, rundir=None):
     if orig_source is not None and source != orig_source:
         if not redirect:
             raise SuiteServiceFileError(
-                "the name '%s' already points to %s.\nUse "
-                "--redirect to re-use an existing name and run "
-                "directory." % (reg, orig_source))
+                f"the name '{reg}' already points to {orig_source}.\nUse "
+                "--redirect to re-use an existing name and run directory.")
         LOG.warning(
-            "the name '%(reg)s' points to %(old)s.\nIt will now"
-            " be redirected to %(new)s.\nFiles in the existing %(reg)s run"
-            " directory will be overwritten.\n",
-            {'reg': reg, 'old': orig_source, 'new': source})
+            f"the name '{reg}' points to {orig_source}.\nIt will now be "
+            f"redirected to {source}.\nFiles in the existing {reg} run "
+            "directory will be overwritten.\n")
         # Remove symlink to the original suite.
         os.unlink(os.path.join(srv_d, SuiteFiles.Service.SOURCE))
 
@@ -576,7 +576,7 @@ def register(reg=None, source=None, redirect=False, rundir=None):
             source_str = source
         os.symlink(source_str, target)
 
-    print('REGISTERED %s -> %s' % (reg, source))
+    print(f'REGISTERED {reg} -> {source}')
     return reg
 
 
@@ -669,3 +669,61 @@ def _load_local_item(item, path):
             return file_.read()
     except IOError:
         return None
+
+
+def check_nested_run_dirs(reg):
+    """Disallow nested run dirs e.g. trying to register foo/bar where foo is
+    already a valid suite directory.
+
+    Args:
+        reg (str): suite name
+    """
+    exc_msg = (
+        'Nested run directories not allowed - cannot register suite name '
+        '"%s" as "%s" is already a valid run directory.')
+
+    def _check_child_dirs(path, depth_count=1):
+        for result in os.scandir(path):
+            if result.is_dir() and not result.is_symlink():
+                if is_valid_run_dir(result.path):
+                    raise SuiteServiceFileError(exc_msg % (reg, result.path))
+                if depth_count < MAX_SCAN_DEPTH:
+                    _check_child_dirs(result.path, depth_count + 1)
+
+    reg_path = os.path.normpath(reg)
+    parent_dir = os.path.dirname(reg_path)
+    while parent_dir != '':
+        if is_valid_run_dir(parent_dir):
+            raise SuiteServiceFileError(
+                exc_msg % (reg, get_cylc_run_abs_path(parent_dir)))
+        parent_dir = os.path.dirname(parent_dir)
+
+    reg_path = get_cylc_run_abs_path(reg_path)
+    if os.path.isdir(reg_path):
+        _check_child_dirs(reg_path)
+
+
+def is_valid_run_dir(path):
+    """Return True if path is a valid run directory, else False.
+
+    Args:
+        path (str): if this is a relative path, it is taken to be relative to
+            the cylc-run directory.
+    """
+    path = get_cylc_run_abs_path(path)
+    if os.path.isdir(os.path.join(path, SuiteFiles.Service.DIRNAME)):
+        return True
+    return False
+
+
+def get_cylc_run_abs_path(path):
+    """Return the absolute path under the cylc-run directory for the specified
+    relative path.
+
+    If the specified path is already absolute, just return it.
+    The path need not exist.
+    """
+    if os.path.isabs(path):
+        return path
+    cylc_run_dir = os.path.expandvars(get_platform()['run directory'])
+    return os.path.join(cylc_run_dir, path)

tests/unit/test_suite_files.py

@@ -14,10 +14,10 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import unittest
-
+import pytest
 from unittest import mock
 
+import os.path
 from cylc.flow import suite_files
 from cylc.flow.exceptions import SuiteServiceFileError
 
@@ -180,66 +180,146 @@ def get_register_test_cases():
     ]
 
 
-class TestSuiteFiles(unittest.TestCase):
-
-    @mock.patch('os.unlink')
-    @mock.patch('os.makedirs')
-    @mock.patch('os.symlink')
-    @mock.patch('os.readlink')
-    @mock.patch('os.path.isfile')
-    @mock.patch('os.path.isabs')
-    @mock.patch('os.getcwd')
-    @mock.patch('os.path.abspath')
-    @mock.patch('cylc.flow.suite_files.get_suite_srv_dir')
-    def test_register(
-            self,
-            mocked_get_suite_srv_dir,
-            mocked_abspath,
-            mocked_getcwd,
-            mocked_isabs,
-            mocked_isfile,
-            mocked_readlink,
-            mocked_symlink,
-            mocked_makedirs,
-            mocked_unlink
-    ):
-        """Test the register function."""
-        def mkdirs_standin(_, exist_ok=False):
-            return True
-
-        mocked_abspath.side_effect = lambda x: x
-
-        for reg, source, redirect, cwd, isabs, isfile, \
-            suite_srv_dir, readlink, expected_symlink, \
-            expected, e_expected, e_message \
-                in get_register_test_cases():
-            mocked_getcwd.side_effect = lambda: cwd
-            mocked_isabs.side_effect = lambda x: isabs
-
-            mocked_isfile.side_effect = lambda x: isfile
-            mocked_get_suite_srv_dir.return_value = str(suite_srv_dir)
-            mocked_makedirs.return_value = True
-            mocked_unlink.return_value = True
-            if readlink == OSError:
-                mocked_readlink.side_effect = readlink
-            else:
-                mocked_readlink.side_effect = lambda x: readlink
-
-            if e_expected is None:
-                reg = suite_files.register(reg, source, redirect)
-                self.assertEqual(expected, reg)
-                if mocked_symlink.call_count > 0:
-                    # first argument, of the first call
-                    arg0 = mocked_symlink.call_args[0][0]
-                    self.assertEqual(expected_symlink, arg0)
-            else:
-                with self.assertRaises(e_expected) as cm:
-                    suite_files.register(reg, source, redirect)
-                if e_message is not None:
-                    the_exception = cm.exception
-                    self.assertTrue(e_message in str(the_exception),
-                                    str(the_exception))
-
-
-if __name__ == '__main__':
-    unittest.main()
+@mock.patch('os.unlink')
+@mock.patch('os.makedirs')
+@mock.patch('os.symlink')
+@mock.patch('os.readlink')
+@mock.patch('os.path.isfile')
+@mock.patch('os.path.isabs')
+@mock.patch('os.getcwd')
+@mock.patch('os.path.abspath')
+@mock.patch('cylc.flow.suite_files.get_suite_srv_dir')
+@mock.patch('cylc.flow.suite_files.check_nested_run_dirs')
+def test_register(mocked_check_nested_run_dirs,
+                  mocked_get_suite_srv_dir,
+                  mocked_abspath,
+                  mocked_getcwd,
+                  mocked_isabs,
+                  mocked_isfile,
+                  mocked_readlink,
+                  mocked_symlink,
+                  mocked_makedirs,
+                  mocked_unlink):
+    """Test the register function."""
+    def mkdirs_standin(_, exist_ok=False):
+        return True
+
+    mocked_abspath.side_effect = lambda x: x
+    # mocked_check_nested_run_dirs - no side effect as we're just ignoring it
+
+    for (reg, source, redirect, cwd, isabs, isfile, suite_srv_dir,
+            readlink, expected_symlink, expected, e_expected,
+            e_message) in get_register_test_cases():
+        mocked_getcwd.side_effect = lambda: cwd
+        mocked_isabs.side_effect = lambda x: isabs
+
+        mocked_isfile.side_effect = lambda x: isfile
+        mocked_get_suite_srv_dir.return_value = str(suite_srv_dir)
+        mocked_makedirs.return_value = True
+        mocked_unlink.return_value = True
+        if readlink == OSError:
+            mocked_readlink.side_effect = readlink
+        else:
+            mocked_readlink.side_effect = lambda x: readlink
+
+        if e_expected is None:
+            reg = suite_files.register(reg, source, redirect)
+            assert reg == expected
+            if mocked_symlink.call_count > 0:
+                # first argument, of the first call
+                arg0 = mocked_symlink.call_args[0][0]
+                assert arg0 == expected_symlink
+        else:
+            with pytest.raises(e_expected) as exc:
+                suite_files.register(reg, source, redirect)
+            if e_message is not None:
+                assert e_message in str(exc.value)
+
+
+@pytest.mark.parametrize(
+    'path, expected',
+    [('a/b/c', '/mock_cylc_dir/a/b/c'),
+     ('/a/b/c', '/a/b/c')]
+)
+def test_get_cylc_run_abs_path(path, expected, monkeypatch):
+    monkeypatch.setattr('cylc.flow.suite_files.get_platform',
+                        lambda: {'run directory': '/mock_cylc_dir'})
+    assert suite_files.get_cylc_run_abs_path(path) == expected
+
+
+@pytest.mark.parametrize(
+    'path, expected',
+    [('service/dir/exists', True),
+     ('flow/file/exists', False),  # Non-run dirs can still contain flow.cylc
+     ('nothing/exists', False)]
+)
+@pytest.mark.parametrize('is_abs_path', [False, True])
+def test_is_valid_run_dir(path, expected, is_abs_path, monkeypatch):
+    """Test that a directory is correctly identified as a valid run dir when
+    it contains a service dir.
+    """
+    prefix = os.sep if is_abs_path is True else 'mock_cylc_dir'
+    flow_file = os.path.join(prefix, 'flow', 'file', 'exists', 'flow.cylc')
+    serv_dir = os.path.join(prefix, 'service', 'dir', 'exists', '.service')
+    monkeypatch.setattr('os.path.isfile', lambda x: x == flow_file)
+    monkeypatch.setattr('os.path.isdir', lambda x: x == serv_dir)
+    monkeypatch.setattr('cylc.flow.suite_files.get_platform',
+                        lambda: {'run directory': 'mock_cylc_dir'})
+    path = os.path.normpath(path)
+    if is_abs_path:
+        path = os.path.join(os.sep, path)
+
+    assert suite_files.is_valid_run_dir(path) is expected, (
+        f'Is "{path}" a valid run dir?')
+
+
+@pytest.mark.parametrize('direction', ['parents', 'children'])
+def test_register_nested_run_dirs(direction, monkeypatch):
+    """Test that a suite cannot be registered in a subdir of another suite."""
+    monkeypatch.setattr('cylc.flow.suite_files.get_cylc_run_abs_path',
+                        lambda x: x)
+
+    if direction == "parents":
+        monkeypatch.setattr('cylc.flow.suite_files.os.scandir', lambda x: [])
+        monkeypatch.setattr('cylc.flow.suite_files.is_valid_run_dir',
+                            lambda x: x == os.path.join('bright', 'falls'))
+        # Not nested in run dir - ok:
+        suite_files.check_nested_run_dirs('alan/wake')
+        # It is itself a run dir - ok:
+        suite_files.check_nested_run_dirs('bright/falls')
+        # Nested in a run dir - bad:
+        for func in (suite_files.check_nested_run_dirs, suite_files.register):
+            for path in ('bright/falls/light', 'bright/falls/light/and/power'):
+                with pytest.raises(SuiteServiceFileError) as exc:
+                    func(path)
+                assert 'Nested run directories not allowed' in str(exc.value)
+
+    else:
+        dirs = ['a', 'a/a', 'a/R', 'a/a/a', 'a/a/R',
+                'a/b', 'a/b/a', 'a/b/b',
+                'a/c', 'a/c/a', 'a/c/a/a', 'a/c/a/a/a', 'a/c/a/a/a/R',
+                'a/d', 'a/d/a', 'a/d/a/a', 'a/d/a/a/a', 'a/d/a/a/a/a',
+                'a/d/a/a/a/a/R']
+        run_dirs = [d for d in dirs if 'R' in d]
+
+        def mock_scandir(path):
+            return [mock.Mock(path=d, is_dir=lambda: True,
+                              is_symlink=lambda: False) for d in dirs
+                    if (d.startswith(path) and len(d) == len(path) + 2)]
+        monkeypatch.setattr('cylc.flow.suite_files.os.scandir', mock_scandir)
+        monkeypatch.setattr('cylc.flow.suite_files.os.path.isdir',
+                            lambda x: x in dirs)
+        monkeypatch.setattr('cylc.flow.suite_files.is_valid_run_dir',
+                            lambda x: x in run_dirs)
+
+        # No run dir nested below - ok:
+        for path in ('a/a/a', 'a/b'):
+            suite_files.check_nested_run_dirs(path)
+        # Run dir nested below - bad:
+        for func in (suite_files.check_nested_run_dirs, suite_files.register):
+            for path in ('a', 'a/a', 'a/c'):
+                with pytest.raises(SuiteServiceFileError) as exc:
+                    func(path)
+                assert 'Nested run directories not allowed' in str(exc.value)
+        # Run dir nested below max scan depth - not ideal but passes:
+        suite_files.check_nested_run_dirs('a/d')