use NB_USER rather than getpass.getuser() #132
Conversation
|
Is this extension designed to only work with jupyter/docker-stacks images where |
|
@manics It should work in the more general case. So either the authenticator needs to pass in something useful in @vivian-rook's deployment, or the username needs to be configurable here. |
jupyter_rsession_proxy/__init__.py
Outdated
| @@ -45,7 +45,7 @@ def rewrite_auth(response, request): | |||
|
|
|||
| def setup_rserver(): | |||
| def _get_env(port): | |||
| return dict(USER=getpass.getuser()) | |||
| return dict(USER=os.environ.get('NB_USER')) | |||
There was a problem hiding this comment.
I think we should go through the following hierarchy:
- If
JUPYTER_USERis set, use that. That's the logged-in username, if using a JupyterHub - If
NB_USERis set, use that. This defaults tojovyanI think? - Use
getpass.getuser()if neither of these are set.
There was a problem hiding this comment.
Is JUPYTER_USER guaranteed to exist as a system user? I only see JUPYTERHUB_USER in PAWS, and that is not a system user in the container.
The args to --server-user and --user-identity need to be a system user, as I understand it.
There was a problem hiding this comment.
Should the rsession-proxy use the above order, but have additional logic to verify that it is also a system user?
There was a problem hiding this comment.
sorry, I meant JUPYTERHUB_USER not JUPYTER_USER.
@vivian-rook I don't think it matters for rsession-proxy that it is a system user. Is that right, @ryanlovett?
There was a problem hiding this comment.
@yuvipanda I think the problem with #129 is that Chicocvenancio not a system user.
There was a problem hiding this comment.
ah goddamit, apparently i left my reading comprehension skills at home. Sorry!
In that case, can we try:
pwd.getpwuid(os.getuid())[0], which should return a username that's in thepwddatabase.- If this fails, then use
NB_USER.
How does that sound?
There was a problem hiding this comment.
Something like this?
There was a problem hiding this comment.
This LGTM. @ryanlovett looks ok to you?
Thanks a lot, @vivian-rook!
|
@yuvipanda The only suggestion I have is to have a fallback in the call to retrieve NB_USER, e.g. |
|
@vivian-rook if you can test this and it works, am happy to merge. |
|
@yuvipanda, @vivian-rook works for me with local jupyterhub. |
|
@yuvipanda Works for me off the latest build in toolforge/paws#173 |
|
Congrats on your first merged pull request in this project! 🎉 |
|
Thanks a lot, @vivian-rook @chicocvenancio and @ryanlovett! |
Sorry for the open/close PRs this gets us unblocked in a way that doesn't use my repository.
Resolves #129