-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tornado's web.authenticated
callling deprecated get_current_user
#1012
Comments
I may have found the issues and will report back once I have confirmed. |
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 |
I ran into this again today, and am investigating. I think the problem is that by the time More specifically, my class's So I'm not sure if it's currently possible to use To 'solve' this, the checking code of 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 |
I'm going to try to reproduce this issue, to make sure my understanding is completely correct - decorators and |
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 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). |
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) |
web.authenticated
callling deprecated get_current_user
web.authenticated
callling deprecated get_current_user
We have indeed introduced the requirement that We do this in JupyterHub because get_current_user had to become async, so we replaced the It is not quite true that tornado requires |
Description
In Jupyter Server 2.0 we are deprecating
self.get_current_user
in favor ofself.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'sweb.authenticated
decorator getsself.current_user
which results inself.get_current_user()
being called ifself._current_user
is not set. The result is that Jupyter Server 2 raises an exception because the deprecatedself.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 ofProxyHandler
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 ofself.current_user
here:https://github.com/tornadoweb/tornado/blob/master/tornado/web.py#L3194
When
self._current_user
is not set, Tornado itself callsself.get_current_user()
here:https://github.com/tornadoweb/tornado/blob/master/tornado/web.py#L1344
There are two related issues I believe:
self.prepare()
method in Jupyter Server's handler appears to have logic that will ensure thatself.current_user
is set. It is likely that the handler logic in Jupyter Server Proxy is resulting inprepare
not being called early enough or that the logic to setcurrent_user
is too late withinprepare
.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.The text was updated successfully, but these errors were encountered: