jupyterhub / tmpauthenticator

JupyterHub authenticator that hands out temporary accounts for everyone. For use in tmpnb.org
BSD 3-Clause "New" or "Revised" License
23 stars 18 forks source link

Deprecate TmpAuthenticator.process_user for a configurable traitlet? #39

Closed consideRatio closed 1 year ago

consideRatio commented 1 year ago

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.

yuvipanda commented 1 year ago

Yes we should.

yuvipanda commented 1 year ago

Also let's see if we can instead piggyback on something that already exists in the base class?

yuvipanda commented 1 year ago

I don't see anything that does this exact thing (process user objects) in the base Authenticator class.

consideRatio commented 1 year ago

@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 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, 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.

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?

yuvipanda commented 1 year ago

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 :(

consideRatio commented 1 year ago

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.

    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.

yuvipanda commented 1 year ago

I suggest we remove process_user and point users towards post_auth_hook in the changelog.

Yep let's do it