Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
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
| @@ -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): | |||
takluyver
Jul 30, 2020
Member
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.
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.
takluyver
Jul 30, 2020
Member
(Just realised 'as is' is ambiguous - I mean as you have already done it in this PR)
(Just realised 'as is' is ambiguous - I mean as you have already done it in this PR)
RobertRosca
Jul 31, 2020
Author
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.
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.
takluyver
Jul 31, 2020
Member
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?
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?
RobertRosca
Aug 4, 2020
Author
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.
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:
passBut, at least to me, the |
If the permissions for the
resourcesdirectory do not allow the owner to write into it (e.g. as is the case in some directories using ACL, or ifsite-packagesis set to read-only) then the install phase will fail with an error saying thatkernel.jsoncould not be written as the copiedresourcesdirectory is read-only.This PR fixes that by adding a permission check to
write_kernel_spec. Ifresourceshas no write permissions it sets only the owner permission (leaving group/other unchanged) to 7 so thatkernel.jsoncan be written into it.Also added in a test which creates a
resourcesdirectory with read-only permissions to check that this has worked correctly. For the test to workwrite_kernel_specnow has a newresourcesoptional argument, which is set toRESOURCESby default.@takluyver😄