gabotorresruiz commented on code in PR #45452:
URL: https://github.com/apache/superset/pull/45452#discussion_r4330498352
##########
superset/mcp_service/guest_token_verifier.py:
##########
@@ -112,6 +117,12 @@ def _verify_guest_sync(self, token: str) -> dict[str, Any]
| None:
logger.debug("Guest token failed structural validation;
deferring")
return None
+ try:
+ validate_guest_token_identity(parsed["user"])
Review Comment:
Small consolidation question, not a blocker: the same check now lives in
four places (here, `UserSchema` in `api.py`, `create_guest_access_token` and
`get_guest_user_from_request`), while `get_guest_user_from_token` is the one
function every `GuestUser` is built from, including the three callers in
`superset/tasks/`. Would the consume-side check be better placed there? Nothing
reaches those task callers with an unvalidated token once this lands, so it is
about having one copy rather than closing a gap.
##########
superset/security/manager.py:
##########
@@ -6165,6 +6167,7 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
token = self.parse_jwt_guest_token(raw_token)
if token.get("user") is None:
raise ValueError("Guest token does not contain a user claim")
+ validate_guest_token_identity(token["user"])
Review Comment:
Confirmed. I deleted the line on this branch and
`test_invalid_identity_rejected_through_request_flow[overlong]` and
`[trailing-newline]` both fail, so it is pinned now. The `UPDATING.md` entry
also says the part I cared about, that an already issued token stops resolving
and the request becomes unauthenticated. Thanks.
##########
superset/security/manager.py:
##########
@@ -6109,6 +6110,7 @@ def create_guest_access_token(
rls: list[GuestTokenRlsRule],
datasets: Optional[list[int]] = None,
) -> bytes:
+ validate_guest_token_identity(user)
Review Comment:
Confirmed, and the 400 is real: with the new `except
GuestTokenIdentityError` removed,
`test_guest_token_invalid_identity_after_hook` comes back 500, and with it in
place 400. Deleting the guard itself fails both that case and
`test_mint_rejects_invalid_identity`. The 400 bodies name the field and never
echo the value, which is what I was hoping for.
--
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]
