dosubot[bot] commented on code in PR #39197:
URL: https://github.com/apache/superset/pull/39197#discussion_r3230606320
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1547,3 +1547,62 @@ def test_validate_child_in_parent_multilayer_null_params(
assert not sm._validate_child_in_parent_multilayer(
child_slice_id=1, parent_slice=parent_slice
)
+
+
+def test_user_view_menu_names_for_guest_user(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that user_view_menu_names resolves permissions from the guest
+ user's roles instead of querying by user_id (which is None for guests).
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_role = mocker.MagicMock(spec=Role)
+ mock_role.id = 99
+
+ mock_guest = mocker.MagicMock()
+ mock_guest.is_anonymous = False
+ mock_guest.roles = [mock_role]
+
+ mock_g = SimpleNamespace(user=mock_guest)
+ mocker.patch("superset.security.manager.g", new=mock_g)
+ mocker.patch.object(sm, "is_guest_user", return_value=True)
+
+ mock_result = [SimpleNamespace(name="[PostgreSQL].[my_table](id:1)")]
+ mock_query = mocker.MagicMock()
+ mock_query.join.return_value = mock_query
+ mock_query.filter.return_value = mock_query
+ mock_query.all.return_value = mock_result
+ mocker.patch.object(sm.session, "query", return_value=mock_query)
+
+ result = sm.user_view_menu_names("datasource_access")
+
+ assert result == {"[PostgreSQL].[my_table](id:1)"}
Review Comment:
<!-- Answer -->
I partially agree with Copilot's suggestion. The core regression this PR
fixes is that guest users must **not** go through the authenticated user path
(which calls `get_user_id()` and queries `assoc_user_role` by a `None`
user_id). Asserting that `get_user_id()` is never called when `is_guest_user()`
is `True` would directly encode the regression invariant and make the test more
resilient to future refactors.
Here's a patch that strengthens both tests:
```diff
diff --git a/tests/unit_tests/security/manager_test.py
b/tests/unit_tests/security/manager_test.py
index ...... 100644
--- a/tests/unit_tests/security/manager_test.py
+++ b/tests/unit_tests/security/manager_test.py
@@ def test_user_view_menu_names_for_guest_user(
sm = SupersetSecurityManager(appbuilder)
mock_role = mocker.MagicMock(spec=Role)
mock_role.id = 99
mock_guest = mocker.MagicMock()
mock_guest.is_anonymous = False
mock_guest.roles = [mock_role]
mock_g = SimpleNamespace(user=mock_guest)
mocker.patch("superset.security.manager.g", new=mock_g)
mocker.patch.object(sm, "is_guest_user", return_value=True)
+ mock_get_user_id = mocker.patch(
+ "superset.security.manager.get_user_id",
+ side_effect=AssertionError("get_user_id must not be called for
guest users"),
+ )
mock_result = [SimpleNamespace(name="[PostgreSQL].[my_table](id:1)")]
mock_query = mocker.MagicMock()
mock_query.join.return_value = mock_query
mock_query.filter.return_value = mock_query
mock_query.all.return_value = mock_result
mocker.patch.object(sm.session, "query", return_value=mock_query)
result = sm.user_view_menu_names("datasource_access")
assert result == {"[PostgreSQL].[my_table](id:1)"}
+ mock_get_user_id.assert_not_called()
+ # Verify filtering used role IDs, not user_id
+ mock_query.filter.assert_called()
@@ def test_user_view_menu_names_for_guest_user_no_roles(
sm = SupersetSecurityManager(appbuilder)
mock_role = mocker.MagicMock(spec=Role)
mock_role.id = None
mock_guest = mocker.MagicMock()
mock_guest.is_anonymous = False
mock_guest.roles = [mock_role]
mock_g = SimpleNamespace(user=mock_guest)
mocker.patch("superset.security.manager.g", new=mock_g)
mocker.patch.object(sm, "is_guest_user", return_value=True)
+ mock_get_user_id = mocker.patch(
+ "superset.security.manager.get_user_id",
+ side_effect=AssertionError("get_user_id must not be called for
guest users"),
+ )
result = sm.user_view_menu_names("datasource_access")
assert result == set()
+ mock_get_user_id.assert_not_called()
```
**What this adds:**
1. **`get_user_id` is patched with `side_effect=AssertionError(...)`** — if
the guest path accidentally falls through to the authenticated user code path,
the test will fail immediately with a clear message rather than silently
returning wrong results.
2. **`mock_get_user_id.assert_not_called()`** — an explicit post-hoc
assertion making the invariant self-documenting in test output.
3. **`mock_query.filter.assert_called()`** on the happy path — confirms the
guest branch actually performed role-based filtering via the query chain.
This directly encodes the regression contract: the guest user path must
resolve permissions from role IDs on the token, never touching `get_user_id()`
or the `assoc_user_role` table by user_id
[[1]](https://github.com/apache/superset/blob/829e4d92d91ceae4b43b1ed3b063ffe45377799c/superset/security/manager.py#L773-L792)
[[2]](https://github.com/apache/superset/pull/39197).
<!-- Dosu Comment Footer -->
*To reply, just mention
[@dosu](https://go.dosu.dev/dosubot?utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=reply-with-mention&utm_term=apache%2Fsuperset).*
---
Docs are dead. Just use
[Dosu](https://dosu.dev?utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=static-docs-tagline&utm_term=apache%2Fsuperset).
[](https://app.dosu.dev/response-feedback?utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=static-docs-feedback&utm_term=apache%2Fsuperset&message_id=37d63c5e-56dd-4f91-8b40-a2617e4d37c5)
[](https://github.dosu.com/apache/superset?utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=static-docs-ask-repo&utm_term=apache%2Fsuperset)
[](https://app.dosu.dev/signup?referrer=openSource&source=github-footer&utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=static-docs-share-team&utm_term=apache%2Fsuperset)
--
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]
