Skip to content
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

Merged
merged 20 commits into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
3f65147
Add `allow_unauthenticated_access` traitlet and `@allow_unauthenticated`
krassowski Feb 10, 2024
a9cff16
Tests, correct coroutine handling, add decorators where needed,
krassowski Feb 10, 2024
646739e
Align ordering of test cases, improve wording of comments
krassowski Feb 11, 2024
4cbb504
Implement auth and tests for websockets
krassowski Feb 11, 2024
faee488
Use `allow_unauthenticated` in `FilesRedirectHandler` for now
krassowski Feb 11, 2024
204d29f
Do not touch coroutines
krassowski Feb 11, 2024
dccc423
Add runtime check to ensure handlers have auth decorators
krassowski Feb 12, 2024
8e13727
Do not ever raise for extension endpoints, and only
krassowski Feb 13, 2024
e6a8d9a
Add test for extension handler warning, correct type
krassowski Feb 13, 2024
2882516
Fix test for authentication enforcement for extensions
krassowski Feb 13, 2024
4e1d664
Add test for GET redirect in handler tests
krassowski Feb 13, 2024
cd84175
Add `JUPYTER_SERVER_ALLOW_UNAUTHENTICATED_ACCESS` env variable
krassowski Feb 13, 2024
5f6bc16
Better heuristic for `@tornado.web.authenticated`
krassowski Feb 13, 2024
0facfec
Improve docstring/fix typo
krassowski Feb 13, 2024
90d7044
Improve and test tornado.web.authenticated heuristic
krassowski Feb 13, 2024
54c2ea2
Merge branch 'main' into auth-by-default
krassowski Feb 15, 2024
319a4d1
Call `super.prepare()` in all code paths, test it
krassowski Feb 20, 2024
5e7615d
Add test for identity provider being used
krassowski Feb 20, 2024
4265f4e
Ensure that identity provider is used for auth,
krassowski Feb 20, 2024
c3f5d8f
Update minimum `pytest-jupyter`; we need 0.7 as it
krassowski Feb 20, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions jupyter_server/auth/decorator.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,58 @@ async def inner(self, *args, **kwargs):
return cast(FuncT, wrapper(method))

return cast(FuncT, wrapper)


def allow_unauthenticated(method: FuncT) -> FuncT:
Copy link
Contributor

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.

"""A decorator for tornado.web.RequestHandler methods
that allows any user to make the following request.

Selectively disables the 'authentication' layer of REST API which
is active when `ServerApp.allow_unauthenticated_access = False`.

To be used exclusively on endpoints which may be considered public,
for example the login page handler.

.. versionadded:: 2.13

Parameters
----------
method : bound callable
the endpoint method to remove authentication from.
"""

@wraps(method)
def wrapper(self, *args, **kwargs):
return method(self, *args, **kwargs)

setattr(wrapper, "__allow_unauthenticated", True)

return cast(FuncT, wrapper)


def ws_authenticated(method: FuncT) -> FuncT:
"""A decorator for websockets derived from `WebSocketHandler`
that authenticates user before allowing to proceed.

Differently from tornado.web.authenticated, does not redirect
to the login page, which would be meaningless for websockets.

.. versionadded:: 2.13

Parameters
----------
method : bound callable
the endpoint method to add authentication for.
"""

@wraps(method)
def wrapper(self, *args, **kwargs):
user = self.current_user
if user is None:
self.log.warning("Couldn't authenticate WebSocket connection")
raise HTTPError(403)
return method(self, *args, **kwargs)

setattr(wrapper, "__allow_unauthenticated", False)

return cast(FuncT, wrapper)
4 changes: 4 additions & 0 deletions jupyter_server/auth/login.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from tornado.escape import url_escape

from ..base.handlers import JupyterHandler
from .decorator import allow_unauthenticated
from .security import passwd_check, set_password


Expand Down Expand Up @@ -73,6 +74,7 @@ def _redirect_safe(self, url, default=None):
url = default
self.redirect(url)

@allow_unauthenticated
def get(self):
"""Get the login form."""
if self.current_user:
Expand All @@ -81,6 +83,7 @@ def get(self):
else:
self._render()

@allow_unauthenticated
def post(self):
"""Post a login."""
user = self.current_user = self.identity_provider.process_login_form(self)
Expand Down Expand Up @@ -110,6 +113,7 @@ def passwd_check(self, a, b):
"""Check a passwd."""
return passwd_check(a, b)

@allow_unauthenticated
def post(self):
"""Post a login form."""
typed_password = self.get_argument("password", default="")
Expand Down
2 changes: 2 additions & 0 deletions jupyter_server/auth/logout.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
# Copyright (c) Jupyter Development Team.
# Distributed under the terms of the Modified BSD License.
from ..base.handlers import JupyterHandler
from .decorator import allow_unauthenticated


class LogoutHandler(JupyterHandler):
"""An auth logout handler."""

@allow_unauthenticated
def get(self):
"""Handle a logout."""
self.identity_provider.clear_login_cookie(self)
Expand Down
49 changes: 45 additions & 4 deletions jupyter_server/base/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import warnings
from http.client import responses
from logging import Logger
from typing import TYPE_CHECKING, Any, Awaitable, Sequence, cast
from typing import TYPE_CHECKING, Any, Awaitable, Coroutine, Sequence, cast
from urllib.parse import urlparse

import prometheus_client
Expand All @@ -29,7 +29,7 @@
from jupyter_server import CallContext
from jupyter_server._sysinfo import get_sys_info
from jupyter_server._tz import utcnow
from jupyter_server.auth.decorator import authorized
from jupyter_server.auth.decorator import allow_unauthenticated, authorized
from jupyter_server.auth.identity import User
from jupyter_server.i18n import combine_translations
from jupyter_server.services.security import csp_report_uri
Expand Down Expand Up @@ -630,6 +630,16 @@ async def prepare(self) -> Awaitable[None] | None: # type:ignore[override]
self.set_cors_headers()
if self.request.method not in {"GET", "HEAD", "OPTIONS"}:
self.check_xsrf_cookie()

if not self.settings.get("allow_unauthenticated_access", False):
if not self.request.method:
raise HTTPError(403)
method = getattr(self, self.request.method.lower())
if not getattr(method, "__allow_unauthenticated", False):
# reuse `web.authenticated` logic, which redirects to the login
# page on GET and HEAD and otherwise raises 403
return web.authenticated(lambda _method: None)(self)
krassowski marked this conversation as resolved.
Show resolved Hide resolved

return super().prepare()

# ---------------------------------------------------------------
Expand Down Expand Up @@ -794,6 +804,7 @@ def finish(self, *args: Any, **kwargs: Any) -> Future[Any]:
self.set_header("Content-Type", set_content_type)
return super().finish(*args, **kwargs)

@allow_unauthenticated
def options(self, *args: Any, **kwargs: Any) -> None:
"""Get the options."""
if "Access-Control-Allow-Headers" in self.settings.get("headers", {}):
Expand Down Expand Up @@ -1002,6 +1013,18 @@ def compute_etag(self) -> str | None:
"""Compute the etag."""
return None

# access is allowed as this class is used to serve static assets on login page
# TODO: create an allow-list of files used on login page and remove this decorator
@allow_unauthenticated
def get(self, path: str, include_body: bool = True) -> Coroutine[Any, Any, None]:
return super().get(path, include_body)

# access is allowed as this class is used to serve static assets on login page
# TODO: create an allow-list of files used on login page and remove this decorator
@allow_unauthenticated
def head(self, path: str) -> Awaitable[None]:
return super().head(path)

@classmethod
def get_absolute_path(cls, roots: Sequence[str], path: str) -> str:
"""locate a file to serve on our static file search path"""
Expand Down Expand Up @@ -1036,6 +1059,7 @@ class APIVersionHandler(APIHandler):

_track_activity = False

@allow_unauthenticated
def get(self) -> None:
"""Get the server version info."""
# not authenticated, so give as few info as possible
Expand All @@ -1048,6 +1072,7 @@ class TrailingSlashHandler(web.RequestHandler):
This should be the first, highest priority handler.
"""

@allow_unauthenticated
def get(self) -> None:
"""Handle trailing slashes in a get."""
assert self.request.uri is not None
Expand All @@ -1064,6 +1089,7 @@ def get(self) -> None:
class MainHandler(JupyterHandler):
"""Simple handler for base_url."""

@allow_unauthenticated
def get(self) -> None:
"""Get the main template."""
html = self.render_template("main.html")
Expand Down Expand Up @@ -1104,18 +1130,20 @@ async def redirect_to_files(self: Any, path: str) -> None:
self.log.debug("Redirecting %s to %s", self.request.path, url)
self.redirect(url)

@allow_unauthenticated
async def get(self, path: str = "") -> None:
return await self.redirect_to_files(self, path)


class RedirectWithParams(web.RequestHandler):
"""Sam as web.RedirectHandler, but preserves URL parameters"""
"""Same as web.RedirectHandler, but preserves URL parameters"""

def initialize(self, url: str, permanent: bool = True) -> None:
"""Initialize a redirect handler."""
self._url = url
self._permanent = permanent

@allow_unauthenticated
def get(self) -> None:
"""Get a redirect."""
sep = "&" if "?" in self._url else "?"
Expand All @@ -1128,6 +1156,7 @@ class PrometheusMetricsHandler(JupyterHandler):
Return prometheus metrics for this server
"""

@allow_unauthenticated
def get(self) -> None:
"""Get prometheus metrics."""
if self.settings["authenticate_prometheus"] and not self.logged_in:
Expand All @@ -1137,6 +1166,18 @@ def get(self) -> None:
self.write(prometheus_client.generate_latest(prometheus_client.REGISTRY))


class PublicStaticFileHandler(web.StaticFileHandler):
"""Same as web.StaticFileHandler, but decorated to acknowledge that auth is not required."""

@allow_unauthenticated
def head(self, path: str) -> Awaitable[None]:
return super().head(path)

@allow_unauthenticated
def get(self, path: str, include_body: bool = True) -> Coroutine[Any, Any, None]:
return super().get(path, include_body)


# -----------------------------------------------------------------------------
# URL pattern fragments for reuse
# -----------------------------------------------------------------------------
Expand All @@ -1152,6 +1193,6 @@ def get(self) -> None:
default_handlers = [
(r".*/", TrailingSlashHandler),
(r"api", APIVersionHandler),
(r"/(robots\.txt|favicon\.ico)", web.StaticFileHandler),
(r"/(robots\.txt|favicon\.ico)", PublicStaticFileHandler),
(r"/metrics", PrometheusMetricsHandler),
]
23 changes: 22 additions & 1 deletion jupyter_server/base/websocket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -82,6 +82,27 @@ 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)

@no_type_check
def prepare(self, *args, **kwargs):
"""Handle a get request."""
self._maybe_auth()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

._maybe_auth accesses .current_user, but .prepare() below is where it is set. The order needs to be reversed, but I assume there's a reason this is first. I think the override may need to be done a different way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I assume there's a reason this is first

Yes, this is because JupyterHandler.prepare() would raise HTTPError with redirect which does not make sense for websocket.

The order needs to be reversed

So this works in test because .current_user is nominally a getter implemented in tornado.web.RequestHandler. Unfortunately JupyterHandler.prepare() overrides it in a way which will cause a problem for this logic for a non-trivial identity provider (if I see it right, the default user will be used instead of the one provided by IdentityProvider).

It feels like we should move the implementation of setting the current_user from JupyterHandler.prepare to JupyterHandler.get_current_user. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, why is IdentityProvider setting the current user in JupyterHandler and not in AuthenticatedHandler in the first place? I do not see rationale for it in #671

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see the user from identity provider may require awaiting. I guess I can just pass an argument to JupyterHandler.prepare() instructing it to not redirect to login page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JupyterHandler is responsible for .current_user being set, since it may be async, it cannot be done in a property. It would be appropriate to override the .current_user getter, which should make it very clear it can never be accessed before .prepare() is called:

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 RuntimeError on accessing current_user too early (from the last comment) in this PR, or would it be better to ensure that this PR does not introduce potentially breaking changes and can be adopted in existing deployments easily?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 .current_user getter can break anything. But at the same time, since folks may call get_current_user() or access the (correct) .current_user, then I guess .get_current_user covers both cases. So maybe it only makes sense to put it in .current_user if it helps to have two distinct, clear error messages.

return super().prepare(*args, **kwargs)

@no_type_check
def open(self, *args, **kwargs):
"""Open the websocket."""
Expand Down
2 changes: 1 addition & 1 deletion jupyter_server/extension/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ def _prepare_handlers(self):
)
new_handlers.append(handler)

webapp.add_handlers(".*$", new_handlers) # type:ignore[arg-type]
webapp.add_handlers(".*$", new_handlers)

def _prepare_templates(self):
"""Add templates to web app settings if extension has templates."""
Expand Down
Loading
Loading