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.

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

Only prefer envs owned by the current user #323

Merged
merged 8 commits into from
Dec 22, 2022

Conversation

minrk
Copy link
Member

@minrk minrk commented Dec 13, 2022

Check ownership of prefix to avoid misidentifying 'shared' environments that can't be.

Heuristic being to prefer user environments to general user config, but not shared environments that may be owned by e.g. root.

in cases of a shared virtualenv, the env may be owned by root and not writable, causing problems for e.g. juptyerlab which writes workspaces to the default config dir, which will fail with a permission error. It makes sense to prefer the env when they are my envs for projects, but not for e.g. creating a user env to be used in jupyterhub. I think an ownership writability check is a good indicator of whether it's a 'shared' env.

More info: jupyterlab/jupyterlab#13580

This would fix the above issue in the real cases I've encountered, but it's not a rigorous check for the writability of the config dir specifically.

Downside of just a writability check: if an admin has write access instead of requiring e.g. root, they will still get the shared env!

With an owner check, this can still happen if a 'shared' environment is owned by one of its users. That ought to be more rare than writable, at least.

closes #318

in cases of a shared virtualenv, this path may not be writable,
causing problems for e.g. juptyerlab which writes workspaces to the default config dir,
which will fail with a permission error
@minrk
Copy link
Member Author

minrk commented Dec 13, 2022

This now has an ownership check, which falls back on W_OK if no owner can be established, which I think will happen on Windows.

If another user created the env, that means it's shared and we shouldn't write config to it
@blink1073
Copy link
Member

@jasongrout had brought up some concerns about special-casing the "writability" of the path here.

@jasongrout
Copy link
Member

@minrk - I'd love to hear your thoughts about the points I raised at #318 (comment). I agree with you that an ownership check is a good first check.

I'm still a bit concerned with the complexity of this check, but given that now two adminstrators have raised the issue as a practical problem, I think it would be a good thing to solve, especially since Min is pointing out that it is likely to be a widespread problem when using JupyterHub.

I think it is really important to be very transparent and helpful to users that want to understand why Jupyter is choosing its paths and how they can affect that choice. We try to give such guidance in jupyter --paths --debug. @minrk, can you modify what jupyter --paths --debug prints out to clearly indicate the nuances of this new check (e.g., by default, it checks ownership, and failing that, checks writability)? The code is around https://github.com/jupyter/jupyter_core/blob/main/jupyter_core/command.py#L232.

@minrk
Copy link
Member Author

minrk commented Dec 14, 2022

@jasongrout thanks! I left comments in #318. I think shared env detection is too complex and fraught with wrong conclusions (e.g. admin vs user getting different results for the same env). I think the solution probably belongs in checks in jupyterlab whatever package is choosing to require writing. I do believe it shouldn't be on the user to solve, though, for standard persistence behavior like jupyterlab workspaces.

@minrk minrk closed this Dec 14, 2022
@minrk minrk deleted the prefer-venv-writable branch December 14, 2022 09:34
@minrk minrk restored the prefer-venv-writable branch December 14, 2022 09:34
@minrk minrk reopened this Dec 14, 2022
@minrk minrk changed the title only prefer virtualenvs for config if the env is writable only prefer envs owned by the current user Dec 14, 2022
@minrk
Copy link
Member Author

minrk commented Dec 14, 2022

Reopening now that it's switched to an ownership check, following discussion in #318

@blink1073 blink1073 changed the title only prefer envs owned by the current user Only prefer envs owned by the current user Dec 22, 2022
@blink1073 blink1073 enabled auto-merge (squash) December 22, 2022 19:34
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 merged commit 1ed25e3 into jupyter:main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: do not prefer env path if it is not writeable
3 participants