feat(api-keys): add API key authentication via FAB SecurityManager#37973
feat(api-keys): add API key authentication via FAB SecurityManager#37973aminghadersohi wants to merge 5 commits intoapache:masterfrom
Conversation
Add API key authentication support to Superset by delegating to Flask-AppBuilder's SecurityManager. This ensures @Protect() works automatically with API keys and benefits the broader FAB ecosystem. Changes: - Update MCP auth to use FAB's SecurityManager.validate_api_key() - Add FAB_API_KEY_ENABLED and FAB_API_KEY_PREFIXES config options - Add frontend UI for API key management (list, create, revoke) - Add migration for ab_api_key table (FAB-managed) - Pin FAB to feature branch with API key support Key design decisions: - API keys validated via prefix-based lookup + hash verification - Key prefix configurable (default: ["sst_"]) for identification - Keys inherit user's RBAC permissions via FAB's has_access() - UUID-based external references for key management Depends on: dpgaspar/Flask-AppBuilder#2431 Related: apache#36175
aminghadersohi
requested review from
betodealmeida,
eschutho,
michael-s-molina,
mistercrunch and
sadpandajoe
as code owners
dosubot
bot
added
api
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const handleCopyKey = () => { | ||
| if (createdKey) { | ||
| navigator.clipboard.writeText(createdKey); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); |
There was a problem hiding this comment.
Suggestion: The clipboard write operation is asynchronous but is neither awaited nor error-handled, so if navigator.clipboard.writeText fails (e.g., due to denied permissions or unsupported environment) it will reject with an unhandled promise while the UI still shows a successful "Copied!" state, misleading the user and potentially causing runtime errors. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Clipboard copy failures silently ignored for API key modal.
- ⚠️ Users see "Copied!" despite clipboard write failure.
- ⚠️ Unhandled promise rejections clutter browser error logs.| const handleCopyKey = () => { | |
| if (createdKey) { | |
| navigator.clipboard.writeText(createdKey); | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| const handleCopyKey = async () => { | |
| if (!createdKey) { | |
| return; | |
| } | |
| try { | |
| await navigator.clipboard.writeText(createdKey); | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| } catch { | |
| addDangerToast(t('Failed to copy API key to clipboard')); |
Steps of Reproduction ✅
1. Log into Superset and open the API key management UI that renders `ApiKeyCreateModal`
from `superset-frontend/src/features/apiKeys/ApiKeyCreateModal.tsx:33-150`.
2. Create a new API key via the modal so that `handleFormSubmit` at lines 50-58 sets
`createdKey` to a non-null value and the component re-renders into the "API Key Created"
branch at lines 80-127.
3. In that success modal, click the "Copy" button wired to `handleCopyKey` at lines 63-69,
in a browser/environment where `navigator.clipboard.writeText` rejects (for example,
clipboard permission denied or clipboard API unsupported for the current context).
4. Observe in browser dev tools that the Promise returned by
`navigator.clipboard.writeText` at line 65 is rejected without any `await`/`catch`,
causing an unhandled promise rejection, while the UI still switches the button label to
"Copied!" via `setCopied(true)` at line 66 even though the key was not copied.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/apiKeys/ApiKeyCreateModal.tsx
**Line:** 63:67
**Comment:**
*Possible Bug: The clipboard write operation is asynchronous but is neither awaited nor error-handled, so if `navigator.clipboard.writeText` fails (e.g., due to denied permissions or unsupported environment) it will reject with an unhandled promise while the UI still shows a successful "Copied!" state, misleading the user and potentially causing runtime errors.
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.There was a problem hiding this comment.
Good catch — the clipboard write is async and should be awaited with error handling. Fixed in d653271: handleCopyKey is now async, awaits the clipboard write, and shows a danger toast on failure instead of silently swallowing the rejected promise.
superset/mcp_service/auth.py
Outdated
| # Try API key authentication via FAB SecurityManager | ||
| api_key_enabled = current_app.config.get("FAB_API_KEY_ENABLED", False) | ||
| if api_key_enabled: |
There was a problem hiding this comment.
Suggestion: The API key extraction unconditionally calls a helper that likely relies on a Flask request context; when MCP tools run with only an application context (as mcp_auth_hook does for internal/non-HTTP operations), this will raise a RuntimeError ("working outside of request context") instead of gracefully skipping API-key auth, breaking flows that previously worked when API keys are enabled. [logic error]
Severity Level: Critical 🚨
- ❌ MCP tools crash when FAB_API_KEY_ENABLED is enabled.
- ❌ FastMCP internal operations fail with RuntimeError, breaking tooling.
- ⚠️ Development flows without HTTP requests become unusable.| # Try API key authentication via FAB SecurityManager | |
| api_key_enabled = current_app.config.get("FAB_API_KEY_ENABLED", False) | |
| if api_key_enabled: | |
| from flask import has_request_context | |
| # Try API key authentication via FAB SecurityManager | |
| api_key_enabled = current_app.config.get("FAB_API_KEY_ENABLED", False) | |
| if api_key_enabled and has_request_context(): |
Steps of Reproduction ✅
1. Enable FAB API key auth by setting `FAB_API_KEY_ENABLED = True` in
`superset_config.py`, causing the API-key branch in `get_user_from_request()`
(`superset/mcp_service/auth.py:119-131`) to execute.
2. Start the standalone MCP server which wraps tools with `mcp_auth_hook()` (defined in
`superset/mcp_service/auth.py`) so that each tool call enters `_setup_user_context()` and
then `get_user_from_request()` inside a Flask *application* context only.
3. Trigger an MCP operation that is not handling an HTTP request (e.g., FastMCP tool
discovery or background invocation), so `mcp_auth_hook._get_app_context_manager()` pushes
`app.app_context()` but no request context is created (no `test_request_context()` or view
handling).
4. During this call, `get_user_from_request()` reaches the API-key block and calls
`sm._extract_api_key_from_request()` at line 123, which accesses `flask.request` and
raises `RuntimeError("Working outside of request context")`; this bubbles out of
`_setup_user_context()` (which only treats "application context" errors as expected) and
crashes the MCP tool execution instead of falling back to `MCP_DEV_USERNAME` or returning
`None` for internal calls.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 119:121
**Comment:**
*Logic Error: The API key extraction unconditionally calls a helper that likely relies on a Flask request context; when MCP tools run with only an application context (as mcp_auth_hook does for internal/non-HTTP operations), this will raise a RuntimeError ("working outside of request context") instead of gracefully skipping API-key auth, breaking flows that previously worked when API keys are enabled.
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.There was a problem hiding this comment.
Valid point — when MCP tools run with only an application context (no HTTP request), _extract_api_key_from_request() would hit a RuntimeError trying to access flask.request. Fixed in d653271 by guarding with has_request_context() so API key auth is only attempted when there's an actual HTTP request.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #37973 +/- ##
==========================================
+ Coverage 60.48% 66.42% +5.93%
==========================================
Files 1931 668 -1263
Lines 76236 51334 -24902
Branches 8568 5779 -2789
==========================================
- Hits 46114 34098 -12016
+ Misses 28017 15850 -12167
+ Partials 2105 1386 -719
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:
|
- Import t from @apache-superset/core instead of @superset-ui/core - Add ApiKeyApi to expected CSRF exempt blueprints set
![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 #b5c2de
Actionable Suggestions - 1
-
requirements/base.txt - 1
- Supply Chain Security Risk · Line 123-123
Additional Suggestions - 5
-
superset-frontend/src/features/apiKeys/ApiKeyCreateModal.tsx - 2
-
Logic Error in Close Handler · Line 71-78The handleClose function sets createdKey to null before checking its value, which prevents onSuccess from being called after successful API key creation. This means the parent component won't refresh the API key list as intended.
Code suggestion
@@ -71,8 +71,8 @@ - const handleClose = () => { - setCreatedKey(null); - setCopied(false); - onHide(); - if (createdKey) { - onSuccess(); - } - }; + const handleClose = () => { + if (createdKey) { + onSuccess(); + } + setCreatedKey(null); + setCopied(false); + onHide(); + };
-
Missing Error Handling for Clipboard · Line 63-69The clipboard write operation lacks error handling. If the Clipboard API fails (e.g., due to permissions or browser limitations), users won't know the copy failed, leading to confusion.
-
-
superset-frontend/src/features/apiKeys/ApiKeyList.tsx - 3
-
Truncate API key prefix display · Line 142-152The key_prefix render appends '...' to the full string, potentially showing long values like 'sk-1234567890...'. Truncate to a fixed length (e.g., 10 chars) for better UX.
Code suggestion
diff --git a/superset-frontend/src/features/apiKeys/ApiKeyList.tsx b/superset-frontend/src/features/apiKeys/ApiKeyList.tsx index 0000000..0000000 100644 --- a/superset-frontend/src/features/apiKeys/ApiKeyList.tsx +++ b/superset-frontend/src/features/apiKeys/ApiKeyList.tsx @@ -142,7 +142,7 @@ render: (prefix: string) => ( <code css={css` background: ${theme.colorFillSecondary}; padding: 2px 6px; border-radius: 3px; `} > - {prefix}... + {prefix.length > 10 ? prefix.substring(0, 10) + '...' : prefix} </code> ), },
-
Specify locale in date formatting · Line 91-91Using undefined locale may lead to inconsistent date formatting across browsers; specify 'en-US' for consistency.
-
Replace custom spans with antd Tags · Line 98-130Use antd Tag components for status badges to avoid custom CSS and follow best practices from AGENTS.md.
Code suggestion
diff --git a/superset-frontend/src/features/apiKeys/ApiKeyList.tsx b/superset-frontend/src/features/apiKeys/ApiKeyList.tsx index 0000000..0000000 100644 --- a/superset-frontend/src/features/apiKeys/ApiKeyList.tsx +++ b/superset-frontend/src/features/apiKeys/ApiKeyList.tsx @@ -1,7 +1,7 @@ import { useCallback, useEffect, useState } from 'react'; import { t, SupersetClient } from '@superset-ui/core'; import { css, useTheme } from '@apache-superset/core/ui'; -import { Button, Table, Modal, Tooltip } from '@superset-ui/core/components'; +import { Button, Table, Modal, Tooltip, Tag } from '@superset-ui/core/components'; import { useToasts } from 'src/components/MessageToasts/withToasts'; import { ApiKeyCreateModal } from './ApiKeyCreateModal'; @@ -99,29 +99,15 @@ const getStatusBadge = (key: ApiKey) => { if (key.revoked_on) { - return ( - <span - css={css` - color: ${theme.colorError}; - `} - > - {t('Revoked')} - </span> - ); + return <Tag color="error">{t('Revoked')}</Tag>; } if (key.expires_on && new Date(key.expires_on) < new Date()) { - return ( - <span - css={css` - color: ${theme.colorWarning}; - `} - > - {t('Expired')} - </span> - ); + return <Tag color="warning">{t('Expired')}</Tag>; } - return ( - <span - css={css` - color: ${theme.colorSuccess}; - `} - > - {t('Active')} - </span> - ); + return <Tag color="success">{t('Active')}</Tag>; }; const columns = [
-
Review Details
-
Files reviewed - 6 · Commit Range:
100c366..100c366- requirements/base.txt
- superset-frontend/src/features/apiKeys/ApiKeyCreateModal.tsx
- superset-frontend/src/features/apiKeys/ApiKeyList.tsx
- superset/config.py
- superset/mcp_service/auth.py
- superset/migrations/versions/2026-02-14_12-00_f1a2b3c4d5e6_add_fab_api_key_table.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
| # flask-sqlalchemy | ||
| # flask-wtf | ||
| flask-appbuilder==5.0.2 | ||
| flask-appbuilder @ git+https://github.com/aminghadersohi/Flask-AppBuilder@amin/ch99414/api-key-auth |
There was a problem hiding this comment.
Switching to a git dependency from a third-party fork introduces supply chain risks, as the branch can change unpredictably and the fork may not be vetted. Flask-AppBuilder is central to authentication and RBAC in Superset, so this could affect security if the fork has untrusted code. Consider using an official release or a pinned commit instead.
Code Review Run #b5c2de
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Acknowledged — this is a temporary pin to the FAB feature branch during development. Once the FAB PR (dpgaspar/Flask-AppBuilder#2431) is merged and released to PyPI, this will be reverted to a standard version pin (e.g., flask-appbuilder>=5.2.0).
There was a problem hiding this comment.
The change pins flask-appbuilder to a development branch temporarily, to be reverted to a standard version pin after the upstream PR merges and releases.
- Add async/await and error handling for clipboard write in ApiKeyCreateModal - Reorder handleClose to call onSuccess before clearing state - Use antd Tag components for status badges instead of custom styled spans - Guard API key extraction with has_request_context() to prevent RuntimeError when MCP runs outside HTTP request context
Code Review Agent Run #bb1233Actionable 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 |
|
Addressed all review feedback in d653271: Fixed:
Acknowledged (no change needed):
|
Code Review Agent Run #a32c1bActionable 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 |
1 participant
SUMMARY
Add API key authentication support to Superset by delegating to Flask-AppBuilder's SecurityManager. This enables programmatic access to Superset for MCP service integrations, CI/CD pipelines, and external applications without requiring browser-based login or frequent JWT refresh.
Architecture: API key auth is implemented at the FAB layer so
@protect()works automatically, avoiding split auth management. This benefits the broader FAB ecosystem.Key changes:
SecurityManager.validate_api_key()FAB_API_KEY_ENABLEDandFAB_API_KEY_PREFIXESconfig optionsab_api_keytable (FAB-managed)Design decisions:
check_password_hash) — no plaintext storageFAB_API_KEY_PREFIXES, default["sst_"]) for identification and efficient DB lookup@protect()checks API keys before JWT — deterministic auth path when prefix is detectedhas_access()mechanismDepends on: dpgaspar/Flask-AppBuilder#2431 (FAB feature branch — all CI green)
Related SIP: #36175
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - New feature, no existing UI changes
TESTING INSTRUCTIONS
pip install git+https://github.com/aminghadersohi/Flask-AppBuilder@amin/ch99414/api-key-authFAB_API_KEY_ENABLED = Trueinsuperset_config.pyPOST /api/v1/security/api_keys/with Bearer JWT authAuthorization: Bearer sst_...on any protected endpointDELETE /api/v1/security/api_keys/<uuid>ADDITIONAL INFORMATION