-
-
Notifications
You must be signed in to change notification settings - Fork 365
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.
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
Check and adjust permissions before kernel spec is written #535
base: main
Are you sure you want to change the base?
Check and adjust permissions before kernel spec is written #535
Conversation
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
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
Outdated
@@ -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): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I think the test can be a bit neater using the pytest def test_write_kernel_spec_permissions(tmp_path): Then pytest takes care of creating a temporary directory, gives you a pathlib Path object to work with, and cleans up afterwards. |
Oops, didn't see your comment, I changed it to use the fixture as you suggested. I originally wrote it using the pathlib methods instead of read_only_resources = tmp_path.joinpath("_RESOURCES")
shutil.copytree(RESOURCES, read_only_resources)
# file used to check that the correct (mocked) resource directory was used
with open(read_only_resources.joinpath('touch'), 'w') as f:
pass But, at least to me, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pitching in, @RobertRosca, and sorry that this stalled. As I mention on a code inline comment - I believe the functional equivalent of the proposed change has already been merged in #593 (and unfortunately also overlooked earlier in #377).
If you have the desire and time, I think it would make sense to isolate this PR to a test of that functionality, so we don't accidentally lose it in the future. We also probably want remove all those whitespace changes from the commit to keep things tidy and isolated to the new test case
Let us know if you'd like to proceed or if we should press on by ourselves. Either way is fine, it's really up to you :)
Happy hacking!
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. | ||
|
There was a problem hiding this comment.
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.
for f in os.listdir(path): | ||
file_path = os.path.join(path, f) | ||
os.chmod(file_path, os.stat(file_path).st_mode | 0o600) | ||
|
There was a problem hiding this comment.
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.
@@ -88,10 +90,41 @@ def test_write_kernel_spec_path(): | |||
shutil.rmtree(path) | |||
|
|||
|
|||
@pytest.mark.skipif(sys.platform == 'win32', | |||
reason="does not run on windows") | |||
def test_write_kernel_spec_permissions(tmp_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should now probably be the main focus of this PR. I believe the implementation from #593 runs on Windows (at least it does for me on Python 3.9.2), so no need to skip that platform.
Hey, no problem! That's alright, I understand these things can drop through the cracks (like you reply did for me 😉) so it's no problem.
Sure, that sounds good to me.
I'm happy to carry on with this, but given it's been a while since you asked I want to check if this is all still needed. Cheers! |
If the permissions for the
resources
directory do not allow the owner to write into it (e.g. as is the case in some directories using ACL, or ifsite-packages
is set to read-only) then the install phase will fail with an error saying thatkernel.json
could not be written as the copiedresources
directory is read-only.This PR fixes that by adding a permission check to
write_kernel_spec
. Ifresources
has no write permissions it sets only the owner permission (leaving group/other unchanged) to 7 so thatkernel.json
can be written into it.Also added in a test which creates a
resources
directory with read-only permissions to check that this has worked correctly. For the test to workwrite_kernel_spec
now has a newresources
optional argument, which is set toRESOURCES
by default.@takluyver 😄