From a55bc587e9c343509e95bd93961e27e331882834 Mon Sep 17 00:00:00 2001 From: Vidar Tonaas Fauske Date: Thu, 8 Dec 2022 17:03:27 +0000 Subject: [PATCH] Make preferred_dir content manager trait (#983) * fix terminal test on Windows * Make preferred_dir content manager trait Move the preferred_dir trait to content manager, but keep the old one for backwards compatibility. The new location should also cause the value to be read later in the init, allowing us to avoid the deferred validation. Also fixes an issue with escaping the root dir on Windows if an absolute path is passed. * Remove leading "/" from preferred_dir Also validates set value to strip any leading slashes * Allow filemanager to have OS path set Will transparently convert to API path in validator. * fix mypy * fix ruff --- jupyter_server/serverapp.py | 49 +------- jupyter_server/services/contents/fileio.py | 3 + .../services/contents/filemanager.py | 31 +++++ jupyter_server/services/contents/manager.py | 21 ++++ tests/test_gateway.py | 6 +- tests/test_serverapp.py | 110 +++++++++++------- tests/test_terminal.py | 8 +- 7 files changed, 134 insertions(+), 94 deletions(-) diff --git a/jupyter_server/serverapp.py b/jupyter_server/serverapp.py index 37eaaffe72..f9c0d1be34 100644 --- a/jupyter_server/serverapp.py +++ b/jupyter_server/serverapp.py @@ -1647,22 +1647,11 @@ def _normalize_dir(self, value): value = os.path.abspath(value) return value - # Because the validation of preferred_dir depends on root_dir and validation - # occurs when the trait is loaded, there are times when we should defer the - # validation of preferred_dir (e.g., when preferred_dir is defined via CLI - # and root_dir is defined via a config file). - _defer_preferred_dir_validation = False - @validate("root_dir") def _root_dir_validate(self, proposal): value = self._normalize_dir(proposal["value"]) if not os.path.isdir(value): raise TraitError(trans.gettext("No such directory: '%r'") % value) - - if self._defer_preferred_dir_validation: - # If we're here, then preferred_dir is configured on the CLI and - # root_dir is configured in a file - self._preferred_dir_validation(self.preferred_dir, value) return value preferred_dir = Unicode( @@ -1679,39 +1668,8 @@ def _preferred_dir_validate(self, proposal): value = self._normalize_dir(proposal["value"]) if not os.path.isdir(value): raise TraitError(trans.gettext("No such preferred dir: '%r'") % value) - - # Before we validate against root_dir, check if this trait is defined on the CLI - # and root_dir is not. If that's the case, we'll defer it's further validation - # until root_dir is validated or the server is starting (the latter occurs when - # the default root_dir (cwd) is used). - cli_config = self.cli_config.get("ServerApp", {}) - if "preferred_dir" in cli_config and "root_dir" not in cli_config: - self._defer_preferred_dir_validation = True - - if not self._defer_preferred_dir_validation: # Validate now - self._preferred_dir_validation(value, self.root_dir) return value - def _preferred_dir_validation(self, preferred_dir: str, root_dir: str) -> None: - """Validate preferred dir relative to root_dir - preferred_dir must be equal or a subdir of root_dir""" - if not preferred_dir.startswith(root_dir): - raise TraitError( - trans.gettext( - "preferred_dir must be equal or a subdir of root_dir. preferred_dir: '%r' root_dir: '%r'" - ) - % (preferred_dir, root_dir) - ) - self._defer_preferred_dir_validation = False - - @observe("root_dir") - def _root_dir_changed(self, change): - self._root_dir_set = True - if not self.preferred_dir.startswith(change["new"]): - self.log.warning( - trans.gettext("Value of preferred_dir updated to use value of root_dir") - ) - self.preferred_dir = change["new"] - @observe("server_extensions") def _update_server_extensions(self, change): self.log.warning(_i18n("server_extensions is deprecated, use jpserver_extensions")) @@ -1893,6 +1851,9 @@ def init_configurables(self): parent=self, log=self.log, ) + # Trigger a default/validation here explicitly while we still support the + # deprecated trait on ServerApp (FIXME remove when deprecation finalized) + self.contents_manager.preferred_dir self.session_manager = self.session_manager_class( parent=self, log=self.log, @@ -2508,10 +2469,6 @@ def initialize( # Parse command line, load ServerApp config files, # and update ServerApp config. super().initialize(argv=argv) - if self._defer_preferred_dir_validation: - # If we're here, then preferred_dir is configured on the CLI and - # root_dir has the default value (cwd) - self._preferred_dir_validation(self.preferred_dir, self.root_dir) if self._dispatching: return # initialize io loop as early as possible, diff --git a/jupyter_server/services/contents/fileio.py b/jupyter_server/services/contents/fileio.py index 3984248d3c..66c28a1f64 100644 --- a/jupyter_server/services/contents/fileio.py +++ b/jupyter_server/services/contents/fileio.py @@ -254,6 +254,9 @@ def _get_os_path(self, path): 404: if path is outside root """ root = os.path.abspath(self.root_dir) # type:ignore + # to_os_path is not safe if path starts with a drive, since os.path.join discards first part + if os.path.splitdrive(path)[0]: + raise HTTPError(404, "%s is not a relative API path" % path) os_path = to_os_path(path, root) if not (os.path.abspath(os_path) + os.path.sep).startswith(root): raise HTTPError(404, "%s is outside root contents directory" % path) diff --git a/jupyter_server/services/contents/filemanager.py b/jupyter_server/services/contents/filemanager.py index b3c04433a9..6de4166e33 100644 --- a/jupyter_server/services/contents/filemanager.py +++ b/jupyter_server/services/contents/filemanager.py @@ -7,7 +7,9 @@ import shutil import stat import sys +import warnings from datetime import datetime +from pathlib import Path import nbformat from anyio.to_thread import run_sync @@ -19,6 +21,7 @@ from jupyter_server import _tz as tz from jupyter_server.base.handlers import AuthenticatedFileHandler from jupyter_server.transutils import _i18n +from jupyter_server.utils import to_api_path from .filecheckpoints import AsyncFileCheckpoints, FileCheckpoints from .fileio import AsyncFileManagerMixin, FileManagerMixin @@ -55,6 +58,34 @@ def _validate_root_dir(self, proposal): raise TraitError("%r is not a directory" % value) return value + @default("preferred_dir") + def _default_preferred_dir(self): + try: + value = self.parent.preferred_dir + if value == self.parent.root_dir: + value = None + except AttributeError: + pass + else: + if value is not None: + warnings.warn( + "ServerApp.preferred_dir config is deprecated in jupyter-server 2.0. Use FileContentsManager.preferred_dir instead", + FutureWarning, + stacklevel=3, + ) + try: + path = Path(value) + return path.relative_to(self.root_dir).as_posix() + except ValueError: + raise TraitError("%s is outside root contents directory" % value) from None + return "" + + @validate("preferred_dir") + def _validate_preferred_dir(self, proposal): + # It should be safe to pass an API path through this method: + proposal["value"] = to_api_path(proposal["value"], self.root_dir) + return super()._validate_preferred_dir(proposal) + @default("checkpoints_class") def _checkpoints_class_default(self): return FileCheckpoints diff --git a/jupyter_server/services/contents/manager.py b/jupyter_server/services/contents/manager.py index 8539d712fb..2bd82f5d09 100644 --- a/jupyter_server/services/contents/manager.py +++ b/jupyter_server/services/contents/manager.py @@ -7,6 +7,7 @@ import warnings from fnmatch import fnmatch +from jupyter_client.utils import run_sync from jupyter_events import EventLogger from nbformat import ValidationError, sign from nbformat import validate as validate_nb @@ -64,10 +65,30 @@ def emit(self, data): root_dir = Unicode("/", config=True) + preferred_dir = Unicode( + "", + config=True, + help=_i18n( + "Preferred starting directory to use for notebooks. This is an API path (`/` separated, relative to root dir)" + ), + ) + + @validate("preferred_dir") + def _validate_preferred_dir(self, proposal): + value = proposal["value"].strip("/") + try: + dir_exists = run_sync(self.dir_exists)(value) + except HTTPError as e: + raise TraitError(e.log_message) from e + if not dir_exists: + raise TraitError(_i18n("Preferred directory not found: %r") % value) + return value + allow_hidden = Bool(False, config=True, help="Allow access to hidden files") notary = Instance(sign.NotebookNotary) + @default("notary") def _notary_default(self): return sign.NotebookNotary(parent=self) diff --git a/tests/test_gateway.py b/tests/test_gateway.py index 0c60d05872..7515e2a947 100644 --- a/tests/test_gateway.py +++ b/tests/test_gateway.py @@ -427,7 +427,7 @@ async def test_gateway_session_lifecycle(init_gateway, jp_root_dir, jp_fetch, cu # Validate session lifecycle functions; create and delete. # create - session_id, kernel_id = await create_session(jp_root_dir, jp_fetch, "kspec_foo") + session_id, kernel_id = await create_session(jp_fetch, "kspec_foo") # ensure kernel still considered running assert await is_session_active(jp_fetch, session_id) is True @@ -622,12 +622,12 @@ async def is_session_active(jp_fetch, session_id): return False -async def create_session(root_dir, jp_fetch, kernel_name): +async def create_session(jp_fetch, kernel_name): """Creates a session for a kernel. The session is created against the server which then uses the gateway for kernel management. """ with mocked_gateway: - nb_path = root_dir / "testgw.ipynb" + nb_path = "/testgw.ipynb" body = json.dumps( {"path": str(nb_path), "type": "notebook", "kernel": {"name": kernel_name}} ) diff --git a/tests/test_serverapp.py b/tests/test_serverapp.py index 7a2ba78784..ab4e12c1b3 100644 --- a/tests/test_serverapp.py +++ b/tests/test_serverapp.py @@ -263,14 +263,18 @@ def test_urls(config, public_url, local_url, connection_url): # Preferred dir tests # ---------------------------------------------------------------------------- +@pytest.mark.filterwarnings("ignore::FutureWarning") def test_valid_preferred_dir(tmp_path, jp_configurable_serverapp): path = str(tmp_path) app = jp_configurable_serverapp(root_dir=path, preferred_dir=path) assert app.root_dir == path assert app.preferred_dir == path assert app.root_dir == app.preferred_dir + assert app.contents_manager.root_dir == path + assert app.contents_manager.preferred_dir == "" +@pytest.mark.filterwarnings("ignore::FutureWarning") def test_valid_preferred_dir_is_root_subdir(tmp_path, jp_configurable_serverapp): path = str(tmp_path) path_subdir = str(tmp_path / "subdir") @@ -279,6 +283,7 @@ def test_valid_preferred_dir_is_root_subdir(tmp_path, jp_configurable_serverapp) assert app.root_dir == path assert app.preferred_dir == path_subdir assert app.preferred_dir.startswith(app.root_dir) + assert app.contents_manager.preferred_dir == "subdir" def test_valid_preferred_dir_does_not_exist(tmp_path, jp_configurable_serverapp): @@ -290,26 +295,46 @@ def test_valid_preferred_dir_does_not_exist(tmp_path, jp_configurable_serverapp) assert "No such preferred dir:" in str(error) +# This tests some deprecated behavior as well +@pytest.mark.filterwarnings("ignore::FutureWarning") @pytest.mark.parametrize( - "root_dir_loc,preferred_dir_loc", + "root_dir_loc,preferred_dir_loc,config_target", [ - ("cli", "cli"), - ("cli", "config"), - ("cli", "default"), - ("config", "cli"), - ("config", "config"), - ("config", "default"), - ("default", "cli"), - ("default", "config"), - ("default", "default"), + ("cli", "cli", "ServerApp"), + ("cli", "cli", "FileContentsManager"), + ("cli", "config", "ServerApp"), + ("cli", "config", "FileContentsManager"), + ("cli", "default", "ServerApp"), + ("cli", "default", "FileContentsManager"), + ("config", "cli", "ServerApp"), + ("config", "cli", "FileContentsManager"), + ("config", "config", "ServerApp"), + ("config", "config", "FileContentsManager"), + ("config", "default", "ServerApp"), + ("config", "default", "FileContentsManager"), + ("default", "cli", "ServerApp"), + ("default", "cli", "FileContentsManager"), + ("default", "config", "ServerApp"), + ("default", "config", "FileContentsManager"), + ("default", "default", "ServerApp"), + ("default", "default", "FileContentsManager"), ], ) def test_preferred_dir_validation( - root_dir_loc, preferred_dir_loc, tmp_path, jp_config_dir, jp_configurable_serverapp + root_dir_loc, + preferred_dir_loc, + config_target, + tmp_path, + jp_config_dir, + jp_configurable_serverapp, ): expected_root_dir = str(tmp_path) - expected_preferred_dir = str(tmp_path / "subdir") - os.makedirs(expected_preferred_dir, exist_ok=True) + + os_preferred_dir = str(tmp_path / "subdir") + os.makedirs(os_preferred_dir, exist_ok=True) + config_preferred_dir = os_preferred_dir if config_target == "ServerApp" else "subdir" + config_preferred_dir = config_preferred_dir + "/" # add trailing slash to ensure it is removed + expected_preferred_dir = "subdir" argv = [] kwargs = {"root_dir": None} @@ -320,18 +345,18 @@ def test_preferred_dir_validation( config_file = jp_config_dir.joinpath("jupyter_server_config.py") if root_dir_loc == "cli": - argv.append(f"--ServerApp.root_dir={expected_root_dir}") + argv.append(f"--{config_target}.root_dir={expected_root_dir}") if root_dir_loc == "config": - config_lines.append(f'c.ServerApp.root_dir = r"{expected_root_dir}"') + config_lines.append(f'c.{config_target}.root_dir = r"{expected_root_dir}"') if root_dir_loc == "default": expected_root_dir = os.getcwd() if preferred_dir_loc == "cli": - argv.append(f"--ServerApp.preferred_dir={expected_preferred_dir}") + argv.append(f"--{config_target}.preferred_dir={config_preferred_dir}") if preferred_dir_loc == "config": - config_lines.append(f'c.ServerApp.preferred_dir = r"{expected_preferred_dir}"') + config_lines.append(f'c.{config_target}.preferred_dir = r"{config_preferred_dir}"') if preferred_dir_loc == "default": - expected_preferred_dir = expected_root_dir + expected_preferred_dir = "" if config_file is not None: config_file.write_text("\n".join(config_lines)) @@ -344,9 +369,9 @@ def test_preferred_dir_validation( jp_configurable_serverapp(**kwargs) else: app = jp_configurable_serverapp(**kwargs) - assert app.root_dir == expected_root_dir - assert app.preferred_dir == expected_preferred_dir - assert app.preferred_dir.startswith(app.root_dir) + assert app.contents_manager.root_dir == expected_root_dir + assert app.contents_manager.preferred_dir == expected_preferred_dir + assert ".." not in expected_preferred_dir def test_invalid_preferred_dir_does_not_exist(tmp_path, jp_configurable_serverapp): @@ -369,45 +394,42 @@ def test_invalid_preferred_dir_does_not_exist_set(tmp_path, jp_configurable_serv assert "No such preferred dir:" in str(error) +@pytest.mark.filterwarnings("ignore::FutureWarning") def test_invalid_preferred_dir_not_root_subdir(tmp_path, jp_configurable_serverapp): path = str(tmp_path / "subdir") os.makedirs(path, exist_ok=True) not_subdir_path = str(tmp_path) - with pytest.raises(TraitError) as error: - app = jp_configurable_serverapp(root_dir=path, preferred_dir=not_subdir_path) - - assert "preferred_dir must be equal or a subdir of root_dir. " in str(error) + with pytest.raises(SystemExit): + jp_configurable_serverapp(root_dir=path, preferred_dir=not_subdir_path) -def test_invalid_preferred_dir_not_root_subdir_set(tmp_path, jp_configurable_serverapp): +async def test_invalid_preferred_dir_not_root_subdir_set(tmp_path, jp_configurable_serverapp): path = str(tmp_path / "subdir") os.makedirs(path, exist_ok=True) - not_subdir_path = str(tmp_path) + not_subdir_path = os.path.relpath(tmp_path, path) app = jp_configurable_serverapp(root_dir=path) with pytest.raises(TraitError) as error: - app.preferred_dir = not_subdir_path + app.contents_manager.preferred_dir = not_subdir_path - assert "preferred_dir must be equal or a subdir of root_dir. " in str(error) + assert "is outside root contents directory" in str(error.value) -def test_observed_root_dir_updates_preferred_dir(tmp_path, jp_configurable_serverapp): - path = str(tmp_path) - new_path = str(tmp_path / "subdir") - os.makedirs(new_path, exist_ok=True) +async def test_absolute_preferred_dir_not_root_subdir_set(tmp_path, jp_configurable_serverapp): + path = str(tmp_path / "subdir") + os.makedirs(path, exist_ok=True) + not_subdir_path = str(tmp_path) - app = jp_configurable_serverapp(root_dir=path, preferred_dir=path) - app.root_dir = new_path - assert app.preferred_dir == new_path + app = jp_configurable_serverapp(root_dir=path) + with pytest.raises(TraitError) as error: + app.contents_manager.preferred_dir = not_subdir_path -def test_observed_root_dir_does_not_update_preferred_dir(tmp_path, jp_configurable_serverapp): - path = str(tmp_path) - new_path = str(tmp_path.parent) - app = jp_configurable_serverapp(root_dir=path, preferred_dir=path) - app.root_dir = new_path - assert app.preferred_dir == path + if os.name == "nt": + assert "is not a relative API path" in str(error.value) + else: + assert "Preferred directory not found" in str(error.value) def test_random_ports(): @@ -446,13 +468,13 @@ def test_misc(jp_serverapp, tmp_path): app.parse_command_line([]) -def test_deprecated_props(jp_serverapp): +def test_deprecated_props(jp_serverapp, tmp_path): app: ServerApp = jp_serverapp with warnings.catch_warnings(): warnings.simplefilter("ignore") app.cookie_options = dict(foo=1) app.get_secure_cookie_kwargs = dict(bar=1) - app.notebook_dir = "foo" + app.notebook_dir = str(tmp_path) app.server_extensions = dict(foo=True) app.kernel_ws_protocol = "foo" app.limit_rate = True diff --git a/tests/test_terminal.py b/tests/test_terminal.py index f011f2abef..3260548f3e 100644 --- a/tests/test_terminal.py +++ b/tests/test_terminal.py @@ -276,4 +276,10 @@ def test_shell_command_override( pytest.importorskip("traitlets", minversion=min_traitlets) argv = shlex.split(f"--ServerApp.terminado_settings={terminado_settings}") app = jp_configurable_serverapp(argv=argv) - assert app.web_app.settings["terminal_manager"].shell_command == expected_shell + if os.name == "nt": + assert app.web_app.settings["terminal_manager"].shell_command in ( + expected_shell, + " ".join(expected_shell), + ) + else: + assert app.web_app.settings["terminal_manager"].shell_command == expected_shell