Repository: jupyterhub/tmpauthenticator (language: python)

Issue:
Deprecate TmpAuthenticator.process_user for a configurable traitlet?
There is a function in TmpAuthenticator called process_user that a subclass could override to do things, but should we perhaps let that be a configurable traitlet?

Then I think it could be configurable without needing to define a new authenticator class, and it could still be something configurable by a subclass by letting it provide a default value for the configurable traitlet.


Discussion / hints:
Yes we should.
Also let's see if we can instead piggyback on something that already exists in the base class?
I don't see anything that does this exact thing (process user objects) in the base Authenticator class.
@yuvipanda I realize now that `TmpAuthenticator` bypasses the typical flow of an authenticator, typically just overriding `authenticate`.

This instead declares a new webrequest handler and configured `login_url` to reference it, therefore bypassing a lot of other base functionality in the `Authenticator` class implemented in the function [`get_authenticated_user`](https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/auth.py#L472) which isn't supposed to be overridden by subclasses (sais so in docstring).

The point I'm coming to is that there is the [`run_post_auth_hook()` being called by the `get_authenticated_user`](https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/auth.py#L523-L529), which allows for a configurable hook to process the user / the authentication data that boostraps the user.

So I figure the situation is that because TmpAuthenticator is bypassing a lot of typical logic by providing a new handler that creates a user etc instead of relying on the default handler that calls the authenticators `get_authenticated_user`, we bypass also for example the configurable [`post_auth_hook`](https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/auth.py#L292-L326).

I'm not sure what to do, but we agree that its an incremental improvement to let process_user be configurable.

I'm thinking maybe we could make use of the `run_auth_hook` instead - but - that is designed for authentication state (`{"name": "username here", "admin": True, "auth_state": {}}` - not a jupyterhub user model object. But, I'm not sure if that is a limitation for users of process_user?

Are you a user of `process_user` with a practical use case?
I don't remember why process_user was added, unfortunately. Maybe git logs have a clue? It could have been added for tmpnb's use case, but what exactly that was I am not sure :(
This is where it was added https://github.com/jupyterhub/tmpauthenticator/commit/8231fdad0899480df7d4916bf6150c86d64be67a

>**Add ability for subclasses to modify created user before spawning**
>
>This is primarily used right now to allow the image to be spawned
>to be specified as a query param, but is generic enough for future
>unforeseen uses

Besides this, I saw no trace of code in the jupyter org, jupyterhub org, or under your username when searching.

I figure process_user was used to probe the web request handler for query string data, and then store it in the user object for use by the spawner later. This is something that can be done by the post_auth_hook as well. The signatures differ a bit, where process_user provides a user object while the post_auth_hook functions are provided the authentication dictionary that includes auth_state etc.

```python
    def process_user(self, user, handler):
    def my_hook(authenticator, handler, authentication):
```

I suggest we remove process_user and point users towards post_auth_hook in the changelog.
> I suggest we remove process_user and point users towards post_auth_hook in the changelog.

Yep let's do it

Fix patch:
diff --git a/tmpauthenticator/__init__.py b/tmpauthenticator/__init__.py
index e19155c..76cf245 100644
--- a/tmpauthenticator/__init__.py
+++ b/tmpauthenticator/__init__.py
@@ -1,4 +1,3 @@
-import inspect
 import uuid
 
 from jupyterhub.auth import Authenticator
@@ -9,57 +8,52 @@
 
 class TmpAuthenticateHandler(BaseHandler):
     """
-    Handler for /tmplogin which is registered by TmpAuthenticator.
+    Provides a GET web request handler for /hub/tmplogin, as registered by
+    TmpAuthenticator's override of Authenticator.get_handlers.
 
-    Creates a new user with a random UUID as username and ensures cookies are
-    updated to let JupyterHub recognize future requests as coming from the newly
-    created user.
+    JupyterHub will redirect here if it doesn't recognize a user via a cookie,
+    but users can also visit /hub/tmplogin explicitly to get setup with a new
+    user.
     """
 
-    def initialize(self, process_user):
-        super().initialize()
-        self.process_user = process_user
-
     async def get(self):
         """
-        Authenticate as a new user.
+        Authenticate as a new random user no matter what.
 
-        Each time /tmplogin is hit, we want to create a brand new user. This lets
-        users hit the hub URL, and immediately get a new server - regardless of wether
-        they had already logged in or not. So /tmplogin really acts as a logout +
-        login mechanism. This only happens when /tmplogin is hit - so you can use
-        other parts of the hub as you normally would.
-        """
-        # Create a new user.
-        #
-        # user_from_username ref: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L504-L505
-        #
-        username = str(uuid.uuid4())
-        user = self.user_from_username(username)
+        This GET request handler mimics parts of what's done by JupyterHub's
+        LoginHandler when a user isn't recognized: to first call
+        BaseHandler.login_user and then redirect the user onwards. The
+        difference is that here users always login as a new user.
+
+        By overwriting any previous user's identifying cookie, it acts as a
+        combination of a logout and login handler.
 
-        # Let a subclasses of TmpAuthenticator process the new user by
-        # overriding TmpAuthenticator.process_user.
+        JupyterHub's LoginHandler ref: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/login.py#L129-L138
+        """
+        # Login as a new user, without checking if we were already logged in
         #
-        user = self.process_user(user, self)
-        if inspect.isawaitable(user):
-            user = await user
+        user = await self.login_user(None)
 
         # Set or overwrite the login cookie to recognize the new user.
         #
-        # set_login_cookie(user) sets a login cookie for the provided user via
-        # set_hub_cookie(user), but only if it doesn't recognize a user from an
-        # pre-existing login cookie. Due to that, we unconditionally call
-        # self.set_hub_cookie(user) here.
+        # login_user calls set_login_cookie(user), that sets a login cookie for
+        # the user via set_hub_cookie(user), but only if it doesn't recognize a
+        # user from an pre-existing login cookie. Due to that, we
+        # unconditionally call self.set_hub_cookie(user) here.
         #
-        # set_login_cookie ref: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L627-L628
-        # set_hub_cookie ref:   https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L623-L625
+        # BaseHandler.login_user:                   https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L823-L843
+        # - BaseHandler.authenticate:               https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L643-L644
+        #   - Authenticator.get_authenticated_user: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/auth.py#L472-L534
+        # - BaseHandler.auth_to_user:               https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L774-L821
+        # - BaseHandler.set_login_cookie:           https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L627-L628
+        #   - BaseHandler.set_session_cookie:       https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L601-L613
+        #   - BaseHandler.set_hub_cookie:           https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L623-L625
         #
-        self.set_login_cookie(user)
         self.set_hub_cookie(user)
 
         # Login complete, redirect the user.
         #
-        # get_next_url ref: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L646-L653
+        # BaseHandler.get_next_url ref: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/base.py#L646-L653
         #
         next_url = self.get_next_url(user)
         self.redirect(next_url)
@@ -101,37 +95,36 @@ def _auto_login_default(self):
         """,
     ).tag(config=True)
 
-    def process_user(self, user, handler):
+    async def authenticate(self, handler, data):
         """
-        Do additional arbitrary things to the created user before spawn.
-
-        user is a user object, and handler is a TmpAuthenticateHandler object
-
-        Should return the new user object.
-
-        This method can be a coroutine.
-
-        Note: This is primarily for overriding in subclasses
+        Always authenticate a new user by generating a universally unique
+        identifier (uuid).
         """
-        return user
+        username = str(uuid.uuid4())
+        return {
+            "name": username,
+        }
 
     def get_handlers(self, app):
         """
         Registers a dedicated endpoint and web request handler for logging in
-        with tmpauthenticator. This is needed as /hub/login is reserved for
-        redirecting to whats returned by login_url.
+        with TmpAuthenticator. This is needed as /hub/login is reserved for
+        redirecting to what's returned by login_url.
 
         ref: https://github.com/jupyterhub/jupyterhub/pull/1066
         """
-        # FIXME: How to do this better?
-        extra_settings = {'process_user': self.process_user}
-        return [('/tmplogin', TmpAuthenticateHandler, extra_settings)]
+        return [("/tmplogin", TmpAuthenticateHandler)]
 
     def login_url(self, base_url):
         """
-        login_url is overridden as intended for Authenticator subclasses by
-        jupyterhub to redirected users to it when they visit /hub/login.
+        login_url is overridden as intended for Authenticator subclasses that
+        provides a custom login handler (for /hub/tmplogin).
+
+        JupyterHub redirects users to this destination from /hub/login if
+        auto_login is set, or if its not set and users press the "Sign in ..."
+        button.
 
         ref: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/auth.py#L708-L723
+        ref: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/handlers/login.py#L118-L147
         """
-        return url_path_join(base_url, 'tmplogin')
+        return url_path_join(base_url, "tmplogin")


Test patch:
diff --git a/tests/test_tmpauthenticator.py b/tests/test_tmpauthenticator.py
index ea6d62e..f7e9a38 100644
--- a/tests/test_tmpauthenticator.py
+++ b/tests/test_tmpauthenticator.py
@@ -5,8 +5,5 @@
 - Validate that TmpAuthenticateHandler.get() creates a new user and updates the
   login cookie even if it was already set.
 - Validate that TmpAuthenticateHandler.get() redirects to something sensible.
-
-Bonus:
-
-- Test process_user by creating a subclass doing something.
+- Validate that a configured post_auth_hook is executed.
 """