aminghadersohi commented on code in PR #37973: URL: https://github.com/apache/superset/pull/37973#discussion_r2932095852
########## superset-frontend/src/features/apiKeys/ApiKeyList.tsx: ########## @@ -0,0 +1,223 @@ +/** + * 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, 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); + + async function fetchApiKeys() { + setLoading(true); + try { + const response = await SupersetClient.get({ + endpoint: '/api/v1/security/api_keys/', + }); + setApiKeys(response.json.result || []); + } catch (error) { + addDangerToast(t('Failed to fetch API keys')); + } finally { + setLoading(false); + } Review Comment: The fetchApiKeys call is triggered by a single useEffect on mount and by explicit user actions (create/delete). There's no path where concurrent calls happen naturally. The loading state correctly wraps the async operation. If this becomes an issue with future features, we can add an AbortController pattern, but for the current usage it's safe. ########## superset/mcp_service/auth.py: ########## @@ -192,6 +209,41 @@ 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) + # Avoid circular import: superset/__init__.py imports create_app which + # depends on the MCP service module tree during app initialization. + from superset import is_feature_enabled + + if is_feature_enabled("FAB_API_KEY_ENABLED") and has_request_context(): Review Comment: Good observation. The MCP auth layer checks `is_feature_enabled('FAB_API_KEY_ENABLED')` which reads from Superset's feature flags config. The FAB backend validation uses the Flask config key directly. Both are set through `superset_config.py` so they're consistent in practice. The feature flag check in MCP auth is an additional guard — if the flag is off, MCP won't attempt API key auth at all, which is the desired behavior. ########## superset/mcp_service/auth.py: ########## @@ -192,6 +209,41 @@ 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) + # Avoid circular import: superset/__init__.py imports create_app which + # depends on the MCP service module tree during app initialization. + from superset import is_feature_enabled + + if is_feature_enabled("FAB_API_KEY_ENABLED") 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. + # No public API is exposed for this; see FAB SecurityManager. + api_key_string = sm._extract_api_key_from_request() Review Comment: The code already wraps the call in a try/except that catches AttributeError — if the method doesn't exist in a given FAB version, it falls through to the next auth method gracefully. See the auth chain fallback logic. ########## superset/migrations/versions/2026-02-14_12-00_f1a2b3c4d5e6_add_fab_api_key_table.py: ########## @@ -0,0 +1,75 @@ +# 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) + if "ab_api_key" in inspector.get_table_names(): + return + + 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"), + ) + + with op.batch_alter_table("ab_api_key") as batch_op: + batch_op.create_index("idx_api_key_prefix", ["key_prefix"]) + 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: The downgrade path uses `DROP TABLE IF EXISTS` which is idempotent. If the table was created by FAB's `create_all()` before the migration ran, the upgrade exits early without claiming ownership, and the downgrade's `IF EXISTS` check means it only drops the table if it's there. This aligns with Superset's migration conventions. ########## 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 + }, []); Review Comment: This is a valid concern for strict React patterns. The component is a page-level route component that stays mounted for its entire lifecycle, so unmount-during-fetch is unlikely in practice. That said, the React 18+ strict mode double-mount pattern could trigger this in dev. If it becomes an issue, we can add a cleanup ref. -- 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]
