Merge pull request #1546 from maresb/fix-spurious-warning

Fix spurious warnings when using docker run --user=X

Author: mathbunnyru

base-notebook/start.sh

@@ -172,49 +172,59 @@ if [ "$(id -u)" == 0 ] ; then
 # The container didn't start as the root user, so we will have to act as the
 # user we started as.
 else
-    # Warn about misconfiguration of: desired username, user id, or group id
-    if [[ -n "${NB_USER}" && "${NB_USER}" != "$(id -un)" ]]; then
-        _log "WARNING: container must be started as root to change the desired user's name with NB_USER!"
-    fi
-    if [[ -n "${NB_UID}" && "${NB_UID}" != "$(id -u)" ]]; then
-        _log "WARNING: container must be started as root to change the desired user's id with NB_UID!"
-    fi
-    if [[ -n "${NB_GID}" && "${NB_GID}" != "$(id -g)" ]]; then
-        _log "WARNING: container must be started as root to change the desired user's group id with NB_GID!"
-    fi
-
     # Warn about misconfiguration of: granting sudo rights
     if [[ "${GRANT_SUDO}" == "1" || "${GRANT_SUDO}" == "yes" ]]; then
         _log "WARNING: container must be started as root to grant sudo permissions!"
     fi
 
+    JOVYAN_UID="$(id -u jovyan 2>/dev/null)"  # The default UID for the jovyan user
+    JOVYAN_GID="$(id -g jovyan 2>/dev/null)"  # The default GID for the jovyan user
+
     # Attempt to ensure the user uid we currently run as has a named entry in
     # the /etc/passwd file, as it avoids software crashing on hard assumptions
     # on such entry. Writing to the /etc/passwd was allowed for the root group
     # from the Dockerfile during build.
     #
     # ref: https://github.com/jupyter/docker-stacks/issues/552
     if ! whoami &> /dev/null; then
-        _log "There is no entry in /etc/passwd for our UID. Attempting to fix..."
+        _log "There is no entry in /etc/passwd for our UID=$(id -u). Attempting to fix..."
         if [[ -w /etc/passwd ]]; then
             _log "Renaming old jovyan user to nayvoj ($(id -u jovyan):$(id -g jovyan))"
 
             # We cannot use "sed --in-place" since sed tries to create a temp file in
             # /etc/ and we may not have write access. Apply sed on our own temp file:
             sed --expression="s/^jovyan:/nayvoj:/" /etc/passwd > /tmp/passwd
-            echo "jovyan:x:$(id -u):$(id -g):,,,:/home/jovyan:/bin/bash" >> /tmp/passwd
+            echo "${NB_USER}:x:$(id -u):$(id -g):,,,:/home/jovyan:/bin/bash" >> /tmp/passwd
             cat /tmp/passwd > /etc/passwd
             rm /tmp/passwd
 
-            _log "Added new jovyan user ($(id -u):$(id -g)). Fixed UID!"
+            _log "Added new ${NB_USER} user ($(id -u):$(id -g)). Fixed UID!"
+
+            if [[ "${NB_USER}" != "jovyan" ]]; then
+                _log "WARNING: user is ${NB_USER} but home is /home/jovyan. You must run as root to rename the home directory!"
+            fi
         else
-            _log "WARNING: unable to fix missing /etc/passwd entry because we don't have write permission."
+            _log "WARNING: unable to fix missing /etc/passwd entry because we don't have write permission. Try setting gid=0 with \"--user=$(id -u):0\"."
         fi
     fi
 
+    # Warn about misconfiguration of: desired username, user id, or group id.
+    # A misconfiguration occurs when the user modifies the default values of
+    # NB_USER, NB_UID, or NB_GID, but we cannot update those values because we
+    # are not root.
+    if [[ "${NB_USER}" != "jovyan" && "${NB_USER}" != "$(id -un)" ]]; then
+        _log "WARNING: container must be started as root to change the desired user's name with NB_USER=\"${NB_USER}\"!"
+    fi
+    if [[ "${NB_UID}" != "${JOVYAN_UID}" && "${NB_UID}" != "$(id -u)" ]]; then
+        _log "WARNING: container must be started as root to change the desired user's id with NB_UID=\"${NB_UID}\"!"
+    fi
+    if [[ "${NB_GID}" != "${JOVYAN_GID}" && "${NB_GID}" != "$(id -g)" ]]; then
+        _log "WARNING: container must be started as root to change the desired user's group id with NB_GID=\"${NB_GID}\"!"
+    fi
+
     # Warn if the user isn't able to write files to ${HOME}
     if [[ ! -w /home/jovyan ]]; then
-        _log "WARNING: no write access to /home/jovyan. Try starting the container with group 'users' (100)."
+        _log "WARNING: no write access to /home/jovyan. Try starting the container with group 'users' (100), e.g. using \"--group-add=users\"."
     fi
 
     # NOTE: This hook is run as the user we started the container as!

base-notebook/test/test_container_options.py

@@ -16,6 +16,12 @@ def test_cli_args(container, http_client):
     resp.raise_for_status()
     logs = c.logs(stdout=True).decode("utf-8")
     LOGGER.debug(logs)
+    assert "ERROR" not in logs
+    warnings = [
+        warning for warning in logs.split("\n") if warning.startswith("WARNING")
+    ]
+    assert len(warnings) == 1
+    assert warnings[0].startswith("WARNING: Jupyter Notebook deprecation notice")
     assert "login_submit" not in resp.text
 
 
@@ -24,7 +30,7 @@ def test_unsigned_ssl(container, http_client):
     """Container should generate a self-signed SSL certificate
     and notebook server should use it to enable HTTPS.
     """
-    container.run(environment=["GEN_CERT=yes"])
+    c = container.run(environment=["GEN_CERT=yes"])
     # NOTE: The requests.Session backing the http_client fixture does not retry
     # properly while the server is booting up. An SSL handshake error seems to
     # abort the retry logic. Forcing a long sleep for the moment until I have
@@ -33,6 +39,13 @@ def test_unsigned_ssl(container, http_client):
     resp = http_client.get("https://localhost:8888", verify=False)
     resp.raise_for_status()
     assert "login_submit" in resp.text
+    logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    warnings = [
+        warning for warning in logs.split("\n") if warning.startswith("WARNING")
+    ]
+    assert len(warnings) == 1
+    assert warnings[0].startswith("WARNING: Jupyter Notebook deprecation notice")
 
 
 def test_uid_change(container):
@@ -45,6 +58,9 @@ def test_uid_change(container):
     )
     # usermod is slow so give it some time
     rv = c.wait(timeout=120)
+    logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     assert rv == 0 or rv["StatusCode"] == 0
     assert "uid=1010(jovyan)" in c.logs(stdout=True).decode("utf-8")
 
@@ -60,6 +76,8 @@ def test_gid_change(container):
     rv = c.wait(timeout=10)
     assert rv == 0 or rv["StatusCode"] == 0
     logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     assert "gid=110(jovyan)" in logs
     assert "groups=110(jovyan),100(users)" in logs
 
@@ -79,6 +97,8 @@ def test_nb_user_change(container):
     time.sleep(10)
     LOGGER.info(f"Checking if the user is changed to {nb_user} by the start script ...")
     output = running_container.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in output
+    assert "WARNING" not in output
     assert (
         f"username: jovyan       -> {nb_user}" in output
     ), f"User is not changed to {nb_user}"
@@ -134,6 +154,8 @@ def test_chown_extra(container):
     rv = c.wait(timeout=120)
     assert rv == 0 or rv["StatusCode"] == 0
     logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     assert "/home/jovyan/.bashrc:1010:101" in logs
     assert "/opt/conda/bin/jupyter:1010:101" in logs
 
@@ -156,6 +178,8 @@ def test_chown_home(container):
     rv = c.wait(timeout=120)
     assert rv == 0 or rv["StatusCode"] == 0
     logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     assert "/home/kitten/.bashrc:1010:101" in logs
 
 
@@ -169,7 +193,10 @@ def test_sudo(container):
     )
     rv = c.wait(timeout=10)
     assert rv == 0 or rv["StatusCode"] == 0
-    assert "uid=0(root)" in c.logs(stdout=True).decode("utf-8")
+    logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
+    assert "uid=0(root)" in logs
 
 
 def test_sudo_path(container):
@@ -183,6 +210,8 @@ def test_sudo_path(container):
     rv = c.wait(timeout=10)
     assert rv == 0 or rv["StatusCode"] == 0
     logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     assert logs.rstrip().endswith("/opt/conda/bin/jupyter")
 
 
@@ -196,24 +225,75 @@ def test_sudo_path_without_grant(container):
     rv = c.wait(timeout=10)
     assert rv == 0 or rv["StatusCode"] == 0
     logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     assert logs.rstrip().endswith("/opt/conda/bin/jupyter")
 
 
-def test_group_add(container, tmpdir):
+def test_group_add(container):
     """Container should run with the specified uid, gid, and secondary
-    group.
+    group. It won't be possible to modify /etc/passwd since gid is nonzero, so
+    additionally verify that setting gid=0 is suggested in a warning.
     """
     c = container.run(
         user="1010:1010",
-        group_add=["users"],
+        group_add=["users"],  # Ensures write access to /home/jovyan
         command=["start.sh", "id"],
     )
     rv = c.wait(timeout=5)
     assert rv == 0 or rv["StatusCode"] == 0
     logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    warnings = [
+        warning for warning in logs.split("\n") if warning.startswith("WARNING")
+    ]
+    assert len(warnings) == 1
+    assert "Try setting gid=0" in warnings[0]
     assert "uid=1010 gid=1010 groups=1010,100(users)" in logs
 
 
+def test_set_uid(container):
+    """Container should run with the specified uid and NB_USER.
+    The /home/jovyan directory will not be writable since it's owned by 1000:users.
+    Additionally verify that "--group-add=users" is suggested in a warning to restore
+    write access.
+    """
+    c = container.run(
+        user="1010",
+        command=["start.sh", "id"],
+    )
+    rv = c.wait(timeout=5)
+    assert rv == 0 or rv["StatusCode"] == 0
+    logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "uid=1010(jovyan) gid=0(root)" in logs
+    warnings = [
+        warning for warning in logs.split("\n") if warning.startswith("WARNING")
+    ]
+    assert len(warnings) == 1
+    assert "--group-add=users" in warnings[0]
+
+
+def test_set_uid_and_nb_user(container):
+    """Container should run with the specified uid and NB_USER."""
+    c = container.run(
+        user="1010",
+        environment=["NB_USER=kitten"],
+        group_add=["users"],  # Ensures write access to /home/jovyan
+        command=["start.sh", "id"],
+    )
+    rv = c.wait(timeout=5)
+    assert rv == 0 or rv["StatusCode"] == 0
+    logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "uid=1010(kitten) gid=0(root)" in logs
+    warnings = [
+        warning for warning in logs.split("\n") if warning.startswith("WARNING")
+    ]
+    assert len(warnings) == 1
+    assert "user is kitten but home is /home/jovyan" in warnings[0]
+
+
 def test_container_not_delete_bind_mount(container, tmp_path):
     """Container should not delete host system files when using the (docker)
     -v bind mount flag and mapping to /home/jovyan.
@@ -235,6 +315,9 @@ def test_container_not_delete_bind_mount(container, tmp_path):
         command=["start.sh", "ls"],
     )
     rv = c.wait(timeout=5)
+    logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     assert rv == 0 or rv["StatusCode"] == 0
     assert p.read_text() == "some-content"
     assert len(list(tmp_path.iterdir())) == 1
@@ -264,4 +347,6 @@ def test_jupyter_env_vars_to_unset_as_root(container, enable_root):
     rv = c.wait(timeout=10)
     assert rv == 0 or rv["StatusCode"] == 0
     logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     assert "I like bananas and stuff, and love to keep secrets!" in logs

base-notebook/test/test_package_managers.py

@@ -29,6 +29,8 @@ def test_package_manager(container, package_manager, version_arg):
     rv = c.wait(timeout=5)
     logs = c.logs(stdout=True).decode("utf-8")
     LOGGER.debug(logs)
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     assert (
         rv == 0 or rv["StatusCode"] == 0
     ), f"Package manager {package_manager} not working"

base-notebook/test/test_pandoc.py

@@ -14,5 +14,7 @@ def test_pandoc(container):
     )
     c.wait(timeout=10)
     logs = c.logs(stdout=True).decode("utf-8")
+    assert "ERROR" not in logs
+    assert "WARNING" not in logs
     LOGGER.debug(logs)
     assert "<p><strong>BOLD</strong></p>" in logs

base-notebook/test/test_python.py

@@ -16,6 +16,8 @@ def test_python_version(container, python_next_version="3.10"):
     )
     cmd = c.exec_run("python --version")
     output = cmd.output.decode("utf-8")
+    assert "ERROR" not in output
+    assert "WARNING" not in output
     actual_python_version = version.parse(output.split()[1])
     assert actual_python_version < version.parse(
         python_next_version

base-notebook/test/test_start_container.py

@@ -27,6 +27,15 @@ def test_start_notebook(container, http_client, env, expected_server):
     resp = http_client.get("http://localhost:8888")
     logs = c.logs(stdout=True).decode("utf-8")
     LOGGER.debug(logs)
+    assert "ERROR" not in logs
+    if expected_server != "notebook":
+        assert "WARNING" not in logs
+    else:
+        warnings = [
+            warning for warning in logs.split("\n") if warning.startswith("WARNING")
+        ]
+        assert len(warnings) == 1
+        assert warnings[0].startswith("WARNING: Jupyter Notebook deprecation notice")
     assert resp.status_code == 200, "Server is not listening"
     assert (
         f"Executing the command: jupyter {expected_server}" in logs
@@ -51,4 +60,6 @@ def test_tini_entrypoint(container, pid=1, command="tini"):
     # Select the PID 1 and get the corresponding command
     cmd = c.exec_run(f"ps -p {pid} -o comm=")
     output = cmd.output.decode("utf-8").strip("\n")
+    assert "ERROR" not in output
+    assert "WARNING" not in output
     assert output == command, f"{command} shall be launched as pid {pid}, got {output}"