Copilot commented on code in PR #38309:
URL: https://github.com/apache/superset/pull/38309#discussion_r2885549675
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -461,8 +461,48 @@ def test_get_dashboard_datasets_not_allowed(self):
response = self.get_assert_metric(uri, "get_datasets")
assert response.status_code == 404
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+ def test_get_dashboard_datasets_with_guest_token(self):
+ """
+ Regression test for #38185: a valid guest token must be able to access
+ GET /api/v1/dashboard/{id}/datasets.
+
+ Prior to this fix, get_datasets did not catch
DashboardAccessDeniedError,
+ causing an unhandled exception for guest users. Additionally, the
endpoint
+ was not handling DashboardNotFoundError unlike get_charts and get_tabs.
+ """
+ from superset.security.guest_token import GuestTokenResourceType
+
+ dashboard = Dashboard.get("world_health")
+
+ # Create a real guest access token (not a mock) and call with the
token header
+ token = security_manager.create_guest_access_token(
+ user={
+ "username": "guest",
+ "first_name": "Guest",
+ "last_name": "User",
+ },
+ resources=[
+ {
+ "type": GuestTokenResourceType.DASHBOARD,
+ "id": str(dashboard.uuid),
+ }
Review Comment:
`create_guest_access_token` access checks for dashboards compare token
resources against `dashboard.id` (legacy) or the embedded dashboard UUID
(`dashboard.embedded[0].uuid`), not `dashboard.uuid`. Using
`str(dashboard.uuid)` here will cause `has_guest_access()` to fail and the
request to be denied. Also, guest token auth is only enabled when the
`EMBEDDED_SUPERSET` feature flag is on (request loader short-circuits
otherwise), so this test needs to enable that flag to exercise the guest-token
path.
##########
superset/security/manager.py:
##########
@@ -2862,9 +2862,15 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
if token.get("type") != "guest":
raise ValueError("This is not a guest token.")
except Exception: # pylint: disable=broad-except
- # The login manager will handle sending 401s.
- # We don't need to send a special error message.
- logger.warning("Invalid guest token", exc_info=True)
+ logger.warning(
+ "Invalid guest token. If you are using a non-default "
+ "JWT_ALGORITHM (e.g. RS256), ensure GUEST_TOKEN_JWT_ALGO "
+ "and GUEST_TOKEN_JWT_SECRET are explicitly set in "
+ "superset_config.py, as the guest token system uses its "
+ "own separate JWT configuration independent of "
+ "Flask-JWT-Extended.",
Review Comment:
This new warning message implies that using a non-default `JWT_ALGORITHM`
requires explicitly setting `GUEST_TOKEN_JWT_ALGO`/`GUEST_TOKEN_JWT_SECRET`.
Since guest tokens already use `GUEST_TOKEN_JWT_*` exclusively (and have
defaults), it would be more accurate to frame this as a common cause of
failures when users *expect* guest tokens to follow `JWT_ALGORITHM`, rather
than sounding mandatory whenever `JWT_ALGORITHM` is customized.
```suggestion
"Invalid guest token. Note that guest tokens always use "
"GUEST_TOKEN_JWT_ALGO and GUEST_TOKEN_JWT_SECRET (with their
"
"own defaults) and do not follow JWT_ALGORITHM or other "
"Flask-JWT-Extended settings. A common cause of invalid
guest "
"tokens is expecting them to use a custom JWT_ALGORITHM; "
"ensure your guest token configuration and the tokens you "
"generate are aligned.",
```
##########
UPDATING.md:
##########
@@ -225,9 +225,17 @@ Note: Pillow is now a required dependency (previously
optional) to support image
- [31590](https://github.com/apache/superset/pull/31590) Marks the begining of
intricate work around supporting dynamic Theming, and breaks support for
[THEME_OVERRIDES](https://github.com/apache/superset/blob/732de4ac7fae88e29b7f123b6cbb2d7cd411b0e4/superset/config.py#L671)
in favor of a new theming system based on AntD V5. Likely this will be in
disrepair until settling over the 5.x lifecycle.
- [32432](https://github.com/apache/superset/pull/31260) Moves the List Roles
FAB view to the frontend and requires `FAB_ADD_SECURITY_API` to be enabled in
the configuration and `superset init` to be executed.
- [34319](https://github.com/apache/superset/pull/34319) Drill to Detail and
Drill By is now supported in Embedded mode, and also with the `DASHBOARD_RBAC`
FF. If you don't want to expose these features in Embedded / `DASHBOARD_RBAC`,
make sure the roles used for Embedded / `DASHBOARD_RBAC`don't have the required
permissions to perform D2D actions.
+- [#38185](https://github.com/apache/superset/issues/38185) **Embedded SDK /
Guest Token — independent JWT configuration**: The guest token system uses its
own JWT configuration keys (`GUEST_TOKEN_JWT_ALGO`, `GUEST_TOKEN_JWT_SECRET`,
`GUEST_TOKEN_JWT_EXP_SECONDS`) which are **completely independent** of
Flask-JWT-Extended's `JWT_ALGORITHM` setting. If you configure `JWT_ALGORITHM =
"RS256"` (or any non-default value) for your login flow, you **must** also
explicitly set the guest token keys in `superset_config.py` to avoid `403
Forbidden` errors on the Embedded SDK's `/api/v1/dashboard/{id}/datasets`
endpoint:
+ ```python
+ # These are SEPARATE from JWT_ALGORITHM used by Flask-JWT-Extended
+ GUEST_TOKEN_JWT_ALGO = "HS256"
+ GUEST_TOKEN_JWT_SECRET = "your-secret-change-me" # noqa: S105
+ GUEST_TOKEN_JWT_EXP_SECONDS = 300 # 5 minutes
+ ```
Review Comment:
This UPDATING entry states users "must" set `GUEST_TOKEN_JWT_*` when
configuring `JWT_ALGORITHM = "RS256"`, and ties that directly to avoiding 403s
on the `/datasets` endpoint. Given guest tokens are signed/verified only with
`GUEST_TOKEN_JWT_*` (with their own defaults) and the 403 regression fixed in
this PR is unrelated to JWT config, the wording here is likely too
strong/misleading. Consider rephrasing to: guest tokens are independent; if you
want guest tokens to use RS256 (or if you misconfigured them expecting
`JWT_ALGORITHM` to apply), explicitly set `GUEST_TOKEN_JWT_ALGO` and provide
the appropriate key material via `GUEST_TOKEN_JWT_SECRET`.
--
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]
