From ff449ad8abfd6cc9e5af83fc0cd41f99d394aec0 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 3 Sep 2024 20:57:55 -0400 Subject: [PATCH] feat: OAuth2 database field (#30126) --- .../CommonParameters.tsx | 31 ++- .../OAuth2ClientField.test.tsx | 181 ++++++++++++++++++ .../OAuth2ClientField.tsx | 112 +++++++++++ .../DatabaseConnectionForm/constants.ts | 3 + .../DatabaseConnectionForm/index.tsx | 2 + .../databases/DatabaseModal/index.test.tsx | 14 ++ .../databases/DatabaseModal/index.tsx | 20 ++ .../src/features/databases/types.ts | 5 + superset/databases/schemas.py | 3 + superset/db_engine_specs/base.py | 2 + .../integration_tests/databases/api_tests.py | 8 + tests/unit_tests/databases/api_test.py | 2 + 12 files changed, 366 insertions(+), 17 deletions(-) create mode 100644 superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.test.tsx create mode 100644 superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/CommonParameters.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/CommonParameters.tsx index d5d09cec862ad..d2170de6f8f91 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/CommonParameters.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/CommonParameters.tsx @@ -161,23 +161,20 @@ export const httpPathField = ({ getValidation, validationErrors, db, -}: FieldPropTypes) => { - console.error(db); - return ( - - ); -}; +}: FieldPropTypes) => ( + +); export const usernameField = ({ required, changeMethods, diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.test.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.test.tsx new file mode 100644 index 0000000000000..1752eb1960727 --- /dev/null +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.test.tsx @@ -0,0 +1,181 @@ +/** + * 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, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom/extend-expect'; +import { ThemeProvider, supersetTheme } from '@superset-ui/core'; +import { DatabaseObject } from 'src/features/databases/types'; +import { OAuth2ClientField } from './OAuth2ClientField'; + +const renderWithTheme = (component: JSX.Element) => + render({component}); + +describe('OAuth2ClientField', () => { + const mockChangeMethods = { + onEncryptedExtraInputChange: jest.fn(), + onParametersChange: jest.fn(), + onChange: jest.fn(), + onQueryChange: jest.fn(), + onParametersUploadFileChange: jest.fn(), + onAddTableCatalog: jest.fn(), + onRemoveTableCatalog: jest.fn(), + onExtraInputChange: jest.fn(), + onSSHTunnelParametersChange: jest.fn(), + }; + + const defaultProps = { + required: false, + onParametersChange: jest.fn(), + onParametersUploadFileChange: jest.fn(), + changeMethods: mockChangeMethods, + validationErrors: null, + getValidation: jest.fn(), + clearValidationErrors: jest.fn(), + field: 'test', + db: { + configuration_method: 'dynamic_form', + database_name: 'test', + driver: 'test', + id: 1, + name: 'test', + is_managed_externally: false, + engine_information: { + supports_oauth2: true, + }, + masked_encrypted_extra: JSON.stringify({ + oauth2_client_info: { + id: 'test-id', + secret: 'test-secret', + authorization_request_uri: 'https://auth-uri', + token_request_uri: 'https://token-uri', + scope: 'test-scope', + }, + }), + } as DatabaseObject, + }; + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('does not show input fields until the collapse trigger is clicked', () => { + const { getByText, getByTestId, queryByTestId } = renderWithTheme( + , + ); + + expect(queryByTestId('client-id')).not.toBeInTheDocument(); + expect(queryByTestId('client-secret')).not.toBeInTheDocument(); + expect( + queryByTestId('client-authorization-request-uri'), + ).not.toBeInTheDocument(); + expect(queryByTestId('client-token-request-uri')).not.toBeInTheDocument(); + expect(queryByTestId('client-scope')).not.toBeInTheDocument(); + + const collapseTrigger = getByText('OAuth2 client information'); + fireEvent.click(collapseTrigger); + + expect(getByTestId('client-id')).toBeInTheDocument(); + expect(getByTestId('client-secret')).toBeInTheDocument(); + expect(getByTestId('client-authorization-request-uri')).toBeInTheDocument(); + expect(getByTestId('client-token-request-uri')).toBeInTheDocument(); + expect(getByTestId('client-scope')).toBeInTheDocument(); + }); + + it('renders the OAuth2ClientField component with initial values', () => { + const { getByTestId, getByText } = renderWithTheme( + , + ); + + const collapseTrigger = getByText('OAuth2 client information'); + fireEvent.click(collapseTrigger); + + expect(getByTestId('client-id')).toHaveValue('test-id'); + expect(getByTestId('client-secret')).toHaveValue('test-secret'); + expect(getByTestId('client-authorization-request-uri')).toHaveValue( + 'https://auth-uri', + ); + expect(getByTestId('client-token-request-uri')).toHaveValue( + 'https://token-uri', + ); + expect(getByTestId('client-scope')).toHaveValue('test-scope'); + }); + + it('handles input changes and triggers onEncryptedExtraInputChange', () => { + const { getByTestId, getByText } = renderWithTheme( + , + ); + + const collapseTrigger = getByText('OAuth2 client information'); + fireEvent.click(collapseTrigger); + + const clientIdInput = getByTestId('client-id'); + fireEvent.change(clientIdInput, { target: { value: 'new-id' } }); + + expect(mockChangeMethods.onEncryptedExtraInputChange).toHaveBeenCalledWith( + expect.objectContaining({ + target: { + name: 'oauth2_client_info', + value: expect.objectContaining({ id: 'new-id' }), + }, + }), + ); + }); + + it('does not render when supports_oauth2 is false', () => { + const props = { + ...defaultProps, + db: { + ...defaultProps.db, + engine_information: { + supports_oauth2: false, + }, + }, + }; + + const { queryByTestId } = renderWithTheme(); + + expect(queryByTestId('client-id')).not.toBeInTheDocument(); + }); + + it('renders empty fields when masked_encrypted_extra is empty', () => { + const props = { + ...defaultProps, + db: { + ...defaultProps.db, + engine_information: { + supports_oauth2: true, + }, + masked_encrypted_extra: '{}', + }, + }; + + const { getByTestId, getByText } = renderWithTheme( + , + ); + + const collapseTrigger = getByText('OAuth2 client information'); + fireEvent.click(collapseTrigger); + + expect(getByTestId('client-id')).toHaveValue(''); + expect(getByTestId('client-secret')).toHaveValue(''); + expect(getByTestId('client-authorization-request-uri')).toHaveValue(''); + expect(getByTestId('client-token-request-uri')).toHaveValue(''); + expect(getByTestId('client-scope')).toHaveValue(''); + }); +}); diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx new file mode 100644 index 0000000000000..ee0ffdeb33f51 --- /dev/null +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx @@ -0,0 +1,112 @@ +/** + * 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 Collapse from 'src/components/Collapse'; +import { Input } from 'src/components/Input'; +import { FormItem } from 'src/components/Form'; +import { FieldPropTypes } from '../../types'; + +interface OAuth2ClientInfo { + id: string; + secret: string; + authorization_request_uri: string; + token_request_uri: string; + scope: string; +} + +export const OAuth2ClientField = ({ changeMethods, db }: 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 || '', + token_request_uri: + encryptedExtra.oauth2_client_info?.token_request_uri || '', + scope: encryptedExtra.oauth2_client_info?.scope || '', + }); + + if (db?.engine_information?.supports_oauth2 !== true) { + return null; + } + + const handleChange = (key: any) => (e: any) => { + const updatedInfo = { + ...oauth2ClientInfo, + [key]: e.target.value, + }; + + setOauth2ClientInfo(updatedInfo); + + const event = { + target: { + name: 'oauth2_client_info', + value: updatedInfo, + }, + }; + changeMethods.onEncryptedExtraInputChange(event); + }; + + return ( + + + + + + + + + + + + + + + + + + + + ); +}; diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/constants.ts b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/constants.ts index 3ba0f3689eb0b..1d9334ce71702 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/constants.ts +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/constants.ts @@ -32,6 +32,7 @@ import { queryField, usernameField, } from './CommonParameters'; +import { OAuth2ClientField } from './OAuth2ClientField'; import { validatedInputField } from './ValidatedInputField'; import { EncryptedField } from './EncryptedField'; import { TableCatalog } from './TableCatalog'; @@ -58,6 +59,7 @@ export const FormFieldOrder = [ 'warehouse', 'role', 'ssh', + 'oauth2_client', ]; const extensionsRegistry = getExtensionsRegistry(); @@ -75,6 +77,7 @@ export const FORM_FIELD_MAP = { default_schema: defaultSchemaField, username: usernameField, password: passwordField, + oauth2_client: OAuth2ClientField, access_token: accessTokenField, database_name: displayField, query: queryField, diff --git a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx index 9cec0b827db67..96740720a6842 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx @@ -32,6 +32,7 @@ const DatabaseConnectionForm = ({ onAddTableCatalog, onChange, onExtraInputChange, + onEncryptedExtraInputChange, onParametersChange, onParametersUploadFileChange, onQueryChange, @@ -75,6 +76,7 @@ const DatabaseConnectionForm = ({ onAddTableCatalog, onRemoveTableCatalog, onExtraInputChange, + onEncryptedExtraInputChange, }, validationErrors, getValidation, diff --git a/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx b/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx index acf6528edde47..7150b863abd50 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx @@ -1723,6 +1723,20 @@ describe('dbReducer', () => { }); }); + test('it will set state to payload from encrypted extra input change', () => { + const action: DBReducerActionType = { + type: ActionType.EncryptedExtraInputChange, + payload: { name: 'foo', value: 'bar' }, + }; + const currentState = dbReducer(databaseFixture, action); + + // extra should be serialized + expect(currentState).toEqual({ + ...databaseFixture, + masked_encrypted_extra: '{"foo":"bar"}', + }); + }); + 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 48882131282b4..53215570a6ade 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/index.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/index.tsx @@ -154,6 +154,7 @@ export enum ActionType { EditorChange, ExtraEditorChange, ExtraInputChange, + EncryptedExtraInputChange, Fetched, InputChange, ParametersChange, @@ -185,6 +186,7 @@ export type DBReducerActionType = type: | ActionType.ExtraEditorChange | ActionType.ExtraInputChange + | ActionType.EncryptedExtraInputChange | ActionType.TextChange | ActionType.QueryChange | ActionType.InputChange @@ -269,6 +271,14 @@ export function dbReducer( [action.payload.name]: actionPayloadJson, }), }; + case ActionType.EncryptedExtraInputChange: + return { + ...trimmedState, + masked_encrypted_extra: JSON.stringify({ + ...JSON.parse(trimmedState.masked_encrypted_extra || '{}'), + [action.payload.name]: action.payload.value, + }), + }; case ActionType.ExtraInputChange: // "extra" payload in state is a string if ( @@ -1656,6 +1666,16 @@ const DatabaseModal: FunctionComponent = ({ value: target.value, }) } + onEncryptedExtraInputChange={({ + target, + }: { + target: HTMLInputElement; + }) => + onChange(ActionType.EncryptedExtraInputChange, { + name: target.name, + value: target.value, + }) + } onRemoveTableCatalog={(idx: number) => { setDB({ type: ActionType.RemoveTableCatalogSheet, diff --git a/superset-frontend/src/features/databases/types.ts b/superset-frontend/src/features/databases/types.ts index b170c7d6c94eb..c0ca1c5d3508e 100644 --- a/superset-frontend/src/features/databases/types.ts +++ b/superset-frontend/src/features/databases/types.ts @@ -113,6 +113,7 @@ export type DatabaseObject = { supports_file_upload?: boolean; disable_ssh_tunneling?: boolean; supports_dynamic_catalog?: boolean; + supports_oauth2?: boolean; }; // SSH Tunnel information @@ -301,6 +302,7 @@ export interface FieldPropTypes { onRemoveTableCatalog: (idx: number) => void; } & { onExtraInputChange: (value: any) => void; + onEncryptedExtraInputChange: (value: any) => void; onSSHTunnelParametersChange: CustomEventHandlerType; }; validationErrors: JsonObject | null; @@ -352,6 +354,9 @@ export interface DatabaseConnectionFormProps { onExtraInputChange: ( event: FormEvent | { target: HTMLInputElement }, ) => void; + onEncryptedExtraInputChange: ( + event: FormEvent | { target: HTMLInputElement }, + ) => void; onAddTableCatalog: () => void; onRemoveTableCatalog: (idx: number) => void; validationErrors: JsonObject | null; diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index 5d44c2006c758..27eb043eb131b 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -985,6 +985,9 @@ class EngineInformationSchema(Schema): "description": "The database supports multiple catalogs in a single connection" } ) + supports_oauth2 = fields.Boolean( + metadata={"description": "The database supports OAuth2"} + ) class DatabaseConnectionSchema(Schema): diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index b5f001971d3d8..678649e85b862 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -2230,6 +2230,7 @@ def get_public_information(cls) -> dict[str, Any]: "supports_file_upload": cls.supports_file_upload, "disable_ssh_tunneling": cls.disable_ssh_tunneling, "supports_dynamic_catalog": cls.supports_dynamic_catalog, + "supports_oauth2": cls.supports_oauth2, } @classmethod @@ -2351,6 +2352,7 @@ def build_sqlalchemy_uri( # pylint: disable=unused-argument parameters: BasicParametersType, encrypted_extra: dict[str, str] | None = None, ) -> str: + # TODO (betodealmeida): this method should also build `connect_args` # make a copy so that we don't update the original query = parameters.get("query", {}).copy() if parameters.get("encryption"): diff --git a/tests/integration_tests/databases/api_tests.py b/tests/integration_tests/databases/api_tests.py index 1b028615b8fe7..bd99733d93cbc 100644 --- a/tests/integration_tests/databases/api_tests.py +++ b/tests/integration_tests/databases/api_tests.py @@ -3254,6 +3254,7 @@ def test_available(self, app, get_available_engine_specs): "supports_file_upload": True, "supports_dynamic_catalog": True, "disable_ssh_tunneling": False, + "supports_oauth2": False, }, "supports_oauth2": False, }, @@ -3279,6 +3280,7 @@ def test_available(self, app, get_available_engine_specs): "supports_file_upload": True, "supports_dynamic_catalog": True, "disable_ssh_tunneling": True, + "supports_oauth2": False, }, "supports_oauth2": False, }, @@ -3336,6 +3338,7 @@ def test_available(self, app, get_available_engine_specs): "supports_file_upload": True, "supports_dynamic_catalog": False, "disable_ssh_tunneling": False, + "supports_oauth2": False, }, "supports_oauth2": False, }, @@ -3361,6 +3364,7 @@ def test_available(self, app, get_available_engine_specs): "supports_file_upload": True, "supports_dynamic_catalog": False, "disable_ssh_tunneling": True, + "supports_oauth2": True, }, "supports_oauth2": True, }, @@ -3418,6 +3422,7 @@ def test_available(self, app, get_available_engine_specs): "supports_file_upload": True, "supports_dynamic_catalog": False, "disable_ssh_tunneling": False, + "supports_oauth2": False, }, "supports_oauth2": False, }, @@ -3431,6 +3436,7 @@ def test_available(self, app, get_available_engine_specs): "supports_file_upload": True, "supports_dynamic_catalog": False, "disable_ssh_tunneling": False, + "supports_oauth2": False, }, "supports_oauth2": False, }, @@ -3465,6 +3471,7 @@ def test_available_no_default(self, app, get_available_engine_specs): "supports_file_upload": True, "supports_dynamic_catalog": False, "disable_ssh_tunneling": False, + "supports_oauth2": False, }, "supports_oauth2": False, }, @@ -3478,6 +3485,7 @@ def test_available_no_default(self, app, get_available_engine_specs): "supports_file_upload": True, "supports_dynamic_catalog": False, "disable_ssh_tunneling": False, + "supports_oauth2": False, }, "supports_oauth2": False, }, diff --git a/tests/unit_tests/databases/api_test.py b/tests/unit_tests/databases/api_test.py index 746bf04cd9098..4c28a3c293edc 100644 --- a/tests/unit_tests/databases/api_test.py +++ b/tests/unit_tests/databases/api_test.py @@ -239,6 +239,7 @@ def test_database_connection( "disable_ssh_tunneling": True, "supports_dynamic_catalog": False, "supports_file_upload": True, + "supports_oauth2": True, }, "expose_in_sqllab": True, "extra": '{\n "metadata_params": {},\n "engine_params": {},\n "metadata_cache_timeout": {},\n "schemas_allowed_for_file_upload": []\n}\n', @@ -311,6 +312,7 @@ def test_database_connection( "disable_ssh_tunneling": True, "supports_dynamic_catalog": False, "supports_file_upload": True, + "supports_oauth2": True, }, "expose_in_sqllab": True, "force_ctas_schema": None,