rusackas commented on code in PR #39197:
URL: https://github.com/apache/superset/pull/39197#discussion_r3259718213
##########
superset/security/manager.py:
##########
@@ -966,6 +966,21 @@ def user_view_menu_names(self, permission_name: str) ->
set[str]:
.join(self.role_model)
)
Review Comment:
Applied in c2c777e4. The `is_guest_user()` check now lives above the
`base_query` construction, so the empty-`role_ids` fast path returns without
doing the multi-join build. Thanks @dosu!
##########
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:
Applied in c2c777e4. Both guest-user tests now patch `get_user_id` with an
`AssertionError` side-effect (so accidental fall-through fails loudly) and
explicitly `assert_not_called()` it afterward. The happy-path test also asserts
`mock_query.filter.assert_called()` to confirm role-based filtering ran. Thanks
@dosu!
--
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]
