diff --git a/pyproject.toml b/pyproject.toml index 38dbe8508e7f..7fcd80941400 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -147,6 +147,7 @@ exasol = ["sqlalchemy-exasol >= 2.4.0, < 8.0"] excel = ["xlrd>=1.2.0, <1.3"] fastmcp = [ "fastmcp>=3.2.4,<4.0", + "joserfc>=1.0.0,<2.0", # tiktoken backs the response-size-guard token estimator. Without # it, the middleware falls back to a coarser character-based # heuristic that under-counts JSON-heavy MCP responses. diff --git a/requirements/development.txt b/requirements/development.txt index bfade642b4a1..f765d445e529 100644 --- a/requirements/development.txt +++ b/requirements/development.txt @@ -183,6 +183,7 @@ cryptography==46.0.7 # -c requirements/base-constraint.txt # apache-superset # authlib + # joserfc # paramiko # pyjwt # pyopenssl @@ -471,6 +472,8 @@ jmespath==1.1.0 # via # boto3 # botocore +joserfc==1.6.8 + # via apache-superset jsonpath-ng==1.7.0 # via # -c requirements/base-constraint.txt diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index ce07d5ac4762..e75f2d3b9849 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -49289,7 +49289,7 @@ "dependencies": { "@ant-design/icons": "^6.2.3", "@apache-superset/core": "*", - "@babel/runtime": "^7.29.2", + "@babel/runtime": "^7.29.7", "@types/json-bigint": "^1.0.4", "@visx/responsive": "^3.12.0", "ace-builds": "^1.44.0", @@ -49420,6 +49420,15 @@ "react-dom": ">=18.0.0" } }, + "packages/superset-ui-core/node_modules/@babel/runtime": { + "version": "7.29.7", + "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.29.7.tgz", + "integrity": "sha512-Nq8OhGWiZIZGV6hLHoyAKLLcJihP/xFeBMGJoUrxTX2psI8dCifzLhZISFb+VWS3wFMRDmCGw5R+dOySCqPLhw==", + "license": "MIT", + "engines": { + "node": ">=6.9.0" + } + }, "packages/superset-ui-core/node_modules/d3-format": { "version": "3.1.2", "resolved": "https://registry.npmjs.org/d3-format/-/d3-format-3.1.2.tgz", diff --git a/superset-frontend/packages/superset-ui-core/package.json b/superset-frontend/packages/superset-ui-core/package.json index 8571a5cc224d..8b552c2dbb6d 100644 --- a/superset-frontend/packages/superset-ui-core/package.json +++ b/superset-frontend/packages/superset-ui-core/package.json @@ -26,7 +26,7 @@ "dependencies": { "@ant-design/icons": "^6.2.3", "@apache-superset/core": "*", - "@babel/runtime": "^7.29.2", + "@babel/runtime": "^7.29.7", "@types/json-bigint": "^1.0.4", "@visx/responsive": "^3.12.0", "ace-builds": "^1.44.0", diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/DatabaseConnectionForm.test.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/DatabaseConnectionForm.test.tsx new file mode 100644 index 000000000000..0c7ef6b683d1 --- /dev/null +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/DatabaseConnectionForm.test.tsx @@ -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. + */ +import { + ConfigurationMethod, + DatabaseObject, + Engines, +} from 'src/features/databases/types'; +import { computeInitialIsPublic } from './index'; + +const baseDb: Partial = { + configuration_method: ConfigurationMethod.DynamicForm, + database_name: 'test', + driver: 'apsw', + id: 1, + name: 'test', + is_managed_externally: false, + engine: Engines.GSheet, +}; + +test('computeInitialIsPublic: returns true when database is null/undefined', () => { + expect(computeInitialIsPublic(null)).toBe(true); + expect(computeInitialIsPublic(undefined)).toBe(true); +}); + +test('computeInitialIsPublic: returns true for non-gsheets engines', () => { + expect( + computeInitialIsPublic({ ...baseDb, engine: 'postgres' as string }), + ).toBe(true); +}); + +test('computeInitialIsPublic: returns true for fresh gsheets connections', () => { + expect(computeInitialIsPublic({ ...baseDb })).toBe(true); + expect( + computeInitialIsPublic({ ...baseDb, masked_encrypted_extra: '{}' }), + ).toBe(true); +}); + +test('computeInitialIsPublic: returns false when masked_encrypted_extra has content', () => { + expect( + computeInitialIsPublic({ + ...baseDb, + masked_encrypted_extra: JSON.stringify({ + service_account_info: { type: 'service_account' }, + }), + }), + ).toBe(false); +}); + +test('computeInitialIsPublic: returns false when parameters.service_account_info is set', () => { + expect( + computeInitialIsPublic({ + ...baseDb, + parameters: { service_account_info: '{"key":"value"}' }, + }), + ).toBe(false); +}); + +test('computeInitialIsPublic: returns false when parameters.oauth2_client_info is set (OAuth2-only edit)', () => { + expect( + computeInitialIsPublic({ + ...baseDb, + parameters: { + // oauth2_client_info isn't in DatabaseParameters typing yet; this + // mirrors how an OAuth2-only edit-mode payload can arrive. + oauth2_client_info: { id: 'client-id' }, + } as DatabaseObject['parameters'], + }), + ).toBe(false); +}); diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.test.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.test.tsx index ce12f75945fe..1839aaf23a49 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.test.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.test.tsx @@ -52,6 +52,7 @@ describe('EncryptedField', () => { const createMockChangeMethods = () => ({ onEncryptedExtraInputChange: jest.fn(), + onClearEncryptedExtraKey: jest.fn(), onParametersChange: jest.fn(), onChange: jest.fn(), onQueryChange: jest.fn(), @@ -92,7 +93,12 @@ describe('EncryptedField', () => { isValidating: false, isEditMode: false, editNewDb: false, - db: createMockDb('gsheets'), + // Default to bigquery so existing credential-UI assertions aren't + // affected by the gsheets-specific public/private dropdown. New tests + // below override the engine to 'gsheets' to cover the dropdown gating. + db: createMockDb('bigquery'), + isPublic: false, + setIsPublic: jest.fn(), }; // Use actual encryptedCredentialsMap for data-driven tests @@ -124,22 +130,32 @@ describe('EncryptedField', () => { expect(() => render()).not.toThrow(); - expectParametersChange(props.changeMethods, undefined, ''); - expect(props.changeMethods.onParametersChange).toHaveBeenCalledTimes(1); + // No engine-specific field name → mount effect skips the clear so it + // doesn't write `parameters[undefined] = ''` to state. + expect(props.changeMethods.onParametersChange).not.toHaveBeenCalled(); }); test.each([ - ['null engine', null, null], - ['undefined engine', undefined, undefined], - ['empty string engine', '', ''], - ])('handles %s gracefully', (_description, engine, expectedName) => { + ['null engine', null], + ['undefined engine', undefined], + ['empty string engine', ''], + ])('handles %s gracefully', (_description, engine) => { const mockDb = createMockDb(engine); const props = { ...defaultProps, db: mockDb }; expect(() => render()).not.toThrow(); - expectParametersChange(props.changeMethods, expectedName, ''); - expect(props.changeMethods.onParametersChange).toHaveBeenCalledTimes(1); + expect(props.changeMethods.onParametersChange).not.toHaveBeenCalled(); + }); + + test('does not call onParametersChange when db is undefined (async load)', () => { + const props = { ...defaultProps, db: undefined }; + + expect(() => render()).not.toThrow(); + + // Async edit-mode load: db hasn't arrived yet. The mount effect must + // NOT race with the incoming credentials by clearing them. + expect(props.changeMethods.onParametersChange).not.toHaveBeenCalled(); }); }); @@ -300,7 +316,7 @@ describe('EncryptedField', () => { expectParametersChange( props.changeMethods, - 'service_account_info', // gsheets default + 'credentials_info', // bigquery default '', ); }); @@ -328,8 +344,9 @@ describe('EncryptedField', () => { expect(() => render()).not.toThrow(); - // Should still render the upload UI with undefined field name - expectParametersChange(props.changeMethods, undefined, ''); + // Mount effect skips the parameters clear when there's no + // engine-specific field name to write to. + expect(props.changeMethods.onParametersChange).not.toHaveBeenCalled(); }); test('renders gracefully with malformed database parameters', () => { @@ -357,7 +374,7 @@ describe('EncryptedField', () => { expect(screen.getByText('Service Account')).toBeInTheDocument(); const textarea = screen.getByRole('textbox'); - expect(textarea).toHaveAttribute('name', 'service_account_info'); + expect(textarea).toHaveAttribute('name', 'credentials_info'); expect(textarea).toHaveAttribute( 'placeholder', 'Paste content of service credentials JSON file here', @@ -379,4 +396,109 @@ describe('EncryptedField', () => { expect(select).toBeInTheDocument(); }); }); + + // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks + describe('Google Sheets public/private dropdown', () => { + const gsheetsProps = { + ...defaultProps, + db: createMockDb('gsheets'), + }; + + test('renders the dropdown for gsheets', () => { + render(); + + expect( + screen.getByText('Type of Google Sheets allowed'), + ).toBeInTheDocument(); + expect( + screen.getByText('Publicly shared sheets only'), + ).toBeInTheDocument(); + }); + + test('does not render the dropdown for non-gsheets engines', () => { + render(); + + expect( + screen.queryByText('Type of Google Sheets allowed'), + ).not.toBeInTheDocument(); + }); + + test('hides credential inputs when isPublic is true', () => { + render(); + + expect( + screen.queryByText( + 'How do you want to enter service account credentials?', + ), + ).not.toBeInTheDocument(); + expect(screen.queryByText('Upload credentials')).not.toBeInTheDocument(); + expect(screen.queryByText('Service Account')).not.toBeInTheDocument(); + }); + + test('shows credential inputs when isPublic is false', () => { + render(); + + expect( + screen.getByText( + 'How do you want to enter service account credentials?', + ), + ).toBeInTheDocument(); + expect(screen.getByText('Upload credentials')).toBeInTheDocument(); + }); + + test('hides credential textarea in edit mode when isPublic is true', () => { + render(); + + expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + expect(screen.queryByText('Service Account')).not.toBeInTheDocument(); + }); + + test('toggling back to public clears stored credentials', () => { + const setIsPublic = jest.fn(); + const changeMethods = createMockChangeMethods(); + render( + , + ); + + const dropdown = screen.getByText('Public and privately shared sheets'); + fireEvent.mouseDown(dropdown); + fireEvent.click(screen.getByText('Publicly shared sheets only')); + + expect(setIsPublic).toHaveBeenCalledWith(true); + + // Clears in-flight `parameters.*` so the save-time merge does nothing. + expect(changeMethods.onParametersChange).toHaveBeenCalledWith( + expect.objectContaining({ + target: expect.objectContaining({ + name: 'service_account_info', + value: '', + }), + }), + ); + expect(changeMethods.onParametersChange).toHaveBeenCalledWith( + expect.objectContaining({ + target: expect.objectContaining({ + name: 'oauth2_client_info', + value: '', + }), + }), + ); + + // Also deletes `masked_encrypted_extra` keys directly via the dedicated + // `ClearEncryptedExtraKey` action so previously stored credentials + // don't survive a toggle in edit mode. + expect(changeMethods.onClearEncryptedExtraKey).toHaveBeenCalledWith( + 'service_account_info', + ); + expect(changeMethods.onClearEncryptedExtraKey).toHaveBeenCalledWith( + 'oauth2_client_info', + ); + expect(changeMethods.onEncryptedExtraInputChange).not.toHaveBeenCalled(); + }); + }); }); diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx index cd21f730a9a0..a653efb7d7e9 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx @@ -18,7 +18,7 @@ */ import { useState, useEffect } from 'react'; import { t } from '@apache-superset/core/translation'; -import { SupersetTheme, css } from '@apache-superset/core/theme'; +import { SupersetTheme } from '@apache-superset/core/theme'; import { Input, Button, @@ -29,7 +29,7 @@ import { } from '@superset-ui/core/components'; import { Icons } from '@superset-ui/core/components/Icons'; import { useToasts } from 'src/components/MessageToasts/withToasts'; -import { DatabaseParameters, FieldPropTypes } from '../../types'; +import { DatabaseParameters, Engines, FieldPropTypes } from '../../types'; import { infoTooltip, CredentialInfoForm } from '../styles'; enum CredentialInfoOptions { @@ -51,13 +51,17 @@ export const EncryptedField = ({ isEditMode, db, editNewDb, + isPublic = true, + setIsPublic, }: FieldPropTypes) => { const [fileList, setFileList] = useState([]); const [uploadOption, setUploadOption] = useState( CredentialInfoOptions.JsonUpload.valueOf(), ); const { addDangerToast } = useToasts(); - const showCredentialsInfo = !isEditMode; + const isGSheets = db?.engine === Engines.GSheet; + const showCredentialsInfo = !isEditMode && (!isGSheets || !isPublic); + const showCredentialsSection = !isGSheets || !isPublic; const encryptedField = db?.engine && encryptedCredentialsMap[db.engine as keyof typeof encryptedCredentialsMap]; @@ -68,6 +72,29 @@ export const EncryptedField = ({ ? JSON.stringify(paramValue) : paramValue; + const handlePublicToggle = (value: string) => { + const nextIsPublic = value === 'true'; + setIsPublic?.(nextIsPublic); + if (nextIsPublic) { + // Clear in-flight `parameters.*` so the save-time merge in + // DatabaseModal/index.tsx doesn't write them into masked_encrypted_extra. + changeMethods.onParametersChange({ + target: { name: 'service_account_info', value: '' }, + }); + changeMethods.onParametersChange({ + target: { name: 'oauth2_client_info', value: '' }, + }); + // Also delete any previously-stored values from masked_encrypted_extra + // itself (loaded in edit mode), since the save-time merge preserves + // existing keys and only overwrites them when `parameters.*` is truthy. + // Use the dedicated `ClearEncryptedExtraKey` action so the generic + // `EncryptedExtraInputChange` handler keeps its master semantics (store + // empty strings, never delete) for any other caller. + changeMethods.onClearEncryptedExtraKey('service_account_info'); + changeMethods.onClearEncryptedExtraKey('oauth2_client_info'); + } + }; + const readTextFile = (file: File): Promise => new Promise((resolve, reject) => { const reader = new FileReader(); @@ -77,6 +104,11 @@ export const EncryptedField = ({ }); useEffect(() => { + // Skip the initial clear when there's no engine-specific field name yet + // (e.g. the db prop hasn't finished loading): writing the falsy + // `encryptedField` as a key would pollute parameters with `false: ''`, + // and racing the async credential load isn't useful anyway. + if (!encryptedField) return; changeMethods.onParametersChange({ target: { name: encryptedField, @@ -87,6 +119,23 @@ export const EncryptedField = ({ return ( + {isGSheets && ( + <> + {t('Type of Google Sheets allowed')} + setUploadOption(option as number)} options={[ { @@ -111,9 +158,10 @@ export const EncryptedField = ({ /> )} - {uploadOption === CredentialInfoOptions.CopyPaste || - isEditMode || - editNewDb ? ( + {showCredentialsSection && + (uploadOption === CredentialInfoOptions.CopyPaste || + isEditMode || + editNewDb) ? (
{t('Service Account')} { const mockChangeMethods = { onEncryptedExtraInputChange: jest.fn(), + onClearEncryptedExtraKey: jest.fn(), onParametersChange: jest.fn(), onChange: jest.fn(), onQueryChange: jest.fn(), @@ -180,4 +181,85 @@ describe('OAuth2ClientField', () => { expect(getByTestId('client-token-request-uri')).toHaveValue(''); expect(getByTestId('client-scope')).toHaveValue(''); }); + + test('renders nothing when engine is gsheets and isPublic is true', () => { + const props = { + ...defaultProps, + isPublic: true, + db: { + ...defaultProps.db, + engine: 'gsheets', + }, + }; + + const { queryByText } = render(); + + expect(queryByText('OAuth2 client information')).not.toBeInTheDocument(); + }); + + test('renders normally when engine is gsheets but isPublic is false', () => { + const props = { + ...defaultProps, + isPublic: false, + db: { + ...defaultProps.db, + engine: 'gsheets', + }, + }; + + const { getByText } = render(); + + expect(getByText('OAuth2 client information')).toBeInTheDocument(); + }); + + test('re-syncs local state when masked_encrypted_extra is cleared', () => { + const props = { + ...defaultProps, + db: { + ...defaultProps.db, + engine: 'gsheets', + }, + isPublic: false, + }; + + const { getByTestId, getByText, rerender } = render( + , + ); + + fireEvent.click(getByText('OAuth2 client information')); + expect(getByTestId('client-id')).toHaveValue('test-id'); + + // Simulate the gsheets dropdown toggling to "public" — the parent + // dispatches an EncryptedExtraInputChange that drops the + // oauth2_client_info key from masked_encrypted_extra. + rerender( + , + ); + + expect(getByTestId('client-id')).toHaveValue(''); + expect(getByTestId('client-secret')).toHaveValue(''); + }); + + test.each([ + ['the literal string "null"', 'null'], + ['malformed JSON', 'not json'], + ['a JSON primitive', '42'], + ['a JSON array', '[1, 2, 3]'], + ])('mounts safely when masked_encrypted_extra is %s', (_label, value) => { + const props = { + ...defaultProps, + db: { + ...defaultProps.db, + masked_encrypted_extra: value, + }, + }; + + expect(() => render()).not.toThrow(); + }); }); diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx index d120cc88cb61..f87d22c3b9ee 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx @@ -17,10 +17,14 @@ * under the License. */ -import { useState } from 'react'; +import { useEffect, useState } from 'react'; import { t } from '@apache-superset/core/translation'; -import { Input, Collapse, FormItem } from '@superset-ui/core/components'; -import { CustomParametersChangeType, FieldPropTypes } from '../../types'; +import { Input, Collapse, Form, FormItem } from '@superset-ui/core/components'; +import { + CustomParametersChangeType, + Engines, + FieldPropTypes, +} from '../../types'; const LABELS = { CLIENT_ID: t('Client ID'), @@ -42,22 +46,56 @@ export const OAuth2ClientField = ({ changeMethods, db, default_value: defaultValue, + isPublic = true, }: FieldPropTypes) => { - const encryptedExtra = JSON.parse(db?.masked_encrypted_extra || '{}'); - const [oauth2ClientInfo, setOauth2ClientInfo] = useState({ - id: encryptedExtra.oauth2_client_info?.id || '', - secret: encryptedExtra.oauth2_client_info?.secret || '', - authorization_request_uri: - encryptedExtra.oauth2_client_info?.authorization_request_uri || - defaultValue?.authorization_request_uri || - '', - token_request_uri: - encryptedExtra.oauth2_client_info?.token_request_uri || - defaultValue?.token_request_uri || - '', - scope: - encryptedExtra.oauth2_client_info?.scope || defaultValue?.scope || '', - }); + const deriveOauth2ClientInfo = (): OAuth2ClientInfo => { + // `masked_encrypted_extra` is user/backend-supplied and historically + // sometimes the string "null" — JSON.parse('null') returns null, and + // malformed JSON throws. Defend against both so a single bad value + // can't crash the component. + let parsed: unknown; + try { + parsed = JSON.parse(db?.masked_encrypted_extra || '{}'); + } catch (e) { + // Only swallow JSON.parse's own SyntaxError; let real programmer + // errors (RangeError, TypeError from a broken stub, etc.) propagate. + if (!(e instanceof SyntaxError)) throw e; + parsed = {}; + } + const encryptedExtra = + parsed && typeof parsed === 'object' && !Array.isArray(parsed) + ? (parsed as { oauth2_client_info?: Partial }) + : {}; + const info = encryptedExtra.oauth2_client_info; + return { + id: info?.id || '', + secret: info?.secret || '', + authorization_request_uri: + info?.authorization_request_uri || + defaultValue?.authorization_request_uri || + '', + token_request_uri: + info?.token_request_uri || defaultValue?.token_request_uri || '', + scope: info?.scope || defaultValue?.scope || '', + }; + }; + + const [oauth2ClientInfo, setOauth2ClientInfo] = useState( + deriveOauth2ClientInfo, + ); + + // Re-sync local state when masked_encrypted_extra changes (e.g., when the + // gsheets dropdown toggles back to private after we cleared stored creds). + useEffect(() => { + setOauth2ClientInfo(deriveOauth2ClientInfo()); + // eslint-disable-next-line react-hooks/exhaustive-deps -- `defaultValue` is a + // static per-engine default that doesn't change after the form opens; + // depending on it would cause spurious re-syncs when the parent re-renders. + }, [db?.masked_encrypted_extra]); + + if (db?.engine === Engines.GSheet && isPublic) { + return null; + } const handleChange = (key: any) => (e: any) => { const updatedInfo = { @@ -84,7 +122,7 @@ export const OAuth2ClientField = ({ key: 'oauth2-client-information', label: t('OAuth2 client information'), children: ( - <> +
- +
), }, ]} diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.test.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.test.tsx new file mode 100644 index 000000000000..be2857415f86 --- /dev/null +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.test.tsx @@ -0,0 +1,87 @@ +/** + * 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 { render } from 'spec/helpers/testing-library'; +import { + ConfigurationMethod, + DatabaseObject, +} from 'src/features/databases/types'; +import { TableCatalog } from './TableCatalog'; + +const HELPER_TEXT = + 'In order to connect to non-public sheets you need to either provide a service account or configure an OAuth2 client.'; + +const buildDb = (overrides: Partial = {}): DatabaseObject => ({ + configuration_method: ConfigurationMethod.DynamicForm, + database_name: 'test', + driver: 'test', + id: 1, + name: 'test', + is_managed_externally: false, + engine: 'gsheets', + catalog: [{ name: '', value: '' }], + ...overrides, +}); + +const baseProps = { + required: true, + onParametersChange: jest.fn(), + onParametersUploadFileChange: jest.fn(), + changeMethods: { + onParametersChange: jest.fn(), + onChange: jest.fn(), + onQueryChange: jest.fn(), + onParametersUploadFileChange: jest.fn(), + onAddTableCatalog: jest.fn(), + onRemoveTableCatalog: jest.fn(), + onExtraInputChange: jest.fn(), + onEncryptedExtraInputChange: jest.fn(), + onClearEncryptedExtraKey: jest.fn(), + onSSHTunnelParametersChange: jest.fn(), + }, + validationErrors: null, + getValidation: jest.fn(), + clearValidationErrors: jest.fn(), + field: 'catalog', + isValidating: false, +}; + +test('TableCatalog: hides credentials helper text for gsheets when isPublic is true', () => { + const { queryByText } = render( + , + ); + + expect(queryByText(HELPER_TEXT)).not.toBeInTheDocument(); +}); + +test('TableCatalog: shows credentials helper text for gsheets when isPublic is false', () => { + const { getByText } = render( + , + ); + + expect(getByText(HELPER_TEXT)).toBeInTheDocument(); +}); + +test('TableCatalog: defaults to public (helper hidden) when isPublic is not provided', () => { + const { queryByText } = render( + , + ); + + expect(queryByText(HELPER_TEXT)).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx index 87456f342442..e7510d971bf3 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx @@ -25,7 +25,7 @@ import { import { Icons } from '@superset-ui/core/components/Icons'; import { Typography } from '@superset-ui/core/components/Typography'; import { StyledFooterButton, StyledCatalogTable } from '../styles'; -import { CatalogObject, FieldPropTypes } from '../../types'; +import { CatalogObject, Engines, FieldPropTypes } from '../../types'; export const TableCatalog = ({ required, @@ -33,9 +33,11 @@ export const TableCatalog = ({ getValidation, validationErrors, db, + isPublic = true, }: FieldPropTypes) => { const tableCatalog = db?.catalog || []; const catalogError = validationErrors || {}; + const showCredentialsHelper = db?.engine !== Engines.GSheet || !isPublic; return ( @@ -109,13 +111,15 @@ export const TableCatalog = ({ + {t('Add sheet')}
-
-
- {t( - 'In order to connect to non-public sheets you need to either provide a service account or configure an OAuth2 client.', - )} + {showCredentialsHelper && ( +
+
+ {t( + 'In order to connect to non-public sheets you need to either provide a service account or configure an OAuth2 client.', + )} +
-
+ )} ); }; diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx index f7d531ab19c6..c8f8c20288b1 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx @@ -16,11 +16,39 @@ * specific language governing permissions and limitations * under the License. */ +import { useEffect, useState } from 'react'; import { SupersetTheme } from '@apache-superset/core/theme'; import { Form } from '@superset-ui/core/components'; import { FormFieldOrder, FORM_FIELD_MAP } from './constants'; import { formScrollableStyles, validatedFormStyles } from '../styles'; -import { DatabaseConnectionFormProps } from '../../types'; +import { + DatabaseConnectionFormProps, + DatabaseObject, + Engines, +} from '../../types'; + +export const computeInitialIsPublic = ( + database: Partial | null | undefined, +): boolean => { + if (!database || database.engine !== Engines.GSheet) return true; + if ( + database.masked_encrypted_extra && + database.masked_encrypted_extra !== '{}' + ) { + return false; + } + if (database.parameters?.service_account_info) return false; + // OAuth2-only gsheets connections store creds under + // `parameters.oauth2_client_info` rather than (or in addition to) + // `masked_encrypted_extra` during edit; respect that too. + if ( + (database.parameters as { oauth2_client_info?: unknown }) + ?.oauth2_client_info + ) { + return false; + } + return true; +}; const DatabaseConnectionForm = ({ dbModel, @@ -33,6 +61,7 @@ const DatabaseConnectionForm = ({ onChange, onExtraInputChange, onEncryptedExtraInputChange, + onClearEncryptedExtraKey, onParametersChange, onParametersUploadFileChange, onQueryChange, @@ -52,6 +81,21 @@ const DatabaseConnectionForm = ({ required?: string[]; }; + const [isPublic, setIsPublic] = useState(() => + computeInitialIsPublic(db), + ); + + // Re-derive when switching to a different database, when the engine + // changes, or when any of the credential fields read by + // computeInitialIsPublic arrive async on edit load. The setter is a no-op + // when the result is unchanged, so over-running this effect is harmless. + useEffect(() => { + setIsPublic(computeInitialIsPublic(db)); + // eslint-disable-next-line react-hooks/exhaustive-deps -- `db` identity + // alone churns on every reducer dispatch; the dep list below tracks each + // field that computeInitialIsPublic actually reads. + }, [db?.id, db?.engine, db?.masked_encrypted_extra, db?.parameters]); + return (
Object.keys(parameters.properties).includes(key) || key === 'database_name', - ).map(field => + ).map(field => { + // Render as JSX so each field's hooks live on its own fiber. + // Calling the component as a function (e.g. FORM_FIELD_MAP[field]({...})) + // makes the field's hooks register against this parent's fiber and + // breaks the rules of hooks once the parent owns its own state. // @ts-expect-error TODO: fix ComponentClass for SSHTunnelSwitchComponent not having call signature. - FORM_FIELD_MAP[field]({ - required: parameters.required?.includes(field), - changeMethods: { - onParametersChange, - onChange, - onQueryChange, - onParametersUploadFileChange, - onAddTableCatalog, - onRemoveTableCatalog, - onExtraInputChange, - onEncryptedExtraInputChange, - }, - validationErrors, - getValidation, - clearValidationErrors, - db, - key: field, - field, - default_value: parameters.properties[field]?.default, - description: parameters.properties[field]?.description, - isEditMode, - sslForced, - editNewDb, - isValidating, - placeholder: getPlaceholder ? getPlaceholder(field) : undefined, - }), - )} + const FieldComponent = FORM_FIELD_MAP[field]; + return ( + + ); + })}
); diff --git a/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx b/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx index 455c4740a260..c4601c2f93bf 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx @@ -1761,6 +1761,122 @@ describe('dbReducer', () => { }); }); + test('EncryptedExtraInputChange stores empty value verbatim (does not delete)', () => { + const action: DBReducerActionType = { + type: ActionType.EncryptedExtraInputChange, + payload: { name: 'service_account_info', value: '' }, + }; + const currentState = dbReducer( + { + ...databaseFixture, + masked_encrypted_extra: JSON.stringify({ + service_account_info: { type: 'service_account' }, + other: 'keep-me', + }), + }, + action, + ); + + // Generic input change must not delete keys — that's reserved for the + // explicit `ClearEncryptedExtraKey` action used by the public/private + // toggle. Backends may distinguish absent-key from empty-string. + expect(currentState).toEqual({ + ...databaseFixture, + masked_encrypted_extra: '{"service_account_info":"","other":"keep-me"}', + }); + }); + + test.each([ + ['the literal string "null"', 'null'], + ['malformed JSON', 'not json'], + ['a JSON primitive', '42'], + ['a JSON array', '[1,2,3]'], + ])( + 'EncryptedExtraInputChange recovers when masked_encrypted_extra is %s', + (_label, value) => { + const action: DBReducerActionType = { + type: ActionType.EncryptedExtraInputChange, + payload: { name: 'foo', value: 'bar' }, + }; + const currentState = dbReducer( + { ...databaseFixture, masked_encrypted_extra: value }, + action, + ); + + // Reducer recovers and starts fresh — no crash, no leaked bad value. + expect(currentState).toEqual({ + ...databaseFixture, + masked_encrypted_extra: '{"foo":"bar"}', + }); + }, + ); + + test('ClearEncryptedExtraKey removes the named key from masked_encrypted_extra', () => { + const action: DBReducerActionType = { + type: ActionType.ClearEncryptedExtraKey, + payload: { name: 'service_account_info' }, + }; + const currentState = dbReducer( + { + ...databaseFixture, + masked_encrypted_extra: JSON.stringify({ + service_account_info: { type: 'service_account' }, + other: 'keep-me', + }), + }, + action, + ); + + expect(currentState).toEqual({ + ...databaseFixture, + masked_encrypted_extra: '{"other":"keep-me"}', + }); + }); + + test('ClearEncryptedExtraKey is a no-op when the key is already absent', () => { + const action: DBReducerActionType = { + type: ActionType.ClearEncryptedExtraKey, + payload: { name: 'missing' }, + }; + const currentState = dbReducer( + { + ...databaseFixture, + masked_encrypted_extra: '{"other":"keep-me"}', + }, + action, + ); + + expect(currentState).toEqual({ + ...databaseFixture, + masked_encrypted_extra: '{"other":"keep-me"}', + }); + }); + + test.each([ + ['the literal string "null"', 'null'], + ['malformed JSON', 'not json'], + ['a JSON primitive', '42'], + ['a JSON array', '[1,2,3]'], + ])( + 'ClearEncryptedExtraKey recovers when masked_encrypted_extra is %s', + (_label, value) => { + const action: DBReducerActionType = { + type: ActionType.ClearEncryptedExtraKey, + payload: { name: 'service_account_info' }, + }; + const currentState = dbReducer( + { ...databaseFixture, masked_encrypted_extra: value }, + action, + ); + + // Recovery path produces a clean `{}` rather than crashing. + expect(currentState).toEqual({ + ...databaseFixture, + masked_encrypted_extra: '{}', + }); + }, + ); + test('it will set state to payload from extra input change when checkbox', () => { const action: DBReducerActionType = { type: ActionType.ExtraInputChange, diff --git a/superset-frontend/src/features/databases/DatabaseModal/index.tsx b/superset-frontend/src/features/databases/DatabaseModal/index.tsx index 1fa38c5e1521..166657a084b1 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/index.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/index.tsx @@ -167,6 +167,7 @@ export enum ActionType { ExtraEditorChange, ExtraInputChange, EncryptedExtraInputChange, + ClearEncryptedExtraKey, Fetched, InputChange, ParametersChange, @@ -199,6 +200,7 @@ export type DBReducerActionType = | ActionType.ExtraEditorChange | ActionType.ExtraInputChange | ActionType.EncryptedExtraInputChange + | ActionType.ClearEncryptedExtraKey | ActionType.TextChange | ActionType.QueryChange | ActionType.InputChange @@ -285,14 +287,57 @@ export function dbReducer( [action.payload.name]: actionPayloadJson, }), }; - case ActionType.EncryptedExtraInputChange: + case ActionType.EncryptedExtraInputChange: { + // `masked_encrypted_extra` can arrive as the literal string "null" or + // malformed JSON from older payloads — defend the parse so a single + // bad value can't crash the reducer. + let parsedUnknown: unknown; + try { + parsedUnknown = JSON.parse(trimmedState.masked_encrypted_extra || '{}'); + } catch (e) { + if (!(e instanceof SyntaxError)) throw e; + parsedUnknown = {}; + } + const parsed: Record = + parsedUnknown && + typeof parsedUnknown === 'object' && + !Array.isArray(parsedUnknown) + ? (parsedUnknown as Record) + : {}; + // Generic input change: store the value as-is (including empty string). + // Use `ClearEncryptedExtraKey` if the intent is to remove the key. return { ...trimmedState, masked_encrypted_extra: JSON.stringify({ - ...JSON.parse(trimmedState.masked_encrypted_extra || '{}'), + ...parsed, [action.payload.name]: action.payload.value, }), }; + } + case ActionType.ClearEncryptedExtraKey: { + // Same defensive parse as EncryptedExtraInputChange — see comment above. + let parsedUnknown: unknown; + try { + parsedUnknown = JSON.parse(trimmedState.masked_encrypted_extra || '{}'); + } catch (e) { + if (!(e instanceof SyntaxError)) throw e; + parsedUnknown = {}; + } + const parsed: Record = + parsedUnknown && + typeof parsedUnknown === 'object' && + !Array.isArray(parsedUnknown) + ? (parsedUnknown as Record) + : {}; + // Explicit key removal — used by the gsheets public/private toggle to + // drop previously stored service_account_info / oauth2_client_info so + // the save-time merge in this modal doesn't carry them forward. + delete parsed[action.payload.name as string]; + return { + ...trimmedState, + masked_encrypted_extra: JSON.stringify(parsed), + }; + } case ActionType.ExtraInputChange: if ( action.payload.name === 'schema_cache_timeout' || @@ -1866,6 +1911,11 @@ const DatabaseModal: FunctionComponent = ({ value: target.value, }) } + onClearEncryptedExtraKey={(name: string) => + handleChangeWithValidation(ActionType.ClearEncryptedExtraKey, { + name, + }) + } onRemoveTableCatalog={(idx: number) => { setDB({ type: ActionType.RemoveTableCatalogSheet, diff --git a/superset-frontend/src/features/databases/DatabaseModal/styles.ts b/superset-frontend/src/features/databases/DatabaseModal/styles.ts index 8aa9aa16ec29..6e065acebb60 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/styles.ts +++ b/superset-frontend/src/features/databases/DatabaseModal/styles.ts @@ -120,7 +120,7 @@ export const formScrollableStyles = (theme: SupersetTheme) => css` export const antDModalStyles = (theme: SupersetTheme) => css` .ant-select-dropdown { - height: ${theme.sizeUnit * 40}px; + max-height: ${theme.sizeUnit * 40}px; } .ant-modal-header { @@ -384,6 +384,15 @@ export const EditHeaderSubtitle = styled.div` `; export const CredentialInfoForm = styled.div` + margin-top: ${({ theme }) => theme.sizeUnit * 4}px; + + /* Match the label-to-input spacing used by LabeledErrorBoundInput's + StyledInput so the bare +