feat: improve GSheets OAuth2

superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx

@@ -52,10 +52,7 @@ export const EncryptedField = ({
   const [fileToUpload, setFileToUpload] = useState<string | null | undefined>(
     null,
   );
-  const [isPublic, setIsPublic] = useState<boolean>(true);
-  const showCredentialsInfo =
-    db?.engine === 'gsheets' ? !isEditMode && !isPublic : !isEditMode;
-  const isEncrypted = isEditMode && db?.masked_encrypted_extra !== '{}';
+  const showCredentialsInfo = !isEditMode;
   const encryptedField =
     db?.engine &&
     encryptedCredentialsMap[db.engine as keyof typeof encryptedCredentialsMap];
@@ -67,33 +64,9 @@ export const EncryptedField = ({
       : paramValue;
   return (
     <CredentialInfoForm>
-      {db?.engine === 'gsheets' && (
-        <div className="catalog-type-select">
-          <FormLabel
-            css={(theme: SupersetTheme) => labelMarginBottom(theme)}
-            required
-          >
-            {t('Type of Google Sheets allowed')}
-          </FormLabel>
-          <AntdSelect
-            style={{ width: '100%' }}
-            defaultValue={isEncrypted ? 'false' : 'true'}
-            onChange={(value: string) =>
-              setIsPublic(castStringToBoolean(value))
-            }
-          >
-            <AntdSelect.Option value="true" key={1}>
-              {t('Publicly shared sheets only')}
-            </AntdSelect.Option>
-            <AntdSelect.Option value="false" key={2}>
-              {t('Public and privately shared sheets')}
-            </AntdSelect.Option>
-          </AntdSelect>
-        </div>
-      )}
       {showCredentialsInfo && (
         <>
-          <FormLabel required>
+          <FormLabel>
             {t('How do you want to enter service account credentials?')}
           </FormLabel>
           <AntdSelect
@@ -115,7 +88,7 @@ export const EncryptedField = ({
       isEditMode ||
       editNewDb ? (
         <div className="input-container">
-          <FormLabel required>{t('Service Account')}</FormLabel>
+          <FormLabel>{t('Service Account')}</FormLabel>
           <textarea
             className="input-form"
             name={encryptedField}
@@ -140,7 +113,7 @@ export const EncryptedField = ({
             css={(theme: SupersetTheme) => infoTooltip(theme)}
           >
             <div css={{ display: 'flex', alignItems: 'center' }}>
-              <FormLabel required>{t('Upload Credentials')}</FormLabel>
+              <FormLabel>{t('Upload Credentials')}</FormLabel>
               <InfoTooltip
                 tooltip={t(
                   'Use the JSON file you automatically downloaded when creating your service account.',

superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx

@@ -22,7 +22,7 @@ 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';
+import { CustomParametersChangeType, FieldPropTypes } from '../../types';
 
 const LABELS = {
   CLIENT_ID: 'Client ID',
@@ -40,16 +40,25 @@ interface OAuth2ClientInfo {
   scope: string;
 }
 
-export const OAuth2ClientField = ({ changeMethods, db }: FieldPropTypes) => {
+export const OAuth2ClientField = ({
+  changeMethods,
+  db,
+  default_value: defaultValue,
+}: FieldPropTypes) => {
   const encryptedExtra = JSON.parse(db?.masked_encrypted_extra || '{}');
   const [oauth2ClientInfo, setOauth2ClientInfo] = useState<OAuth2ClientInfo>({
     id: encryptedExtra.oauth2_client_info?.id || '',
     secret: encryptedExtra.oauth2_client_info?.secret || '',
     authorization_request_uri:
-      encryptedExtra.oauth2_client_info?.authorization_request_uri || '',
+      encryptedExtra.oauth2_client_info?.authorization_request_uri ||
+      defaultValue?.authorization_request_uri ||
+      '',
     token_request_uri:
-      encryptedExtra.oauth2_client_info?.token_request_uri || '',
-    scope: encryptedExtra.oauth2_client_info?.scope || '',
+      encryptedExtra.oauth2_client_info?.token_request_uri ||
+      defaultValue?.token_request_uri ||
+      '',
+    scope:
+      encryptedExtra.oauth2_client_info?.scope || defaultValue?.scope || '',
   });
 
   const handleChange = (key: any) => (e: any) => {
@@ -60,13 +69,14 @@ export const OAuth2ClientField = ({ changeMethods, db }: FieldPropTypes) => {
 
     setOauth2ClientInfo(updatedInfo);
 
-    const event = {
+    const event: CustomParametersChangeType = {
       target: {
+        type: 'object',
         name: 'oauth2_client_info',
         value: updatedInfo,
       },
     };
-    changeMethods.onEncryptedExtraInputChange(event);
+    changeMethods.onParametersChange(event);
   };
 
   return (

superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/TableCatalog.tsx

@@ -40,7 +40,7 @@ export const TableCatalog = ({
       <div>
         {tableCatalog?.map((sheet: CatalogObject, idx: number) => (
           <>
-            <FormLabel className="catalog-label" required>
+            <FormLabel className="catalog-label">
               {t('Google Sheet Name and URL')}
             </FormLabel>
             <div className="catalog-name">
@@ -105,6 +105,13 @@ export const TableCatalog = ({
           + {t('Add sheet')}
         </StyledFooterButton>
       </div>
+      <div className="helper">
+        <div>
+          {t(
+            'In order to connect to non-public sheets you need to either provide a service account or configure an OAuth2 client.',
+          )}
+        </div>
+      </div>
     </StyledCatalogTable>
   );
 };

superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/constants.ts

@@ -52,16 +52,16 @@ export const FormFieldOrder = [
   'http_path_field',
   'database_name',
   'project_id',
+  'catalog',
   'credentials_info',
   'service_account_info',
-  'catalog',
   'query',
   'encryption',
   'account',
   'warehouse',
   'role',
   'ssh',
-  'oauth2_client',
+  'oauth2_client_info',
 ];
 
 const extensionsRegistry = getExtensionsRegistry();
@@ -79,7 +79,7 @@ export const FORM_FIELD_MAP = {
   default_schema: defaultSchemaField,
   username: usernameField,
   password: passwordField,
-  oauth2_client: OAuth2ClientField,
+  oauth2_client_info: OAuth2ClientField,
   access_token: accessTokenField,
   database_name: displayField,
   query: queryField,

superset-frontend/src/features/databases/types.ts

@@ -265,7 +265,7 @@ export interface ExtraJson {
 }
 
 export type CustomTextType = {
-  value?: string | boolean | number;
+  value?: string | boolean | number | object;
   type?: string | null;
   name?: string;
   checked?: boolean;

superset/commands/database/validate.py

@@ -96,7 +96,7 @@ def run(self) -> None:
             server_cert=self._properties.get("server_cert", ""),
             extra=self._properties.get("extra", "{}"),
             impersonate_user=self._properties.get("impersonate_user", False),
-            encrypted_extra=serialized_encrypted_extra,
+            encrypted_extra=json.dumps(encrypted_extra),
         )
         database.set_sqlalchemy_uri(sqlalchemy_uri)
         database.db_engine_spec.mutate_db_for_connection_test(database)

superset/db_engine_specs/base.py

@@ -138,7 +138,9 @@ class TimeGrain(NamedTuple):
 }
 
 
-class TimestampExpression(ColumnClause):  # pylint: disable=abstract-method, too-many-ancestors
+class TimestampExpression(
+    ColumnClause
+):  # pylint: disable=abstract-method, too-many-ancestors
     def __init__(self, expr: str, col: ColumnClause, **kwargs: Any) -> None:
         """Sqlalchemy class that can be used to render native column elements respecting
         engine-specific quoting rules as part of a string-based expression.
@@ -397,9 +399,9 @@ class BaseEngineSpec:  # pylint: disable=too-many-public-methods
     max_column_name_length: int | None = None
     try_remove_schema_from_table_name = True  # pylint: disable=invalid-name
     run_multiple_statements_as_one = False
-    custom_errors: dict[
-        Pattern[str], tuple[str, SupersetErrorType, dict[str, Any]]
-    ] = {}
+    custom_errors: dict[Pattern[str], tuple[str, SupersetErrorType, dict[str, Any]]] = (
+        {}
+    )
 
     # List of JSON path to fields in `encrypted_extra` that should be masked when the
     # database is edited. By default everything is masked.

superset/db_engine_specs/gsheets.py

@@ -67,6 +67,18 @@ class GSheetsParametersSchema(Schema):
             "field_name": "service_account_info",
         },
     )
+    oauth2_client_info = EncryptedString(
+        required=False,
+        metadata={
+            "description": "OAuth2 client information",
+            "default": {
+                "scope": " ".join(SCOPES),
+                "authorization_request_uri": "https://accounts.google.com/o/oauth2/v2/auth",
+                "token_request_uri": "https://oauth2.googleapis.com/token",
+            },
+        },
+        allow_none=True,
+    )
 
 
 class GSheetsParametersType(TypedDict):
@@ -165,8 +177,22 @@ def build_sqlalchemy_uri(
         _: GSheetsParametersType,
         encrypted_extra: None | (dict[str, Any]) = None,
     ) -> str:
+        if encrypted_extra and "oauth2_client_info" in encrypted_extra:
+            del encrypted_extra["oauth2_client_info"]
+
         return "gsheets://"
 
+    @staticmethod
+    def update_params_from_encrypted_extra(
+        database: Database,
+        params: dict[str, Any],
+    ) -> None:
+        """
+        Remove `oauth2_client_info` from `encrypted_extra`.
+        """
+        if "oauth2_client_info" in params.get("encrypted_extra", {}):
+            del params["encrypted_extra"]["oauth2_client_info"]
+
     @classmethod
     def get_parameters_from_uri(
         cls,
@@ -221,18 +247,6 @@ def validate_parameters(
         if isinstance(encrypted_credentials, str):
             encrypted_credentials = json.loads(encrypted_credentials)
 
-        if not table_catalog:
-            # Allowing users to submit empty catalogs
-            errors.append(
-                SupersetError(
-                    message="Sheet name is required",
-                    error_type=SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR,
-                    level=ErrorLevel.WARNING,
-                    extra={"catalog": {"idx": 0, "name": True}},
-                ),
-            )
-            return errors
-
         # We need a subject in case domain wide delegation is set, otherwise the
         # check will fail. This means that the admin will be able to add sheets
         # that only they have access, even if later users are not able to access

superset/models/core.py

@@ -38,6 +38,7 @@
 import sshtunnel
 from flask import g, request
 from flask_appbuilder import Model
+from marshmallow.exceptions import ValidationError
 from sqlalchemy import (
     Boolean,
     Column,
@@ -122,7 +123,9 @@ class ConfigurationMethod(StrEnum):
     DYNAMIC_FORM = "dynamic_form"
 
 
-class Database(Model, AuditMixinNullable, ImportExportMixin):  # pylint: disable=too-many-public-methods
+class Database(
+    Model, AuditMixinNullable, ImportExportMixin
+):  # pylint: disable=too-many-public-methods
     """An ORM object that stores Database related information"""
 
     __tablename__ = "dbs"
@@ -397,9 +400,7 @@ def get_effective_user(self, object_url: URL) -> str | None:
         return (
             username
             if (username := get_username())
-            else object_url.username
-            if self.impersonate_user
-            else None
+            else object_url.username if self.impersonate_user else None
         )
 
     @contextmanager
@@ -1133,9 +1134,13 @@ def is_oauth2_enabled(self) -> bool:
         admins to create custom OAuth2 clients from the Superset UI, and assign them to
         specific databases.
         """
-        encrypted_extra = json.loads(self.encrypted_extra or "{}")
-        oauth2_client_info = encrypted_extra.get("oauth2_client_info", {})
-        return bool(oauth2_client_info) or self.db_engine_spec.is_oauth2_enabled()
+        try:
+            client_config = self.get_oauth2_config()
+        except ValidationError:
+            logger.warning("Invalid OAuth2 client configuration for database %s", self)
+            client_config = None
+
+        return client_config or self.db_engine_spec.is_oauth2_enabled()
 
     def get_oauth2_config(self) -> OAuth2ClientConfig | None:
         """

superset/sqllab/api.py

@@ -415,6 +415,9 @@ def execute_sql_query(self) -> FlaskResponse:
             )
             # return the execution result without special encoding
             return json_success(command_result["payload"], response_status)
+        except Exception as ex:
+            print(ex)
+            raise
         except SqlLabException as ex:
             payload = {"errors": [ex.to_dict()]}
 

superset/sqllab/sql_json_executer.py

@@ -111,7 +111,6 @@ def execute(
                     [SupersetError(**params) for params in data["errors"]]  # type: ignore
                 )
             # old string-only error message
-            print(data)
             raise SupersetGenericDBErrorException(data["error"])  # type: ignore
 
         return SqlJsonExecutionStatus.HAS_RESULTS