This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 542e4661837 fix(gsheets): pass the OAuth2 token and delegation subject
through connect_args (#44709)
542e4661837 is described below
commit 542e466183794860fdf3c344c6e918bbebc30bc8
Author: Amin Ghadersohi <[email protected]>
AuthorDate: Wed Sep 30 15:17:20 2026 +1000
fix(gsheets): pass the OAuth2 token and delegation subject through
connect_args (#44709)
---
superset/db_engine_specs/gsheets.py | 46 ++++++-
superset/translations/messages.pot | 5 +
tests/unit_tests/db_engine_specs/test_gsheets.py | 160 ++++++++++++++++++++++-
3 files changed, 208 insertions(+), 3 deletions(-)
diff --git a/superset/db_engine_specs/gsheets.py
b/superset/db_engine_specs/gsheets.py
index 4ff0b73e868..b1edb0cceb0 100644
--- a/superset/db_engine_specs/gsheets.py
+++ b/superset/db_engine_specs/gsheets.py
@@ -87,6 +87,11 @@ class GSheetsParametersSchema(Schema):
)
+# Secure-extra key a service-account connection sets to impersonate the
+# logged-in user through Google Workspace domain-wide delegation.
+DELEGATION_KEY = "domain_wide_delegation"
+
+
class GSheetsParametersType(TypedDict, total=False):
service_account_info: str
catalog: dict[str, str] | None
@@ -261,7 +266,43 @@ class GSheetsEngineSpec(ShillelaghEngineSpec):
url = url.update_query_dict({"subject": email})
if user_token:
- url = url.update_query_dict({"access_token": user_token})
+ # Pass the token through ``connect_args`` rather than the URL.
+ # ``update_params_from_encrypted_extra`` stores the catalog (and
any
+ # service account) in ``connect_args["adapter_kwargs"]``, and
SQLAlchemy
+ # merges ``connect_args`` over the dialect's own arguments
shallowly,
+ # so a token in the URL would be dropped and the query would run
+ # without credentials. For the same reason a ``subject`` set on
the URL
+ # above is carried over, so it isn't dropped either.
+ connect_args = engine_kwargs.setdefault("connect_args", {})
+ adapter_kwargs = connect_args.setdefault("adapter_kwargs", {})
+ gsheetsapi_kwargs = adapter_kwargs.setdefault("gsheetsapi", {})
+ gsheetsapi_kwargs["access_token"] = user_token
+ if subject := url.query.get("subject"):
+ gsheetsapi_kwargs.setdefault("subject", subject)
+
+ encrypted_extra = database.get_encrypted_extra()
+ if encrypted_extra.get("service_account_info"):
+ # A service account from the secure extra always sits in
+ # ``connect_args``, so a ``subject`` on the URL never reaches
+ # shillelagh and the query runs as the service account itself.
+ # Honouring it for every such connection would break service
+ # accounts without Google Workspace domain-wide delegation (the
+ # connection form enables impersonation for every Google Sheets
+ # database), so the connection has to opt in explicitly.
+ subject = url.query.get("subject")
+ url = url.difference_update_query(["subject"])
+ if not user_token and encrypted_extra.get(DELEGATION_KEY) is True:
+ if not subject:
+ # Never fall back to the service account's own access.
+ raise SupersetException(
+ __(
+ "This Google Sheets connection impersonates the "
+ "logged-in user, who has no e-mail address."
+ )
+ )
+ connect_args = engine_kwargs.setdefault("connect_args", {})
+ adapter_kwargs = connect_args.setdefault("adapter_kwargs", {})
+ adapter_kwargs.setdefault("gsheetsapi", {})["subject"] =
subject
return url, engine_kwargs
@@ -334,6 +375,9 @@ class GSheetsEngineSpec(ShillelaghEngineSpec):
if "oauth2_client_info" in params:
del params["oauth2_client_info"]
+ # Read by ``impersonate_user``; not a shillelagh argument.
+ params.pop(DELEGATION_KEY, None)
+
if "service_account_info" in params:
sa_info = params.pop("service_account_info")
connect_args = params.setdefault("connect_args", {})
diff --git a/superset/translations/messages.pot
b/superset/translations/messages.pot
index 3ec1a0a4fe2..151695e6a36 100644
--- a/superset/translations/messages.pot
+++ b/superset/translations/messages.pot
@@ -15188,6 +15188,11 @@ msgstr ""
msgid "This CSS uses @import, which cannot be saved"
msgstr ""
+msgid ""
+"This Google Sheets connection impersonates the logged-in user, who has no "
+"e-mail address."
+msgstr ""
+
msgid ""
"This JSON object is generated dynamically when clicking the save or "
"overwrite button in the dashboard view. It is exposed here for reference "
diff --git a/tests/unit_tests/db_engine_specs/test_gsheets.py
b/tests/unit_tests/db_engine_specs/test_gsheets.py
index 86321f6ecb9..52f724c1e33 100644
--- a/tests/unit_tests/db_engine_specs/test_gsheets.py
+++ b/tests/unit_tests/db_engine_specs/test_gsheets.py
@@ -17,18 +17,24 @@
# pylint: disable=import-outside-toplevel, invalid-name, line-too-long
+# ``GSheetsEngineSpec`` is imported inside each test on purpose: its module
+# binds ``superset.db`` and ``superset.security_manager`` at import time, so it
+# has to load after the app fixture has initialized them.
+
from __future__ import annotations
from datetime import datetime
from typing import Any, TYPE_CHECKING
+from unittest.mock import MagicMock
from urllib.parse import parse_qs, urlparse
import pandas as pd
import pytest
+import sqlalchemy
from pytest_mock import MockerFixture
from requests.exceptions import HTTPError
from shillelagh.exceptions import UnauthenticatedError
-from sqlalchemy.engine.url import make_url
+from sqlalchemy.engine.url import make_url, URL
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import OAuth2TokenRefreshError, SupersetException
@@ -574,6 +580,7 @@ def test_impersonate_user_username(mocker: MockerFixture)
-> None:
from superset.db_engine_specs.gsheets import GSheetsEngineSpec
database = mocker.MagicMock()
+ database.get_encrypted_extra.return_value = {}
database.get_impersonation_email.return_value = "[email protected]"
url = make_url("gsheets://")
@@ -633,6 +640,7 @@ def test_impersonate_user_access_token(mocker:
MockerFixture) -> None:
from superset.db_engine_specs.gsheets import GSheetsEngineSpec
database = mocker.MagicMock()
+ database.get_encrypted_extra.return_value = {}
assert GSheetsEngineSpec.impersonate_user(
database,
@@ -640,7 +648,78 @@ def test_impersonate_user_access_token(mocker:
MockerFixture) -> None:
user_token="access-token", # noqa: S106
url=make_url("gsheets://"),
engine_kwargs={},
- ) == (make_url("gsheets://?access_token=access-token"), {})
+ ) == (
+ make_url("gsheets://"),
+ {
+ "connect_args": {
+ "adapter_kwargs": {"gsheetsapi": {"access_token":
"access-token"}}
+ }
+ },
+ )
+
+
+def test_impersonate_user_access_token_with_catalog(mocker: MockerFixture) ->
None:
+ """
+ Test that the access token reaches the adapter when a catalog is
configured.
+
+ The catalog is stored in ``connect_args``, which SQLAlchemy merges
shallowly
+ over the dialect arguments built from the URL.
+ """
+ from superset.db_engine_specs.gsheets import GSheetsEngineSpec
+
+ database: MagicMock = mocker.MagicMock(encrypted_extra=None)
+ database.get_encrypted_extra.return_value = {}
+ catalog = {"sheet": "https://docs.google.com/spreadsheets/d/1/edit#gid=0"}
+ url, engine_kwargs = GSheetsEngineSpec.impersonate_user(
+ database,
+ username=None,
+ user_token="access-token", # noqa: S106
+ url=make_url("gsheets://"),
+ engine_kwargs={"catalog": catalog},
+ )
+ GSheetsEngineSpec.update_params_from_encrypted_extra(database,
engine_kwargs)
+
+ engine = sqlalchemy.create_engine(url, **engine_kwargs)
+ connect = mocker.patch.object(engine.dialect, "connect")
+ engine.pool._creator()
+
+ adapter_kwargs = connect.call_args.kwargs["adapter_kwargs"]["gsheetsapi"]
+ assert adapter_kwargs["access_token"] == "access-token" # noqa: S105
+ assert adapter_kwargs["catalog"] == catalog
+
+
+def test_impersonate_user_username_and_access_token(mocker: MockerFixture) ->
None:
+ """
+ Test that ``subject`` and the access token both reach the adapter
alongside a
+ catalog.
+
+ The catalog puts its own ``adapter_kwargs`` in ``connect_args``, which
+ SQLAlchemy merges shallowly over the URL-derived ones, so anything left
only
+ on the URL would be dropped.
+ """
+ from superset.db_engine_specs.gsheets import GSheetsEngineSpec
+
+ database: MagicMock = mocker.MagicMock(encrypted_extra=None)
+ database.get_impersonation_email.return_value = "[email protected]"
+ database.get_encrypted_extra.return_value = {}
+ catalog = {"sheet": "https://docs.google.com/spreadsheets/d/1/edit#gid=0"}
+ url, engine_kwargs = GSheetsEngineSpec.impersonate_user(
+ database,
+ username="alice",
+ user_token="access-token", # noqa: S106
+ url=make_url("gsheets://"),
+ engine_kwargs={"catalog": catalog},
+ )
+ GSheetsEngineSpec.update_params_from_encrypted_extra(database,
engine_kwargs)
+
+ engine = sqlalchemy.create_engine(url, **engine_kwargs)
+ connect = mocker.patch.object(engine.dialect, "connect")
+ engine.pool._creator()
+
+ adapter_kwargs = connect.call_args.kwargs["adapter_kwargs"]["gsheetsapi"]
+ assert adapter_kwargs["access_token"] == "access-token" # noqa: S105
+ assert adapter_kwargs["subject"] == "[email protected]"
+ assert adapter_kwargs["catalog"] == catalog
def test_is_oauth2_enabled_no_config(mocker: MockerFixture) -> None:
@@ -1193,3 +1272,80 @@ def test_convert_dttm(
from superset.db_engine_specs.gsheets import GSheetsEngineSpec
assert_convert_dttm(GSheetsEngineSpec, target_type, expected_result, dttm)
+
+
+def _service_account_adapter_kwargs(
+ mocker: MockerFixture,
+ encrypted_extra: dict[str, Any],
+ email: str | None = "[email protected]",
+) -> tuple[URL, dict[str, Any]]:
+ """What reaches shillelagh for a secure-extra service account,
impersonating."""
+ from superset.db_engine_specs.gsheets import GSheetsEngineSpec
+
+ database: MagicMock =
mocker.MagicMock(encrypted_extra=json.dumps(encrypted_extra))
+ database.get_impersonation_email.return_value = email
+ database.get_encrypted_extra.return_value = encrypted_extra
+ url, engine_kwargs = GSheetsEngineSpec.impersonate_user(
+ database,
+ username="alice",
+ user_token=None,
+ url=make_url("gsheets://"),
+ engine_kwargs={},
+ )
+ GSheetsEngineSpec.update_params_from_encrypted_extra(database,
engine_kwargs)
+
+ engine = sqlalchemy.create_engine(url, **engine_kwargs)
+ connect = mocker.patch.object(engine.dialect, "connect")
+ engine.pool._creator()
+ return url, connect.call_args.kwargs["adapter_kwargs"]["gsheetsapi"]
+
+
+def test_impersonate_user_service_account_without_delegation(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Without the opt-in, a secure-extra service account is not asked to
impersonate
+ the user, and no ``subject`` is left on the URL to suggest otherwise.
+ """
+ service_account = {"type": "service_account", "client_email":
"[email protected]"}
+ url, adapter_kwargs = _service_account_adapter_kwargs(
+ mocker, {"service_account_info": service_account}
+ )
+
+ assert "subject" not in url.query
+ assert adapter_kwargs.get("subject") is None
+ assert adapter_kwargs["service_account_info"] == service_account
+
+
+def test_impersonate_user_service_account_with_delegation(
+ mocker: MockerFixture,
+) -> None:
+ """
+ With ``domain_wide_delegation`` the user reaches shillelagh as the subject,
+ instead of being dropped by the shallow ``connect_args`` merge.
+ """
+ service_account = {"type": "service_account", "client_email":
"[email protected]"}
+ url, adapter_kwargs = _service_account_adapter_kwargs(
+ mocker,
+ {"service_account_info": service_account, "domain_wide_delegation":
True},
+ )
+
+ assert adapter_kwargs["subject"] == "[email protected]"
+ assert adapter_kwargs["service_account_info"] == service_account
+ assert "domain_wide_delegation" not in adapter_kwargs
+ assert "subject" not in url.query
+
+
+def test_impersonate_user_service_account_delegation_needs_an_email(
+ mocker: MockerFixture,
+) -> None:
+ """A delegated connection never falls back to the service account's
access."""
+ with pytest.raises(SupersetException, match="no e-mail"):
+ _service_account_adapter_kwargs(
+ mocker,
+ {
+ "service_account_info": {"type": "service_account"},
+ "domain_wide_delegation": True,
+ },
+ email=None,
+ )
