jupyter / jupyter_core

Core Jupyter functionality
https://jupyter-core.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
195 stars 181 forks source link

Only prefer envs owned by the current user #323

Closed minrk closed 1 year ago

minrk commented 1 year ago

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: https://github.com/jupyterlab/jupyterlab/issues/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

minrk commented 1 year ago

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.

blink1073 commented 1 year ago

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

jasongrout commented 1 year ago

@minrk - I'd love to hear your thoughts about the points I raised at https://github.com/jupyter/jupyter_core/issues/318#issuecomment-1330543591. 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 commented 1 year ago

@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 commented 1 year ago

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