diff --git a/jupyterhub/apihandlers/users.py b/jupyterhub/apihandlers/users.py index e27d0cdc..aa4488ae 100644 --- a/jupyterhub/apihandlers/users.py +++ b/jupyterhub/apihandlers/users.py @@ -63,10 +63,10 @@ class UserListAPIHandler(APIHandler): self.db.commit() try: yield gen.maybe_future(self.authenticator.add_user(user)) - except Exception: + except Exception as e: self.log.error("Failed to create user: %s" % name, exc_info=True) del self.users[user] - raise web.HTTPError(400, "Failed to create user: %s" % name) + raise web.HTTPError(400, "Failed to create user %s: %s" % (name, str(e))) else: created.append(user) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index c81caf04..c2220b98 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -6,9 +6,10 @@ from grp import getgrnam import pipes import pwd +import re from shutil import which import sys -from subprocess import check_call +from subprocess import Popen, PIPE, STDOUT from tornado import gen import pamela @@ -271,10 +272,7 @@ class LocalAuthenticator(Authenticator): def add_user(self, user): """Add a new user - By default, this just adds the user to the whitelist. - - Subclasses may do more extensive things, - such as adding actual unix users. + If self.create_system_users, the user will attempt to be created. """ user_exists = yield gen.maybe_future(self.system_user_exists(user)) if not user_exists: @@ -300,7 +298,11 @@ class LocalAuthenticator(Authenticator): name = user.name cmd = [ arg.replace('USERNAME', name) for arg in self.add_user_cmd ] + [name] self.log.info("Creating user: %s", ' '.join(map(pipes.quote, cmd))) - check_call(cmd) + p = Popen(cmd, stdout=PIPE, stderr=STDOUT) + p.wait() + if p.returncode: + err = p.stdout.read().decode('utf8', 'replace') + raise RuntimeError("Failed to create system user %s: %s" % (name, err)) class PAMAuthenticator(LocalAuthenticator): diff --git a/jupyterhub/tests/test_auth.py b/jupyterhub/tests/test_auth.py index 5907244a..92722b07 100644 --- a/jupyterhub/tests/test_auth.py +++ b/jupyterhub/tests/test_auth.py @@ -3,7 +3,6 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. -from subprocess import CalledProcessError from unittest import mock import pytest @@ -96,12 +95,24 @@ def test_cant_add_system_user(io_loop): authenticator.add_user_cmd = ['jupyterhub-fake-command'] authenticator.create_system_users = True - def check_call(cmd, *a, **kw): - raise CalledProcessError(1, cmd) + class DummyFile: + def read(self): + return b'dummy error' - with mock.patch.object(auth, 'check_call', check_call): - with pytest.raises(CalledProcessError): + class DummyPopen: + def __init__(self, *args, **kwargs): + self.args = args + self.kwargs = kwargs + self.returncode = 1 + self.stdout = DummyFile() + + def wait(self): + return + + with mock.patch.object(auth, 'Popen', DummyPopen): + with pytest.raises(RuntimeError) as exc: io_loop.run_sync(lambda : authenticator.add_user(user)) + assert str(exc.value) == 'Failed to create system user lioness4321: dummy error' def test_add_system_user(io_loop): @@ -111,10 +122,15 @@ def test_add_system_user(io_loop): authenticator.add_user_cmd = ['echo', '/home/USERNAME'] record = {} - def check_call(cmd, *a, **kw): - record['cmd'] = cmd + class DummyPopen: + def __init__(self, cmd, *args, **kwargs): + record['cmd'] = cmd + self.returncode = 0 + + def wait(self): + return - with mock.patch.object(auth, 'check_call', check_call): + with mock.patch.object(auth, 'Popen', DummyPopen): io_loop.run_sync(lambda : authenticator.add_user(user)) assert record['cmd'] == ['echo', '/home/lioness4321', 'lioness4321']