fix(gsheets): pass the OAuth2 token and delegation subject through connect_args

[aminghadersohi]

SUMMARY

Google Sheets OAuth2 connections never use the signed-in user's access token when a catalog is configured. The connection form always configures one.

• GSheetsEngineSpec.impersonate_user adds the token to the engine URL (?access_token=).
• update_params_from_encrypted_extra stores the catalog, and any service account, in connect_args["adapter_kwargs"]["gsheetsapi"].
• create_engine merges connect_args over the dialect's own connect arguments shallowly. That dict therefore replaces the adapter kwargs that shillelagh built from the URL, and the token is lost.

Every query then runs without credentials. A private sheet fails with UnauthenticatedError, needs_oauth2() matches it, and the user is sent back to the Google consent screen on every query. This happens even right after a successful token refresh.

The token is now placed in the same connect_args adapter kwargs, so it survives the merge. It is also no longer part of the engine URL. When impersonation also set a subject on the URL, that subject is copied into the same adapter kwargs so it isn't dropped by the merge either.

Service accounts: impersonation needs an explicit opt-in

The same merge drops the subject for a service account stored in the secure extra: the service account always sits in connect_args, so with "Impersonate logged in user" enabled every query still ran as the service account itself, silently. Honouring the subject for every such connection would break every service account without Google Workspace domain-wide delegation, because the connection form enables impersonation for every Google Sheets database.

A secure-extra service account now impersonates the user only when the secure extra also sets "domain_wide_delegation": true:

• opted in: the user's e-mail reaches shillelagh as the delegation subject through connect_args. If Google refuses the delegation the query fails; a user without an e-mail is refused. It never falls back to the service account's own access.
• not opted in: the misleading subject is removed from the URL (it never took effect).
• the key is removed before create_engine (it is not a shillelagh argument).
• credentials given in the URL (service account file, application default credentials) are unchanged.

Verified against a real service account: without the opt-in the query runs as the service account (as before); with it, the user was sent to Google as the delegation subject and Google refused the token (invalid_grant, no delegation configured), so the query failed instead of returning the service account's rows.

BEFORE/AFTER

• Before: OAuth2 database with a catalog. The stored token is refreshed, but the request reaches Google with no Authorization header (HTTP 401), and the user is prompted to authorize again.
• After: the query runs with the user's token.

TESTING INSTRUCTIONS

• pytest tests/unit_tests/db_engine_specs/test_gsheets.py. test_impersonate_user_service_account_* (3 tests) fail without the service-account change. The new test_impersonate_user_access_token_with_catalog builds the engine exactly as Database._get_sqla_engine does (impersonate_user, then update_params_from_encrypted_extra, then create_engine) and checks the kwargs that reach the DB-API connect(). It fails without the change. test_impersonate_user_username_and_access_token does the same with an impersonation subject and a catalog, and checks that subject, access_token and catalog all arrive. It also fails without the change.
• Live: a Google Sheets database with an OAuth2 client and a catalog pointing at a private sheet. Authorize once, then run a query in SQL Lab. Before the change it prompts for authorization again. After the change it returns rows.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Code Review Agent Run #995b06

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: 67a154e..67a154e

  • 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 an incremental AI Review.

• /review full - 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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.56%. Comparing base (732e655) to head (bc3735b).
⚠️ Report is 20 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44709      +/-   ##
==========================================
+ Coverage   81.53%   81.56%   +0.02%     
==========================================
  Files        2974     2976       +2     
  Lines      180360   180600     +240     
  Branches    41749    41766      +17     
==========================================
+ Hits       147065   147305     +240     
+ Misses      30577    30573       -4     
- Partials     2718     2722       +4     

Flag	Coverage Δ
hive	36.68% <5.55%> (-0.01%)	⬇️
mysql	55.94% <5.55%> (-0.05%)	⬇️
postgres	55.95% <5.55%> (-0.05%)	⬇️
presto	38.60% <5.55%> (-0.01%)	⬇️
python	85.71% <100.00%> (+0.03%)	⬆️
sqlite	55.67% <5.55%> (-0.05%)	⬇️
unit	78.49% <100.00%> (+0.08%)	⬆️

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]

The flagged issue is correct. When both username and user_token are provided, the current implementation overwrites the adapter_kwargs dictionary, which can lead to the loss of other necessary configuration parameters (like the subject or catalog) that were previously stored there.

To resolve this, you should use setdefault to ensure you are merging the new access_token into the existing gsheetsapi configuration rather than replacing the entire dictionary. The implementation in the provided diff already correctly uses setdefault to avoid this issue:

            connect_args = engine_kwargs.setdefault("connect_args", {})
            adapter_kwargs = connect_args.setdefault("adapter_kwargs", {})
            adapter_kwargs.setdefault("gsheetsapi", {})["access_token"] = user_token

This approach preserves existing keys in adapter_kwargs and gsheetsapi.

superset/db_engine_specs/gsheets.py

connect_args = engine_kwargs.setdefault("connect_args", {})
            adapter_kwargs = connect_args.setdefault("adapter_kwargs", {})
            adapter_kwargs.setdefault("gsheetsapi", {})["access_token"] = user_token

[bito-code-review]

Code Review Agent Run #62eb45

Actionable Suggestions - 1

• superset/db_engine_specs/gsheets.py - 1

  • Subject dropped on tokenless path · Line 269-270

Additional Suggestions - 2

• tests/unit_tests/db_engine_specs/test_gsheets.py - 2

  • Inline imports violate rule 12745 · Line 651-653
    BITO adaptive rule 12745 requires module-level imports unless a documented circular dependency justifies inlining. The inline `create_engine` and `GSheetsEngineSpec` imports (mirroring sibling tests) carry no justifying comment, and this file already imports `superset.utils.oauth2` and other `superset` modules at top level, so no cycle is evident. Hoist both imports to the module header or document the exception.
  • Unannotated mock variables rule 12787 · Line 655-655
    BITO adaptive rule 12787 requires explicit type annotations on mock variables in test files. `user` here (and `database` at line 661) are unannotated `mocker.MagicMock()` assignments. Annotate them as `user: MagicMock = mocker.MagicMock()` (importing `MagicMock` from `unittest.mock`) so the mock type is statically visible, per the project's typing standards.

Review Details

• Files reviewed - 2 · Commit Range: 67a154e..263c191

  • 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 an incremental AI Review.

• /review full - 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

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	92c2b44
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6abbf34d4aa3df00080ce6fc
😎 Deploy Preview	https://deploy-preview-44709--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.

[bito-code-review]

Code Review Agent Run #8c104f

Actionable Suggestions - 1

• tests/unit_tests/db_engine_specs/test_gsheets.py - 1

  • Redundant Local Import · Line 1309-1309

Review Details

• Files reviewed - 3 · Commit Range: 263c191..5ef6dd8

  • superset/db_engine_specs/gsheets.py
  • tests/unit_tests/db_engine_specs/test_gsheets.py
  • superset/translations/messages.pot
• 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 an incremental AI Review.

• /review full - 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]

Code Review Agent Run #e49ddb

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: 5ef6dd8..fa1309e

  • 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 an incremental AI Review.

• /review full - 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]

Addressing the Additional Suggestions in Code Review Agent Run #62eb45 (tests/unit_tests/db_engine_specs/test_gsheets.py):

• Inline imports (rule 12745): fixed in 9b141eb. create_engine now comes from a module-level import sqlalchemy. A plain module-level create_engine would be shadowed by the local create_engine mocks in the existing get_table_names tests. The local GSheetsEngineSpec imports stay, and a comment in the module header documents why: superset/db_engine_specs/gsheets.py binds superset.db and superset.security_manager at import time, so it has to load after the app fixture has initialized them.
• Unannotated mock variables (rule 12787): fixed in 9b141eb. The user and database mocks added by this PR are annotated as MagicMock.

The other findings were already covered: the subject on the tokenless service-account path in f608349, the subject with a token in 263c191, and the redundant SupersetException import in fa1309e. All 41 GSheets unit tests and the file-scoped pre-commit checks pass.

[bito-code-review]

Code Review Agent Run #50150a

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: fa1309e..f566396

  • 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 an incremental AI Review.

• /review full - 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

[rebenitez1802]

Approving — I traced the root cause and the fix end-to-end and both reproduce against SQLAlchemy 2.0.52 + shillelagh 1.4.4: the shallow connect_args merge replaces the dialect's URL-derived adapter_kwargs, so an access_token on the URL is dropped whenever a catalog/service-account puts its own adapter_kwargs["gsheetsapi"] in connect_args; moving the token into that same dict fixes it. Security model looks intact too — token is off the URL, delegation is gated behind the Admin-only secure-extra flag, it fails closed when the impersonated user has no email, and per-user engine-cache isolation is preserved.

One non-blocking test note:

🟡 test_impersonate_user_username_and_access_token passes on unmodified master, so it doesn't actually guard the fix.

This test configures no catalog, so engine_kwargs has no pre-existing connect_args — but the shallow-merge regression only manifests when a connect_args adapter_kwargs exists to replace the URL-derived one. On master, impersonate_user puts both subject and access_token on the URL and leaves engine_kwargs empty, so shillelagh parses both straight from the URL and the two assertions still pass. So it guards the new subject-carry lines in isolation, but a full revert of the fix would not turn it red.

Consider giving this test a catalog (mirroring test_impersonate_user_access_token_with_catalog) so that on the pre-change code the shallow merge would drop the URL subject/access_token and the test would fail. That's also the one spot where the subject-carry would be exercised together with a catalog.

[aminghadersohi]

@rebenitez1802 your test note is addressed in bc3735b. test_impersonate_user_username_and_access_token now configures a catalog, so a connect_args adapter_kwargs exists to replace the URL-derived one. It fails against master's gsheets.py and passes with this PR, and it asserts subject, access_token and catalog together.

[bito-code-review]

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it