Copilot commented on code in PR #39197:
URL: https://github.com/apache/superset/pull/39197#discussion_r3223688239
##########
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:
The query builder is constructed unconditionally, even though the guest-user
branch can return early when there are no `role_ids`. Consider computing
`role_ids` and handling the empty-role case before building `base_query` to
avoid unnecessary query construction in that fast-path.
##########
superset/security/manager.py:
##########
@@ -966,6 +966,21 @@ def user_view_menu_names(self, permission_name: str) ->
set[str]:
.join(self.role_model)
)
+ # Guest users (embedded dashboards) have is_anonymous=False but no
+ # database identity, so querying by user_id returns nothing. Instead,
+ # resolve permissions directly from the roles attached to the guest
+ # token (typically the Public role).
+ if self.is_guest_user():
+ role_ids = [role.id for role in g.user.roles if role.id is not
None]
Review Comment:
If `g.user.roles` can contain `None` (e.g., if `get_guest_user_from_token()`
ends up with a missing role from `find_role(...)`), `role.id` will raise an
`AttributeError`. Make this comprehension resilient by skipping falsy roles
(e.g., `if role and role.id is not None`) so a misconfigured/missing guest role
fails safely (empty permissions) instead of erroring.
##########
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:
These tests heavily mock the SQLAlchemy query chain, so they don’t assert
the most important regression behavior described in the PR: that the guest path
avoids the authenticated `user_id` query. Strengthen the test by explicitly
asserting `get_user_id()` is not called when `is_guest_user()` is True (e.g.,
patch it to raise or spy on it), and/or assert that filtering is performed
using the guest role IDs rather than `assoc_user_role.c.user_id == ...`.
--
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]
