rusackas opened a new pull request, #42897: URL: https://github.com/apache/superset/pull/42897
### SUMMARY `database_user_oauth2_tokens` only ever carries one live token per `(user_id, database_id)` pair -- `OAuth2StoreTokenCommand` always 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 raises `MultipleResultsFound` if duplicates exist. This is a follow-up to #42211, which fixed an unrelated `purge_oauth2_tokens` filter bug on the same table. During review there, [Vitor-Avila asked](https://github.com/apache/superset/pull/42211#discussion_r0000000) whether multiple tokens per user+db should be blocked, and [rusackas agreed](https://github.com/apache/superset/pull/42211#discussion_r0000001) it was worth enforcing but out of scope for that fix: > Fair question, but that's pre-existing behavior separate from this bug. There's no unique constraint on user_id + database_id today, just an index for lookups, so nothing stops multiple tokens from piling up. Worth a follow-up if we want to enforce one token per user per db, but I don't think it should block this 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 ```bash pytest tests/unit_tests/models/core_test.py -k oauth2 -v pytest tests/unit_tests/migrations/test_enforce_oauth2_token_uniqueness.py -v ``` - `test_oauth2_tokens_unique_per_user_and_database` (new): confirms a second row for the same `(user_id, database_id)` pair raises `IntegrityError`. - `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's `upgrade()`/`downgrade()` against an in-memory SQLite engine seeded with pre-existing duplicate rows, and checks the dedupe-then-uniquify pre-flight, the resulting index's `unique` flag, that new duplicates are rejected post-upgrade, and that `downgrade()` restores a plain index without reversing the dedupe deletions. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [x] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [x] Migration is atomic, supports rollback & is backwards-compatible - [x] Confirm DB migration upgrade and downgrade tested - [x] Runtime estimates and downtime expectations provided: single-table `DELETE` (only affects pre-existing duplicate rows, expected to be rare-to-none in practice) plus an index rebuild on `database_user_oauth2_tokens`, which is typically small. No measurable downtime expected for realistic table sizes. - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
