EnxDev commented on code in PR #44695:
URL: https://github.com/apache/superset/pull/44695#discussion_r4137490371
##########
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:
Might be misreading the merge, but I don't think queries actually send this
subject when the credentials are a service account. `impersonate_user` puts it
on the URL query, the dialect's `create_connect_args` picks it up into
`adapter_kwargs`, and then `update_params_from_encrypted_extra` supplies its
own `connect_args["adapter_kwargs"]`. SQLAlchemy's `union` at
`sqlalchemy/engine/create.py:634` is shallow, so that key replaces the
dialect's and the subject is lost.
If that's right, it would explain why modal-created DBs (always
`impersonate_user=true`) query fine on a non-DWD service account. It also means
"as queries do" isn't quite accurate here. Worth confirming on your live setup
whether a query with impersonation on really goes out with the subject?
##########
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:
The modal forces `impersonate_user = true` for every gsheets save
(`DatabaseModal/index.tsx:1051`), and on edit `getValidation` sends the stored
flag back. So opening an existing DB and adding a sheet still sets the subject
here, and a non-DWD service account still gets "The URL could not be
identified", the same failure this PR is fixing.
Your live test ran with impersonation off, which I think you only get
through the API. Could you try create then edit in the modal? If the subject
really never reaches queries, `subject = None` everywhere might be the more
consistent fix.
--
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]
