From 1cef8b8f74d004fef9fa8398cf92ee74fb138902 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Garc=C3=ADa=20Garz=C3=B3n?= Date: Wed, 24 Jul 2024 20:36:14 +0200 Subject: [PATCH 01/11] GH-42: testscases middleware overhandling excepts - Unintended errors in code should be 500 - Non Http custom exceptions should be 500 - Handled exceptions should use handler (and be 418 in this case) But all of them are reported as 401 Unauthorized even when the entrypoint does not requires authorization. --- tests/test_middleware.py | 42 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index e33c6b7..8902936 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -26,3 +26,45 @@ async def test_middleware_on_logout(get_app): response = await client.get("/user") assert response.status_code == 403 # Forbidden + +@pytest.mark.anyio +async def test_middleware_do_not_interfer_user_errors(get_app): + app=get_app() + @app.get('/unexpected_error') + def unexpected(): + undefined_id # Intended code error + + async with AsyncClient(app=app, base_url="http://test") as client: + response = await client.get("/unexpected_error") + assert response.status_code == 500 # Internal server error + +@pytest.mark.anyio +async def test_middleware_ignores_custom_exceptions(get_app): + class MyCustomException(Exception): pass + app=get_app() + @app.get('/custom_exception') + def custom_exception(): + raise MyCustomException() + + async with AsyncClient(app=app, base_url="http://test") as client: + response = await client.get("/custom_exception") + assert response.status_code == 500 # Internal server error + +@pytest.mark.anyio +async def test_middleware_ignores_handled_custom_exceptions(get_app): + class MyCustomException(Exception): pass + app=get_app() + @app.exception_handler(MyCustomException) + async def unicorn_exception_handler(request, exc): + return JSONResponse( + status_code=418, + content={"message": f"I am a Teapot!"}, + ) + + @app.get('/custom_exception') + def custom_exception(): + raise MyCustomException() + + async with AsyncClient(app=app, base_url="http://test") as client: + response = await client.get("/custom_exception") + assert response.status_code == 418 # I am a teapot! From cb8b4458f4fadabc354a4325e82840aa2ce43485 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Garc=C3=ADa=20Garz=C3=B3n?= Date: Thu, 25 Jul 2024 08:40:16 +0200 Subject: [PATCH 02/11] GH-42: Covering jwt cases and modify others I added a case of exception that should be handled: as auth errors: jwt validation errors Using same approach as expiration check, but maybe it should be a HttpException in order to be properly handled both by fastapi and users. Also unexpected errors are not transformed to 500 either on the test setup. Maybe because we are not using Fastapi TestClient. --- tests/test_middleware.py | 42 +++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 8902936..ffa6934 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -1,5 +1,7 @@ import pytest from httpx import AsyncClient +from fastapi.responses import JSONResponse +from fastapi_oauth2.exceptions import OAuth2AuthenticationError @pytest.mark.anyio @@ -31,40 +33,54 @@ async def test_middleware_on_logout(get_app): async def test_middleware_do_not_interfer_user_errors(get_app): app=get_app() @app.get('/unexpected_error') - def unexpected(): + def my_entry_point(): undefined_id # Intended code error async with AsyncClient(app=app, base_url="http://test") as client: - response = await client.get("/unexpected_error") - assert response.status_code == 500 # Internal server error + with pytest.raises(NameError): + await client.get("/unexpected_error") @pytest.mark.anyio async def test_middleware_ignores_custom_exceptions(get_app): class MyCustomException(Exception): pass app=get_app() @app.get('/custom_exception') - def custom_exception(): + def my_entry_point(): raise MyCustomException() async with AsyncClient(app=app, base_url="http://test") as client: - response = await client.get("/custom_exception") - assert response.status_code == 500 # Internal server error + with pytest.raises(MyCustomException): + await client.get("/custom_exception") @pytest.mark.anyio async def test_middleware_ignores_handled_custom_exceptions(get_app): - class MyCustomException(Exception): pass + class MyHandledException(Exception): pass app=get_app() - @app.exception_handler(MyCustomException) + @app.exception_handler(MyHandledException) async def unicorn_exception_handler(request, exc): return JSONResponse( status_code=418, - content={"message": f"I am a Teapot!"}, + content={"details": "I am a custom Teapot!"}, ) - @app.get('/custom_exception') - def custom_exception(): - raise MyCustomException() + @app.get('/handled_exception') + def my_entry_point(): + raise MyHandledException() async with AsyncClient(app=app, base_url="http://test") as client: - response = await client.get("/custom_exception") + response = await client.get("/handled_exception") assert response.status_code == 418 # I am a teapot! + assert response.json() == {"details": "I am a custom Teapot!"} + +@pytest.mark.anyio +async def test_middleware_reports_invalid_jwt(get_app): + async with AsyncClient(app=get_app(with_ssr=False), base_url="http://test") as client: + await client.get("/auth") # Simulate login + # Insert a bad token instead + from jose import jwt + badtoken=jwt.encode({"bad": "token"}, 'badsecret', 'HS256') + client.cookies.update(dict(Authorization=f"Bearer: {badtoken}")) + + with pytest.raises(OAuth2AuthenticationError, match="401: Signature verification failed.") as ctx: + response = await client.get("/user") + From 089d648a7d293540e349a6445a377aba94b94df0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Garc=C3=ADa=20Garz=C3=B3n?= Date: Thu, 25 Jul 2024 10:28:28 +0200 Subject: [PATCH 03/11] GH-42: error handling ignores library user code exceptions - Removed the generic error handling in __call__() - Introduced specific error handling inside authenticate() for jwt decoding. - One use of jwt is in token generation in core, but in this case it won't be a authorization error but maybe a configuration one, we should see the details in logging or debugging platforms. - For sure, other authorization errors caught previously as Exception now run on the wild. Review required on that. --- src/fastapi_oauth2/middleware.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/fastapi_oauth2/middleware.py b/src/fastapi_oauth2/middleware.py index 8481947..b54b5b2 100644 --- a/src/fastapi_oauth2/middleware.py +++ b/src/fastapi_oauth2/middleware.py @@ -108,7 +108,10 @@ async def authenticate(self, request: Request) -> Optional[Tuple[Auth, User]]: if not scheme or not param: return Auth(), User() - token_data = Auth.jwt_decode(param) + try: + token_data = Auth.jwt_decode(param) + except JOSEError as e: + raise OAuth2AuthenticationError(401, str(e)) if token_data["exp"] and token_data["exp"] < int(datetime.now(timezone.utc).timestamp()): raise OAuth2AuthenticationError(401, "Token expired") @@ -152,9 +155,5 @@ def __init__( async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: if scope["type"] == "http": - try: - return await self.auth_middleware(scope, receive, send) - except (JOSEError, Exception) as e: - middleware = PlainTextResponse(str(e), status_code=401) - return await middleware(scope, receive, send) + return await self.auth_middleware(scope, receive, send) await self.default_application_middleware(scope, receive, send) From b3ce107d58b86ea7c0e05de573e201cff87b6fdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Garc=C3=ADa=20Garz=C3=B3n?= Date: Tue, 30 Jul 2024 18:26:52 +0200 Subject: [PATCH 04/11] GH-22: Using starlette like exception handling That is: - raising starlette.authentication.AuthenticationError - providing an on_error callback turning starlet 400 into 401 to keep same api - letting the user provide their own on_error when instantiating the middleware. --- src/fastapi_oauth2/middleware.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/fastapi_oauth2/middleware.py b/src/fastapi_oauth2/middleware.py index b54b5b2..f353752 100644 --- a/src/fastapi_oauth2/middleware.py +++ b/src/fastapi_oauth2/middleware.py @@ -16,10 +16,13 @@ from jose.jwt import encode as jwt_encode from starlette.authentication import AuthCredentials from starlette.authentication import AuthenticationBackend +from starlette.authentication import AuthenticationError from starlette.authentication import BaseUser from starlette.middleware.authentication import AuthenticationMiddleware from starlette.requests import Request +from starlette.requests import HTTPConnection from starlette.responses import PlainTextResponse +from starlette.responses import Response from starlette.types import ASGIApp from starlette.types import Receive from starlette.types import Scope @@ -111,9 +114,9 @@ async def authenticate(self, request: Request) -> Optional[Tuple[Auth, User]]: try: token_data = Auth.jwt_decode(param) except JOSEError as e: - raise OAuth2AuthenticationError(401, str(e)) + raise AuthenticationError(str(e)) if token_data["exp"] and token_data["exp"] < int(datetime.now(timezone.utc).timestamp()): - raise OAuth2AuthenticationError(401, "Token expired") + raise AuthenticationError("Token expired") user = User(token_data) auth = Auth(user.pop("scope", [])) @@ -138,6 +141,7 @@ def __init__( app: ASGIApp, config: Union[OAuth2Config, dict], callback: Callable[[Auth, User], Union[Awaitable[None], None]] = None, + on_error: Callable[[HTTPConnection, AuthenticationError], Response] | None = None, **kwargs, # AuthenticationMiddleware kwargs ) -> None: """Initiates the middleware with the given configuration. @@ -151,9 +155,13 @@ def __init__( elif not isinstance(config, OAuth2Config): raise TypeError("config is not a valid type") self.default_application_middleware = app - self.auth_middleware = AuthenticationMiddleware(app, backend=OAuth2Backend(config, callback), **kwargs) + self.auth_middleware = AuthenticationMiddleware(app, backend=OAuth2Backend(config, callback), on_error = on_error or self.on_error, **kwargs) async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: if scope["type"] == "http": return await self.auth_middleware(scope, receive, send) await self.default_application_middleware(scope, receive, send) + + @staticmethod + def on_error(conn: HTTPConnection, exc: Exception) -> Response: + return PlainTextResponse(str(exc), status_code=401) From 8c93004e1c8aa6bbc97675de4215bdb1c1cfbd13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Garc=C3=ADa=20Garz=C3=B3n?= Date: Tue, 30 Jul 2024 18:38:40 +0200 Subject: [PATCH 05/11] GB-42: Tests adapted to new error handling --- tests/test_middleware.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index ffa6934..736e9b1 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -1,7 +1,7 @@ import pytest from httpx import AsyncClient from fastapi.responses import JSONResponse -from fastapi_oauth2.exceptions import OAuth2AuthenticationError +from starlette.authentication import AuthenticationError @pytest.mark.anyio @@ -81,6 +81,7 @@ async def test_middleware_reports_invalid_jwt(get_app): badtoken=jwt.encode({"bad": "token"}, 'badsecret', 'HS256') client.cookies.update(dict(Authorization=f"Bearer: {badtoken}")) - with pytest.raises(OAuth2AuthenticationError, match="401: Signature verification failed.") as ctx: - response = await client.get("/user") + response = await client.get("/user") + assert response.status_code == 401 # Not authenticated + assert response.text == "Signature verification failed." From 0041465252ecd7037996636cced779f6db468347 Mon Sep 17 00:00:00 2001 From: Artyom Vancyan Date: Wed, 31 Jul 2024 11:34:12 +0400 Subject: [PATCH 06/11] Improve code formatting --- tests/test_middleware.py | 45 ++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 736e9b1..ea62828 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -1,7 +1,7 @@ import pytest -from httpx import AsyncClient from fastapi.responses import JSONResponse -from starlette.authentication import AuthenticationError +from httpx import AsyncClient +from jose import jwt @pytest.mark.anyio @@ -29,33 +29,43 @@ async def test_middleware_on_logout(get_app): response = await client.get("/user") assert response.status_code == 403 # Forbidden + @pytest.mark.anyio -async def test_middleware_do_not_interfer_user_errors(get_app): - app=get_app() - @app.get('/unexpected_error') +async def test_middleware_do_not_interfere_user_errors(get_app): + app = get_app() + + @app.get("/unexpected_error") def my_entry_point(): - undefined_id # Intended code error + raise NameError # Intended code error async with AsyncClient(app=app, base_url="http://test") as client: with pytest.raises(NameError): await client.get("/unexpected_error") + @pytest.mark.anyio async def test_middleware_ignores_custom_exceptions(get_app): - class MyCustomException(Exception): pass - app=get_app() - @app.get('/custom_exception') + class MyCustomException(Exception): + pass + + app = get_app() + + @app.get("/custom_exception") def my_entry_point(): raise MyCustomException() async with AsyncClient(app=app, base_url="http://test") as client: - with pytest.raises(MyCustomException): + with pytest.raises(MyCustomException): await client.get("/custom_exception") + @pytest.mark.anyio async def test_middleware_ignores_handled_custom_exceptions(get_app): - class MyHandledException(Exception): pass - app=get_app() + class MyHandledException(Exception): + pass + + app = get_app() + @app.exception_handler(MyHandledException) async def unicorn_exception_handler(request, exc): return JSONResponse( @@ -63,25 +73,24 @@ async def unicorn_exception_handler(request, exc): content={"details": "I am a custom Teapot!"}, ) - @app.get('/handled_exception') + @app.get("/handled_exception") def my_entry_point(): raise MyHandledException() async with AsyncClient(app=app, base_url="http://test") as client: response = await client.get("/handled_exception") - assert response.status_code == 418 # I am a teapot! + assert response.status_code == 418 # I am a teapot! assert response.json() == {"details": "I am a custom Teapot!"} + @pytest.mark.anyio async def test_middleware_reports_invalid_jwt(get_app): async with AsyncClient(app=get_app(with_ssr=False), base_url="http://test") as client: await client.get("/auth") # Simulate login # Insert a bad token instead - from jose import jwt - badtoken=jwt.encode({"bad": "token"}, 'badsecret', 'HS256') + badtoken = jwt.encode({"bad": "token"}, "badsecret", "HS256") client.cookies.update(dict(Authorization=f"Bearer: {badtoken}")) response = await client.get("/user") - assert response.status_code == 401 # Not authenticated + assert response.status_code == 401 # Not authenticated assert response.text == "Signature verification failed." - From 252c00e1d2a5d2271fc995a595459c72b6b24074 Mon Sep 17 00:00:00 2001 From: Artyom Vancyan Date: Wed, 31 Jul 2024 11:36:11 +0400 Subject: [PATCH 07/11] - Fix typing for lower versions of Python interpreter - Use `default_on_error` instead of defining new one --- src/fastapi_oauth2/middleware.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/fastapi_oauth2/middleware.py b/src/fastapi_oauth2/middleware.py index f353752..76ee47e 100644 --- a/src/fastapi_oauth2/middleware.py +++ b/src/fastapi_oauth2/middleware.py @@ -19,9 +19,8 @@ from starlette.authentication import AuthenticationError from starlette.authentication import BaseUser from starlette.middleware.authentication import AuthenticationMiddleware -from starlette.requests import Request from starlette.requests import HTTPConnection -from starlette.responses import PlainTextResponse +from starlette.requests import Request from starlette.responses import Response from starlette.types import ASGIApp from starlette.types import Receive @@ -31,7 +30,6 @@ from .claims import Claims from .config import OAuth2Config from .core import OAuth2Core -from .exceptions import OAuth2AuthenticationError class Auth(AuthCredentials): @@ -141,8 +139,7 @@ def __init__( app: ASGIApp, config: Union[OAuth2Config, dict], callback: Callable[[Auth, User], Union[Awaitable[None], None]] = None, - on_error: Callable[[HTTPConnection, AuthenticationError], Response] | None = None, - **kwargs, # AuthenticationMiddleware kwargs + on_error: Optional[Callable[[HTTPConnection, AuthenticationError], Response]] = None, ) -> None: """Initiates the middleware with the given configuration. @@ -155,13 +152,10 @@ def __init__( elif not isinstance(config, OAuth2Config): raise TypeError("config is not a valid type") self.default_application_middleware = app - self.auth_middleware = AuthenticationMiddleware(app, backend=OAuth2Backend(config, callback), on_error = on_error or self.on_error, **kwargs) + on_error = on_error or AuthenticationMiddleware.default_on_error + self.auth_middleware = AuthenticationMiddleware(app, backend=OAuth2Backend(config, callback), on_error=on_error) async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: if scope["type"] == "http": return await self.auth_middleware(scope, receive, send) await self.default_application_middleware(scope, receive, send) - - @staticmethod - def on_error(conn: HTTPConnection, exc: Exception) -> Response: - return PlainTextResponse(str(exc), status_code=401) From 4e2a8320d56685d8004fe69e8ed5f54ea460374d Mon Sep 17 00:00:00 2001 From: Artyom Vancyan Date: Wed, 31 Jul 2024 12:14:04 +0400 Subject: [PATCH 08/11] Fix `status_code` based on the default `on_error` change --- tests/test_middleware.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index ea62828..0d71c02 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -92,5 +92,5 @@ async def test_middleware_reports_invalid_jwt(get_app): client.cookies.update(dict(Authorization=f"Bearer: {badtoken}")) response = await client.get("/user") - assert response.status_code == 401 # Not authenticated + assert response.status_code == 400 assert response.text == "Signature verification failed." From fea22348539c8af972012849fd1e835ad275b422 Mon Sep 17 00:00:00 2001 From: Artyom Vancyan Date: Wed, 31 Jul 2024 12:25:01 +0400 Subject: [PATCH 09/11] Remove `client.get("/auth")` login simulation call --- tests/test_middleware.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 0d71c02..2302d9b 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -86,7 +86,6 @@ def my_entry_point(): @pytest.mark.anyio async def test_middleware_reports_invalid_jwt(get_app): async with AsyncClient(app=get_app(with_ssr=False), base_url="http://test") as client: - await client.get("/auth") # Simulate login # Insert a bad token instead badtoken = jwt.encode({"bad": "token"}, "badsecret", "HS256") client.cookies.update(dict(Authorization=f"Bearer: {badtoken}")) From 00bf9ab9ee86263b2b38d406aa0404b40f119671 Mon Sep 17 00:00:00 2001 From: Artyom Vancyan Date: Wed, 31 Jul 2024 14:37:51 +0400 Subject: [PATCH 10/11] GH-42: Update documentation according to the changes --- docs/integration/integration.md | 9 +++++++-- docs/references/tutorials.md | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/integration/integration.md b/docs/integration/integration.md index ca53dc1..30a2654 100644 --- a/docs/integration/integration.md +++ b/docs/integration/integration.md @@ -11,8 +11,9 @@ section covers its integration into a FastAPI app. The `OAuth2Middleware` is an authentication middleware which means that its usage makes the `user` and `auth` attributes available in the [request](https://www.starlette.io/requests/) context. It has a mandatory argument `config` of -[`OAuth2Config`](/integration/configuration#oauth2config) instance that has been discussed at the previous section and -an optional argument `callback` which is a callable that is called when the authentication succeeds. +[`OAuth2Config`](/integration/configuration#oauth2config) instance that has been discussed in the previous section and +optional arguments `callback` and `on_error` that accept callables as values and are called when the authentication +succeeds and fails correspondingly. ```python app: FastAPI @@ -20,10 +21,14 @@ app: FastAPI def on_auth_success(auth: Auth, user: User): """This could be async function as well.""" +def on_auth_error(conn: HTTPConnection, exc: Exception) -> Response: + return JSONResponse({"detail": str(exc)}, status_code=400) + app.add_middleware( OAuth2Middleware, config=OAuth2Config(...), callback=on_auth_success, + on_error=on_auth_error, ) ``` diff --git a/docs/references/tutorials.md b/docs/references/tutorials.md index e1aada2..48fa0d1 100644 --- a/docs/references/tutorials.md +++ b/docs/references/tutorials.md @@ -140,7 +140,7 @@ async def error_handler(request: Request, exc: OAuth2AuthenticationError): return RedirectResponse(url="/login", status_code=303) ``` -The complete list of exceptions is the following. +The complete list of exceptions raised by the middleware is the following. - `OAuth2Error` - Base exception for all errors raised by the FastAPI OAuth2 library. - `OAuth2AuthenticationError` - An exception is raised when the authentication fails. From 3a0cc00f03644d46c961b9d42b03f27628bcecd0 Mon Sep 17 00:00:00 2001 From: Artyom Vancyan Date: Wed, 31 Jul 2024 14:38:28 +0400 Subject: [PATCH 11/11] Upgrade the version to `1.1.0` --- src/fastapi_oauth2/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fastapi_oauth2/__init__.py b/src/fastapi_oauth2/__init__.py index 5becc17..6849410 100644 --- a/src/fastapi_oauth2/__init__.py +++ b/src/fastapi_oauth2/__init__.py @@ -1 +1 @@ -__version__ = "1.0.0" +__version__ = "1.1.0"