EnxDev commented on code in PR #44769:
URL: https://github.com/apache/superset/pull/44769#discussion_r4137514349
##########
superset/commands/database/oauth2.py:
##########
@@ -117,18 +120,30 @@ def run(self) -> DatabaseUserOAuth2Tokens:
)
def validate(self) -> None:
- if error := self._parameters.get("error"):
- raise OAuth2Error(error)
+ if self._parameters.get("error"):
+ raise OAuth2RejectedError("The OAuth2 provider denied the request")
- self._state = decode_oauth2_state(self._parameters["state"])
+ try:
+ state = self._parameters["state"]
Review Comment:
`code` doesn't get the same treatment as `state`. A callback with a valid
state, a matching session and no `code` passes `validate()`, then
`self._parameters["code"]` throws a KeyError inside the `try` at line 86. That
gets logged and returned as a 500 "Token exchange failed" with
`error_type=KeyError`, after the PKCE verifier has already been deleted from
the KV store.
Could we reject a missing `code` here with an `OAuth2RejectedError`, and add
a `missing_code` case to the parametrized API test?
--
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]
