Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check and adjust permissions before kernel spec is written #535

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Check and adjust permissions before kernel spec is written #535

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
57485af
Add `write_kernel_spec` test when resources has restrictive permissions
RobertRosca Jul 30, 2020
bae4d33
Change permissions for `resources` if too restrictive
RobertRosca Jul 30, 2020
1752d72
Remove debug print statement
RobertRosca Jul 30, 2020
424f598
Remove resources argument
RobertRosca Jul 31, 2020
f22f5b4
Add warnings
RobertRosca Jul 31, 2020
6bfcab8
Use mock patch to change resources directory
RobertRosca Jul 31, 2020
dfec211
Create subdirectory instead of `dirs_exist_ok`
RobertRosca Jul 31, 2020
b5b4770
Skip permission test on Windows
RobertRosca Jul 31, 2020
09d4345
Remove warning
RobertRosca Aug 4, 2020
74c6bbb
Use marker file to check correct RESOURCE dir copied
RobertRosca Aug 4, 2020
61bdc3f
Remove redundant warning import
RobertRosca Aug 4, 2020
b2e1a54
Use `tmp_path` fixture to create test directory
RobertRosca Aug 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions ipykernel/kernelspec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,27 +59,37 @@ 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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ambivalent about this - on the one hand, I don't like adding function parameters just for tests, but on the other, I don't like using mocking unless it's necessary. So leave this as is unless someone else asks for a change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Just realised 'as is' is ambiguous - I mean as you have already done it in this PR)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woo now I know how to use the mock patch thing :D never used it before.

Ran into a small problem though. I want to test that the mock patch worked correctly, and that the path the patched RESOURCES variable points to has the correct permissions set.

But the RESOURCES variable comes from from ipykernel.kernelspec import RESOURCES at the top, and I can't think of an easy way to get the patched variable without doing import ipykernel.kernelspec so that I can call ipykernel.kernelspec.RESOURCES directly in the with mock.patch(...) block.

At the same time I also think that this permission-changing behaviour should be a bit more verbose and raise a warning. So I did the whole "two birds with one stone" thing and added a warning to write_kernel_spec whenever the permissions are modified, and then the test checks that this warning is raised.

To be honest, I'm like 50/50 split on if the warning is useful or pointless, so if you think the warning shouldn't be there I'm happy to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it shouldn't warn - read-only files in site-packages doesn't indicate that anything is going wrong or needs to be fixed. And people don't like unnecessary warnings.

If you want to ensure that it's picking up your test copy of the resources dir, why not stick an extra file in there, and then check that it exists in the resulting directory?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, good idea, implemented it like you said with a marker file.

My concern that RESOURCES didn't get mocked properly is probably silly, my thinking is more that the way the source resources directory is specified for write_kernel_spec might change at some point and just silently break this test.

"""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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be removed, since resources were added, but then removed as a parameter to write_kernel_spec in this PR.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the relevant piece of this code has already been merged in #593 (and unfortunately earlier in #377, which was overlooked). I think that we don't need the directory to have the execute bit set, only have it be writeable. We don't need all of the files to be writeable - since those are icons - they just need to be readable - which gets taken care of by copytree just above this section, since if they weren't readable from the RESOURCES directory, that would have failed.

I might have overlooked something subtle, though, but either way this would need to be rebased on master, as it conflicts with what ended up making it to ipykernel 5.5.0 in #593.

# write kernel.json
kernel_dict = get_kernel_dict(extra_arguments)

if overrides:
kernel_dict.update(overrides)
with open(pjoin(path, 'kernel.json'), 'w') as f:
json.dump(kernel_dict, f, indent=1)

return path


Expand Down
19 changes: 19 additions & 0 deletions ipykernel/tests/test_kernelspec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,25 @@ 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)
takluyver marked this conversation as resolved.
Show resolved Hide resolved

# 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)

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()
Expand Down