aminghadersohi commented on code in PR #44769:
URL: https://github.com/apache/superset/pull/44769#discussion_r4127486811
##########
superset/exceptions.py:
##########
@@ -439,6 +439,29 @@ def __init__(self, error: str):
)
+class OAuth2RejectedError(SupersetErrorException):
+ """
+ Exception for when the OAuth2 callback itself can't be completed.
+
+ Raised when the provider denies authorization or the callback request
+ can't be trusted (missing/invalid state, no authenticated session).
+ These are conditions the caller could correct by retrying the OAuth2
+ flow, so unlike ``OAuth2Error`` this maps to a 4xx response instead of
+ a 500.
+ """
+
+ status = 400
+
+ def __init__(self, error: str):
+ super().__init__(
+ SupersetError(
+ message=error,
+ error_type=SupersetErrorType.OAUTH2_REDIRECT_ERROR,
+ level=ErrorLevel.WARNING,
+ )
+ )
Review Comment:
Fixed in 7e1fba659660e698f7bd69726fa72c96a402ca5b at the input's source:
provider error text is no longer interpolated into OAuth2RejectedError. All its
call sites now supply fixed application-authored messages, retaining useful
rejection reasons without reflecting caller input. Regression coverage checks
the serialized error and formatted traceback for ordinary and CR/LF-containing
provider errors. The exception class itself needs no change.\n\nValidation: 12
OAuth2 command unit tests passed; pre-commit run --files for all three changed
files passed (including mypy, Ruff and pylint).
##########
tests/unit_tests/commands/databases/oauth2_test.py:
##########
@@ -107,8 +107,29 @@ def test_validate_database_not_found(
def test_validate_oauth2_error(mock_parameters: OAuth2ProviderResponseSchema)
-> None:
mock_parameters["error"] = "OAuth2 failure"
command = OAuth2StoreTokenCommand(mock_parameters)
- with pytest.raises(OAuth2Error, match="Something went wrong while doing
OAuth2"):
+ with pytest.raises(OAuth2RejectedError, match="OAuth2 failure") as
exc_info:
command.validate()
+ assert exc_info.value.status == 400
+
+
+def test_validate_missing_state(
+ mock_parameters: OAuth2ProviderResponseSchema,
+) -> None:
+ del mock_parameters["state"]
+ command = OAuth2StoreTokenCommand(mock_parameters)
+ with pytest.raises(OAuth2RejectedError) as exc_info:
+ command.validate()
+ assert exc_info.value.status == 400
+
+
+def test_validate_invalid_state(
+ mock_parameters: OAuth2ProviderResponseSchema,
+) -> None:
+ mock_parameters["state"] = "not-a-valid-jwt"
+ command = OAuth2StoreTokenCommand(mock_parameters)
+ with pytest.raises(OAuth2RejectedError) as exc_info:
+ command.validate()
+ assert exc_info.value.status == 400
Review Comment:
Fixed in 7e1fba659660e698f7bd69726fa72c96a402ca5b: added docstrings to
test_validate_missing_state and test_validate_invalid_state describing their
HTTP 400 expectations, and documented the added regression
tests.\n\nValidation: 12 OAuth2 command unit tests passed; pre-commit run
--files for all three changed files passed (including mypy, Ruff and pylint).
##########
superset/commands/database/oauth2.py:
##########
@@ -118,17 +121,31 @@ def run(self) -> DatabaseUserOAuth2Tokens:
def validate(self) -> None:
if error := self._parameters.get("error"):
- raise OAuth2Error(error)
+ raise OAuth2RejectedError(
+ f"The OAuth2 provider denied the request: {error}"
+ )
Review Comment:
Fixed in 7e1fba659660e698f7bd69726fa72c96a402ca5b: authorization denial uses
the constant message "The OAuth2 provider denied the request"; the provider
error value is not included in the exception or response. Tests verify it
cannot appear in the serialized error or formatted traceback, including a CR/LF
log-forging payload.\n\nValidation: 12 OAuth2 command unit tests passed;
pre-commit run --files for all three changed files passed (including mypy, Ruff
and pylint).
##########
superset/commands/database/oauth2.py:
##########
@@ -118,17 +121,31 @@ def run(self) -> DatabaseUserOAuth2Tokens:
def validate(self) -> None:
if error := self._parameters.get("error"):
- raise OAuth2Error(error)
+ raise OAuth2RejectedError(
+ f"The OAuth2 provider denied the request: {error}"
+ )
+
+ try:
+ state = self._parameters["state"]
+ except KeyError:
+ raise OAuth2RejectedError(
+ "The OAuth2 callback is missing the state parameter"
+ ) from None
- self._state = decode_oauth2_state(self._parameters["state"])
+ try:
+ self._state = decode_oauth2_state(state)
+ except (jwt.PyJWTError, ValidationError):
+ raise OAuth2RejectedError("The OAuth2 state parameter is invalid")
from None
Review Comment:
This does not apply to the PR head: validate already catches both
jwt.PyJWTError and marshmallow.ValidationError. OAuth2StateSchema.database_id
is a required fields.Int, not a UUID; missing or non-integer values fail schema
validation before the post_load dictionary lookup, so they do not raise the
claimed KeyError. 7e1fba659660e698f7bd69726fa72c96a402ca5b adds regression
tests using genuinely signed JWTs with a missing database_id and a non-integer
database_id; both return OAuth2RejectedError with status 400. No production
catch broadening is needed.\n\nValidation: 12 OAuth2 command unit tests passed;
pre-commit run --files for all three changed files passed (including mypy, Ruff
and pylint).
--
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]
