fix(models): purge_oauth2_tokens filters on wrong column#42211
Conversation
`Database.purge_oauth2_tokens()` filters `database_user_oauth2_tokens`
by the token-table primary key against the `Database` primary key,
instead of by the `database_id` foreign key. Because token PKs and
database PKs are independent auto-increment sequences, the delete
removes the row whose token PK happens to equal `self.id` rather than
the tokens actually associated with this database.
The existing unit test happens to pass because it inserts one database
and one token per database in an empty schema, so PK=1 aligns with PK=1
by coincidence. Any scenario where the token PK and database PK diverge
(the normal state once more than one token has ever existed) leaves the
wrong rows deleted and the intended rows in place.
Fix scopes the filter by `database_id`, matching the docstring
("Delete all OAuth2 tokens associated with this database"), the FK
declared on the model, and the migration that introduced the table
(`2024-03-20_16-02_678eefb4ab44_add_access_token_table.py`).
Adds a regression test that exercises the case the existing test
misses: several tokens on the first database (so token PKs advance
past 1), then a second database, then purge the second database. The
existing `test_purge_oauth2_tokens` case is preserved unchanged and
still passes with the fix in place.
Signed-off-by: Martin Brodeur <addressedemartin@gmail.com>
Code Review Agent Run #a11b1cActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42211 +/- ##
==========================================
- Coverage 65.19% 65.19% -0.01%
==========================================
Files 2768 2768
Lines 156081 156081
Branches 35719 35719
==========================================
- Hits 101754 101751 -3
- Misses 52365 52367 +2
- Partials 1962 1963 +1
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:
|
There was a problem hiding this comment.
Pull request overview
Fixes a correctness bug in Database.purge_oauth2_tokens() where it deleted OAuth2 token rows by matching the token table primary key against the database primary key, instead of deleting tokens associated with the database via the database_id foreign key.
Changes:
- Update
purge_oauth2_tokens()to filterDatabaseUserOAuth2Tokensbydatabase_id == self.id. - Add a regression unit test that reproduces the PK-drift scenario (token PKs no longer coincidentally align with database PKs) to ensure purging one database cannot delete another database’s tokens.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
superset/models/core.py |
Corrects the bulk-delete filter to target tokens associated with the database via database_id. |
tests/unit_tests/models/core_test.py |
Adds a regression test that forces PK drift to catch the original bug and prevent regressions. |
| database2.purge_oauth2_tokens() | ||
|
|
||
| assert ( | ||
| session.query(DatabaseUserOAuth2Tokens) | ||
| .filter_by(database_id=database1.id) | ||
| .count() | ||
| == 5 | ||
| ) | ||
| assert ( | ||
| session.query(DatabaseUserOAuth2Tokens) | ||
| .filter_by(database_id=database2.id) | ||
| .count() | ||
| == 0 | ||
| ) |
There was a problem hiding this comment.
Could you also call database1.purge_oauth2_tokens() next and then assert it deletes ALL tokens for this connection? If it was previously deleting by the token PK, I believe it would delete one token as opposed to all from a connection, right?
|
Yes, your understanding is correct. The previous implementation was incorrectly filtering by the token's primary key ( superset/models/core.py |
| for i in range(5): | ||
| session.add( | ||
| DatabaseUserOAuth2Tokens( | ||
| user_id=user.id, | ||
| database_id=database1.id, | ||
| access_token=f"db1_access_{i}", # noqa: S106 | ||
| access_token_expiration=datetime(2023, 1, 1), | ||
| refresh_token=f"db1_refresh_{i}", # noqa: S106 | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I didn't know we could add several tokens for the same user and DB. Should that be blocked, instead? If we allow multiple token for the same user + DB combination, how to tell which one to use?
Vitor-Avila
left a comment
Vitor-Avila
left a comment
There was a problem hiding this comment.
Left two non-blocking comments. One just to increase the test coverage, and the other about a potential related issue that we could track in a separate ticket/PR, your call!
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Confirmed the fix is correct: database_id is the right column — it's the declared FK to dbs.id (superset/models/core.py:1531-1535), matches the docstring ("Delete all OAuth2 tokens associated with this database"), and matches the migration (678eefb4ab44_add_access_token_table.py) which gives the token table its own independent PK sequence, unrelated to dbs.id. The old DatabaseUserOAuth2Tokens.id == self.id filter was comparing two unrelated auto-increment sequences, so it was really a coincidence-dependent no-op/misfire rather than a scoped delete. .delete() on the bulk query is unchanged in style from the original and doesn't need synchronize_session here since nothing in the same session depends on in-memory DatabaseUserOAuth2Tokens instances staying in sync afterward.
Worth calling out for anyone triaging severity later: the old code was bad in two directions at once — it could delete an unrelated database's token row purely because that row's PK happened to equal self.id (cross-connection data loss), and it would leave this database's real tokens un-purged, i.e. not revoked when a caller (_handle_oauth2 in superset/commands/database/update.py) explicitly changed client id/secret/scope/endpoints expecting old tokens to be invalidated. That second part is a minor hygiene/security gap more than a boundary violation, but it's the actual trigger for calling purge_oauth2_tokens() in the first place, so worth having in the PR history.
On the test: test_purge_oauth2_tokens_scoped_by_database_id does pin the bug — it inserts 5 tokens on database1 first (advancing token PKs to 2-6), then creates database2 (PK 2), so purging database2 under the old filter (id == self.id) would delete the database1 token whose PK happens to be 2. That's exactly the scenario the old code got wrong, and the new test fails against unpatched code per the PR's testing instructions.
+1 to Vitor's suggestion on core_test.py:1454 — extending the test to also call database1.purge_oauth2_tokens() and assert it removes all 5 tokens would additionally prove the old code under-deleted (only removing one PK-matching row instead of the full set for a database), which is the mirror image of the over-deletion case already covered. Combined, the two assertions would fully characterize both failure modes of the old filter.
Also checked for the same PK-vs-FK confusion pattern elsewhere in the file (grep -n '\.id == self\.id' superset/models/core.py) — this is the only occurrence, no other query has the same mix-up.
Nothing blocking here; nice catch and a clean, well-targeted regression test.
SUMMARY
Database.purge_oauth2_tokens()atsuperset/models/core.py:1440filters thedatabase_user_oauth2_tokenstable by the token-table primary key against theDatabaseprimary key, instead of by thedatabase_idforeign key. Because token PKs and database PKs are independent auto-increment sequences, the delete removes the row whose token PK happens to equalself.idrather than the tokens actually associated with this database.The existing unit test at
tests/unit_tests/models/core_test.py::test_purge_oauth2_tokenshappens to pass because it inserts one database and one token per database in an empty schema, so PK=1 aligns with PK=1 by coincidence. Any scenario where the token PK and database PK diverge (the normal state once more than one token has ever existed) leaves the wrong rows deleted and the intended rows in place.The one-line fix scopes the filter by
database_id:This matches:
database_id = Column(Integer, ForeignKey("dbs.id", ondelete="CASCADE"))), and2024-03-20_16-02_678eefb4ab44_add_access_token_table.py, which declaressa.PrimaryKeyConstraint("id")on its own independent SERIAL sequence).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — internal ORM filter change, no UI.
TESTING INSTRUCTIONS
Added
test_purge_oauth2_tokens_scoped_by_database_idalongside the existing test. It inserts several tokens ondatabase1first (so token PKs advance past 1), then createsdatabase2, then purgesdatabase2. The current filter would delete one ofdatabase1's tokens; the fix leaves them all in place.Both
test_purge_oauth2_tokens(existing) andtest_purge_oauth2_tokens_scoped_by_database_id(new) pass with the fix. The new test fails on unpatchedmaster.ADDITIONAL INFORMATION
Discussed privately with the Superset Security team; opening here as a correctness fix per their request.