fix(gsheets): don't require domain-wide delegation to validate a connection

[aminghadersohi]

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.

[codecov]

Codecov Report

❌ Patch coverage is 26.31579% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.67%. Comparing base (e7dccd4) to head (929a634).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/db_engine_specs/gsheets.py	26.31%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #43309      +/-   ##
==========================================
- Coverage   66.73%   66.67%   -0.07%     
==========================================
  Files        2876     2876              
  Lines      164228   164149      -79     
  Branches    37890    37850      -40     
==========================================
- Hits       109598   109443     -155     
- Misses      52471    52546      +75     
- Partials     2159     2160       +1     

Flag	Coverage Δ
hive	38.11% <26.31%> (+<0.01%)	⬆️
mysql	57.76% <26.31%> (-0.01%)	⬇️
postgres	57.80% <26.31%> (-0.01%)	⬇️
presto	40.04% <26.31%> (+<0.01%)	⬆️
python	59.18% <26.31%> (-0.01%)	⬇️
sqlite	57.44% <26.31%> (-0.01%)	⬇️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bito-code-review]

Code Review Agent Run #88aeff

Actionable Suggestions - 0

Additional Suggestions - 2

• superset/db_engine_specs/gsheets.py - 2

  • CWE-404: Resource Not Released In Destructor · Line 376-390
    The `_get_validation_connections` method creates connection objects that are returned to the caller but never explicitly closed. SQLAlchemy 2.0 documentation recommends context managers (`with engine.connect() as conn:`) for guaranteed cleanup. While the old code at lines 382-393 had the same implicit cleanup pattern, this new implementation creates up to 2 connections per validation where the original created 1, increasing the likelihood of resource pressure.
  • Avoid catching bare Exception · Line 400-400
    Avoid catching blind exception `Exception`. Consider catching a more specific exception or re-raising with context.

Review Details

• Files reviewed - 2 · Commit Range: bb6dd05..bb6dd05

  • superset/db_engine_specs/gsheets.py
  • tests/unit_tests/db_engine_specs/test_gsheets.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[bito-code-review]

The flagged issue is correct. The current implementation of _get_validation_connections eagerly calls .connect() for all subjects, which can trigger an invalid_grant error if the service account lacks domain-wide delegation, preventing successful validation even if the service-account-only connection would have worked.

To resolve this, you should modify _get_validation_connections to return a list of callables (or a generator) that create and connect the engines lazily, rather than returning a list of already-connected Connection objects. Then, update the validation loop to call these factories only when needed and handle potential connection errors gracefully.

Would you like me to fetch all other comments on this PR to validate them and implement a comprehensive fix?

superset/db_engine_specs/gsheets.py

@classmethod
    def _get_validation_connections(cls, encrypted_credentials: dict[str, Any]) -> list[Callable[[], Connection]]:
        subjects: list[str | None] = [None]
        if g.user and g.user.email:
            subjects.append(g.user.email)

        return [
            lambda s=subject: create_engine(
                "gsheets://",
                connect_args={"adapter_kwargs": {"gsheetsapi": {"service_account_info": encrypted_credentials, "subject": s}}},
                future=True,
            ).connect()
            for subject in subjects
        ]

[Vitor-Avila]

@aminghadersohi I might be wrong here, but I think query execution later would actually use the logged in user's email? At least the Preset docs says "In order to create the connection, it is required that the email address associated with your Preset account has access to the Google Sheets file." Unsure if the doc here was explicitly calling out only the connection creation, or if it meant to include later execution.

If that's really true (might be worth manually testing) then this would either:

• Allow the connection to be created, but then queries fail after; or
• In case these changes also affect query execution, queries now bypass OAuth2 validation.

Another important detail is that Superset has an "Impersonate logged in user" checkbox that typically controls OAuth2 enablement. If we want to support Service Account auth to bypass OAuth, we probably want to gate this behind this checkbox. I think in the past editing the GSheets connection wouldn't even allow you to disable the checkbox, so we might need to allow that.

Now, a bigger question would be: do we even need to continue supporting OAuth2-type connection via Service Account? This feature was introduced before native support for DB OAuth2 was added to Superset (GSheets included, and it doesn't even require a Service Account). With that in mind, one possible outcome would be to create a SIP to deprecate this "OAuth2 validation" for Service Account auth, and keep only:

• Public Sheets
• Private Sheets via Service Account
• Private Sheets via native OAuth2

This would be a breaking change as existing connections with Service Account + Domain-wide delegation would have to migrate to OAuth2, but might make more sense long-term.

Curious if @betodealmeida has any thoughts here as well.

[aminghadersohi]

@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.

[Vitor-Avila]

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.

One limitation here is that the checkbox only shows up to be changed after the connection is created. So you might need to make the URL check loose, which is not great.

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.

Oh, I meant to say that SA + DWD is deprecated + the checkbox is unchecked (and no longer works for SA auth) + queries always use SA only.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	929a634
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a87369f0451af00080873f7
😎 Deploy Preview	https://deploy-preview-43309--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[aminghadersohi]

Confirmed, and that kills the gating idea — thanks, I'd have shipped a fix with a hole in it.

The Advanced tab that hosts the checkbox is gated on useTabLayout = isEditMode || useSqlAlchemyForm (DatabaseModal/index.tsx#L763), and the only other render of ExtraOptions is inside renderFinishState() (#L1999-L2002), which runs after the connection exists. So on the create path through the dynamic form — exactly the path this bug is on — impersonate_user is never user-settable, and at Connect time it's whatever #L1051-L1053 hardcodes. Reading the flag during validation would just be reading our own hardcoded true. Making the URL check loose to compensate would trade a false negative for a false positive, which is worse: we'd tell people the sheet is fine and let them find out at query time.

Your clarification also lands differently than I'd read it the first time. "SA + DWD deprecated, checkbox unchecked and no longer wired to SA auth, queries always SA-only" is a coherent model, and it's better than what's there now — the current design authenticates validation as one identity and queries as another, and no amount of patching inside validate_parameters fixes that split. It does mean an existing SA + DWD connection that relies on per-user delegation stops working and has to move to native OAuth2, so I agree it's SIP territory rather than something to slip into a bug fix.

So I don't think this PR should carry the behavior change. Two ways to land it, and I'd rather you pick than guess:

1. Close it in favor of the SIP, and let the deprecation carry the fix. Cleanest if the SIP is likely to happen soon.
2. Narrow it to the error message only — no behavior change at all, just stop reporting every failure in _can_read_url as "check for typos and make sure that 'Type of Google Sheets allowed' selection matches the input" and surface the actual cause. That message is wrong for invalid_grant today, and it's equally wrong for a permission error or a network blip; it sends people to re-check a URL that was never the problem. This is useful whichever way the SIP goes, and it's non-breaking.

Happy to do either, or to help with the SIP write-up if that's where this is heading. Not asking for a merge on the current diff either way — the two commits on it now are the incomplete fix plus a test, and I'll strip or rewrite them to match whichever option you want.

[bito-code-review]

Code Review Agent Run #a0357f

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: bb6dd05..d8e4fc1

  • tests/unit_tests/db_engine_specs/test_gsheets.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[aminghadersohi]

Closing this in favour of a SIP.

@Vitor-Avila you convinced me. The core problem isn't in validate_parameters at all — it's that validation authenticates as one identity and queries authenticate as another, and every fix that stays inside the validator either papers over that split or trades a false negative for a false positive. Both of my attempts here died on it. Deprecating SA + domain-wide delegation the way you described — checkbox unwired from SA auth, queries always SA-only, per-user impersonation served by native OAuth2 — resolves the split at the design level rather than patching around it, so that's the direction we're taking.

I'll put up the SIP. When I do, I'll carry over the findings from this PR so they don't get lost:

• The modal hardcodes impersonate_user = true for GSheets on save (DatabaseModal/index.tsx#L1051-L1053), overriding the backend's impersonate_user = Column(Boolean, default=False) (superset/models/core.py#L212). That single line is what makes delegation mandatory for every UI-created connection.
• The impersonate control isn't reachable during create: the Advanced tab is gated on useTabLayout = isEditMode || useSqlAlchemyForm (#L763), and the only other ExtraOptions render is inside renderFinishState() (#L1999-L2002).
• Today's user-visible symptom is a service account without domain-wide delegation being rejected at Connect with "The URL could not be identified. Please check for typos and make sure that 'Type of Google Sheets allowed' selection matches the input" — when the real cause is an invalid_grant from the delegation lookup. Worth fixing that message regardless of how the SIP lands, since it's equally misleading for a permission error or a network failure.

One alternative I'll raise in the SIP for completeness rather than relitigate here: your original checkbox-gating idea may survive if the hardcode is removed and the control is exposed during create, since existing connections keep their stored impersonate_user = true and would be unaffected. That's a smaller, non-breaking change. You may still prefer the full deprecation as the cleaner long-term model — that's a reasonable call and the SIP is the right venue to weigh both.

Thanks for the careful review here; you caught a wrong premise in my PR description before it turned into a fix that would have broken query execution for the people it claimed to help.

[Vitor-Avila]

thanks @aminghadersohi

[betodealmeida]

I think in general OAuth2 is always better than a service account. With a service account, the subject field lowers permissions, so if you forget to enable user impersonation everything continues to work and people get access to everything in the org. But with OAuth2 you start with zero perms and the personal token raises permissions, so if you forget to enable user impersonation things break and users can't access anything. One breaks silently and with a big blast radius; the other breaks loudly and has zero blast radius.