Copilot commented on code in PR #37862:
URL: https://github.com/apache/superset/pull/37862#discussion_r2790608339
##########
superset/async_events/async_query_manager.py:
##########
@@ -177,9 +177,14 @@ def validate_session(response: Response) -> Response:
session["async_channel_id"] = async_channel_id
session["async_user_id"] = user_id
- sub = str(user_id) if user_id else None
+ # Conditionally include 'sub' claim only when user_id is
present
+ # RFC 7519 specifies 'sub' as optional; omitting it when None
+ # prevents PyJWT 2.10.1 InvalidSubjectError for guest users
+ payload = {"channel": async_channel_id}
+ if user_id:
Review Comment:
The condition `if user_id:` checks truthiness rather than presence; if a
valid user id could ever be `0`, the `sub` claim would be omitted unexpectedly.
Use an explicit `user_id is not None` check to match the intent of “include sub
when user_id is present.”
```suggestion
if user_id is not None:
```
##########
superset/async_events/async_query_manager.py:
##########
@@ -177,9 +177,14 @@ def validate_session(response: Response) -> Response:
session["async_channel_id"] = async_channel_id
session["async_user_id"] = user_id
- sub = str(user_id) if user_id else None
+ # Conditionally include 'sub' claim only when user_id is
present
+ # RFC 7519 specifies 'sub' as optional; omitting it when None
+ # prevents PyJWT 2.10.1 InvalidSubjectError for guest users
Review Comment:
These inline comments are likely to become stale because they hardcode a
specific library version ("PyJWT 2.10.1"). Consider rephrasing to be timeless
(e.g., describe the `sub` claim type requirement when present) without pinning
an exact version number.
```suggestion
# Conditionally include 'sub' claim only when user_id is
present.
# RFC 7519 specifies 'sub' as optional; omitting it when
user_id is
# None avoids subject validation errors in JWT libraries
such as PyJWT.
```
##########
superset/async_events/async_query_manager.py:
##########
@@ -177,9 +177,14 @@ def validate_session(response: Response) -> Response:
session["async_channel_id"] = async_channel_id
session["async_user_id"] = user_id
- sub = str(user_id) if user_id else None
+ # Conditionally include 'sub' claim only when user_id is
present
+ # RFC 7519 specifies 'sub' as optional; omitting it when None
+ # prevents PyJWT 2.10.1 InvalidSubjectError for guest users
+ payload = {"channel": async_channel_id}
+ if user_id:
+ payload["sub"] = str(user_id)
token = jwt.encode(
- {"channel": async_channel_id, "sub": sub},
+ payload,
self._jwt_secret,
algorithm="HS256",
)
Review Comment:
This change fixes a regression for guest users, but there’s no unit test
covering the end-to-end behavior of `validate_session` creating a JWT for
`user_id=None` and `parse_channel_id_from_request` decoding it successfully.
Please add a unit test (there are existing tests in
`tests/unit_tests/async_events/async_query_manager_tests.py`) to prevent
reintroducing the `InvalidSubjectError` failure mode.
--
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]
