Tornado's `web.authenticated` callling deprecated `get_current_user`

[ellisonbg]

Description

In Jupyter Server 2.0 we are deprecating self.get_current_user in favor of self.current_users in the handlers. I am tracking down a bug when using Jupyter Server proxy with Jupyter Server 2. My current hypothesis is that Tornado's web.authenticated decorator gets self.current_user which results in self.get_current_user() being called if self._current_user is not set. The result is that Jupyter Server 2 raises an exception because the deprecated self.get_current_user() is called.

Reproduce

I don't yet have a simple way of reproducing this yet, but here is a rough sequence of calls that is resulting in this problem:

First, when Jupyter Server proxy receives a request that needs to be proxied, the self.proxy() method of ProxyHandler gets called:

https://github.com/jupyterhub/jupyter-server-proxy/blob/main/jupyter_server_proxy/handlers.py#L269

This method is decorated with Tornado's web.authenticated decorator. This decorator, retrieves the value of self.current_user here:

https://github.com/tornadoweb/tornado/blob/master/tornado/web.py#L3194

When self._current_user is not set, Tornado itself calls self.get_current_user() here:

https://github.com/tornadoweb/tornado/blob/master/tornado/web.py#L1344

There are two related issues I believe:

1. First, the self.prepare() method in Jupyter Server's handler appears to have logic that will ensure that self.current_user is set. It is likely that the handler logic in Jupyter Server Proxy is resulting in prepare not being called early enough or that the logic to set current_user is too late within prepare.
2. More importantly, I think there is a deeper problem that may cause problems for us. This is that self.get_current_user() is a public API of the base Tornado handler (see here). I am not sure what it means for our subclass of the Tornado handler class to deprecate a public method in the base class. We can tell people to not call it when using our subclass, but Tornado itself can still call it (as in this case).

Expected behavior

Jupyter Server proxy, which doesn't directly call self.get_current_user() should work without modification with Jupyter Server 2.

[ellisonbg]

I may have found the issues and will report back once I have confirmed.

[ellisonbg]

OK, this doesn't appear to be an issue in Jupyter Server or Jupyter Server Proxy. Looks like in this particular case, there was a subclass of the JSP handler that was overriding prepare but not calling the base class prepare (which does all of the identity/user handling). Closing for now.

[yuvipanda]

I ran into this again today, and am investigating.

I think the problem is that by the time web.authenticated is called, self.current_user must already be set because web.authenticated uses it (https://github.com/tornadoweb/tornado/blob/65a9e48f8ce645f104e3e0aa772222e70b0376d9/tornado/web.py#L3287). So even if I call super.prepare() that doesn't actually work.

More specifically, my class's prepare' (and hence my super.prepare()call) are only called in https://github.com/tornadoweb/tornado/blob/master/tornado/web.py#L3301, butcurrent_user` is already called before that, in https://github.com/tornadoweb/tornado/blob/65a9e48f8ce645f104e3e0aa772222e70b0376d9/tornado/web.py#L3287.

So I'm not sure if it's currently possible to use prepare to enforce authentication in a JupyterServer subclass.

To 'solve' this, the checking code of web.authenticated needs to be called after the super.prepare call. Since web.authenticated is a decorator that gets a handle to the RequestHandler via the function it is decorating, this hacky code accomplishes that:

    async def prepare(self, *args, **kwargs):
        await super().prepare(*args, **kwargs)
        @web.authenticated
        def fake_prepare(self):
            pass
        fake_prepare(self)

However, that's far from ideal :)

I'd really like to put @web.authenticated on prepare on a project. I'm also not sure what it means for jupyter_server to deprecate a method from tornado - as you mention, other parts of tornado may call this too.

[yuvipanda]

I'm going to try to reproduce this issue, to make sure my understanding is completely correct - decorators and super calls are mixed here, and can be tricky :)

[yuvipanda]

Here's a simple reproducer:

import json

from jupyter_server.extension.handler import ExtensionHandlerMixin
from jupyter_server.base.handlers import APIHandler
import tornado


class PingHandler(ExtensionHandlerMixin, APIHandler):
    # The following decorator should be present on all verb methods (head, get, post,
    # patch, put, delete, options) to ensure only authorized user can request the
    # Jupyter server
    @property
    def ping_response(self):
        return self.settings["ping_response"]

    @tornado.web.authenticated
    def prepare(self):
        pass

    def get(self):
        self.finish(json.dumps({
            "ping_response": self.ping_response
        }))

I just put that into handlers.py generated from the excellent cookiecutter, and can reproduce this error. If I hit this handler, I get:

    HTTPServerRequest(protocol='http', host='127.0.0.1:8888', method='GET', uri='/repro-prepare/ping', version='HTTP/1.1', remote_ip='127.0.0.1')
    Traceback (most recent call last):
      File "/Users/yuvipanda/.local/share/virtualenvs/ws-exploit/lib/python3.11/site-packages/tornado/web.py", line 1767, in _execute
        result = self.prepare()
                 ^^^^^^^^^^^^^^
      File "/Users/yuvipanda/.local/share/virtualenvs/ws-exploit/lib/python3.11/site-packages/tornado/web.py", line 3287, in wrapper
        if not self.current_user:
               ^^^^^^^^^^^^^^^^^
      File "/Users/yuvipanda/.local/share/virtualenvs/ws-exploit/lib/python3.11/site-packages/tornado/web.py", line 1424, in current_user
        self._current_user = self.get_current_user()
                             ^^^^^^^^^^^^^^^^^^^^^^^
      File "/Users/yuvipanda/.local/share/virtualenvs/ws-exploit/lib/python3.11/site-packages/jupyter_server/base/handlers.py", line 176, in get_current_user
        raise RuntimeError(msg)
    RuntimeError: Calling `PingHandler.get_current_user()` directly is deprecated in jupyter-server 2.0. Use `self.current_user` instead (works in all versions).

[yuvipanda]

Here's the simplified workaround code I'm using now instead:

    async def prepare(self, *args, **kwargs):
        """
        Enforce authentication on *all* requests.

        This method is called *before* any other method for all requests.
        See https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.prepare.
        """
        # Due to https://github.com/jupyter-server/jupyter_server/issues/1012,
        # we can not decorate `prepare` with `@web.authenticated`.
        # `super().prepare`, which calls `JupyterHandler.prepare`, *must* be called
        # before `@web.authenticated` can work. Since `@web.authenticated` is a decorator
        # that relies on the decorated method to get access to request information, we can
        # not call it directly. Instead, we create an empty lambda that takes a request_handler,
        # decorate that with web.authenticated, and call the decorated function.
        await super().prepare(*args, **kwargs)

        web.authenticated(lambda request_handler: None)(self)

[minrk]

We have indeed introduced the requirement that .prepare() is called before .current_user is accessed, and I believe that's okay because the only thing that is prohibited by this, is decorating .prepare() itself with @web.authenticated, which I wouldn't generally expect to work, and putting that at the end of prepare works just as well.

We do this in JupyterHub because get_current_user had to become async, so we replaced the .current_user accessor property with something that raises if it hasn't been populated yet, since it can't be populated synchronously anymore.

It is not quite true that tornado requires .get_current_user() to be allowed to be called, because the actual public API for handlers to retrieve the current user is handler.current_user. It is tornado's default implementation of the .current_user property that calls get_current_user, so it is appropriate to override the property to raise informatively, (i.e. raise RuntimeError(".current_user accessed before being set in .prepare()")), everything ought to work fine).