Why are OS users created on successful authentication?

[mwilbz]

First of all, thanks for publishing this project, it's super helpful!

I'm running a JupyterHub server on Kubernetes using the Zero to JupyterHub project, using this authenticator installed on top of the jupyterhub/jupyterhub-k8s image. The first time I tried it, I got a 500 error and saw the following logs from the hub container:

[I 2019-06-19 23:49:13.515 JupyterHub samlauthenticator:487] The SAML Assertion matched the configured values
useradd: Permission denied.
useradd: cannot lock /etc/passwd; try again later.
[E 2019-06-19 23:49:13.521 JupyterHub samlauthenticator:562] Failed to add user
[W 2019-06-19 23:49:13.522 JupyterHub base:670] Failed login for unknown user

This is, of course, because the jovyan user in the jupyterhub/jupyterhub-k8s image is not the root user. This seems desirable! I've gotten around this issue simply by removing the code in this library that adds system users on successful authentication. But I wonder if I'm missing out on some JupyterHub features by doing so. The Authenticator base class doesn't seem to require system users (only LocalAuthenticator or PAMAuthenticator) so I'm wondering why it's added here.

If this change makes sense for all users or should be an option for some, I'm more than happy to make a contribution :)

[distortedsignal]

Hey @mwilbz! Thanks for taking the time to test this code and make this issue. I'm glad that you could get some use out of it.

Originally, this project was intended to be used in container applications where users probably wouldn't be using each-other's JupyterHub instances. I think that as a first step, "make a system user" was the right call because it was easy to implement.

Since your use case dictates that we can't create system users, I think it's time to re-investigate that initial call. Looking at the code in its current form, the user add code is on lines 520 to 536.

I want to make sure that the solution that we come up with fits your use case. Do you mind posting a diff of your code so that I can see what you're doing? I think that 1) this should be a relatively simple fix and 2) this would be a good issue for a new developer on the project.

If you want to, I can assign this one to you, and we can work together on this. What do you think?

[mwilbz]

My patch is pretty simple!

<             return self._optional_user_add(username)
---
>             return True # return self._optional_user_add(username)

I could definitely open a PR with this change + removing the _optional_user_add method. Another option is to be backwards compatible and add a config option to the SAMLAuthenticator class to allow users to turn off the user-creation behavior. I'd lean towards the latter but I'm open to both!

Edit: Definitely feel free to assign this issue to me!

[distortedsignal]

Let me think about this - I think it might be best to maintain the backward compatibility of the change (for 0.0.5) and then change the default in 0.1.0.

So things that need to change for this to get into the codebase:

• Add a traitlet to the class - something like prevent_system_user_create - and have it default to False for the first release
• Change the logic in the function to look like the following:

  return self.prevent_system_user_create or self._optional_user_add(username) # Rely on Python's or to execute the second statement lazily

• Add tests to the TestCreateUser class
• Doc new traitlet in README.md
• Add @mwilbz to the ATTRIBUTION.md doc

If you want to take care of the first two or three of those, I can take care of... however many you don't want to take. Make a PR to this repo, I'll probably make a PR to the branch your PR is coming from, and we can go from there.

Thanks for driving this! I'll do my best to help you get this to DONE ASAP. 😄