bito-code-review[bot] commented on code in PR #44709:
URL: https://github.com/apache/superset/pull/44709#discussion_r4121345943
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant Local Import</b></div>
<div id="fix">
`SupersetException` is already imported at module scope (`from
superset.exceptions import OAuth2TokenRefreshError, SupersetException`), so the
local import in
`test_impersonate_user_service_account_delegation_needs_an_email` is redundant.
Unlike the per-test local imports of `GSheetsEngineSpec`/`create_engine`
(symbols not bound at module level), this one re-imports an existing module
binding. Drop the local import and use the module-level name.
</div>
</div>
<small><i>Code Review Run #8c104f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
