fix(roles): prevent 404 and silent user removal on large role edits#40178
Conversation
dosubot
Bot
added
authentication:RBAC
Code Review Agent Run #33fbdbActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| 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 | ||
| ) | ||
| }, | ||
| ) |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ 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.Steps of Reproduction ✅
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.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this comment.
The response now serializes {"user_ids": [u.id for u in users]} — the IDs that were actually queried from the DB and persisted — so the response is always an accurate reflection of the roles user assignments after the save.
|
The PR diff shows that the superset/security/manager.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40178 +/- ##
==========================================
+ Coverage 54.57% 64.16% +9.59%
==========================================
Files 2591 2591
Lines 138283 138293 +10
Branches 32083 32084 +1
==========================================
+ Hits 75462 88736 +13274
+ Misses 62476 48027 -14449
- Partials 345 1530 +1185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…he IDs that were actually queried from the DB and persisted — so the response is always an accurate reflection of the roles user assignments after the save.
madhushreeag
force-pushed
the
fix-prevent-error-saving-roles
branch
from
01cf8a5 to
e019cd2
Compare
madhushreeag
force-pushed
the
fix-prevent-error-saving-roles
branch
from
e019cd2 to
05a0ef7
Compare
|
/review |
Code Review Agent Run #182ed0Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #919152Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
RoleUserPutSchema is defined in flask_appbuilder/security/sqla/apis/role/schema.py and registered into the OpenAPI spec by the base RoleApi class that SupersetRoleApi inherits from. The $ref is valid and resolves correctly at runtime; it just isn't visible in the Superset source tree since it lives in the installed FAB package. |
…he roles users are persisted, it now calls _log_audit_event("RoleUsersUpdated", {"role_id": role_id, "user_ids": [...]}) with the IDs that were actually saved. This is consistent with the existing audit logging on post_add, post_update, and post_delete on the same class.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #6d00f2Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…st_add, post_update, and post_delete.
madhushreeag
force-pushed
the
fix-prevent-error-saving-roles
branch
from
e380581 to
f44add4
Compare
Code Review Agent Run #f199f7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@madhushreeag some questions/comments before I post a full review:
|
return non-deterministic boundaries — some users appear on multiple pages (duplicates) while others are skipped. Add orderBy support to fetchPaginatedData and fetch role users ordered by id ASC so page boundaries are stable. Add ID deduplication in setData as a safety net.
You're right that the DB has unique indexes — there are no duplicate rows in ab_user_role. The duplicates are a pagination artifact, not a data integrity issue. Since fetchPaginatedData fires all page requests concurrently via Promise.all, the same row can fall on page 3 in one request and page 4 in another, depending on what the query planner does at that instant. The fix is ORDER BY id ASC on the users fetch, which makes page boundaries stable. The deduplication is just a safety net. I can still remove the dedupe if it feels redundant. The backend changes have been removed from this PR. The ORDER BY fix eliminates duplicates at the source, so there's nothing incorrect being sent to FAB — no backend override needed. Agreed that a few thousand integer IDs shouldn't overflow a default request size limit — a role with 950 users is only a few KB in the request body. We haven't hit 413 in practice with the current fix in place. We were seeing 404 due to the pagination issue. |
madhushreeag
changed the title
|
/review |
Code Review Agent Run #3e06e0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
Amazing, thanks for fixing this! ❤️ I tested this on a large deployment and was able to repro the issue. A few follow-up questions came to mind:
- Do we perhaps have similar bugs on other components based on
FecthPaginatedOptions? Could be worthwhile throwing Claude at this to double check. - Going to even larger scale, could this proactive full fetching become a bottleneck, and if so, could we replace this with real pagination, where we initially just fetch the first page, and when scrolling down or starting to type, the list is dynamically updated with values?
|
Bito Automatic Review Skipped – PR Already Merged |
…40178) Co-authored-by: madhushree agarwal <madhushree_agarwal@apple.com>
SUMMARY
When editing a role with many users (e.g. ~950) and removing just one, far more users were silently dropped, sometimes with 130+ extra removals.
Root cause: fetchPaginatedData fetches page 0 first to get the total count, then fires all remaining page requests concurrently via Promise.all, without an ORDER BY. The database returns rows in non-deterministic order, so page boundaries shift between requests: some users appear on two pages (duplicates) and others are skipped entirely. The frontend ends up with the correct item count but significantly fewer unique IDs. Submitting that deduped set as the full replacement list silently removes the skipped users.
We also saw 404 on save from FAB's update_role_users, which does User.id.in_(user_ids) to look up submitted IDs (SQL IN deduplicates), then checks len(users) != len(user_ids). With duplicates in the submitted list, the deduped DB result was shorter than the raw input — e.g. 819 != 951 — triggering the 404.
Fix:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Screen.Recording.2026-05-18.at.11.00.56.PM.mov
After:
Screen.Recording.2026-05-18.at.11.18.12.PM.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION