From 57485af34dfab50f16b7e957ea2fe0131176509d Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Thu, 30 Jul 2020 15:20:33 +0200 Subject: [PATCH 01/12] Add `write_kernel_spec` test when resources has restrictive permissions Test creates a resources directory with 0500 permissions which cannot be written into. Tests if `write_kernel_spec` correctly adjusts the permissions so that it can work with this resources directory --- ipykernel/tests/test_kernelspec.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ipykernel/tests/test_kernelspec.py b/ipykernel/tests/test_kernelspec.py index a3b9771c7..0e8bb5967 100644 --- a/ipykernel/tests/test_kernelspec.py +++ b/ipykernel/tests/test_kernelspec.py @@ -88,6 +88,26 @@ def test_write_kernel_spec_path(): shutil.rmtree(path) +def test_write_kernel_spec_permissions(): + read_only_resources = tempfile.mkdtemp() + shutil.copytree(RESOURCES, read_only_resources, dirs_exist_ok=True) + + # create copy of `RESOURCES` with no write permissions + os.chmod(read_only_resources, 0o500) + for f in os.listdir(read_only_resources): + os.chmod(os.path.join(read_only_resources, f), 0o400) + print(os.path.join(read_only_resources, f), oct(os.stat(os.path.join(read_only_resources, f)).st_mode)) + + path = write_kernel_spec(resources=read_only_resources) + assert_is_spec(path) + + # ensure permissions are not loosened too much, original permission was + # 0o500, so the 'fixed' one should be 0o700, still no rw for group/other + assert os.stat(path).st_mode & 0o77 == 0o00 + + shutil.rmtree(path) + + def test_install_kernelspec(): path = tempfile.mkdtemp() From bae4d3358036db08474bd44e3967d72db6a81703 Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Thu, 30 Jul 2020 15:22:11 +0200 Subject: [PATCH 02/12] Change permissions for `resources` if too restrictive Adds an optional `resources` argument used for testing. Now checks the permissions of the copied resources directory and adjusts them so that the `kernel.json` file can be written if required Preserves original group and other permissions --- ipykernel/kernelspec.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/ipykernel/kernelspec.py b/ipykernel/kernelspec.py index bce8051e0..e5cb7eba9 100644 --- a/ipykernel/kernelspec.py +++ b/ipykernel/kernelspec.py @@ -59,19 +59,29 @@ def get_kernel_dict(extra_arguments=None): } -def write_kernel_spec(path=None, overrides=None, extra_arguments=None): +def write_kernel_spec(path=None, overrides=None, extra_arguments=None, resources=RESOURCES): """Write a kernel spec directory to `path` - + If `path` is not specified, a temporary directory is created. If `overrides` is given, the kernelspec JSON is updated before writing. - + if 'resources' is given, copies resources from non-standard location. + The path to the kernelspec is always returned. """ if path is None: path = os.path.join(tempfile.mkdtemp(suffix='_kernels'), KERNEL_NAME) - + # stage resources - shutil.copytree(RESOURCES, path) + shutil.copytree(resources, path) + + # change permission if resources directory is not read-write able + if not os.access(path, os.W_OK | os.R_OK): + # changes permissions only for owner, do not touch group/other + os.chmod(path, os.stat(path).st_mode | 0o700) + for f in os.listdir(path): + file_path = os.path.join(path, f) + os.chmod(file_path, os.stat(file_path).st_mode | 0o600) + # write kernel.json kernel_dict = get_kernel_dict(extra_arguments) @@ -79,7 +89,7 @@ def write_kernel_spec(path=None, overrides=None, extra_arguments=None): kernel_dict.update(overrides) with open(pjoin(path, 'kernel.json'), 'w') as f: json.dump(kernel_dict, f, indent=1) - + return path From 1752d721d4754b3392772c8a96cf2dd408b5014e Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Thu, 30 Jul 2020 15:38:27 +0200 Subject: [PATCH 03/12] Remove debug print statement --- ipykernel/tests/test_kernelspec.py | 1 - 1 file changed, 1 deletion(-) diff --git a/ipykernel/tests/test_kernelspec.py b/ipykernel/tests/test_kernelspec.py index 0e8bb5967..a14c9c382 100644 --- a/ipykernel/tests/test_kernelspec.py +++ b/ipykernel/tests/test_kernelspec.py @@ -96,7 +96,6 @@ def test_write_kernel_spec_permissions(): os.chmod(read_only_resources, 0o500) for f in os.listdir(read_only_resources): os.chmod(os.path.join(read_only_resources, f), 0o400) - print(os.path.join(read_only_resources, f), oct(os.stat(os.path.join(read_only_resources, f)).st_mode)) path = write_kernel_spec(resources=read_only_resources) assert_is_spec(path) From 424f598a56939fa7e766641a078316a2f45e28f5 Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Fri, 31 Jul 2020 21:11:19 +0200 Subject: [PATCH 04/12] Remove resources argument --- ipykernel/kernelspec.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ipykernel/kernelspec.py b/ipykernel/kernelspec.py index e5cb7eba9..f160e0d78 100644 --- a/ipykernel/kernelspec.py +++ b/ipykernel/kernelspec.py @@ -59,7 +59,7 @@ def get_kernel_dict(extra_arguments=None): } -def write_kernel_spec(path=None, overrides=None, extra_arguments=None, resources=RESOURCES): +def write_kernel_spec(path=None, overrides=None, extra_arguments=None): """Write a kernel spec directory to `path` If `path` is not specified, a temporary directory is created. @@ -72,7 +72,7 @@ def write_kernel_spec(path=None, overrides=None, extra_arguments=None, resources path = os.path.join(tempfile.mkdtemp(suffix='_kernels'), KERNEL_NAME) # stage resources - shutil.copytree(resources, path) + shutil.copytree(RESOURCES, path) # change permission if resources directory is not read-write able if not os.access(path, os.W_OK | os.R_OK): @@ -96,10 +96,10 @@ def write_kernel_spec(path=None, overrides=None, extra_arguments=None, resources def install(kernel_spec_manager=None, user=False, kernel_name=KERNEL_NAME, display_name=None, prefix=None, profile=None): """Install the IPython kernelspec for Jupyter - + Parameters ---------- - + kernel_spec_manager: KernelSpecManager [optional] A KernelSpecManager to use for installation. If none provided, a default instance will be created. @@ -118,7 +118,7 @@ def install(kernel_spec_manager=None, user=False, kernel_name=KERNEL_NAME, displ Returns ------- - + The path where the kernelspec was installed. """ if kernel_spec_manager is None: @@ -153,12 +153,12 @@ def install(kernel_spec_manager=None, user=False, kernel_name=KERNEL_NAME, displ class InstallIPythonKernelSpecApp(Application): """Dummy app wrapping argparse""" name = 'ipython-kernel-install' - + def initialize(self, argv=None): if argv is None: argv = sys.argv[1:] self.argv = argv - + def start(self): import argparse parser = argparse.ArgumentParser(prog=self.name, From f22f5b4c249e47e6c837b42f761fb47100ee171e Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Fri, 31 Jul 2020 22:23:06 +0200 Subject: [PATCH 05/12] Add warnings --- ipykernel/kernelspec.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ipykernel/kernelspec.py b/ipykernel/kernelspec.py index f160e0d78..95a36d84b 100644 --- a/ipykernel/kernelspec.py +++ b/ipykernel/kernelspec.py @@ -11,6 +11,7 @@ import shutil import sys import tempfile +import warnings from jupyter_client.kernelspec import KernelSpecManager @@ -76,6 +77,9 @@ def write_kernel_spec(path=None, overrides=None, extra_arguments=None): # change permission if resources directory is not read-write able if not os.access(path, os.W_OK | os.R_OK): + warnings.warn( + UserWarning("resources is not writable, adjusting permissions before creating kernel") + ) # changes permissions only for owner, do not touch group/other os.chmod(path, os.stat(path).st_mode | 0o700) for f in os.listdir(path): From 6bfcab830f29a9bfd188794cd12e31cd090fc0dc Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Fri, 31 Jul 2020 22:23:40 +0200 Subject: [PATCH 06/12] Use mock patch to change resources directory --- ipykernel/tests/test_kernelspec.py | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/ipykernel/tests/test_kernelspec.py b/ipykernel/tests/test_kernelspec.py index a14c9c382..43e836f9a 100644 --- a/ipykernel/tests/test_kernelspec.py +++ b/ipykernel/tests/test_kernelspec.py @@ -13,6 +13,8 @@ except ImportError: import mock # py2 +import pytest + from jupyter_core.paths import jupyter_data_dir from ipykernel.kernelspec import ( @@ -97,14 +99,23 @@ def test_write_kernel_spec_permissions(): for f in os.listdir(read_only_resources): os.chmod(os.path.join(read_only_resources, f), 0o400) - path = write_kernel_spec(resources=read_only_resources) - assert_is_spec(path) + with mock.patch('ipykernel.kernelspec.RESOURCES', read_only_resources): + with pytest.warns(UserWarning): + path = write_kernel_spec() - # ensure permissions are not loosened too much, original permission was - # 0o500, so the 'fixed' one should be 0o700, still no rw for group/other - assert os.stat(path).st_mode & 0o77 == 0o00 + assert_is_spec(path) - shutil.rmtree(path) + # ensure permissions actually restricted + # not sure how to run these as `RESOURCES` is directly imported from + # `ipykernel.kernelspec` so it can't be mocked + # print(RESOURCES, oct(os.stat(RESOURCES).st_mode & 0o777)) + # assert os.stat(RESOURCES).st_mode & 0o777 == 0o500 + + # ensure permissions are not loosened too much, original permission was + # 0o500, so the 'fixed' one should be 0o700, still no rw for group/other + assert os.stat(path).st_mode & 0o77 == 0o00 + + shutil.rmtree(path) def test_install_kernelspec(): From dfec211c37253afc7245326883f2b6fdb738f06b Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Fri, 31 Jul 2020 22:28:13 +0200 Subject: [PATCH 07/12] Create subdirectory instead of `dirs_exist_ok` --- ipykernel/tests/test_kernelspec.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ipykernel/tests/test_kernelspec.py b/ipykernel/tests/test_kernelspec.py index 43e836f9a..31c8f295e 100644 --- a/ipykernel/tests/test_kernelspec.py +++ b/ipykernel/tests/test_kernelspec.py @@ -91,8 +91,8 @@ def test_write_kernel_spec_path(): def test_write_kernel_spec_permissions(): - read_only_resources = tempfile.mkdtemp() - shutil.copytree(RESOURCES, read_only_resources, dirs_exist_ok=True) + read_only_resources = os.path.join(tempfile.mkdtemp(), "_RESOURCES") + shutil.copytree(RESOURCES, read_only_resources) # create copy of `RESOURCES` with no write permissions os.chmod(read_only_resources, 0o500) From b5b4770a03cac80e929f6745cacf794f4865869f Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Fri, 31 Jul 2020 22:39:49 +0200 Subject: [PATCH 08/12] Skip permission test on Windows --- ipykernel/tests/test_kernelspec.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ipykernel/tests/test_kernelspec.py b/ipykernel/tests/test_kernelspec.py index 31c8f295e..69ad22f83 100644 --- a/ipykernel/tests/test_kernelspec.py +++ b/ipykernel/tests/test_kernelspec.py @@ -89,7 +89,8 @@ def test_write_kernel_spec_path(): assert_is_spec(path) shutil.rmtree(path) - +@pytest.mark.skipif(sys.platform == 'win32', + reason="does not run on windows") def test_write_kernel_spec_permissions(): read_only_resources = os.path.join(tempfile.mkdtemp(), "_RESOURCES") shutil.copytree(RESOURCES, read_only_resources) From 09d4345c403844ee9a5d6f86839d4bfc63191306 Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Tue, 4 Aug 2020 09:33:03 +0200 Subject: [PATCH 09/12] Remove warning --- ipykernel/kernelspec.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/ipykernel/kernelspec.py b/ipykernel/kernelspec.py index 95a36d84b..b10622fed 100644 --- a/ipykernel/kernelspec.py +++ b/ipykernel/kernelspec.py @@ -77,9 +77,6 @@ def write_kernel_spec(path=None, overrides=None, extra_arguments=None): # change permission if resources directory is not read-write able if not os.access(path, os.W_OK | os.R_OK): - warnings.warn( - UserWarning("resources is not writable, adjusting permissions before creating kernel") - ) # changes permissions only for owner, do not touch group/other os.chmod(path, os.stat(path).st_mode | 0o700) for f in os.listdir(path): From 74c6bbba60d7efb4c7da64119315144670ee2a60 Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Tue, 4 Aug 2020 09:45:10 +0200 Subject: [PATCH 10/12] Use marker file to check correct RESOURCE dir copied --- ipykernel/tests/test_kernelspec.py | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/ipykernel/tests/test_kernelspec.py b/ipykernel/tests/test_kernelspec.py index 69ad22f83..d06385dee 100644 --- a/ipykernel/tests/test_kernelspec.py +++ b/ipykernel/tests/test_kernelspec.py @@ -89,28 +89,30 @@ def test_write_kernel_spec_path(): assert_is_spec(path) shutil.rmtree(path) + @pytest.mark.skipif(sys.platform == 'win32', reason="does not run on windows") def test_write_kernel_spec_permissions(): read_only_resources = os.path.join(tempfile.mkdtemp(), "_RESOURCES") shutil.copytree(RESOURCES, read_only_resources) - # create copy of `RESOURCES` with no write permissions + # file used to check that the correct (mocked) resource directory was used + with open(read_only_resources + '/touch', 'w') as f: + pass + + # make copy of resources directory read-only os.chmod(read_only_resources, 0o500) for f in os.listdir(read_only_resources): os.chmod(os.path.join(read_only_resources, f), 0o400) with mock.patch('ipykernel.kernelspec.RESOURCES', read_only_resources): - with pytest.warns(UserWarning): - path = write_kernel_spec() + path = write_kernel_spec() assert_is_spec(path) - # ensure permissions actually restricted - # not sure how to run these as `RESOURCES` is directly imported from - # `ipykernel.kernelspec` so it can't be mocked - # print(RESOURCES, oct(os.stat(RESOURCES).st_mode & 0o777)) - # assert os.stat(RESOURCES).st_mode & 0o777 == 0o500 + # if `touch` is missing then the wrong resources directory was copied by + # `write_kernel_spec`, `RESOURCES` mocking didn't work? + assert os.path.isfile(path + '/touch') # ensure permissions are not loosened too much, original permission was # 0o500, so the 'fixed' one should be 0o700, still no rw for group/other @@ -122,7 +124,7 @@ def test_write_kernel_spec_permissions(): def test_install_kernelspec(): path = tempfile.mkdtemp() - try: + try: test = InstallIPythonKernelSpecApp.launch_instance(argv=['--prefix', path]) assert_is_spec(os.path.join( path, 'share', 'jupyter', 'kernels', KERNEL_NAME)) @@ -132,21 +134,21 @@ def test_install_kernelspec(): def test_install_user(): tmp = tempfile.mkdtemp() - + with mock.patch.dict(os.environ, {'HOME': tmp}): install(user=True) data_dir = jupyter_data_dir() - + assert_is_spec(os.path.join(data_dir, 'kernels', KERNEL_NAME)) def test_install(): system_jupyter_dir = tempfile.mkdtemp() - + with mock.patch('jupyter_client.kernelspec.SYSTEM_JUPYTER_PATH', [system_jupyter_dir]): install() - + assert_is_spec(os.path.join(system_jupyter_dir, 'kernels', KERNEL_NAME)) From 61bdc3f3a3b5e15b83e8517c0ad7a7493fc947ce Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Tue, 4 Aug 2020 09:50:05 +0200 Subject: [PATCH 11/12] Remove redundant warning import --- ipykernel/kernelspec.py | 1 - 1 file changed, 1 deletion(-) diff --git a/ipykernel/kernelspec.py b/ipykernel/kernelspec.py index b10622fed..f160e0d78 100644 --- a/ipykernel/kernelspec.py +++ b/ipykernel/kernelspec.py @@ -11,7 +11,6 @@ import shutil import sys import tempfile -import warnings from jupyter_client.kernelspec import KernelSpecManager From b2e1a54bd485002c4d3a7ad4d9b4a53eb19c8d5a Mon Sep 17 00:00:00 2001 From: Robert Rosca <32569096+RobertRosca@users.noreply.github.com> Date: Fri, 7 Aug 2020 14:01:32 +0200 Subject: [PATCH 12/12] Use `tmp_path` fixture to create test directory --- ipykernel/tests/test_kernelspec.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ipykernel/tests/test_kernelspec.py b/ipykernel/tests/test_kernelspec.py index d06385dee..264f2fea8 100644 --- a/ipykernel/tests/test_kernelspec.py +++ b/ipykernel/tests/test_kernelspec.py @@ -92,8 +92,8 @@ def test_write_kernel_spec_path(): @pytest.mark.skipif(sys.platform == 'win32', reason="does not run on windows") -def test_write_kernel_spec_permissions(): - read_only_resources = os.path.join(tempfile.mkdtemp(), "_RESOURCES") +def test_write_kernel_spec_permissions(tmp_path): + read_only_resources = os.path.join(tmp_path, "_RESOURCES") shutil.copytree(RESOURCES, read_only_resources) # file used to check that the correct (mocked) resource directory was used