rusackas commented on code in PR #41421:
URL: https://github.com/apache/superset/pull/41421#discussion_r3473555671


##########
superset/db_engine_specs/databricks.py:
##########
@@ -474,6 +545,76 @@ class 
DatabricksNativeEngineSpec(DatabricksDynamicBaseEngineSpec):
     supports_dynamic_catalog = True
     supports_cross_catalog_queries = True
 
+    # OAuth 2.0 support
+    supports_oauth2 = True
+    oauth2_exception = OAuth2RedirectError
+    oauth2_scope = "sql"
+
+    # OAuth2 endpoints are determined dynamically based on cloud provider
+    oauth2_authorization_request_uri = ""  # Set dynamically
+    oauth2_token_request_uri = ""  # Set dynamically
+
+    @classmethod
+    def get_oauth2_authorization_uri(
+        cls,
+        config: "OAuth2ClientConfig",
+        state: "OAuth2State",
+        code_verifier: str | None = None,
+    ) -> str:
+        """
+        Return URI for initial OAuth2 request with dynamic endpoint detection.
+        """
+        from superset import db
+        from superset.models.core import Database
+
+        # Get the database to detect cloud provider
+        database_id = state["database_id"]
+        if database := db.session.get(Database, database_id):
+            provider = cls._detect_cloud_provider(database)
+            # Update config with the correct authorization URI for the cloud 
provider
+            from typing import cast
+
+            config = cast(
+                "OAuth2ClientConfig",
+                dict(config)
+                | {
+                    "authorization_request_uri": 
cls._oauth2_endpoints[provider][
+                        "authorization_request_uri"
+                    ]
+                },
+            )

Review Comment:
   Good catch — the `{}` in the provider templates never gets an `account_id` 
substituted, so the per-cloud fallback URI is genuinely broken. The detection 
works, the substitution does not. I need to wire `account_id` through from 
config/extra before this is safe to merge.



##########
superset/db_engine_specs/databricks.py:
##########
@@ -685,6 +826,76 @@ class 
DatabricksPythonConnectorEngineSpec(DatabricksDynamicBaseEngineSpec):
 
     supports_dynamic_schema = supports_catalog = supports_dynamic_catalog = 
True
 
+    # OAuth 2.0 support
+    supports_oauth2 = True
+    oauth2_exception = OAuth2RedirectError
+    oauth2_scope = "sql"
+
+    # OAuth2 endpoints are determined dynamically based on cloud provider
+    oauth2_authorization_request_uri = ""  # Set dynamically
+    oauth2_token_request_uri = ""  # Set dynamically
+
+    @classmethod
+    def get_oauth2_authorization_uri(
+        cls,
+        config: "OAuth2ClientConfig",
+        state: "OAuth2State",
+        code_verifier: str | None = None,
+    ) -> str:
+        """
+        Return URI for initial OAuth2 request with dynamic endpoint detection.
+        """
+        from superset import db
+        from superset.models.core import Database
+
+        # Get the database to detect cloud provider
+        database_id = state["database_id"]
+        if database := db.session.get(Database, database_id):
+            provider = cls._detect_cloud_provider(database)
+            # Update config with the correct authorization URI for the cloud 
provider
+            from typing import cast
+
+            config = cast(
+                "OAuth2ClientConfig",
+                dict(config)
+                | {
+                    "authorization_request_uri": 
cls._oauth2_endpoints[provider][
+                        "authorization_request_uri"
+                    ]
+                },
+            )

Review Comment:
   Same root cause as the legacy spec — the provider template keeps a literal 
`{}`. Tracking it on the other thread.



##########
docs/user_docs_versioned_docs/version-6.0.0/configuration/databases.mdx:
##########
@@ -519,6 +519,82 @@ For a connection to a SQL endpoint you need to use the 
HTTP path from the endpoi
 {"connect_args": {"http_path": "/sql/1.0/endpoints/****", "driver_path": 
"/path/to/odbc/driver"}}
 ```
 
+##### OAuth2 Authentication
+
+Superset supports OAuth2 authentication for Databricks, allowing users to 
authenticate with their personal Databricks accounts instead of using shared 
access tokens. This provides better security and audit capabilities.
+
+###### Prerequisites
+
+1. Create an OAuth2 application in your Databricks account:
+   - Go to your Databricks account console
+   - Navigate to **Settings** → **Developer** → **OAuth apps**
+   - Create a new OAuth app with the redirect URI: 
`http://your-superset-host:port/api/v1/database/oauth2/`
+
+2. Configure OAuth2 in your `superset_config.py`:
+
+```python
+from datetime import timedelta
+
+# OAuth2 configuration for Databricks
+# OAuth2 endpoints are automatically detected based on your Databricks cloud 
provider
+DATABASE_OAUTH2_CLIENTS = {
+    "Databricks (legacy)": {
+        "id": "your-databricks-client-id",
+        "secret": "your-databricks-client-secret",
+        "scope": "sql",
+        # OAuth2 endpoints are auto-detected based on hostname, but can be 
overridden:
+        # AWS: "authorization_request_uri": 
"https://accounts.cloud.databricks.com/oidc/accounts/{account_id}/v1/authorize";,
+        # Azure: "authorization_request_uri": 
"https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/authorize";,
+        # GCP: "authorization_request_uri": 
"https://accounts.gcp.databricks.com/oidc/accounts/{account_id}/v1/authorize";,

Review Comment:
   Right — `account_id` is in the URL templates but never read or substituted 
from config. This is the real gap; the cloud-provider detection works but the 
placeholder resolution does not.



##########
superset/db_engine_specs/databricks.py:
##########
@@ -474,6 +545,76 @@ class 
DatabricksNativeEngineSpec(DatabricksDynamicBaseEngineSpec):
     supports_dynamic_catalog = True
     supports_cross_catalog_queries = True
 
+    # OAuth 2.0 support
+    supports_oauth2 = True
+    oauth2_exception = OAuth2RedirectError
+    oauth2_scope = "sql"
+
+    # OAuth2 endpoints are determined dynamically based on cloud provider
+    oauth2_authorization_request_uri = ""  # Set dynamically
+    oauth2_token_request_uri = ""  # Set dynamically
+
+    @classmethod
+    def get_oauth2_authorization_uri(
+        cls,
+        config: "OAuth2ClientConfig",
+        state: "OAuth2State",
+        code_verifier: str | None = None,
+    ) -> str:
+        """
+        Return URI for initial OAuth2 request with dynamic endpoint detection.
+        """
+        from superset import db
+        from superset.models.core import Database
+
+        # Get the database to detect cloud provider
+        database_id = state["database_id"]
+        if database := db.session.get(Database, database_id):
+            provider = cls._detect_cloud_provider(database)
+            # Update config with the correct authorization URI for the cloud 
provider
+            from typing import cast
+
+            config = cast(
+                "OAuth2ClientConfig",
+                dict(config)
+                | {
+                    "authorization_request_uri": 
cls._oauth2_endpoints[provider][
+                        "authorization_request_uri"
+                    ]
+                },
+            )
+
+        return super().get_oauth2_authorization_uri(config, state, 
code_verifier)
+
+    @classmethod
+    def get_oauth2_token(
+        cls,
+        config: "OAuth2ClientConfig",
+        code: str,
+        code_verifier: str | None = None,
+    ) -> "OAuth2TokenResponse":
+        """
+        Exchange authorization code for refresh/access tokens with dynamic 
endpoint.
+
+        The token request URI is resolved when the OAuth2 config is built (see
+        `get_oauth2_config`), so it already targets the correct cloud provider 
and
+        account. Only fall back to the AWS endpoint when no URI is configured.
+        """
+        from typing import cast
+
+        if not config.get("token_request_uri"):
+            config = cast(
+                "OAuth2ClientConfig",
+                dict(config)
+                | {
+                    "token_request_uri": cls._oauth2_endpoints["aws"][
+                        "token_request_uri"
+                    ]
+                },
+            )

Review Comment:
   Same unresolved-`{}` issue as the auth URI — the AWS token fallback needs a 
real `account_id` too. Part of the same fix.



##########
superset/db_engine_specs/databricks.py:
##########
@@ -685,6 +826,76 @@ class 
DatabricksPythonConnectorEngineSpec(DatabricksDynamicBaseEngineSpec):
 
     supports_dynamic_schema = supports_catalog = supports_dynamic_catalog = 
True
 
+    # OAuth 2.0 support
+    supports_oauth2 = True
+    oauth2_exception = OAuth2RedirectError
+    oauth2_scope = "sql"
+
+    # OAuth2 endpoints are determined dynamically based on cloud provider
+    oauth2_authorization_request_uri = ""  # Set dynamically
+    oauth2_token_request_uri = ""  # Set dynamically
+
+    @classmethod
+    def get_oauth2_authorization_uri(
+        cls,
+        config: "OAuth2ClientConfig",
+        state: "OAuth2State",
+        code_verifier: str | None = None,
+    ) -> str:
+        """
+        Return URI for initial OAuth2 request with dynamic endpoint detection.
+        """
+        from superset import db
+        from superset.models.core import Database
+
+        # Get the database to detect cloud provider
+        database_id = state["database_id"]
+        if database := db.session.get(Database, database_id):
+            provider = cls._detect_cloud_provider(database)
+            # Update config with the correct authorization URI for the cloud 
provider
+            from typing import cast
+
+            config = cast(
+                "OAuth2ClientConfig",
+                dict(config)
+                | {
+                    "authorization_request_uri": 
cls._oauth2_endpoints[provider][
+                        "authorization_request_uri"
+                    ]
+                },
+            )
+
+        return super().get_oauth2_authorization_uri(config, state, 
code_verifier)
+
+    @classmethod
+    def get_oauth2_token(
+        cls,
+        config: "OAuth2ClientConfig",
+        code: str,
+        code_verifier: str | None = None,
+    ) -> "OAuth2TokenResponse":
+        """
+        Exchange authorization code for refresh/access tokens with dynamic 
endpoint.
+
+        The token request URI is resolved when the OAuth2 config is built (see
+        `get_oauth2_config`), so it already targets the correct cloud provider 
and
+        account. Only fall back to the AWS endpoint when no URI is configured.
+        """
+        from typing import cast
+
+        if not config.get("token_request_uri"):
+            config = cast(
+                "OAuth2ClientConfig",
+                dict(config)
+                | {
+                    "token_request_uri": cls._oauth2_endpoints["aws"][
+                        "token_request_uri"
+                    ]
+                },
+            )

Review Comment:
   Same root cause as the other token-fallback thread — the template 
placeholder is never resolved. Folding into the same fix.



-- 
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]

Reply via email to