fix: correct Security menu case for MySQL deployments - #40527
Conversation
Code Review Agent Run #832168Actionable 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 |
|
Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️ Please read our New Contributor Welcome & Expectations guide. We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40527 +/- ##
==========================================
- Coverage 64.60% 64.58% -0.02%
==========================================
Files 2684 2684
Lines 148267 148366 +99
Branches 34150 34180 +30
==========================================
+ Hits 95785 95828 +43
- Misses 50740 50781 +41
- Partials 1742 1757 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #bd40ceActionable 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 |
Code Review Agent Run #446e08Actionable 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 |
|
I haven't heard any cases with non-MySQL metadata database deployment so might be we can limit this migration to such dialect? For example, |
|
@hainenber I'm inclined to keep it dialect-agnostic. That stray lowercase |
Code Review Agent Run #41875bActionable 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 |
| class GroupsListView(BaseSupersetView): | ||
| route_base = "/" | ||
| class_permission_name = "security" | ||
| class_permission_name = "Security" |
There was a problem hiding this comment.
It seems to me that we're changing an internal property just to make the UI work. Can we instead change how the menus are surfaced? Maybe using lower() in the comparison?
There was a problem hiding this comment.
The catch is that "security" lowercase in these five views is the actual outlier. Every other view uses its properly cased name, and the ADMIN_ONLY_VIEW_MENUS / PUBLIC_EXCLUDED_VIEW_MENUS both key off "Security". So this isn't inventing a value to please the UI so much as bringing five stragglers in line with the name everything else already expects.
lower() would be tempting, but the case-sensitive comparisons aren't in one place. Some are in our _is_admin_only, but plenty live in FAB itself (the menu builder, add_permission_view_menu...) which we'd have to override as well. Also, normalizing it in the code while the DB stores it in the other casing has the same collation quirk that caused the bug, so it feels more like a ban-aid. Existing installs already have the stray row too, so the migration's needed either way.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #a1318eActionable 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 |
…iews Five views that back the Security menu navbar items declared `class_permission_name = "security"` (lowercase). Flask-AppBuilder uses this name to look up or insert the corresponding `ab_view_menu` row. On MySQL's default case-insensitive collation, the lookup finds whatever row exists first, meaning the DB can end up storing `"security"` instead of `"Security"`. Because Superset's Python comparisons in `sync_role_definitions` and `SupersetSecurityManager` are case-sensitive, `"security" != "Security"` → the Security menu disappears from the UI on MySQL/MariaDB deployments. Fixes: - Change `class_permission_name` to `"Security"` in users_list.py, roles.py, groups.py, logs.py, and user_registrations.py. - Add migration `b4a3f2e1d0c9` that normalises any existing lowercase `"security"` row: renames it to `"Security"` if it is the only row, or merges its `ab_permission_view` entries into the correctly-cased row and deletes the duplicate if both exist. Closes #40330 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erge - Point down_revision to ce6bd21901ab (the actual current chain head, not a1b2c3d4e5f6 which already had two successors) - In the "both rows exist" merge path, transfer ab_permission_view_role bindings from the lowercase PVM to the surviving uppercase PVM before deleting the duplicate, so no role silently loses a Security permission Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previous commits used a1b2c3d4e5f6 then ce6bd21901ab, both of which are mid-chain on master. The true head is 33d7e0e21daa (add_semantic_layers). Branching off mid-chain caused 'Multiple head revisions' in CI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…c05) Master's single head advanced to 31dae2559c05; this migration still pointed at 33d7e0e21daa, making it a sibling and producing multiple alembic heads. Re-point so the chain is linear. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Joe Li <joe@preset.io>
…ent head Re-anchor the migration onto the current single head (9e1f3b8c4d2a) to resolve the multiple-heads branch, and add integration tests covering the lowercase-only rename, the duplicate-row merge with role-binding survival, and the clean-install no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9b4e0d7 to
e6d5741
Compare
Code Review Agent Run #1b7a56Actionable 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 |
…ware MySQL's default case-insensitive collation on ab_view_menu.name means filter_by(name='security') also matches 'Security', and the unique key cannot hold both spellings at once. Assert exact-case names in Python and skip the duplicate-row merge scenario on MySQL, where that state cannot be arranged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bito Automatic Review Skipped – PR Already Merged |
… head Master's migration head moved to b4a3f2e1d0c9 (apache#40527) after the last rebase; chaining off it restores the single-head invariant (per @rusackas's review note). No schema change.
… head Master's migration head moved to b4a3f2e1d0c9 (apache#40527) after the last rebase; chaining off it restores the single-head invariant (per @rusackas's review note). No schema change.
… head Master's migration head moved to b4a3f2e1d0c9 (apache#40527); chaining off it restores the single-head invariant. Note: if the charts sibling (apache#40129) merges first, this needs one final re-point onto 7c4a8d09ca37. No schema change.
… head Master's migration head moved to b4a3f2e1d0c9 (apache#40527); chaining off it restores the single-head invariant. Note: if the charts sibling (apache#40129) merges first, this needs one final re-point onto 7c4a8d09ca37. No schema change.
SUMMARY
On MySQL (and MariaDB) with the default case-insensitive collation, the Security menu — containing List Users, List Roles, and List Groups — disappears for admin users after a fresh install or upgrade.
Root cause: Five Superset views (
UsersListView,RolesListView,GroupsListView,ActionLogView,UserRegistrationsView) declaredclass_permission_name = "security"(lowercase). Flask-AppBuilder uses this string as the key to look up or insert a row in theab_view_menutable. On MySQL,WHERE name = 'security'matches'Security'(case-insensitive), so whichever casing is inserted first wins. The DB ends up with a lowercase"security"row, while all of Superset's Python comparisons (m.name == "Security",ADMIN_ONLY_VIEW_MENUS, etc.) are case-sensitive — causing them to silently miss the row.Fix:
class_permission_nameto"Security"in all five view files so FAB registers the correct name from the start.b4a3f2e1d0c9to normalise existing databases:"security"exists → rename to"Security".ab_permission_viewrows into the correctly-cased entry and delete the duplicate.The workaround documented in discussions (
UPDATE ab_view_menu SET name='Security' WHERE name='security') is exactly what this migration automates.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — the Security menu either appears or doesn't; no visual diff available in this environment.
TESTING INSTRUCTIONS
Fresh MySQL install (reproduces the bug):
Upgrade path (migration):
Unit test:
ADDITIONAL INFORMATION
Migration notes: The migration is a simple
UPDATE(or merge) onab_view_menu. Row count is at most a handful of rows; downtime is zero. Downgrade is a no-op (there is no safe way to restore the original incorrect casing without knowing which state the DB was in).🤖 Generated with Claude Code