bito-code-review[bot] commented on code in PR #44709:
URL: https://github.com/apache/superset/pull/44709#discussion_r4121924127
##########
tests/unit_tests/db_engine_specs/test_gsheets.py:
##########
@@ -1155,3 +1231,89 @@ 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 sqlalchemy import create_engine
+
+ from superset.db_engine_specs.gsheets import GSheetsEngineSpec
+
+ user = mocker.MagicMock()
+ user.email = email
+ mocker.patch(
+ "superset.db_engine_specs.gsheets.security_manager.find_user",
+ return_value=user,
+ )
+ database = mocker.MagicMock(encrypted_extra=json.dumps(encrypted_extra))
+ 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 = 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."""
+ from superset.exceptions import SupersetException
Review Comment:
<!-- Bito Reply -->
The redundant local import of `SupersetException` has been successfully
removed, and the unit tests and pre-commit checks are passing as expected.
--
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]
