bito-code-review[bot] commented on code in PR #44453:
URL: https://github.com/apache/superset/pull/44453#discussion_r4056046006
##########
superset/db_engine_specs/bigquery.py:
##########
@@ -973,6 +1055,111 @@ def get_dbapi_exception_mapping(cls) ->
dict[type[Exception], type[Exception]]:
return {DefaultCredentialsError: SupersetDBAPIConnectionError}
+ @staticmethod
+ def update_params_from_encrypted_extra(
+ database: Database,
+ params: dict[str, Any],
+ ) -> None:
+ """
+ Forward the secure extra to the dialect without `oauth2_client_info`.
+
+ The OAuth2 client configuration is consumed by Superset itself
+ (``Database.get_oauth2_config``); sqlalchemy-bigquery rejects unknown
+ engine arguments.
+ """
+ BaseEngineSpec.update_params_from_encrypted_extra(database, params)
+ params.pop("oauth2_client_info", None)
+
+ @classmethod
+ def _get_oauth2_user_token(cls, database: Database) -> str | None:
+ """
+ Return the current user's OAuth2 access token for the database, if any.
+ """
+ if not database.impersonate_user or database.id is None:
+ return None
+ if not (g and hasattr(g, "user") and getattr(g.user, "id", None) is
not None):
+ return None
+ oauth2_config = database.get_oauth2_config()
+ if oauth2_config is None:
+ return None
+ return get_oauth2_access_token(oauth2_config, database.id, g.user.id,
cls)
+
+ @classmethod
+ def impersonate_user(
+ cls,
+ database: Database,
+ username: str | None,
+ user_token: str | None,
+ url: URL,
+ engine_kwargs: dict[str, Any],
+ ) -> tuple[URL, dict[str, Any]]:
+ """
+ Run the connection as the user, with their personal OAuth2 access
token.
+
+ The token is wrapped in a ``bigquery.Client`` handed to
sqlalchemy-bigquery
+ through ``connect_args["client"]`` together with
``user_supplied_client=true``
+ in the URL, which is the driver's documented way of bringing your own
+ credentials. BigQuery has no notion of a proxy user, so ``username``
is not
+ used.
+
+ Without a token, sqlalchemy-bigquery would silently fall back to the
+ service account or ADC, i.e. run the query as Superset rather than as
the
+ user. When OAuth2 is configured the user is asked to authorize instead.
+ Background jobs (no user) and unsaved databases (test connection)
cannot
+ complete the OAuth2 dance and keep the previous behaviour.
+ """
+ if not user_token:
+ if (
+ database.is_oauth2_enabled()
+ and database.id is not None
+ and g
+ and hasattr(g, "user")
+ and getattr(g.user, "id", None) is not None
+ ):
+ database.start_oauth2_dance()
+ return url, engine_kwargs
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-250: Silent Privilege Fallback</b></div>
<div id="fix">
`impersonate_user` silently returns unchanged kwargs when `user_token` is
empty and the dance conditions fail. In Celery workers `g.user` is absent (the
`hasattr(g, "user")` gate in `Database._get_sqla_engine` yields
`access_token=None`), so scheduled queries/reports on an impersonation-enabled
database run under the service account/ADC — not the user's IAM — contradicting
the per-user promise in the `supports_oauth2` comment and `df_to_sql`'s "don't
fall back to ADC" intent. Consider failing closed for saved databases.
([CWE-250](https://cwe.mitre.org/data/definitions/250.html))
</div>
</div>
<small><i>Code Review Run #2b2b0b</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]
