Repository: jupyterhub/jupyterhub

Issue:
Can we override `Authenticator.check_allowed` in jupyterhub/oauthenticator and assume authentication argument be passed?
In [oauthenticator #594](https://github.com/jupyterhub/oauthenticator/pull/594) we are currently planning to override the `check_allowed` as considered and proposed in https://github.com/jupyterhub/oauthenticator/pull/594#issuecomment-1519086377 and the comments below. The [`check_allowed` function signature](https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/auth.py#L432-L438) is...

```python
    def check_allowed(self, username, authentication=None):
        """Check if a username is allowed to authenticate based on configuration

        Return True if username is allowed, False otherwise.
        No allowed_users set means any username is allowed.

        Names are normalized *before* being checked against the allowed set.
```

What came as a surprise to me now was that check_allowed was also being called by JupyterHub without passing the authentication dictionary (aka. `auth_model`), and having error messages related to specifically that `allowed_users` check failing.

It seems that also the [LocalAuthenticator](https://github.com/jupyterhub/jupyterhub/blob/main/jupyterhub/auth.py#L871-L875) class overrides `check_allowed`, and adds a check for beloning to either allowed_users or allowed_groups very similar to what we want to do. Our unique challenge is that we rely on being passed `authentication` aka `auth_model`, and that isn't passed at all times when check_allowed is used.

In practice it seems that only if the deprecated `JupyterHub.api_tokens` is used or if `JupyterHub.load_roles` is configured, and they involve user api tokens or similar, then `check_allowed` will be called without an authentication dictionary.

### Using `JupyterHub.load_roles`

[`JupyterHub.init_role_assignment`](https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/app.py#L2274-L2295)

```python
    async def init_role_assignment(self):
        """
        This function loads config from `JupyterHub.load_roles`
        """
        # tokens are added separately
        kinds = ['users', 'services', 'groups']

        # ...

        for role_spec in self.load_roles:
        
            # ...
        
            # add users, services, and/or groups,
            # tokens need to be checked for permissions
            for kind in kinds:
                orm_role_bearers = []
                if kind in role_spec:
                    for name in role_spec[kind]:
                        if kind == 'users':
                            name = self.authenticator.normalize_username(name)
                            if not (
                                await maybe_future(
                                    self.authenticator.check_allowed(name, None)
                                )
                            ):
                                raise ValueError(
                                    f"Username {name} is not in Authenticator.allowed_users"
                                )
```

### Using `JupyterHub.api_tokens`

[`JupyterHub.api_tokens`](https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/app.py#L1049-L1060)

```python
    api_tokens = Dict(
        Unicode(),
        help="""PENDING DEPRECATION: consider using services

        Dict of token:username to be loaded into the database.

        Allows ahead-of-time generation of API tokens for use by externally managed services,
        which authenticate as JupyterHub users.

        Consider using services for general services that talk to the JupyterHub API.
        """,
    ).tag(config=True)
```

[`JupyterHub.init_api_tokens`](https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/app.py#L2345-L2349)

```python
    async def init_api_tokens(self):
        """Load predefined API tokens (for services) into database"""

        await self._add_tokens(self.service_tokens, kind='service')
        await self._add_tokens(self.api_tokens, kind='user')
        
        # ...
```

[`JupyterHub._add_token`](https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/app.py#L2274-L2295)

```python
    async def _add_tokens(self, token_dict, kind):
        """Add tokens for users or services to the database"""
        if kind == 'user':
            Class = orm.User
        elif kind == 'service':
            Class = orm.Service
        else:
            raise ValueError("kind must be user or service, not %r" % kind)

        db = self.db
        for token, name in token_dict.items():
            if kind == 'user':
                name = self.authenticator.normalize_username(name)
                if not (
                    await maybe_future(self.authenticator.check_allowed(name, None))
                ):
                    raise ValueError(
                        "Token user name %r is not in Authenticator.allowed_users"
                        % name
                    )
                if not self.authenticator.validate_username(name):
                    raise ValueError("Token user name %r is not valid" % name)
```

Discussion:

Patch:
diff --git a/jupyterhub/app.py b/jupyterhub/app.py
index 3a5e3371..eab6ef60 100644
--- a/jupyterhub/app.py
+++ b/jupyterhub/app.py
@@ -2041,21 +2041,20 @@ class JupyterHub(Application):
 
         TOTAL_USERS.set(total_users)
 
-    async def _get_or_create_user(self, username):
+    async def _get_or_create_user(self, username, hint):
         """Create user if username is found in config but user does not exist"""
-        if not (await maybe_future(self.authenticator.check_allowed(username, None))):
-            raise ValueError(
-                "Username %r is not in Authenticator.allowed_users" % username
-            )
         user = orm.User.find(self.db, name=username)
         if user is None:
             if not self.authenticator.validate_username(username):
                 raise ValueError("Username %r is not valid" % username)
-            self.log.info(f"Creating user {username}")
+            self.log.info(f"Creating user {username} found in {hint}")
             user = orm.User(name=username)
             self.db.add(user)
             roles.assign_default_roles(self.db, entity=user)
             self.db.commit()
+            f = self.authenticator.add_user(user)
+            if f:
+                await maybe_future(f)
         return user
 
     async def init_groups(self):
@@ -2064,7 +2063,9 @@ class JupyterHub(Application):
 
         if self.authenticator.manage_groups and self.load_groups:
             raise ValueError("Group management has been offloaded to the authenticator")
+
         for name, contents in self.load_groups.items():
+            self.log.debug("Loading group %s", name)
             group = orm.Group.find(db, name)
 
             if group is None:
@@ -2080,9 +2081,11 @@ class JupyterHub(Application):
             if 'users' in contents:
                 for username in contents['users']:
                     username = self.authenticator.normalize_username(username)
-                    user = await self._get_or_create_user(username)
+                    user = await self._get_or_create_user(
+                        username, hint=f"group: {name}"
+                    )
                     if group not in user.groups:
-                        self.log.debug(f"Adding user {username} to  group {name}")
+                        self.log.debug(f"Adding user {username} to group {name}")
                         group.users.append(user)
 
             if 'properties' in contents:
@@ -2110,8 +2113,9 @@ class JupyterHub(Application):
         roles_with_new_permissions = []
         for role_spec in self.load_roles:
             role_name = role_spec['name']
+            self.log.debug("Loading role %s", role_name)
             if role_name in default_roles_dict:
-                self.log.debug(f"Overriding default role {role_name}")
+                self.log.debug("Overriding default role %s", role_name)
                 # merge custom role spec with default role spec when overriding
                 # so the new role can be partially defined
                 default_role_spec = default_roles_dict.pop(role_name)
@@ -2198,34 +2202,33 @@ class JupyterHub(Application):
                     for name in role_spec[kind]:
                         if kind == 'users':
                             name = self.authenticator.normalize_username(name)
-                            if not (
-                                await maybe_future(
-                                    self.authenticator.check_allowed(name, None)
-                                )
-                            ):
-                                raise ValueError(
-                                    f"Username {name} is not in Authenticator.allowed_users"
-                                )
                         Class = orm.get_class(kind)
                         orm_obj = Class.find(db, name)
                         if orm_obj is not None:
                             orm_role_bearers.append(orm_obj)
                         else:
-                            self.log.info(
-                                f"Found unexisting {kind} {name} in role definition {role_name}"
-                            )
                             if kind == 'users':
-                                orm_obj = await self._get_or_create_user(name)
+                                orm_obj = await self._get_or_create_user(
+                                    name, hint=f"role: {role_name}"
+                                )
                                 orm_role_bearers.append(orm_obj)
                             elif kind == 'groups':
+                                self.log.info(
+                                    f"Creating group {name} found in role: {role_name}"
+                                )
                                 group = orm.Group(name=name)
                                 db.add(group)
                                 db.commit()
                                 orm_role_bearers.append(group)
-                            else:
+                            elif kind == "services":
                                 raise ValueError(
-                                    f"{kind} {name} defined in config role definition {role_name} but not present in database"
+                                    f"Found undefined service {name} in role {role_name}. Define it first in c.JupyterHub.services."
                                 )
+                            else:
+                                # this can't happen now, but keep the `else` in case we introduce a problem
+                                # in the declaration of `kinds` above
+                                raise ValueError(f"Unhandled role member kind: {kind}")
+
                         # Ensure all with admin role have admin flag
                         if role_name == 'admin':
                             orm_obj.admin = True
@@ -2285,20 +2288,12 @@ class JupyterHub(Application):
         for token, name in token_dict.items():
             if kind == 'user':
                 name = self.authenticator.normalize_username(name)
-                if not (
-                    await maybe_future(self.authenticator.check_allowed(name, None))
-                ):
-                    raise ValueError(
-                        "Token user name %r is not in Authenticator.allowed_users"
-                        % name
-                    )
                 if not self.authenticator.validate_username(name):
                     raise ValueError("Token user name %r is not valid" % name)
             if kind == 'service':
                 if not any(service["name"] == name for service in self.services):
                     self.log.warning(
-                        "Warning: service '%s' not in services, creating implicitly. It is recommended to register services using services list."
-                        % name
+                        f"service {name} not in services, creating implicitly. It is recommended to register services using services list."
                     )
             orm_token = orm.APIToken.find(db, token)
             if orm_token is None:


Test patch:
diff --git a/jupyterhub/tests/test_app.py b/jupyterhub/tests/test_app.py
index cae40a60..f1967dce 100644
--- a/jupyterhub/tests/test_app.py
+++ b/jupyterhub/tests/test_app.py
@@ -251,7 +251,6 @@ async def test_load_groups(tmpdir, request):
     hub.init_db()
     db = hub.db
     await hub.init_role_creation()
-
     await hub.init_users()
     await hub.init_groups()
 
@@ -435,3 +434,42 @@ def test_launch_instance(request, argv, sys_argv):
         assert hub.argv == sys_argv[1:]
     else:
         assert hub.argv == argv
+
+
+async def test_user_creation(tmpdir, request):
+    allowed_users = {"in-allowed", "in-group-in-allowed", "in-role-in-allowed"}
+    groups = {
+        "group": {
+            "users": ["in-group", "in-group-in-allowed"],
+        }
+    }
+    roles = [
+        {
+            "name": "therole",
+            "users": ["in-role", "in-role-in-allowed"],
+        }
+    ]
+
+    cfg = Config()
+    cfg.Authenticator.allowed_users = allowed_users
+    cfg.JupyterHub.load_groups = groups
+    cfg.JupyterHub.load_roles = roles
+    ssl_enabled = getattr(request.module, "ssl_enabled", False)
+    kwargs = dict(config=cfg)
+    if ssl_enabled:
+        kwargs['internal_certs_location'] = str(tmpdir)
+    hub = MockHub(**kwargs)
+    hub.init_db()
+
+    await hub.init_role_creation()
+    await hub.init_role_assignment()
+    await hub.init_users()
+    await hub.init_groups()
+    assert hub.authenticator.allowed_users == {
+        "admin",  # added by default config
+        "in-allowed",
+        "in-group-in-allowed",
+        "in-role-in-allowed",
+        "in-group",
+        "in-role",
+    }