From 01e7d2e89228bc72fc658b09a79515855b565b00 Mon Sep 17 00:00:00 2001 From: Vidar Tonaas Fauske <510760+vidartf@users.noreply.github.com> Date: Thu, 15 Sep 2022 15:58:56 +0100 Subject: [PATCH] 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. --- jupyter_server/serverapp.py | 53 +-------- jupyter_server/services/contents/fileio.py | 3 + .../services/contents/filemanager.py | 24 ++++ jupyter_server/services/contents/manager.py | 21 ++++ tests/test_gateway.py | 6 +- tests/test_serverapp.py | 109 +++++++++++------- 6 files changed, 121 insertions(+), 95 deletions(-) diff --git a/jupyter_server/serverapp.py b/jupyter_server/serverapp.py index 025c98746c..ef005f7965 100644 --- a/jupyter_server/serverapp.py +++ b/jupyter_server/serverapp.py @@ -1641,22 +1641,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( @@ -1673,39 +1662,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")) @@ -1887,6 +1845,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, @@ -2086,7 +2047,7 @@ def init_resources(self): ) return - old_soft, old_hard = resource.getrlimit(resource.RLIMIT_NOFILE) + old_soft, old_hard = resource.getrlimit(resource.RLIMIT_NOFILE) # noqa soft = self.min_open_files_limit hard = old_hard if old_soft < soft: @@ -2097,7 +2058,7 @@ def init_resources(self): old_soft, soft, old_hard, hard ) ) - resource.setrlimit(resource.RLIMIT_NOFILE, (soft, hard)) + resource.setrlimit(resource.RLIMIT_NOFILE, (soft, hard)) # noqa def _get_urlparts(self, path=None, include_token=False): """Constructs a urllib named tuple, ParseResult, @@ -2505,10 +2466,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 e1d6ae66dc..57314ebbc5 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) + # 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..f39813e84d 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 @@ -55,6 +57,28 @@ 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 ContentsManager.preferred_dir with a relative path 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) + return "/" + @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 089a71fc65..c12b01c8db 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 @@ -75,10 +76,30 @@ def emit(self, data): root_dir = Unicode("/", config=True) + preferred_dir = Unicode( + "/", + config=True, + help=_i18n( + "Preferred starting directory to use for notebooks, relative to the server root dir." + ), + ) + + @validate("preferred_dir") + def _validate_preferred_dir(self, proposal): + value = proposal["value"] + 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 ff9370d766..0e9216ca25 100644 --- a/tests/test_gateway.py +++ b/tests/test_gateway.py @@ -431,7 +431,7 @@ async def test_gateway_session_lifecycle(init_gateway, jp_root_dir, jp_fetch): # 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_kernel_running(jp_fetch, kernel_id) is True @@ -583,12 +583,12 @@ async def test_channel_queue_get_msg_when_response_router_had_finished(): # # Test methods below... # -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 5a7a585aef..92ff3cf70d 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,45 @@ 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" + expected_preferred_dir = "/subdir" argv = [] kwargs = {"root_dir": None} @@ -320,18 +344,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 +368,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 +393,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 +467,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