aminghadersohi commented on code in PR #37973:
URL: https://github.com/apache/superset/pull/37973#discussion_r2980469552
##########
superset/mcp_service/auth.py:
##########
@@ -192,6 +209,55 @@ def get_user_from_request() -> User:
if hasattr(g, "user") and g.user:
return g.user
+ # Try API key authentication via FAB SecurityManager
+ # Only attempt when in a request context (not for MCP internal operations
+ # like tool discovery that run with only an application context)
+ # Use the Flask config key FAB_API_KEY_ENABLED (not the feature flag),
+ # because the config key controls whether FAB registers the API key
+ # endpoints and validation logic. The feature flag with the same name
+ # in DEFAULT_FEATURE_FLAGS only controls the frontend UI visibility.
+ if current_app.config.get("FAB_API_KEY_ENABLED", False) and
has_request_context():
+ sm = current_app.appbuilder.sm
+ # _extract_api_key_from_request is FAB's internal method for reading
+ # the Bearer token from the Authorization header and matching prefixes.
+ # Not all FAB versions include this method, so guard with hasattr.
+ if not hasattr(sm, "_extract_api_key_from_request"):
+ logger.debug(
+ "FAB SecurityManager does not have
_extract_api_key_from_request; "
+ "API key authentication is not available in this FAB version"
+ )
+ else:
+ api_key_string = sm._extract_api_key_from_request()
+ if api_key_string is not None:
+ if not hasattr(sm, "validate_api_key"):
+ logger.warning(
+ "FAB SecurityManager does not have validate_api_key; "
+ "cannot validate API key"
+ )
+ raise ValueError(
+ "API key validation is not available in this FAB
version."
+ )
+ user = sm.validate_api_key(api_key_string)
+ if user:
+ # Reload user with all relationships eagerly loaded to
avoid
+ # detached-instance errors during later permission checks.
+ user_with_rels = load_user_with_relationships(
+ username=user.username,
+ )
+ if user_with_rels is None:
+ logger.warning(
+ "Failed to reload API key user %s with
relationships; "
+ "using original user object which may have
lazy-loaded "
+ "relationships",
+ user.username,
+ )
+ return user
+ return user_with_rels
+ raise ValueError(
+ "Invalid or expired API key. "
+ "Create a new key at /api/v1/security/api_keys/."
+ )
Review Comment:
Good catch. Fixed in commit 0359dad808 — changed `raise ValueError(...)` to
`raise PermissionError(...)` for both API key validation failures. This ensures
the `GlobalErrorHandlerMiddleware` routes these through the `isinstance(error,
PermissionError)` branch (returning "Permission denied") instead of the
`ValueError` branch ("Invalid parameter").
##########
superset/migrations/versions/2026-03-13_12-00_f1a2b3c4d5e6_add_fab_api_key_table.py:
##########
@@ -0,0 +1,85 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add FAB api key table
+
+Revision ID: f1a2b3c4d5e6
+Revises: a1b2c3d4e5f6
+Create Date: 2026-02-14 12:00:00.000000
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "f1a2b3c4d5e6"
+down_revision = "a1b2c3d4e5f6"
+
+
+def upgrade() -> None:
+ """Create ab_api_key table for FAB API key authentication.
+
+ This table is managed by FAB's SecurityManager. For fresh installs,
+ FAB's create_all() handles table creation. This migration ensures
+ existing Superset installs get the table on upgrade.
+ """
+ conn = op.get_bind()
+ inspector = sa.inspect(conn)
+ table_exists = "ab_api_key" in inspector.get_table_names()
+
+ if not table_exists:
+ op.create_table(
+ "ab_api_key",
+ sa.Column("id", sa.Integer(), nullable=False),
+ sa.Column("uuid", sa.String(length=36), nullable=False),
+ sa.Column("name", sa.String(length=256), nullable=False),
+ sa.Column("key_hash", sa.String(length=256), nullable=False),
+ sa.Column("key_prefix", sa.String(length=16), nullable=False),
+ sa.Column("user_id", sa.Integer(), nullable=False),
+ sa.Column("scopes", sa.Text(), nullable=True),
+ sa.Column("active", sa.Boolean(), nullable=False,
server_default=sa.true()),
+ sa.Column("created_on", sa.DateTime(), nullable=True),
+ sa.Column("expires_on", sa.DateTime(), nullable=True),
+ sa.Column("revoked_on", sa.DateTime(), nullable=True),
+ sa.Column("last_used_on", sa.DateTime(), nullable=True),
+ sa.ForeignKeyConstraint(["user_id"], ["ab_user.id"],
ondelete="CASCADE"),
+ sa.PrimaryKeyConstraint("id"),
+ sa.UniqueConstraint("uuid"),
+ )
+
+ # Always ensure indexes exist (table may have been created by FAB's
+ # create_all() without these indexes)
+ existing_indexes = (
+ {idx["name"] for idx in inspector.get_indexes("ab_api_key")}
+ if table_exists
+ else set()
+ )
+
+ with op.batch_alter_table("ab_api_key") as batch_op:
+ if "idx_api_key_prefix" not in existing_indexes:
+ batch_op.create_index("idx_api_key_prefix", ["key_prefix"])
+ if "idx_api_key_user_id" not in existing_indexes:
+ batch_op.create_index("idx_api_key_user_id", ["user_id"])
+
+
+def downgrade() -> None:
+ """Drop ab_api_key table if it exists."""
+ conn = op.get_bind()
+ inspector = sa.inspect(conn)
+ if "ab_api_key" not in inspector.get_table_names():
+ return
+ op.drop_table("ab_api_key")
Review Comment:
Good catch. Fixed in commit 0359dad808 — the downgrade() now only drops the
indexes (`idx_api_key_prefix` and `idx_api_key_user_id`) that were
conditionally added by upgrade(), rather than dropping the entire `ab_api_key`
table. This preserves data that FAB's `create_all()` may have already created.
##########
requirements/base.txt:
##########
@@ -120,7 +120,7 @@ flask==2.3.3
# flask-session
# flask-sqlalchemy
# flask-wtf
-flask-appbuilder==5.0.2
+flask-appbuilder @
git+https://github.com/aminghadersohi/Flask-AppBuilder@amin/ch99414/api-key-auth
Review Comment:
The upstream FAB PR has been merged and released as FAB 5.2.0. The
dependency has been updated from the fork to the official release. This is
resolved.
##########
superset-frontend/src/features/apiKeys/ApiKeyList.tsx:
##########
@@ -0,0 +1,233 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useRef, useState } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import { t } from '@apache-superset/core/translation';
+import { css, useTheme } from '@apache-superset/core/theme';
+import {
+ Button,
+ Table,
+ Modal,
+ Tag,
+ Tooltip,
+} from '@superset-ui/core/components';
+import { useToasts } from 'src/components/MessageToasts/withToasts';
+import { ApiKeyCreateModal } from './ApiKeyCreateModal';
+
+export interface ApiKey {
+ uuid: string;
+ name: string;
+ key_prefix: string;
+ active: boolean;
+ created_on: string;
+ expires_on: string | null;
+ revoked_on: string | null;
+ last_used_on: string | null;
+ scopes: string | null;
+}
+
+export function ApiKeyList() {
+ const theme = useTheme();
+ const { addDangerToast, addSuccessToast } = useToasts();
+ const [apiKeys, setApiKeys] = useState<ApiKey[]>([]);
+ const [loading, setLoading] = useState(false);
+ const [showCreateModal, setShowCreateModal] = useState(false);
+ const fetchCounterRef = useRef(0);
+
+ async function fetchApiKeys() {
+ fetchCounterRef.current += 1;
+ const thisRequest = fetchCounterRef.current;
+ setLoading(true);
+ try {
+ const response = await SupersetClient.get({
+ endpoint: '/api/v1/security/api_keys/',
+ });
+ // Only apply results if this is still the most recent request
+ if (thisRequest === fetchCounterRef.current) {
+ setApiKeys(response.json.result || []);
+ }
+ } catch (error) {
+ if (thisRequest === fetchCounterRef.current) {
+ addDangerToast(t('Failed to fetch API keys'));
+ }
+ } finally {
+ if (thisRequest === fetchCounterRef.current) {
+ setLoading(false);
+ }
+ }
+ }
+
+ useEffect(() => {
+ fetchApiKeys();
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);
+
+ function handleRevokeKey(keyUuid: string) {
+ Modal.confirm({
+ title: t('Revoke API Key'),
+ content: t(
+ 'Are you sure you want to revoke this API key? This action cannot be
undone.',
+ ),
+ okText: t('Revoke'),
+ okType: 'danger',
+ cancelText: t('Cancel'),
+ onOk: async () => {
+ try {
+ await SupersetClient.delete({
+ endpoint: `/api/v1/security/api_keys/${keyUuid}`,
+ });
+ addSuccessToast(t('API key revoked successfully'));
+ fetchApiKeys();
+ } catch (error) {
+ addDangerToast(t('Failed to revoke API key'));
+ }
+ },
+ });
+ }
+
+ const formatDate = (dateString: string | null) => {
+ if (!dateString) return '-';
+ return new Date(dateString).toLocaleDateString(undefined, {
+ year: 'numeric',
+ month: 'short',
+ day: 'numeric',
+ });
+ };
+
+ const getStatusBadge = (key: ApiKey) => {
+ if (key.revoked_on) {
+ return <Tag color="error">{t('Revoked')}</Tag>;
+ }
+ if (key.expires_on && new Date(key.expires_on) < new Date()) {
+ return <Tag color="warning">{t('Expired')}</Tag>;
+ }
+ return <Tag color="success">{t('Active')}</Tag>;
+ };
Review Comment:
The `getStatusBadge` function already checks `key.active` at line 120. When
`\!key.active`, it returns an 'Inactive' tag. The flow is: check expiry first,
then check revoked, then check active status, then default to 'Active'. This
covers all states correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
