-
Notifications
You must be signed in to change notification settings - Fork 304
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
Add an option to have authentication enabled for all endpoints by default #1392
Changes from 6 commits
3f65147
a9cff16
646739e
4cbb504
faee488
204d29f
dccc423
8e13727
e6a8d9a
2882516
4e1d664
cd84175
5f6bc16
0facfec
90d7044
54c2ea2
319a4d1
5e7615d
4265f4e
c3f5d8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
from typing import Optional, no_type_check | ||
from urllib.parse import urlparse | ||
|
||
from tornado import ioloop | ||
from tornado import ioloop, web | ||
from tornado.iostream import IOStream | ||
|
||
# ping interval for keeping websockets alive (30 seconds) | ||
|
@@ -82,6 +82,26 @@ def check_origin(self, origin: Optional[str] = None) -> bool: | |
def clear_cookie(self, *args, **kwargs): | ||
"""meaningless for websockets""" | ||
|
||
@no_type_check | ||
def _maybe_auth(self): | ||
"""Verify authentication if required""" | ||
if not self.settings.get("allow_unauthenticated_access", False): | ||
if not self.request.method: | ||
raise web.HTTPError(403) | ||
method = getattr(self, self.request.method.lower()) | ||
if not getattr(method, "__allow_unauthenticated", False): | ||
# rather than re-using `web.authenticated` which also redirects | ||
# to login page on GET, just raise 403 if user is not known | ||
user = self.current_user | ||
if user is None: | ||
self.log.warning("Couldn't authenticate WebSocket connection") | ||
raise web.HTTPError(403) | ||
|
||
def prepare(self, *args, **kwargs): | ||
"""Handle a get request.""" | ||
self._maybe_auth() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, this is because
So this works in test because It feels like we should move the implementation of setting the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, why is IdentityProvider setting the current user in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see the user from identity provider may require There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JupyterHandler is responsible for @property
def current_user(self):
if not hasattr(self, "_jupyter_current_user"):
raise RuntimeError(".current_user accessed before being populated in JupyterHandler.prepare()")
return self. _jupyter_current_user
@current_user.setter
def current_user(self, user):
self._jupyter_current_user = user Any time that error is hit is necessarily code that is bypassing Jupyter Server authentication. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 5e7615d (added test) and 4265f4e (fixed it). Would you like me to add the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the code, I guess we already have this RuntimeError in get_current_user. So maybe it's just the message that wants clarifying? Because it's already there, I don't think moving the RuntimeError to the |
||
return super().prepare(*args, **kwargs) | ||
|
||
@no_type_check | ||
def open(self, *args, **kwargs): | ||
"""Open the websocket.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -370,6 +370,7 @@ def init_settings( | |
"login_url": url_path_join(base_url, "/login"), | ||
"xsrf_cookies": True, | ||
"disable_check_xsrf": jupyter_app.disable_check_xsrf, | ||
"allow_unauthenticated_access": jupyter_app.allow_unauthenticated_access, | ||
"allow_remote_access": jupyter_app.allow_remote_access, | ||
"local_hostnames": jupyter_app.local_hostnames, | ||
"authenticate_prometheus": jupyter_app.authenticate_prometheus, | ||
|
@@ -1214,6 +1215,21 @@ def _deprecated_password_config(self, change: t.Any) -> None: | |
""", | ||
) | ||
|
||
allow_unauthenticated_access = Bool( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For ease of use in JupyterHub deployments, I would also suggest making this available as an env variable. I know you can setup a _config.json file reasonably easily, but given the possible high impact here, I think it's worth making an env var for this too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in cd84175 |
||
True, | ||
config=True, | ||
help="""Allow requests unauthenticated access to endpoints without authentication rules. | ||
|
||
When set to `True` (default in jupyter-server 2.0, subject to change | ||
in the future), any request to an endpoint without an authentication rule | ||
(either `@tornado.web.authenticated`, or `@allow_unauthenticated`) | ||
will be permitted, regardless of whether user has logged in or not. | ||
|
||
When set to `False`, logging in will be required for access to each endpoint, | ||
excluding the endpoints marked with `@allow_unauthenticated` decorator. | ||
""", | ||
) | ||
|
||
allow_remote_access = Bool( | ||
config=True, | ||
help="""Allow requests where the Host header doesn't point to a local server | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps something even more aggressive, e.g.
@UNAUTHENTICATED
.