aminghadersohi commented on code in PR #44695:
URL: https://github.com/apache/superset/pull/44695#discussion_r4139863919
##########
superset/db_engine_specs/gsheets.py:
##########
@@ -411,11 +460,13 @@ def validate_parameters(
)
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
- # them.
- subject = g.user.email if g.user else None
+ # Impersonate the admin only when the database impersonates users, as
+ # queries do (see ``impersonate_user``). With domain wide delegation
this
Review Comment:
You're right; my earlier explanation stopped at the URL and missed the
shallow merge. Fixed in 4954f82e9910ecc6d9ca0ae13821f6c95537582a by making
validation use `subject=None`, preserving the existing service-account query
behavior rather than enabling delegation implicitly.
The query path calls impersonation before encrypted-extra processing
(`superset/models/core.py:685-694`); the latter supplies
`connect_args.adapter_kwargs` (`superset/db_engine_specs/gsheets.py:380-392`).
The new `test_query_service_account_subject`
(`tests/unit_tests/db_engine_specs/test_gsheets.py:1287-1324`) creates a real
engine through `Database._get_sqla_engine`, executes a literal query, and spies
on the actual DBAPI `connect` arguments. With impersonation enabled, the URL
contains the admin email but the final adapter arguments contain the
service-account credentials/catalog and no subject. The disabled case is
covered too.
Confirmed locally with SQLAlchemy 2.0.52 / shillelagh 1.4.5; this is an
actual connection-argument test, not a live Google request. All 63 GSheets
tests pass, and changed-file pre-commit passes, including mypy. The misleading
validation comment is corrected at `gsheets.py:463-473`.
##########
superset/db_engine_specs/gsheets.py:
##########
@@ -411,11 +460,13 @@ def validate_parameters(
)
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
- # them.
- subject = g.user.email if g.user else None
+ # Impersonate the admin only when the database impersonates users, as
+ # queries do (see ``impersonate_user``). With domain wide delegation
this
+ # means that the admin will be able to add sheets that only they have
+ # access to; without delegation a subject makes every check fail.
+ subject = (
+ g.user.email if g.user and properties.get("impersonate_user") else
None
Review Comment:
Fixed in 4954f82e9910ecc6d9ca0ae13821f6c95537582a. Validation now always
passes `subject=None` (`superset/db_engine_specs/gsheets.py:463-473`), so the
modal's forced/stored `impersonate_user=true` no longer causes a non-delegated
service account to validate as the admin. No frontend change is needed, and
query-time behavior is unchanged.
`test_validate_parameters_service_account_subject`
(`tests/unit_tests/db_engine_specs/test_gsheets.py:1240-1284`) covers
omitted/false/true flags and both serialized create credentials and dictionary
edit credentials, with a nonempty catalog and a logged-in admin. Both true-flag
cases fail against d375346e and pass with this commit. The separate real-engine
test confirms that this matches the final query connection arguments.
I did not run a live browser/Google create-edit flow; these are backend
regressions for its payloads. The GSheets suite passes all 63 tests and
changed-file pre-commit passes. The service-account sharing requirement and
delegation limitation are documented in
`docs/admin_docs/configuration/google-sheets.mdx`.
--
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]
