codeant-ai-for-open-source[bot] commented on code in PR #37973: URL: https://github.com/apache/superset/pull/37973#discussion_r2910647663
########## superset-frontend/src/features/apiKeys/ApiKeyList.tsx: ########## @@ -0,0 +1,224 @@ +/** + * 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); + } + } + + 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>; + }; + + const columns = [ + { + title: t('Name'), + dataIndex: 'name', + key: 'name', + }, + { + title: t('Key Prefix'), + dataIndex: 'key_prefix', + key: 'key_prefix', + render: (prefix: string) => ( + <code + css={css` + background: ${theme.colorFillSecondary}; + padding: 2px 6px; + border-radius: 3px; + `} + > + {prefix}... + </code> + ), + }, + { + title: t('Created'), + dataIndex: 'created_on', + key: 'created_on', + render: formatDate, + }, + { + title: t('Last Used'), + dataIndex: 'last_used_on', + key: 'last_used_on', + render: formatDate, + }, + { + title: t('Status'), + key: 'status', + render: (_: unknown, record: ApiKey) => getStatusBadge(record), + }, + { + title: t('Actions'), + key: 'actions', + render: (_: unknown, record: ApiKey) => ( + <> + {!record.revoked_on && ( + <Tooltip title={t('Revoke this API key')}> + <Button + type="link" + danger + onClick={() => handleRevokeKey(record.uuid)} + > + {t('Revoke')} + </Button> + </Tooltip> + )} + </> + ), + }, + ]; + + return ( + <div> + <div + css={css` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 4}px; + `} + > + <div> + <p + css={css` + margin-bottom: ${theme.sizeUnit * 2}px; + `} + > + {t('API keys allow scoped programmatic access to Superset.')} + </p> + <p + css={css` + margin-bottom: 0; + `} + > + {t('Keys are shown only once at creation. Store them securely.')} + </p> + </div> + <Button type="primary" onClick={() => setShowCreateModal(true)}> + {t('Create API Key')} + </Button> + </div> + <Table + columns={columns} + data={apiKeys} + loading={loading} + rowKey="uuid" + pagination={{ pageSize: 10 }} + /> + {showCreateModal && ( + <ApiKeyCreateModal + show={showCreateModal} + onHide={() => { + setShowCreateModal(false); + fetchApiKeys(); + }} + onSuccess={() => { + fetchApiKeys(); + }} + /> + )} Review Comment: **Suggestion:** The API key list is fetched twice whenever a newly created key modal is closed (once via `onSuccess` and once via `onHide`), causing duplicate network requests and redundant state updates every time a key is created. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ API key creation triggers duplicate list refresh requests. - ⚠️ Extra load on /api/v1/security/api_keys/ endpoint. ``` </details> ```suggestion {showCreateModal && ( <ApiKeyCreateModal show={showCreateModal} onHide={() => { setShowCreateModal(false); }} onSuccess={() => { fetchApiKeys(); }} /> )} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render the `ApiKeyList` component from `superset-frontend/src/features/apiKeys/ApiKeyList.tsx:45` in the frontend (the API keys management UI). 2. Click the "Create API Key" button defined at `ApiKeyList.tsx:199-201`, which sets `showCreateModal` to true and renders `ApiKeyCreateModal` at `ApiKeyList.tsx:210-221`. 3. In `ApiKeyCreateModal` (`superset-frontend/src/features/apiKeys/ApiKeyCreateModal.tsx:42-85`), submit the form successfully so `handleFormSubmit` at lines 52-58 sets `createdKey`, then close the "API Key Created" modal (click Done or close icon), triggering `handleClose` at lines 78-85. 4. `handleClose` sees `createdKey` is truthy and calls `onSuccess()` (wired to `fetchApiKeys` at `ApiKeyList.tsx:217-218`), then calls `onHide()` (wired to a closure that also calls `fetchApiKeys` at `ApiKeyList.tsx:213-216`), causing two consecutive `SupersetClient.get` calls to `/api/v1/security/api_keys/` via `fetchApiKeys` at `ApiKeyList.tsx:52-58` for a single modal close. ``` </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:** 210:221 **Comment:** *Performance: The API key list is fetched twice whenever a newly created key modal is closed (once via `onSuccess` and once via `onHide`), causing duplicate network requests and redundant state updates every time a key is created. 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=b901824ba992c2fd7f5d257efcddb7e629fc47c07045ccda1f5df0a475b42a7e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=b901824ba992c2fd7f5d257efcddb7e629fc47c07045ccda1f5df0a475b42a7e&reaction=dislike'>👎</a> ########## superset/migrations/versions/2026-02-14_12-00_f1a2b3c4d5e6_add_fab_api_key_table.py: ########## @@ -0,0 +1,73 @@ +# 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.text("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.""" + op.drop_table("ab_api_key") Review Comment: **Suggestion:** The downgrade step unconditionally drops the `ab_api_key` table, even though the upgrade is explicitly guarded to only create it when it does not already exist; this means that on installations where FAB has already created `ab_api_key` (via `create_all()`), applying this migration and then downgrading will delete a table and data that were not created by this migration, and can also cause errors if the table was removed independently. To align downgrade behavior with upgrade and avoid unintended data loss or errors, the downgrade should first check whether `ab_api_key` exists before attempting to drop it. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Downgrading can drop FAB-managed `ab_api_key` table unexpectedly. - ❌ Loss of all stored API keys for affected deployment. - ⚠️ API key-based authentication broken after downgrade rollback. - ⚠️ Manual data restoration required to recover lost API keys. ``` </details> ```suggestion conn = op.get_bind() inspector = sa.inspect(conn) if "ab_api_key" not in inspector.get_table_names(): return ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Deploy Superset with the FAB version where the `ab_api_key` table is managed/created by FAB's `SecurityManager.create_all()` (as documented in this migration's docstring at `superset/migrations/versions/2026-02-14_12-00_f1a2b3c4d5e6_add_fab_api_key_table.py:34-38`, which states "This table is managed by FAB's SecurityManager. For fresh installs, FAB's create_all() handles table creation."). 2. Run `superset db upgrade` to bring the database to revision `f1a2b3c4d5e6` (the revision ID declared at lines 19-21 and 29 in the same migration file). During `upgrade()` (lines 33-68), the migration obtains an inspector and returns early at line 42 if `"ab_api_key"` is already in `inspector.get_table_names()`, so it does not create the table in this scenario and the existing FAB-managed `ab_api_key` table (with any API key data) remains. 3. Later, follow the documented downgrade workflow using `superset db downgrade` (this CLI command is referenced in the docs at `docs/developer_docs/contributing/howtos.md:492` and `docs/versioned_docs/version-6.0.0/contributing/development.mdx:1010`, as found via Grep), targeting a revision before `f1a2b3c4d5e6` (e.g., `superset db downgrade a1b2c3d4e5f6` or `superset db downgrade -1`). 4. Alembic executes this migration's `downgrade()` function at lines 71-73 in `superset/migrations/versions/2026-02-14_12-00_f1a2b3c4d5e6_add_fab_api_key_table.py`, which unconditionally calls `op.drop_table("ab_api_key")` even though `upgrade()` may not have created the table; as a result, the pre-existing FAB-managed `ab_api_key` table and all stored API key data are dropped as part of the downgrade. ``` </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:** 73:73 **Comment:** *Logic Error: The downgrade step unconditionally drops the `ab_api_key` table, even though the upgrade is explicitly guarded to only create it when it does not already exist; this means that on installations where FAB has already created `ab_api_key` (via `create_all()`), applying this migration and then downgrading will delete a table and data that were not created by this migration, and can also cause errors if the table was removed independently. To align downgrade behavior with upgrade and avoid unintended data loss or errors, the downgrade should first check whether `ab_api_key` exists before attempting to drop it. 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=14d4e9a46a0a53c56a7ebe163a1eb826b651cad606549b7d8e1fbacc0dd39c94&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=14d4e9a46a0a53c56a7ebe163a1eb826b651cad606549b7d8e1fbacc0dd39c94&reaction=dislike'>👎</a> ########## superset-frontend/src/features/apiKeys/ApiKeyList.tsx: ########## @@ -0,0 +1,224 @@ +/** + * 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); + } + } + + 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>; + }; + + const columns = [ + { + title: t('Name'), + dataIndex: 'name', + key: 'name', + }, + { + title: t('Key Prefix'), + dataIndex: 'key_prefix', + key: 'key_prefix', + render: (prefix: string) => ( + <code + css={css` + background: ${theme.colorFillSecondary}; + padding: 2px 6px; + border-radius: 3px; + `} + > + {prefix}... + </code> + ), + }, + { + title: t('Created'), + dataIndex: 'created_on', + key: 'created_on', + render: formatDate, + }, + { + title: t('Last Used'), + dataIndex: 'last_used_on', + key: 'last_used_on', + render: formatDate, + }, + { + title: t('Status'), + key: 'status', + render: (_: unknown, record: ApiKey) => getStatusBadge(record), + }, + { + title: t('Actions'), + key: 'actions', + render: (_: unknown, record: ApiKey) => ( + <> + {!record.revoked_on && ( + <Tooltip title={t('Revoke this API key')}> + <Button + type="link" + danger + onClick={() => handleRevokeKey(record.uuid)} + > + {t('Revoke')} + </Button> + </Tooltip> + )} + </> + ), + }, + ]; + + return ( + <div> + <div + css={css` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 4}px; + `} + > + <div> + <p + css={css` + margin-bottom: ${theme.sizeUnit * 2}px; + `} + > + {t('API keys allow scoped programmatic access to Superset.')} + </p> + <p + css={css` + margin-bottom: 0; + `} + > + {t('Keys are shown only once at creation. Store them securely.')} + </p> + </div> + <Button type="primary" onClick={() => setShowCreateModal(true)}> + {t('Create API Key')} + </Button> + </div> + <Table + columns={columns} + data={apiKeys} + loading={loading} + rowKey="uuid" + pagination={{ pageSize: 10 }} + /> Review Comment: **Suggestion:** The table component is given a `data` prop instead of the expected `dataSource`, so even though API keys are loaded into state, they will not be rendered in the table, making the list appear empty. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ API keys page shows empty table despite existing keys. - ⚠️ Users cannot revoke or manage existing API keys. ``` </details> ```suggestion <Table columns={columns} dataSource={apiKeys} loading={loading} rowKey="uuid" pagination={{ pageSize: 10 }} /> ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render the `ApiKeyList` component from `superset-frontend/src/features/apiKeys/ApiKeyList.tsx:45` in the Superset frontend. 2. On mount, `useEffect` at `ApiKeyList.tsx:66-69` calls `fetchApiKeys`, which issues `SupersetClient.get({ endpoint: '/api/v1/security/api_keys/' })` at lines 52-57 and receives one or more API keys in `response.json.result`. 3. `fetchApiKeys` stores the array in state via `setApiKeys(response.json.result || [])` at `ApiKeyList.tsx:58`, and React re-renders with `apiKeys` populated. 4. The `<Table>` from `@superset-ui/core/components` is rendered at `ApiKeyList.tsx:203-209` with `data={apiKeys}` instead of the `dataSource` prop used consistently for tables elsewhere in the codebase (e.g., many `dataSource={...}` usages found via Grep), so the table does not bind the rows and the API keys list appears empty despite state containing data. ``` </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:** 203:209 **Comment:** *Logic Error: The table component is given a `data` prop instead of the expected `dataSource`, so even though API keys are loaded into state, they will not be rendered in the table, making the list appear empty. 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=30bcecb485fbe7e751793c181833deaed5999b5876511cade56c605fd174a428&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37973&comment_hash=30bcecb485fbe7e751793c181833deaed5999b5876511cade56c605fd174a428&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]
