aminghadersohi commented on PR #43309:
URL: https://github.com/apache/superset/pull/43309#issuecomment-5345474818
@Vitor-Avila you're right, and this invalidates a claim I made in the PR
description. Thanks for catching it — I traced it properly and the docs you
linked describe real behavior, not just connection creation.
**Query execution does use the logged-in user's email.** The chain:
1.
`superset-frontend/src/features/databases/DatabaseModal/index.tsx#L1051-L1053`
— the modal unconditionally sets `impersonate_user = true` for any GSheets
connection on save (`// this needs to be added by default to gsheets`). So
every connection created through the UI has it on, regardless of the checkbox.
2. `superset/models/core.py#L677` — `if self.impersonate_user:` →
`db_engine_spec.impersonate_user(...)` with `effective_username` from
`get_username()`, i.e. the logged-in user.
3. `GSheetsEngineSpec.impersonate_user` → `url.update_query_dict({"subject":
user.email})`.
4. shillelagh's GSheets dialect merges the URL query into adapter kwargs
(`create_connect_args` → `adapter_kwargs.update(extract_query(url))`), so
`subject` reaches the adapter and forces domain-wide delegation at query time
too.
So my "`subject` isn't passed at query time" statement only holds for
connections created via the API without `impersonate_user`, not for the modal
path this bug is about. Your first bullet is the accurate outcome for this PR
as it stands: the connection would be created and then queries would fail.
That's not a good trade against the current failure mode, so I'm reworking it
rather than asking for a merge.
On your second bullet — this PR can't affect query execution at all, since
`validate_parameters` is only reachable from `POST
/api/v1/database/validate_parameters/` and nothing in it feeds engine creation.
So there's no OAuth2 bypass. But that's exactly the flaw: it makes validation
diverge from execution, which is the wrong direction.
**On gating it behind the checkbox** — I think that's the correct narrow
fix, and the plumbing already exists: `impersonate_user` is part of the
validate-parameters payload (`DatabaseValidateParametersSchema`,
`superset/databases/schemas.py#L481`) and the full properties dict is handed to
`validate_parameters` unmodified
(`superset/commands/database/validate.py#L80`). So validation can read the real
setting and pass `subject` only when impersonation is on, which makes it mirror
execution exactly instead of guessing. That change is only coherent alongside
dropping the forced `impersonate_user = true` at `index.tsx#L1051`, otherwise
the flag is always `true` and nothing changes — which also matches your
recollection that you couldn't disable the checkbox for GSheets. I'll
restructure the PR that way unless you'd rather see it split into backend and
frontend changes.
**On deprecating SA-based impersonation via a SIP** — that's a maintainer
call and I'd defer to you and @betodealmeida. I'd only note it's orthogonal to
this bug: whichever way that lands, today a service account without domain-wide
delegation fails with `The URL could not be identified... 'Type of Google
Sheets allowed' selection matches the input`, which points the user at their
URL and their sheet-type dropdown when the actual cause is an `invalid_grant`
from the delegation lookup. Even if SA + DWD is deprecated, surfacing the real
error rather than a misleading one seems worth doing on its own, and I'm happy
to narrow this PR to just that if the broader behavior change should wait for a
SIP.
Keeping this in draft in the meantime.
--
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]
