aminghadersohi opened a new pull request, #43309: URL: https://github.com/apache/superset/pull/43309
## Why Adding a Google Sheets connection in "Public and privately shared sheets" mode with a service-account JSON is rejected in the UI with `The URL could not be identified. Please check for typos and make sure that 'Type of Google Sheets allowed' selection matches the input.` — even when the URL is correct and the service account can read the sheet. `GSheetsEngineSpec.validate_parameters` unconditionally passed `subject = g.user.email` to shillelagh's `gsheetsapi` adapter. Passing a `subject` makes Google authenticate through **domain-wide delegation**, impersonating that user. A service account without domain-wide delegation configured gets back `invalid_grant: Invalid email or User ID`, the validation `SELECT` fails, and the failure surfaces as the misleading `TABLE_DOES_NOT_EXIST_ERROR` above. Domain-wide delegation is a Google Workspace admin-only setting. The common setup — create a service account, share the sheet with its email — could therefore never pass validation, making the connection impossible to create through the modal. `POST /api/v1/database/` does not go through `validate_parameters`, so the API path worked while the UI path did not. Notably, `subject` is *not* passed at query time: `update_params_from_encrypted_extra` only sets `service_account_info` and `catalog`. Impersonation has its own correctly-scoped home in `GSheetsEngineSpec.impersonate_user`. Validation was the only place forcing delegation, so it was validating under different credentials than the connection actually uses. Alternatives considered: dropping `subject` entirely is simpler, but it regresses domain-wide delegation setups where the sheet is shared with the admin rather than the service account, which the original comment explicitly intended to support. Hence the fallback below. ## What Validate each catalog URL as the service account itself first — matching how the connection behaves at query time — and only fall back to impersonating the current user if that read fails. Domain-wide delegation setups keep working; setups without it now succeed instead of failing with a misleading error. The URL read is extracted into `_can_read_url`, and connection setup into `_get_validation_connections`. ## Blast radius Google Sheets connections only, and only the `POST /api/v1/database/validate_parameters/` path (the UI Connect button). No change to query execution, impersonation, OAuth2 connections (still skipped), auth, or any other engine spec. No schema or config change. Strictly widens what validates successfully — no connection that validated before will stop validating. ## How to test Two regression tests in `tests/unit_tests/db_engine_specs/test_gsheets.py`, both of which fail on `master`: - `test_validate_parameters_without_domain_wide_delegation` — a sheet readable by the service account validates, and the first connection is built with `subject: None`. - `test_validate_parameters_falls_back_to_domain_wide_delegation` — when the service account itself can't read the sheet, validation falls back to impersonating the current user and still succeeds. The two existing `validate_parameters` catalog tests were updated to mock a distinct engine per subject so both paths are asserted. ``` pytest tests/unit_tests/db_engine_specs/test_gsheets.py # 34 passed pytest tests/unit_tests/databases/ # 213 passed, 1 skipped pre-commit run --files superset/db_engine_specs/gsheets.py tests/unit_tests/db_engine_specs/test_gsheets.py # all hooks pass ``` ## Risk & rollback Low. The failure mode would be validation accepting a sheet that the connection can't later read — but the primary attempt now uses exactly the credentials the connection uses at query time, so this is strictly closer to reality than before. Back-out is a plain revert; there is no flag or migration. ## Review guidance Start with `superset/db_engine_specs/gsheets.py`. The riskiest hunk is `_get_validation_connections` — specifically the ordering decision (service account first, impersonation second) and the fact that both connections are now built eagerly. Building a shillelagh connection is local and does no network I/O, and `any()` short-circuits so the second is only *used* when the first read fails. I'd welcome a sanity check from someone running domain-wide delegation that the fallback preserves their flow. -- 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]
