Skip to content

Conversation

@aminghadersohi
Copy link
Contributor

SUMMARY

Fixes the AUTH_RATE_LIMITED configuration to work correctly when set to True. Previously, enabling this setting would cause the application to fail during startup.

Before this fix:
When AUTH_RATE_LIMITED = True is set in superset_config.py, the application fails to start with:

AttributeError: 'NoneType' object has no attribute '__module__'

After this fix:
The application starts successfully with rate limiting properly applied to the auth view.

Root cause:
Flask-AppBuilder's register_views() method tries to apply rate limiting to self.auth_view.blueprint, but in certain initialization contexts (e.g., MCP service's flask_singleton), the blueprint is None, causing the error.

Solution:

  1. Apply rate limiting directly in Superset's register_views() method after the auth view is created
  2. Prevent the parent's register_views() from attempting to re-apply rate limiting by temporarily disabling AUTH_RATE_LIMITED during the super() call

This ensures rate limiting works correctly regardless of how many Flask app instances are created.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A - Backend fix

TESTING INSTRUCTIONS

Before fix (reproduces error):

  1. Set AUTH_RATE_LIMITED = True in superset_config.py
  2. Run: PYTHONPATH=. superset run -p 8088
  3. Observe: Application fails with AttributeError

After fix (works correctly):

  1. Set AUTH_RATE_LIMITED = True in superset_config.py
  2. Run: PYTHONPATH=. superset run -p 8088
  3. Observe: Application starts successfully
  4. Verify rate limiting is working by making multiple POST requests to /login/ rapidly

MCP Service test:

  1. Set AUTH_RATE_LIMITED = True and MCP_AUTH_ENABLED = True in superset_config.py
  2. Run: PYTHONPATH=. superset mcp run --port 5008
  3. Observe: MCP service starts successfully

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags: None
  • Changes UI: No
  • Includes DB Migration: No
  • Introduces new feature or API: No
  • Removes existing feature or API: No

When AUTH_RATE_LIMITED = True is set in superset_config.py, the application
would fail to start because Flask-AppBuilder's rate limiting logic tried to
apply rate limiting to the auth view blueprint, but the blueprint was None
in certain initialization contexts (e.g., MCP service's flask_singleton).

This fix applies rate limiting directly in Superset's register_views() method
after the auth view is created, and prevents the parent's register_views()
from attempting to re-apply rate limiting by temporarily disabling the
AUTH_RATE_LIMITED config during the super() call.

The fix ensures rate limiting works correctly regardless of how many Flask
app instances are created (main app, MCP service, etc.).
@dosubot dosubot bot added the authentication Related to authentication label Nov 20, 2025
@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.20%. Comparing base (5320730) to head (a64aab3).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
superset/security/manager.py 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #36195       +/-   ##
===========================================
+ Coverage        0   68.20%   +68.20%     
===========================================
  Files           0      629      +629     
  Lines           0    46209    +46209     
  Branches        0     5003     +5003     
===========================================
+ Hits            0    31518    +31518     
- Misses          0    13446    +13446     
- Partials        0     1245     +1245     
Flag Coverage Δ
hive 43.90% <85.71%> (?)
mysql 67.41% <85.71%> (?)
postgres 67.46% <85.71%> (?)
python 68.18% <85.71%> (?)
sqlite 67.08% <85.71%> (?)
unit 100.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@bito-code-review bito-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Agent Run #1cb99e

Actionable Suggestions - 1
  • superset/security/manager.py - 1
    • Undefined rate limiting attributes and unsafe config handling · Line 2878-2901
Review Details
  • Files reviewed - 1 · Commit Range: 250f448..250f448
    • 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 Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 20, 2025
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm this works. The temporary disabling of the rate limiting config feels slightly janky, but hey, if it works it works 🙂

- Use getattr pattern instead of hasattr + separate None check for DRYness
- Remove .get() on config since AUTH_RATE_LIMITED has default value
@aminghadersohi
Copy link
Contributor Author

Investigating CI failures in test-postgres (previous) and test-postgres (next). The errors appear to be database-related (PendingRollbackError, unique constraint violations) rather than config-related. Checking if these are flaky tests or related to the changes.

@sadpandajoe sadpandajoe merged commit 92d8139 into apache:master Nov 20, 2025
56 checks passed
LuisSanchez pushed a commit to LuisSanchez/superset that referenced this pull request Nov 25, 2025
kshi020302 pushed a commit to jl141/superset that referenced this pull request Nov 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authentication Related to authentication size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants