codeant-ai-for-open-source[bot] commented on code in PR #40178:
URL: https://github.com/apache/superset/pull/40178#discussion_r3251707075
##########
superset/security/manager.py:
##########
@@ -174,6 +178,38 @@ def post_update(self, item: Model) -> None:
def post_delete(self, item: Model) -> None:
_log_audit_event("RoleDeleted", {"role_name": item.name, "role_id":
item.id})
+ @expose("/<int:role_id>/users", methods=["PUT"])
+ @protect()
+ @safe
+ def update_role_users(self, role_id: int) -> Any:
+ """Override to deduplicate user IDs and handle missing users
gracefully."""
+ try:
+ item = self.update_role_user_schema.load(request.json)
+ role = self.datamodel.get(role_id)
+ if not role:
+ return self.response_404()
+
+ user_ids = list(set(item["user_ids"]))
+ users = (
+ current_app.appbuilder.session.query(User)
+ .filter(User.id.in_(user_ids))
+ .all()
+ )
+ role.user = users
+ self.datamodel.edit(role)
+ return self.response(
+ 200,
+ **{
+ API_RESULT_RES_KEY: self.update_role_user_schema.dump(
+ item, many=False
+ )
+ },
+ )
Review Comment:
**Suggestion:** The endpoint now mutates the incoming `user_ids`
(deduplicating and potentially dropping IDs not found in the DB) but still
returns a payload serialized from the original `item`, so the response can
report users that were not actually persisted. Return the saved user IDs (or
the updated role state) instead of echoing the pre-query request object to
avoid response/data mismatch. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Role users endpoint response misrepresents persisted role-user
assignments.
- ⚠️ External API clients may cache incorrect role membership state.
- ⚠️ Troubleshooting role assignment issues becomes harder for
administrators.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Superset with this PR so that `SupersetRoleApi` is registered as a
FAB API
blueprint (see `superset/security/manager.py:160-165` and `register_views` at
`superset/security/manager.py:3636-3666`).
2. Ensure you have a valid role `R` and at least one existing user `U` (any
admin-created
role and user are sufficient; the role edit UI uses this API via
`updateRoleUsers` in
`superset-frontend/src/features/roles/utils.ts:11-15`).
3. Using an HTTP client authenticated as an admin, call `PUT
/api/v1/security/roles/R/users` (endpoint exposed by `update_role_users` at
`superset/security/manager.py:181-207`) with JSON body `{"user_ids": [U,
9999999]}` where
`9999999` does not correspond to any `User.id` in the database.
4. In `update_role_users` (`superset/security/manager.py:187-199`), the code
deduplicates
into `user_ids = list(set(item["user_ids"]))` and queries `User` with
`.filter(User.id.in_(user_ids)).all()`, so only real users (e.g., `U`) are
loaded and
persisted via `role.user = users` and `self.datamodel.edit(role)`, but the
200 response is
built with `self.update_role_user_schema.dump(item, many=False)` where
`item["user_ids"]`
still contains `[U, 9999999]`, so the response body reports user IDs that
were not
actually saved.
5. Confirm the mismatch by querying role membership via the users API used
by the UI: `GET
/api/v1/security/users/?q=(filters:!((col:roles,opr:rel_m_m,value:R)))`
(same pattern as
`fetchPaginatedData` in
`superset-frontend/src/features/roles/RoleListEditModal.tsx:126-145`); the
results list
only user `U` as a member of role `R`, while the prior PUT response claimed
`[U,
9999999]`, demonstrating the response/data inconsistency.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fsecurity%2Fmanager.py%0A%2A%2ALine%3A%2A%2A%20192%3A207%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20The%20endpoint%20now%20mutates%20the%20incoming%20%60user_ids%60%20%28deduplicating%20and%20potentially%20dropping%20IDs%20not%20found%20in%20the%20DB%29%20but%20still%20returns%20a%20payload%20serialized%20from%20the%20original%20%60item%60%2C%20so%20the%20response%20can%20report%20users%20that%20were%20not%20actually%20persisted.%20Return%20the%20saved%20user%20IDs%20%28or%20the%20updated%20role%20state%29%20instead%20of%20echoing%20the%20pre-query%20request%20object%20to%20avoid%20response%2Fdata%20mismatch.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20con
cise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fsecurity%2Fmanager.py%0A%2A%2ALine%3A%2A%2A%20192%3A207%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20The%20endpoint%20now%20mutates%20the%20incoming%20%60user_ids%60%20%28deduplicating%20and%20potentially%20dropping%20IDs%20not%20found%20in%20the%20DB%29%20but%20still%20returns%20a%20payload%20serialized%20from%20the%20original%20%60item%60%2C%20so%20the%20response%20can%20report%20users%20that%20were%20not%20actually%20persisted.%20Return%20the%20saved%20user%20I
Ds%20%28or%20the%20updated%20role%20state%29%20instead%20of%20echoing%20the%20pre-query%20request%20object%20to%20avoid%20response%2Fdata%20mismatch.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 192:207
**Comment:**
*Api Mismatch: The endpoint now mutates the incoming `user_ids`
(deduplicating and potentially dropping IDs not found in the DB) but still
returns a payload serialized from the original `item`, so the response can
report users that were not actually persisted. Return the saved user IDs (or
the updated role state) instead of echoing the pre-query request object to
avoid response/data mismatch.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40178&comment_hash=4ae0732817db40a63ddaff2c2fc083d1fac97ab8de728a8cecd585e8e507a94f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40178&comment_hash=4ae0732817db40a63ddaff2c2fc083d1fac97ab8de728a8cecd585e8e507a94f&reaction=dislike'>👎</a>
--
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]
