fix(security_manager): custom auth_view issue#39098
Conversation
dosubot
Bot
added
authentication
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39098 +/- ##
=======================================
Coverage 64.54% 64.54%
=======================================
Files 2536 2536
Lines 131168 131174 +6
Branches 30453 30455 +2
=======================================
+ Hits 84659 84663 +4
Misses 45046 45046
- Partials 1463 1465 +2
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:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #ec7c4a
Actionable Suggestions - 1
-
superset/security/manager.py - 1
- Runtime AttributeError Risk · Line 3172-3173
Review Details
-
Files reviewed - 1 · Commit Range:
83c2ad9..83c2ad9- superset/security/manager.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| if self.register_superset_auth_view: | ||
| self.auth_view = self.appbuilder.add_view_no_menu(SupersetAuthView) |
There was a problem hiding this comment.
The conditional registration of SupersetAuthView is good, but the rate limiting code later assumes self.auth_view always exists. If a subclass sets register_superset_auth_view = False, this will cause an AttributeError at runtime. The fix checks for auth_view's existence safely.
Code suggestion
Check the AI-generated fix before applying
- if (
- self.is_auth_limited
- and getattr(self.auth_view, "blueprint", None) is not None
- ):
- self.limiter.limit(self.auth_rate_limit, methods=["POST"])(
- self.auth_view.blueprint
- )
+ if (
+ self.is_auth_limited
+ and getattr(self, 'auth_view', None) is not None and getattr(self.auth_view, "blueprint", None) is not None
- ):
+ self.limiter.limit(self.auth_rate_limit, methods=["POST"])(
+ self.auth_view.blueprint
+ )
Code Review Run #ec7c4a
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #24abf8Actionable 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 |
| SupersetRegisterUserView | ||
| ) | ||
| if self.register_superset_auth_view: | ||
| self.auth_view = self.appbuilder.add_view_no_menu(SupersetAuthView) |
There was a problem hiding this comment.
i'm actually a bit confused why this is being added here instead of just setting this as the authview in superset's security manager. That would avoid needing extra properties on the security manager and would just follow the established FAB pattern.
There was a problem hiding this comment.
I'm not 100 % convinced this auth_view should be set in the base security manager, as it feels like it interferes with how fab deals with auth provider views. But I don't have the full context on the changes in the theming PR to understand the changes around this.
|
The change adds conditional flags to allow subclasses of SupersetSecurityManager to disable default auth view registration by setting register_superset_auth_view=False, enabling custom auth implementations without overriding the entire register_views method. This provides a cleaner, more flexible way for extensions to customize authentication without directly modifying or overriding core behavior. |
(cherry picked from commit e56f8cc)
2 participants
SUMMARY
The theming PR #31590 broke support for overriding the
/login/endpoint:This PR makes registering
SupersetAuthViewoptional by introducing a new flagregister_superset_auth_viewon theSupersetSecurityManager. To provide a custom auth endpoint, e.g. in OAuth providers, just set the following:While we're at it, we add similar logic for
registeruser_viewto provide similar flexibility for custom user registration views.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION