sadpandajoe commented on code in PR #43525:
URL: https://github.com/apache/superset/pull/43525#discussion_r3867040178
##########
superset/dashboards/filter_state/api.py:
##########
@@ -49,7 +48,6 @@ def get_delete_command(self) ->
type[DeleteFilterStateCommand]:
return DeleteFilterStateCommand
@api
- @has_access_api
@expose("/<int:pk>/filter_state", methods=("POST",))
Review Comment:
Removing `has_access_api` leaves `@api` wrapping `@protect()` on this route.
JWT validation errors other than `NoAuthorizationError` (for example, an
expired or malformed bearer token) now fall into `@api`'s broad exception
handler and become a 500, while the GET/DELETE routes let Flask-JWT-Extended
return the authentication error. Could we remove `@api` from POST and PUT as
well, or otherwise keep `@protect()` outside that broad handler?
##########
superset/dashboards/filter_state/api.py:
##########
@@ -49,7 +48,6 @@ def get_delete_command(self) ->
type[DeleteFilterStateCommand]:
return DeleteFilterStateCommand
@api
- @has_access_api
@expose("/<int:pk>/filter_state", methods=("POST",))
Review Comment:
The existing filter-state API tests authenticate POST/PUT with
`login_as_admin` or `login_as`, so they do not exercise the bearer-token
ordering this fixes. Could we add a regression that sends an access token
without a browser session, asserts POST returns 201 and PUT returns 200, and
would fail again if `has_access_api` is restored?
--
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]
