gabotorresruiz commented on code in PR #44533:
URL: https://github.com/apache/superset/pull/44533#discussion_r4073612247
##########
superset/views/error_handling.py:
##########
@@ -175,6 +176,9 @@ def wraps(self: BaseSupersetView, *args: Any, **kwargs:
Any) -> FlaskResponse:
return json_error_response(utils.error_msg_from_exception(ex),
status=422)
except sshtunnel.BaseSSHTunnelForwarderError as ex:
return handle_ssh_tunnel_error(ex)
+ except NoAuthorizationError as ex:
+ logger.warning("Api failed- no authorization", exc_info=True)
+ return json_error_response(str(ex), status=401)
Review Comment:
Not a blocker, and the 401 itself is clearly right.
`NoAuthorizationError` is only one of the ways `verify_jwt_in_request()`
fails; the rest still land in the broad `except Exception` below. I registered
a view with the same `@api` / `@handle_api_exception` stack on the app fixture
and drove real requests through it on this branch:
| request | with `handle_api_exception` | same view without it |
| --- | --- | --- |
| no token | 401 `Missing Authorization Header` | 401 |
| expired token | 500 `Signature has expired` | 401 |
| garbage token | 500 `Invalid header padding` | 422 |
| tampered signature | 500 `Invalid crypto padding` | 422 |
| malformed `Authorization` header | 500 `Bad Authorization header...` | 422
|
The right column is what `JWTManager`'s own error handlers return once the
exception is allowed to reach them. So an expired token still takes
`logger.exception` and still answers 500.
Catching the family covers all five. I patched this in locally and re-ran
the same matrix, every row became 401:
```python
from flask_jwt_extended.exceptions import JWTExtendedException
from jwt.exceptions import PyJWTError
...
except (JWTExtendedException, PyJWTError) as ex:
logger.warning("Api failed- no authorization", exc_info=True)
return json_error_response(str(ex), status=401)
```
`flask_jwt_extended` answers 422 rather than 401 on the malformed-token
rows, so if you would rather keep its exact statuses, the alternative is to
re-raise and let the app handlers decide. Either way, parametrizing
`TestHandleApiExceptionNoAuthorizationError` over `NoAuthorizationError`,
`ExpiredSignatureError` and `DecodeError` would pin whichever you pick.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
