codeant-ai-for-open-source[bot] commented on code in PR #37973: URL: https://github.com/apache/superset/pull/37973#discussion_r2930449578
########## 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: **Suggestion:** Concurrent `fetchApiKeys` calls can resolve out of order and overwrite newer data with stale results, and they also toggle loading state incorrectly when requests overlap. Track the latest request and only apply `setApiKeys`/`setLoading(false)` for the most recent call. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ API key list can revert to stale data. - ⚠️ Loading spinner can hide during active fetch. - ⚠️ Create/revoke feedback may not match displayed table. ``` </details> ```suggestion const [apiKeys, setApiKeys] = useState<ApiKey[]>([]); const [loading, setLoading] = useState(false); const [showCreateModal, setShowCreateModal] = useState(false); const latestFetchIdRef = useRef(0); async function fetchApiKeys() { const fetchId = ++latestFetchIdRef.current; setLoading(true); try { const response = await SupersetClient.get({ endpoint: '/api/v1/security/api_keys/', }); if (fetchId === latestFetchIdRef.current) { setApiKeys(response.json.result || []); } } catch (error) { addDangerToast(t('Failed to fetch API keys')); } finally { if (fetchId === latestFetchIdRef.current) { setLoading(false); } } } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `/user_info/` route (registered at `superset-frontend/src/views/routes.tsx:25` in the shown snippet, path `/user_info/`), which lazy-loads `src/pages/UserInfo` and renders `ApiKeyList` when `FeatureFlag.FabApiKeyEnabled` is true (`superset-frontend/src/pages/UserInfo/index.tsx:216-221`). 2. On mount, `ApiKeyList` immediately starts request A via `useEffect -> fetchApiKeys()` (`ApiKeyList.tsx:66-68`, request at `55-57`). 3. Before request A finishes (slow network), create a key from the same view: click "Create API Key" (`ApiKeyList.tsx:199-201`), submit modal (`ApiKeyCreateModal.tsx:52-57`), then close modal; this triggers `onSuccess()` (`ApiKeyCreateModal.tsx:78-81`) which calls `fetchApiKeys()` again as request B (`ApiKeyList.tsx:216-218`). 4. If request B returns first, list is updated; if slower request A returns afterward, `setApiKeys(...)` (`ApiKeyList.tsx:58`) overwrites newer data with stale results, and each request independently runs `setLoading(false)` (`ApiKeyList.tsx:62`), so spinner can clear while another fetch is still in flight. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/apiKeys/ApiKeyList.tsx **Line:** 48:63 **Comment:** *Race Condition: Concurrent `fetchApiKeys` calls can resolve out of order and overwrite newer data with stale results, and they also toggle loading state incorrectly when requests overlap. Track the latest request and only apply `setApiKeys`/`setLoading(false)` for the most recent call. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=5e29328c92b101611e5404b820df0301ce475213770e096147f78d1f14cf2f52&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=5e29328c92b101611e5404b820df0301ce475213770e096147f78d1f14cf2f52&reaction=dislike'>👎</a> ########## 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: **Suggestion:** API key auth is gated by `is_feature_enabled("FAB_API_KEY_ENABLED")`, but backend API-key validation in Superset/FAB is controlled by the Flask config key `FAB_API_KEY_ENABLED`. This can disable API key authentication when backend is enabled but the UI feature flag is off, causing valid API keys to be rejected. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ MCP API-key auth fails with split config states. - ⚠️ Protected tools reject valid automation credentials. ``` </details> ```suggestion if current_app.config.get("FAB_API_KEY_ENABLED", False) and has_request_context(): ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure Superset with backend API keys enabled but UI flag disabled (`superset/config.py:1646-1650` shows backend `FAB_API_KEY_ENABLED`, while `superset/config.py:569` keeps feature-flag value in `DEFAULT_FEATURE_FLAGS` separately). 2. Call any protected MCP tool, e.g. `list_charts` (`superset/mcp_service/chart/tool/list_charts.py:66`), which is wrapped by auth because `protect=True` by default (`superset-core/src/superset_core/mcp/decorators.py:11`) and injected via `mcp_auth_hook` (`superset/core/mcp/core_mcp_injection.py:119-122`). 3. Request enters `mcp_auth_hook` (`superset/mcp_service/auth.py:357`) → `_setup_user_context()` (`auth.py:318`) → `get_user_from_request()` (`auth.py:191`). 4. API-key branch is skipped because code checks `is_feature_enabled("FAB_API_KEY_ENABLED")` (`auth.py:219`), which reads feature flags (`superset/utils/feature_flag_manager.py:47-58`) instead of backend config; then code falls through to `ValueError` "No authenticated user found" (`auth.py:267-270`) and rejects a valid API key. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/auth.py **Line:** 217:219 **Comment:** *Logic Error: API key auth is gated by `is_feature_enabled("FAB_API_KEY_ENABLED")`, but backend API-key validation in Superset/FAB is controlled by the Flask config key `FAB_API_KEY_ENABLED`. This can disable API key authentication when backend is enabled but the UI feature flag is off, causing valid API keys to be rejected. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=b4f23107e9a5a2d9b595fbd4ecbb4856cbd1a55d923987f867a0f6ac856a2004&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=b4f23107e9a5a2d9b595fbd4ecbb4856cbd1a55d923987f867a0f6ac856a2004&reaction=dislike'>👎</a> ########## 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: **Suggestion:** The direct call to private method `_extract_api_key_from_request` assumes a specific FAB version and will raise `AttributeError` at runtime when that method is unavailable, causing authentication to crash instead of returning a controlled auth failure. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ MCP request can crash on FAB API mismatch. - ⚠️ Operators get traceback, not actionable auth guidance. ``` </details> ```suggestion extract_api_key = getattr(sm, "_extract_api_key_from_request", None) if extract_api_key is None: raise ValueError( "API key authentication is unavailable because the installed " "Flask-AppBuilder version does not support API key extraction." ) api_key_string = extract_api_key() ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run MCP with API-key path enabled so `get_user_from_request()` enters the API-key branch (`superset/mcp_service/auth.py:219-226`), then invoke a protected tool (auth wrapper path: `superset/core/mcp/core_mcp_injection.py:119-122`, `auth.py:395-397`). 2. Use an environment where SecurityManager lacks `_extract_api_key_from_request` (this is a private FAB API, while Superset only pins `flask-appbuilder==5.2.0` in `requirements/base.txt:123`; PR text also indicates dependency on an external FAB branch). 3. `sm._extract_api_key_from_request()` at `auth.py:224` raises `AttributeError`; there is no local compatibility guard before calling it. 4. Error propagates out of `_setup_user_context()` (`auth.py:326` catches only `RuntimeError`), so MCP auth fails with an unhandled exception instead of a controlled auth failure message. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/auth.py **Line:** 224:224 **Comment:** *Possible Bug: The direct call to private method `_extract_api_key_from_request` assumes a specific FAB version and will raise `AttributeError` at runtime when that method is unavailable, causing authentication to crash instead of returning a controlled auth failure. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=a33c6c7a8db11196f879b3ecce08d8fffb4746056690ba52075bd65e3375aebe&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=a33c6c7a8db11196f879b3ecce08d8fffb4746056690ba52075bd65e3375aebe&reaction=dislike'>👎</a> ########## 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: **Suggestion:** The downgrade path can drop a table that this migration may not have created. In `upgrade()`, the migration exits early when `ab_api_key` already exists (for example, created by FAB's `create_all()`), but `downgrade()` still drops it whenever present. That can cause unintended data loss and break authentication state on downgrade. Make this downgrade a no-op (or otherwise track ownership) so pre-existing FAB-managed tables are not deleted. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Downgrade can delete existing FAB API key records. - ❌ API-key authentication state lost after rollback. - ⚠️ Rollback safety reduced for mixed-managed installations. ``` </details> ```suggestion """No-op: avoid dropping FAB-managed ab_api_key table.""" return ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Initialize Superset app where DB migrations are enabled via `migrate.init_app(..., directory=APP_DIR + "/migrations")` in `superset/initialization/__init__.py:942`, which exposes Alembic upgrade/downgrade flows. 2. Run migration upgrade to revision `f1a2b3c4d5e6`; in `upgrade()` at `superset/migrations/versions/2026-02-14_12-00_f1a2b3c4d5e6_add_fab_api_key_table.py:42-43`, it returns early if `ab_api_key` already exists. 3. Because the same file documents this table as FAB-managed API key storage (`...:34-38`), existing environments can have `ab_api_key` created outside this migration path. 4. Execute a rollback (`flask db downgrade`/Alembic downgrade) to parent revision; `downgrade()` in the same file at `:73-75` drops `ab_api_key` whenever present, even if `upgrade()` did not create it. 5. Observe table/data loss after downgrade: API key records disappear, breaking FAB API-key authentication state until recreated. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/migrations/versions/2026-02-14_12-00_f1a2b3c4d5e6_add_fab_api_key_table.py **Line:** 70:75 **Comment:** *Logic Error: The downgrade path can drop a table that this migration may not have created. In `upgrade()`, the migration exits early when `ab_api_key` already exists (for example, created by FAB's `create_all()`), but `downgrade()` still drops it whenever present. That can cause unintended data loss and break authentication state on downgrade. Make this downgrade a no-op (or otherwise track ownership) so pre-existing FAB-managed tables are not deleted. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=d96649715b5aabc3ba9b207f333723c138e856bcf92224cbedea9d454fd8699b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=d96649715b5aabc3ba9b207f333723c138e856bcf92224cbedea9d454fd8699b&reaction=dislike'>👎</a> -- 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]
