GH-42: Fix: Middleware overhandling exceptions #44
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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.
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.
- 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.
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.
- Use `default_on_error` instead of defining new one
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move middleware error handling from
__call__toauthenticateand make it more specific.Motivation:
This pull request attempts to fix issue GH-42: Middleware exception handling now intercepts any exception raised by user code as AuthenticationError 401, even when the entry point requires no Authentication. This behavior masks coding errors in user code while developing, and bug detection in logs while in production.
The behaviour was reintroduced in aa8f4b3
Removing it from
__call__fixes the problem, but then exceptions really related to authentication must be handled. My first proposal is to move the handling toautenticate(). Instead of wide range handling, I opted to specific handling, just not to maks bugs and issues. But i just detected the JWT problems handling. Not sure if we have to deal any other but as authentication.A second problem is how it is to be handled. I took as reference the previous PR on JWT expiration, and raised OAuth2AuthenticationError. The problem with that is that such exception is not HTTPException and FastApi / Starlette does not handle it as intended.
All Submissions:
Changes to Core Features: