feat(api-keys): add API key authentication via FAB SecurityManager#36173
feat(api-keys): add API key authentication via FAB SecurityManager#36173aminghadersohi wants to merge 14 commits into
Conversation
Code Review Agent Run #2af87aActionable Suggestions - 0Additional Suggestions - 9
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #36173 +/- ##
==========================================
+ Coverage 60.48% 68.12% +7.63%
==========================================
Files 1931 638 -1293
Lines 76236 46765 -29471
Branches 8568 5057 -3511
==========================================
- Hits 46114 31857 -14257
+ Misses 28017 13641 -14376
+ Partials 2105 1267 -838
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:
|
53e36d3 to
3de0f37
Compare
| if not api_key: | ||
| raise ApiKeyNotFoundError() | ||
|
|
||
| ApiKeyDAO.revoke(api_key, revoked_by_fk=g.user.id) |
There was a problem hiding this comment.
nit: Thinking if revoke should belong to the DAO, do we consider a DAO to be an abstraction around CRUD. Some commands also touch the db directly. WDYT?
There was a problem hiding this comment.
Based on my first impression, I think revocation is a command that does a DAO based update on the API Key entity.
| expires_on=expires_on, | ||
| created_on=datetime.utcnow(), | ||
| ) | ||
| db.session.add(api_key) |
There was a problem hiding this comment.
we should just do: return super().create(item, attributes)
| api_key: The ApiKey object to delete | ||
| """ | ||
| db.session.delete(api_key) | ||
| db.session.commit() |
There was a problem hiding this comment.
commit at the DAO level same as above
| if g.user is None or g.user.is_anonymous: | ||
| return self.response_401() | ||
| except NoAuthorizationError: | ||
| return self.response_401() |
There was a problem hiding this comment.
we don't need this, use the @protect() for authorization and authentication
There was a problem hiding this comment.
First pass comments. Before proceeding with this, I suggest opening a SIP that outlines how API keys should work in practice. Some important themes that need discussion:
- Scoping: Optimally, it would be possible to downscope the users creds in the API keys. As the current permission system is very granular, I think it makes sense to group perms together, like
- Dashboard: "no access, read, write"
- Charts: "no access, read, write"
- ...
- Authorization: How will these keys be handled in practice in the security manager
- Auth mechanism: I assume these will be passed as bearer tokens on API/MCP calls but not UI routes, but let's clarify that.
- Long term support: Will current FAB API tokens be supported after these are rolled out?
- Long term security plan: How this relates to the planned security model revamp that's been discussed in SIP-131
etc.
| # Parse expires_on if it's a string | ||
| if isinstance(expires_on, str): | ||
| expires_on = datetime.fromisoformat(expires_on) |
There was a problem hiding this comment.
I'd prefer not to support multiple formats. We should have a Marshmallow schema in place that validates the input and parses the API request body into a standardized data structure with preferably a datetime for expires_on
| if not api_key: | ||
| raise ApiKeyNotFoundError() | ||
|
|
||
| ApiKeyDAO.revoke(api_key, revoked_by_fk=g.user.id) |
There was a problem hiding this comment.
Based on my first impression, I think revocation is a command that does a DAO based update on the API Key entity.
| sa.Column("revoked_on", sa.DateTime(), nullable=True), | ||
| sa.Column("revoked_by_fk", sa.Integer(), nullable=True), |
There was a problem hiding this comment.
Do revoked keys need to stay in the metastore? Or can we just remove them? Not sure what the typical policy is for these, but I assume revoked keys should not be unrevokable, hence they probably don't need to exist in the metastore after they've been revoked. Same for expired keys - I think we can just remove them when they're no longer valid (in the KeyValue DAO we have logic for cleaning up expired entries automatically which we could use for inspiration here).
| import bcrypt | ||
|
|
||
| # API key configuration | ||
| API_KEY_PREFIX = "pst" # Preset/Superset prefix for easy identification |
There was a problem hiding this comment.
Why is this needed? Also, let's not have Preset references in the upstream codebase
| API_KEY_PREFIX = "pst" # Preset/Superset prefix for easy identification | |
| API_KEY_PREFIX = "pst" # Superset prefix for easy identification |
Add database schema and UI for MCP service API key authentication. This PR adds the database layer (models and migration) plus a basic UI for users to manage their API keys. Authentication logic will be implemented separately in the superset-shell fork. Database Changes: - Add ApiKey model in superset/models/mcp.py - Add migration 2025-11-18_12-00_e8f4a3b2c1d0_add_mcp_api_keys.py - Create ab_api_key table with proper indexes and foreign keys - Support for key expiration, revocation, and usage tracking - Workspace-scoped keys for multi-tenant isolation Frontend Changes: - Add API Keys section to User Info page - Add ApiKeyList component to display and manage API keys - Add ApiKeyCreateModal for creating new API keys - API keys shown only once at creation for security The ab_api_key table stores: - bcrypt hash (never plaintext) - Workspace scoping for multi-tenancy - Expiration and revocation support - Usage tracking - User ownership for RBAC Related to preset-io/superset-shell#3252
Alert component is exported from @apache-superset/core/ui, not from @superset-ui/core/components. This fixes the TypeScript build error in the docker build and frontend type checking.
The ApiKey model is generic and not specific to MCP. Renamed the file and updated docstrings to reflect this: - superset/models/mcp.py -> superset/models/api_keys.py - Updated module and class docstrings to remove MCP-specific language - Updated migration description to be generic The model can be used for any API key authentication, not just MCP.
Users can now leave workspace_name blank when creating an API key. The backend will default it to 'default' workspace if not provided. This matches the frontend UI which says 'Optional: Leave blank to use your default workspace'.
…ponents - Fix modal closing immediately after API key creation - Move setShowCreateModal(false) to onHide instead of onSuccess - Change FormModal onSave to empty function to prevent premature close - Restore missing ApiKeyCreateModal.tsx and ApiKeyList.tsx components - Copy button now visible after key creation Users can now copy the API key before the modal closes.
…_name required - Add API key endpoints (get_api_keys, create_api_key, revoke_api_key) to security test allowlist - Make workspace_name field required in ApiKeyPostSchema - Update CreateApiKeyCommand validation to require workspace_name - Fixes test_views_are_secured and test_create_api_key_missing_workspace failures
- Make workspace_name optional with load_default='default' in schema - Update test to verify default workspace is used when not provided - Users can now create API keys without specifying workspace
I got the impression this was just for MCP auth. But seems I was wrong. If these API keys are to be used globally on all our current REST API as is they won't be respected by flask-appbuilder authentication and authorization system. Globally it would probably make more sense to implement this at the flask-appbuilder layer for easier integration with the |
@dpgaspar good callouts. All the more reason to debate this in a SIP before proceeding. |
Enable MCP service to authenticate users via API keys created in the Superset UI. This provides a secure alternative to JWT tokens for programmatic access. Authentication priority: 1. g.user (if already set by middleware) 2. API key from Authorization header (Bearer pst_...) 3. MCP_DEV_USERNAME (development fallback) Features: - API keys prefixed with 'pst_' are recognized and validated - Keys are checked against bcrypt hashes in ab_api_key table - Expired and revoked keys are rejected - last_used_on timestamp updated on successful auth - User inherits all roles and permissions from key owner Configuration: - MCP_API_KEY_AUTH_ENABLED: Toggle API key auth (default: True) Usage: Authorization: Bearer pst_abc123xyz...
dd120f0 to
5082421
Compare
Add API key authentication to Flask-AppBuilder, enabling programmatic access to FAB-protected endpoints without JWT tokens or browser sessions. Changes: - Add ApiKey model (ab_api_key table) with uuid, key_hash, prefix, scopes, active status, expiration, and revocation tracking - Add validate_api_key() and _extract_api_key_from_request() to BaseSecurityManager with concrete SQLA implementations - Modify @Protect() decorator to check API key auth before JWT verification - Add CRUD API endpoints at /api/v1/security/api_keys/ (list, create, get, revoke) gated behind FAB_API_KEY_ENABLED config - Update has_access() to recognize API key authenticated users - Update _create_db() to auto-create ab_api_key table on existing installs - Add 24 tests covering model, SecurityManager methods, CRUD endpoints, and @Protect() decorator integration Config keys: - FAB_API_KEY_ENABLED (bool): Enable API key auth (default: False) - FAB_API_KEY_PREFIXES (list): Key prefixes to recognize (default: ["sst_"]) References: apache/superset#36173 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move API key authentication from Superset-specific implementation to Flask-AppBuilder's SecurityManager layer. This ensures @Protect() works automatically with API keys and downstream apps inherit the feature via their SecurityManager. Changes: - Remove Superset-only API key code (model, DAO, utils, commands) - Update MCP auth to use FAB's SecurityManager.validate_api_key() - Remove API key endpoints from CurrentUserRestApi (now in FAB's ApiKeyApi) - Update frontend to use /api/v1/security/api_keys/ endpoint - Add FAB_API_KEY_ENABLED and FAB_API_KEY_PREFIXES config - Replace old migration with FAB-compatible ab_api_key table migration - Pin FAB to feature branch with API key support - Update key prefix from pst_ to sst_ Depends on: https://github.com/aminghadersohi/Flask-AppBuilder/tree/amin/ch99414/api-key-auth Related: dpgaspar/Flask-AppBuilder#2430 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Superseded by #37973 - fresh branch based on current master with cleaner implementation delegating to FAB SecurityManager. |
Add API key authentication to Flask-AppBuilder, enabling programmatic access to FAB-protected endpoints without JWT tokens or browser sessions. Changes: - Add ApiKey model (ab_api_key table) with uuid, key_hash, prefix, scopes, active status, expiration, and revocation tracking - Add validate_api_key() and _extract_api_key_from_request() to BaseSecurityManager with concrete SQLA implementations - Modify @Protect() decorator to check API key auth before JWT verification - Add CRUD API endpoints at /api/v1/security/api_keys/ (list, create, get, revoke) gated behind FAB_API_KEY_ENABLED config - Update has_access() to recognize API key authenticated users - Update _create_db() to auto-create ab_api_key table on existing installs - Add 24 tests covering model, SecurityManager methods, CRUD endpoints, and @Protect() decorator integration Config keys: - FAB_API_KEY_ENABLED (bool): Enable API key auth (default: False) - FAB_API_KEY_PREFIXES (list): Key prefixes to recognize (default: ["sst_"]) References: apache/superset#36173
Add API key authentication to Flask-AppBuilder, enabling programmatic access to FAB-protected endpoints without JWT tokens or browser sessions. Changes: - Add ApiKey model (ab_api_key table) with uuid, key_hash, prefix, scopes, active status, expiration, and revocation tracking - Add validate_api_key() and _extract_api_key_from_request() to BaseSecurityManager with concrete SQLA implementations - Modify @Protect() decorator to check API key auth before JWT verification - Add CRUD API endpoints at /api/v1/security/api_keys/ (list, create, get, revoke) gated behind FAB_API_KEY_ENABLED config - Update has_access() to recognize API key authenticated users - Update _create_db() to auto-create ab_api_key table on existing installs - Add 24 tests covering model, SecurityManager methods, CRUD endpoints, and @Protect() decorator integration Config keys: - FAB_API_KEY_ENABLED (bool): Enable API key auth (default: False) - FAB_API_KEY_PREFIXES (list): Key prefixes to recognize (default: ["sst_"]) References: apache/superset#36173
Add API key authentication to Flask-AppBuilder, enabling programmatic access to FAB-protected endpoints without JWT tokens or browser sessions. Changes: - Add ApiKey model (ab_api_key table) with uuid, key_hash, prefix, scopes, active status, expiration, and revocation tracking - Add validate_api_key() and _extract_api_key_from_request() to BaseSecurityManager with concrete SQLA implementations - Modify @Protect() decorator to check API key auth before JWT verification - Add CRUD API endpoints at /api/v1/security/api_keys/ (list, create, get, revoke) gated behind FAB_API_KEY_ENABLED config - Update has_access() to recognize API key authenticated users - Update _create_db() to auto-create ab_api_key table on existing installs - Add 24 tests covering model, SecurityManager methods, CRUD endpoints, and @Protect() decorator integration Config keys: - FAB_API_KEY_ENABLED (bool): Enable API key auth (default: False) - FAB_API_KEY_PREFIXES (list): Key prefixes to recognize (default: ["sst_"]) References: apache/superset#36173
* feat: add API key authentication support Add API key authentication to Flask-AppBuilder, enabling programmatic access to FAB-protected endpoints without JWT tokens or browser sessions. Changes: - Add ApiKey model (ab_api_key table) with uuid, key_hash, prefix, scopes, active status, expiration, and revocation tracking - Add validate_api_key() and _extract_api_key_from_request() to BaseSecurityManager with concrete SQLA implementations - Modify @Protect() decorator to check API key auth before JWT verification - Add CRUD API endpoints at /api/v1/security/api_keys/ (list, create, get, revoke) gated behind FAB_API_KEY_ENABLED config - Update has_access() to recognize API key authenticated users - Update _create_db() to auto-create ab_api_key table on existing installs - Add 24 tests covering model, SecurityManager methods, CRUD endpoints, and @Protect() decorator integration Config keys: - FAB_API_KEY_ENABLED (bool): Enable API key auth (default: False) - FAB_API_KEY_PREFIXES (list): Key prefixes to recognize (default: ["sst_"]) References: apache/superset#36173 * fix: apply black formatting to API key auth files * fix: revert to black 23.10 formatting for decorators.py * fix: remove unused imports in test_api_key.py * fix: ensure ApiKey permissions are created when update_perms is False When AppBuilder is initialized with update_perms=False (as Superset does), the standard _add_permission() call in add_view_no_menu() is a no-op. This means ApiKeyApi permissions are never created, causing all API key endpoints to return 403 Forbidden. Fix by explicitly calling add_permissions_view() after registering the ApiKeyApi, which creates the permission-view-menu entries and assigns them to the Admin role regardless of the update_perms flag. This is idempotent and safe when update_perms is True (permissions already exist). Adds tests that verify permissions exist, Admin role has them, and endpoints work when update_perms=False. * feat: add lookup_hash for O(1) key validation, address review feedback - Add lookup_hash column (indexed, unique) for constant-time API key lookup instead of iterating all keys with prefix matching - Add configurable hash algorithms: - FAB_API_KEY_LOOKUP_HASH_METHOD (default: sha256) for fast lookup - FAB_API_KEY_HASH_METHOD (default: falls back to FAB_PASSWORD_HASH_METHOD) - FAB_API_KEY_HASH_SALT_LENGTH for the slow verification hash - Address review feedback from @dpgaspar: - Use sm.current_user instead of custom _get_current_user helper - Update sm.current_user property to handle API key auth - Let create_api_key/revoke_api_key raise instead of returning None - Move imports to module level - Drop getattr guard on user.is_active - Respect update_perms=False without exceptions * style: apply black formatting to models and manager * fix: use HMAC for lookup hash to resolve CodeQL alerts Replace plain SHA-256 with HMAC keyed by SECRET_KEY for the API key lookup hash. This prevents pre-computation of lookup hashes without the server secret and resolves CodeQL's "weak cryptographic hashing algorithm on sensitive data" alerts. * fix: remove unused hashlib import, suppress CodeQL false positive - Remove unused hashlib import (lint failure) - Add lgtm suppression for the HMAC lookup hash (intentional fast lookup index, not password storage — key_hash provides the slow hash) - Use _compute_lookup_hash() in tests instead of raw hmac calls * fix: use BLAKE2b for lookup hash to resolve CodeQL alerts Switch from HMAC-SHA256 to BLAKE2b with native keyed hashing for the API key lookup hash. BLAKE2b is not flagged by CodeQL's weak-sensitive-data-hashing rule (which targets SHA-256/SHA-512/MD5), is fast by design, and supports keying natively without an HMAC wrapper. The lookup hash is an internal optimization detail for O(1) key lookup, not a password storage mechanism (key_hash provides the slow hash for defense in depth). * fix: use scrypt for lookup hash to satisfy CodeQL CodeQL flags all fast hash algorithms (SHA-256, BLAKE2, HMAC) as "insecure for password hashing" when used on sensitive data. Switch to hashlib.scrypt with minimal work parameters (n=2, r=1, p=1) which is nearly as fast as a plain hash but classified as a computationally expensive algorithm by static analysis tools. The lookup hash is an internal O(1) index — the actual password-strength protection is provided by key_hash (via generate_password_hash). * fix: address PR review - 401 vs 403, public method, OpenAPI spec, docs - Rename _extract_api_key_from_request to extract_api_key_from_request (public) - Return 401 for invalid API key, 403 for valid key without permission - Register api_key security scheme in OpenAPI spec - Add api_key alongside jwt in operation_helper security list - Add API key documentation to security.rst and rest_api.rst - Add test for valid key with no permission returning 403 * fix: use black 23.10 formatting for decorators.py to pass CI lint * fix: import USERNAME_READONLY in test_api_key to fix lint * fix: use no-permission role in 403 test instead of ReadOnly * fix: clean up noperms_user in tearDown to prevent test pollution
SUMMARY
Add API key authentication to Apache Superset, implemented at the Flask-AppBuilder (FAB) layer per community feedback. This enables programmatic access via long-lived API keys for MCP service integrations, CI/CD pipelines, external applications, and scheduled jobs.
Architecture Decision
Based on community feedback, API key auth is implemented in FAB's SecurityManager rather than as Superset-specific code. This ensures:
@protect()works automatically with API keysSupersetSecurityManagerHow It Works
POST /api/v1/security/api_keys/(FAB endpoint)sst_by default) for identificationAuthorization: Bearer sst_<key>@protect()decorator validates the key before JWT verificationChanges in This PR
Removed (moved to FAB):
superset/models/api_keys.py- model now in FABsuperset/daos/api_key.py- DAO now in FAB SecurityManagersuperset/utils/api_key.py- key generation/validation now in FABsuperset/commands/api_key/- commands replaced by FAB SecurityManager methodse8f4a3b2c1d0- replaced with FAB-compatible migrationModified:
superset/mcp_service/auth.py- usessecurity_manager.validate_api_key()instead of custom codesuperset/views/users/api.py- removed API key endpoints (now in FAB'sApiKeyApi)superset/config.py- addedFAB_API_KEY_ENABLEDandFAB_API_KEY_PREFIXESconfig/api/v1/security/api_keys/endpointAdded:
f1a2b3c4d5e6forab_api_keytable (compatible with FAB's model schema)Dependencies
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - API-only changes. Frontend API key management UI updated to use new endpoint.
TESTING INSTRUCTIONS
pip install git+https://github.com/aminghadersohi/Flask-AppBuilder@amin/ch99414/api-key-authFAB_API_KEY_ENABLED = Truein config (default in this PR)curl http://localhost:8088/api/v1/security/roles/ \ -H "Authorization: Bearer sst_<returned_key>"ADDITIONAL INFORMATION