brodmart opened a new pull request, #42211:
URL: https://github.com/apache/superset/pull/42211
### SUMMARY
`Database.purge_oauth2_tokens()` at `superset/models/core.py:1440` filters
the `database_user_oauth2_tokens` table 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 at
`tests/unit_tests/models/core_test.py::test_purge_oauth2_tokens` 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.
The one-line fix scopes the filter by `database_id`:
```diff
- db.session.query(DatabaseUserOAuth2Tokens).filter(
- DatabaseUserOAuth2Tokens.id == self.id
- ).delete()
+ db.session.query(DatabaseUserOAuth2Tokens).filter(
+ DatabaseUserOAuth2Tokens.database_id == self.id
+ ).delete()
```
This matches:
- the docstring ("Delete all OAuth2 tokens associated with this database"),
- the FK declared on the model (`database_id = Column(Integer,
ForeignKey("dbs.id", ondelete="CASCADE"))`), and
- the migration that introduced the table
(`2024-03-20_16-02_678eefb4ab44_add_access_token_table.py`, which declares
`sa.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_id` alongside the
existing test. It inserts several tokens on `database1` first (so token PKs
advance past 1), then creates `database2`, then purges `database2`. The current
filter would delete one of `database1`'s tokens; the fix leaves them all in
place.
```bash
pytest tests/unit_tests/models/core_test.py -k purge_oauth2 -v
```
Both `test_purge_oauth2_tokens` (existing) and
`test_purge_oauth2_tokens_scoped_by_database_id` (new) pass with the fix. The
new test fails on unpatched `master`.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Discussed privately with the Superset Security team; opening here as a
correctness fix per their request.
--
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]
