korbit-ai[bot] commented on code in PR #32121:
URL: https://github.com/apache/superset/pull/32121#discussion_r1975249442
##########
superset/security/manager.py:
##########
@@ -733,13 +738,24 @@ def user_view_menu_names(self, permission_name: str) ->
set[str]:
)
if not g.user.is_anonymous:
- # filter by user id
- view_menu_names = (
- base_query.join(assoc_user_role)
- .join(self.user_model)
- .filter(self.user_model.id == get_user_id())
- .filter(self.permission_model.name == permission_name)
- ).all()
+ user_id = get_user_id()
+
+ user_roles_filter = or_(
+ exists().where(
+ (assoc_user_role.c.user_id == user_id)
+ & (assoc_user_role.c.role_id == self.role_model.id)
+ & (self.permission_model.name == permission_name)
+ ),
+ exists().where(
+ (assoc_user_group.c.user_id == user_id)
+ & (assoc_user_group.c.group_id == self.group_model.id)
+ & (assoc_group_role.c.group_id == self.group_model.id)
+ & (assoc_group_role.c.role_id == Role.id)
+ & (self.permission_model.name == permission_name)
+ ),
+ )
Review Comment:
### Incorrect Role ID Reference in Group Permission Filter <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The user group filter clause has incorrect join conditions for role_id,
using Role.id instead of self.role_model.id which is inconsistent with the role
filter clause.
###### Why this matters
This inconsistency could cause permissions to not be correctly retrieved for
users who are members of groups, effectively breaking the group-based access
control.
###### Suggested change ∙ *Feature Preview*
Change Role.id to self.role_model.id in the group filter clause for
consistency:
```python
user_roles_filter = or_(
exists().where(
(assoc_user_role.c.user_id == user_id)
& (assoc_user_role.c.role_id == self.role_model.id)
& (self.permission_model.name == permission_name)
),
exists().where(
(assoc_user_group.c.user_id == user_id)
& (assoc_user_group.c.group_id == self.group_model.id)
& (assoc_group_role.c.group_id == self.group_model.id)
& (assoc_group_role.c.role_id == self.role_model.id)
& (self.permission_model.name == permission_name)
),
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f1e5fd9-577b-40d8-91ee-1c37457783a9?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:8f521978-cade-4015-9b90-93b52899a329 -->
--
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]
