codeant-ai-for-open-source[bot] commented on code in PR #37973: URL: https://github.com/apache/superset/pull/37973#discussion_r2943432838
########## superset-frontend/src/features/apiKeys/ApiKeyCreateModal.tsx: ########## @@ -0,0 +1,160 @@ +/** + * 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 { 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 { Alert } from '@apache-superset/core/components'; +import { + FormModal, + FormItem, + Input, + Button, + Modal, +} from '@superset-ui/core/components'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; + +interface ApiKeyCreateModalProps { + show: boolean; + onHide: () => void; + onSuccess: () => void; +} + +interface FormValues { + name: string; +} + +export function ApiKeyCreateModal({ + show, + onHide, + onSuccess, +}: ApiKeyCreateModalProps) { + const theme = useTheme(); + const { addDangerToast, addSuccessToast } = useToasts(); + const [createdKey, setCreatedKey] = useState<string | null>(null); + const [copied, setCopied] = useState(false); + + const handleFormSubmit = async (values: FormValues) => { + try { + const response = await SupersetClient.post({ + endpoint: '/api/v1/security/api_keys/', + jsonPayload: values, + }); + const key = response.json?.result?.key; + if (!key) { + throw new Error('API response did not include a key'); + } + setCreatedKey(key); + addSuccessToast(t('API key created successfully')); + } catch (error) { + addDangerToast(t('Failed to create API key')); + throw error; + } + }; + + const handleCopyKey = async () => { + if (!createdKey) { + return; + } + try { + await navigator.clipboard.writeText(createdKey); + setCopied(true); + setTimeout(() => setCopied(false), 2000); Review Comment: **Suggestion:** The copy-status timeout is never cleared, so closing the modal before the timer fires can trigger a state update after unmount, and repeated clicks can race with older timers that reset the copied state too early. Store the timeout id in a ref, clear any existing timer before creating a new one, and clean it up on unmount. [resource leak] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ API key modal copy feedback can reset unpredictably. - ⚠️ Timer callback runs after modal unmount lifecycle. - ⚠️ Affects UserInfo API Keys panel interaction flow. ``` </details> ```suggestion 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 { Alert } from '@apache-superset/core/components'; import { FormModal, FormItem, Input, Button, Modal, } from '@superset-ui/core/components'; import { useToasts } from 'src/components/MessageToasts/withToasts'; interface ApiKeyCreateModalProps { show: boolean; onHide: () => void; onSuccess: () => void; } interface FormValues { name: string; } export function ApiKeyCreateModal({ show, onHide, onSuccess, }: ApiKeyCreateModalProps) { const theme = useTheme(); const { addDangerToast, addSuccessToast } = useToasts(); const [createdKey, setCreatedKey] = useState<string | null>(null); const [copied, setCopied] = useState(false); const copiedResetTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); useEffect(() => { () => { if (copiedResetTimeoutRef.current) { clearTimeout(copiedResetTimeoutRef.current); } }; }, []); const handleFormSubmit = async (values: FormValues) => { try { const response = await SupersetClient.post({ endpoint: '/api/v1/security/api_keys/', jsonPayload: values, }); const key = response.json?.result?.key; if (!key) { throw new Error('API response did not include a key'); } setCreatedKey(key); addSuccessToast(t('API key created successfully')); } catch (error) { addDangerToast(t('Failed to create API key')); throw error; } }; const handleCopyKey = async () => { if (!createdKey) { return; } try { await navigator.clipboard.writeText(createdKey); setCopied(true); if (copiedResetTimeoutRef.current) { clearTimeout(copiedResetTimeoutRef.current); } copiedResetTimeoutRef.current = setTimeout(() => setCopied(false), 2000); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open user profile page where API keys UI is mounted: `src/pages/UserInfo/index.tsx:223-249` renders `<ApiKeyList />` under `FeatureFlag.FabApiKeyEnabled`. 2. In `src/features/apiKeys/ApiKeyList.tsx:223-233`, click "Create API Key" (`line 212`) so `<ApiKeyCreateModal />` mounts conditionally via `showCreateModal && ...`. 3. Create a key, then click "Copy" in `src/features/apiKeys/ApiKeyCreateModal.tsx:71-79`; this schedules `setTimeout(() => setCopied(false), 2000)` at `line 78`. 4. Close modal immediately ("Done"/hide path): `handleClose` at `ApiKeyCreateModal.tsx:84-91` calls `onHide`, parent sets `setShowCreateModal(false)` (`ApiKeyList.tsx:226-228`) and unmounts modal (`line 223` condition false); pending timer still fires later and performs stale `setCopied(false)` against unmounted instance. Repeated copy clicks within 2s also leave multiple timers, causing premature copied-state reset races. ``` </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/ApiKeyCreateModal.tsx **Line:** 19:78 **Comment:** *Resource Leak: The copy-status timeout is never cleared, so closing the modal before the timer fires can trigger a state update after unmount, and repeated clicks can race with older timers that reset the copied state too early. Store the timeout id in a ref, clear any existing timer before creating a new one, and clean it up on unmount. 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=17a2a1703504baab4da54948ecc1d9f574d64184d15f8513037946852af98474&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=17a2a1703504baab4da54948ecc1d9f574d64184d15f8513037946852af98474&reaction=dislike'>👎</a> ########## superset/migrations/versions/2026-03-13_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"]) Review Comment: **Suggestion:** Returning early when `ab_api_key` already exists skips index creation entirely, so upgraded environments with a pre-created table may miss required indexes and suffer full table scans for key lookups. Create the table only when missing, but always ensure both indexes exist. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ API key lookups may scan large authentication table. - ⚠️ Slower protected API requests under heavy traffic. - ⚠️ Upgrade path inconsistent for pre-created FAB tables. ``` </details> ```suggestion if "ab_api_key" not in inspector.get_table_names(): 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"), ) inspector = sa.inspect(op.get_bind()) existing_indexes = {idx["name"] for idx in inspector.get_indexes("ab_api_key")} 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"]) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Use an environment where `ab_api_key` already exists before migration (explicitly described at `superset/migrations/versions/2026-03-13_12-00_f1a2b3c4d5e6_add_fab_api_key_table.py:36-38`). 2. Run upgrade; `upgrade()` returns immediately at `:42-43`, so index creation at `:64-66` never executes. 3. Resulting schema can miss `idx_api_key_prefix` / `idx_api_key_user_id` because this migration never verifies existing indexes (no `get_indexes()` logic in file). 4. API-key-protected traffic is broad (`@protect()` appears across many API modules; grep count shows 113 matches), so missing lookup indexes can degrade auth-query latency under load. ``` </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-03-13_12-00_f1a2b3c4d5e6_add_fab_api_key_table.py **Line:** 42:66 **Comment:** *Performance: Returning early when `ab_api_key` already exists skips index creation entirely, so upgraded environments with a pre-created table may miss required indexes and suffer full table scans for key lookups. Create the table only when missing, but always ensure both indexes exist. 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=674fefdaa2b8d6af1fabe5701db41b8017bd4c177a206fea75841874fabf1cfc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=674fefdaa2b8d6af1fabe5701db41b8017bd4c177a206fea75841874fabf1cfc&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]
