fix(models): enforce one OAuth2 token per user+database - #42897
Conversation
…follow-up) `OAuth2StoreTokenCommand` always deletes any existing token before storing a new one, so `database_user_oauth2_tokens` is only ever meant to carry a single row per (user_id, database_id) pair -- but that invariant was only enforced in application code via a plain (non-unique) lookup index. A race between two concurrent OAuth2 callbacks for the same user+database can leave duplicate rows behind, and nothing downstream picks a deterministic one of them. Flagged as an explicit follow-up during review of apache#42211 (which fixed an unrelated purge_oauth2_tokens filter bug on the same table): Vitor-Avila asked whether multiple tokens per user+db should be blocked, and rusackas agreed it was worth enforcing but out of scope for that fix. Adds a migration that dedupes any pre-existing duplicate rows (keeping the highest id, i.e. the most recently issued token) before flipping the index to unique. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dosubot
Bot
added
authentication:sso
Code Review Agent Run #e646a5Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The review comment correctly identifies a MySQL limitation where a table cannot be modified (deleted from) while being read in a subquery. To resolve this in a database-agnostic way, you should fetch the IDs to keep into a Python list first, then perform the deletion using that list. Here is the corrected def upgrade() -> None:
bind = op.get_bind()
metadata = sa.MetaData()
table = sa.Table(TABLE_NAME, metadata, autoload_with=bind)
# Fetch IDs to keep into memory to avoid MySQL 1093 error
keep_ids = bind.execute(
select(func.max(table.c.id))
.group_by(table.c.user_id, table.c.database_id)
).scalars().all()
bind.execute(table.delete().where(table.c.id.notin_(keep_ids)))
drop_index(TABLE_NAME, INDEX_NAME)
create_index(TABLE_NAME, INDEX_NAME, ["user_id", "database_id"], unique=True)superset/migrations/versions/2026-08-07_09-00_da0e3f0081bf_enforce_oauth2_token_uniqueness.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42897 +/- ##
==========================================
- Coverage 66.38% 66.38% -0.01%
==========================================
Files 2857 2857
Lines 161133 161165 +32
Branches 37064 37074 +10
==========================================
+ Hits 106967 106988 +21
- Misses 52147 52155 +8
- Partials 2019 2022 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…2 uniqueness migration - OAuth2StoreTokenCommand deleted the old token then added the new one in the same flush; the unit of work emits INSERTs before DELETEs, which tripped the new (user_id, database_id) unique index on refresh. Flush right after the delete so the old row is gone before the new one lands. - The migration's dedupe DELETE subqueried from the same table, which MySQL rejects (error 1093); read duplicate ids in Python instead. - Dropping the old index outright also failed on MySQL (error 1553) since it was the only index covering the user_id FK; create the new unique index under a temporary name first so an FK-satisfying index always exists, then swap it into place. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #0bce87Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
database_user_oauth2_tokensonly ever carries one live token per(user_id, database_id)pair --OAuth2StoreTokenCommandalways deletes any existing token before storing a new one -- but that invariant was only enforced in application code, via a plain (non-unique) index (idx_user_id_database_id). A race between two concurrent OAuth2 callbacks for the same user+database can leave duplicate rows behind, and nothing downstream (get_oauth2_access_token,refresh_oauth2_token) picks a deterministic one -- both do.filter_by(user_id=..., database_id=...).one_or_none(), which raisesMultipleResultsFoundif duplicates exist.This is a follow-up to #42211, which fixed an unrelated
purge_oauth2_tokensfilter bug on the same table. During review there, Vitor-Avila asked whether multiple tokens per user+db should be blocked, and rusackas agreed it was worth enforcing but out of scope for that fix:This PR closes that gap by making the index unique.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable -- internal schema/model change, no UI.
TESTING INSTRUCTIONS
test_oauth2_tokens_unique_per_user_and_database(new): confirms a second row for the same(user_id, database_id)pair raisesIntegrityError.test_purge_oauth2_tokens_scoped_by_database_id(existing): updated so its PK-drift setup uses distinct users instead of repeating one user against the same database, since the latter now violates the new constraint.tests/unit_tests/migrations/test_enforce_oauth2_token_uniqueness.py(new): runs the migration'supgrade()/downgrade()against an in-memory SQLite engine seeded with pre-existing duplicate rows, and checks the dedupe-then-uniquify pre-flight, the resulting index'suniqueflag, that new duplicates are rejected post-upgrade, and thatdowngrade()restores a plain index without reversing the dedupe deletions.ADDITIONAL INFORMATION
DELETE(only affects pre-existing duplicate rows, expected to be rare-to-none in practice) plus an index rebuild ondatabase_user_oauth2_tokens, which is typically small. No measurable downtime expected for realistic table sizes.