Closed vokimon closed 2 months ago
Hi @vokimon, thanks for the detailed description. I didn't clearly get your point in your proposal, but I am looking forward to your pull request on your proposal.
@ArtyomVancyan i wrote a draft PR, #44. I need more context from you to complete it.
I tried to redo your commit https://github.com/pysnippet/fastapi-oauth2/commit/aa8f4b318816f73749c8cdca4d7ad60c2a61e6c4 without wrapping user code in the error handling.
I am quite confident to have moved the JOSEError
handling to the proper context.
But i have no clue of what you tried to catch with Exception
.
Also i handled it in mirror of what the PR #43 did with expirated tokens. But the exception we are throwing is not an HttpException from FastApi and framework default handlers are missing it.
The Exception
was for in case default AuthenticationMiddleware
raises an exception, but I think it could be caught and reraised separately. The commit you refer to is for handling REST API cases. Also, I agree with the HttpException
; you can include its fix in this PR as well.
Also, the handling is what the PR #43 did with expirated tokens. But the exception we are throwing is not an HttpException from FastApi, and framework default handlers are missing it.
I think you haven't noticed, but the OAuth2AuthenticationError
is HTTPException
. Please take a look at src/fastapi_oauth2/exceptions.py file.
You are right I was handling FastApi.HTTPException, a subclass of Starlette's. That's why my handling ignored them.
Rest API cases with a PlainText response? weird.
Take a look at:
If we adhere to what Starlette does I would say that what this library should raise starlette.authentication.AuthenticationError
which is not a HTTPException. Starlette handles such an exception internally in the default middleware returning a plain text 400 response. That can be overriden by providing the on_error callback to the default middleware. I was hoping that FastApi changes this behaviour to raise a more REST-like response but i didn¡'t find any override in Fastapi.
In summary, my proposal would be:
starlette.authentication.AuthenticationError
from authenticate.I am about to update de PR with those changes.
I'll be away on vacation for some weeks so forgive me if i don't reply further messages until then.
I looked at the links you provided and agreed with all the changes you have made. I made a few minor changes on the same branch that I won't merge before you return from vacation. I will update the related documentation as well.
The main thing I have changed is using AuthenticationMiddleware.default_on_error
when on_error
isn't provided. Also, fixed some typing issues for earlier versions of Python and the failing tests - the cause of the tests failing was that you were calling client.get("/auth")
for login simulation, and the OAuth2Middleware
syncs the Authorization
cookie with headers and it looks for a token in the headers first, so updating only cookies wasn't enough. I should either update client.headers
instead of client.cookies
or remove the login simulation.
@vokimon, please let me know once you review my changes on your branch so I know that we have a consensus and I can merge and include them in the upcoming release.
I am still on vacation and i cannot try them but all your changes makes perfect sense. Thanks for the fixes on coding convention and documentation. You can proceed with the integration.
Bug description
OAuth2Middleware handles exceptions too widely by catching
Exception
and masking any error raised in user code.Because that middleware wraps the whole application and, more importantly, user code, any programming error in user code, like a missing import, or a misspelled variable will be reported as an exception related to Authentication and a 401 http error is emitted, which is confusing and hard to debug because the usual backtrace is not shown. Common fastapi behaviour is emiting a 500 error and an informative traceback on the console.
This makes development quite hard and mask other expected runtime exceptions you might want to handle in upper levels. My current workaround is to edit the library in my development environment by removing that general exception handler. But, on one hand, that is not an easily replicable environment, and secondly, i guess that we will miss some Auth related problem that the original code actually intended to catch.
I propose wrapping code more selectively, keeping user code outside the wrap, and expecting more specific exceptions than Exception.
https://github.com/pysnippet/fastapi-oauth2/blob/d22bb4e4a769875fd382cc5a8640394b34ab2ef4/src/fastapi_oauth2/middleware.py#L151
I use to disable other wide excepts, in paranoid mode, but a shallow look at them i would say they do not interfer with use code. Maybe i am wrong.
https://github.com/pysnippet/fastapi-oauth2/blob/d22bb4e4a769875fd382cc5a8640394b34ab2ef4/src/fastapi_oauth2/core.py#L129 https://github.com/pysnippet/fastapi-oauth2/blob/d22bb4e4a769875fd382cc5a8640394b34ab2ef4/src/fastapi_oauth2/core.py#L131
Reproduction URL
No response
Reproduction steps
Take the example, in any of the entry points in
router_ssr.py
append a line withboo
(an undefined identifier). You get the error message in the browser, if you are not doing rest, but even if you are authenticated the error is a 401. The normal behaviour, that you get if you remove the wide range except is to have a 500 error and a proper backtrace on the server.Screenshots
Logs
No response
Browsers
Firefox
OS
Linux