codeant-ai-for-open-source[bot] commented on code in PR #40071:
URL: https://github.com/apache/superset/pull/40071#discussion_r3227835254
##########
superset/utils/oauth2.py:
##########
@@ -129,15 +129,26 @@ def refresh_oauth2_token(
database_id: int,
user_id: int,
db_engine_spec: type[BaseEngineSpec],
- token: DatabaseUserOAuth2Tokens,
) -> str | None:
+ # pylint: disable=import-outside-toplevel
+ from superset.models.core import DatabaseUserOAuth2Tokens
+
# Use longer TTL for OAuth2 token refresh (may involve network calls)
with DistributedLock(
namespace="refresh_oauth2_token",
ttl_seconds=30,
user_id=user_id,
database_id=database_id,
):
+ # Short circuit in case another request already deleted the token
+ token = (
+ db.session.query(DatabaseUserOAuth2Tokens)
+ .filter_by(user_id=user_id, database_id=database_id)
+ .one_or_none()
+ )
+ if token is None:
+ return None
Review Comment:
**Suggestion:** After re-querying the token under the lock, the code only
checks whether the row exists, but not whether `refresh_token` is still
present. Since `refresh_token` is nullable and can legitimately be `None`, this
can call `get_oauth2_fresh_token` with a null token and trigger an OAuth error
path that deletes the row unnecessarily. Add a post-requery guard for missing
`refresh_token` (and return `None` without refreshing) before invoking the
engine spec. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard and SQL queries fail during concurrent OAuth2 refresh.
- ⚠️ Users forced to re-authenticate despite recent OAuth2 login.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger a database connection using OAuth2 in
`Database.get_sqla_engine()` at
`superset/models/core.py:33-43` by loading a dashboard or chart backed by an
OAuth2-enabled database; this calls `get_oauth2_access_token(oauth2_config,
self.id,
g.user.id, self.db_engine_spec)` at line 35.
2. Ensure there is an existing row in `database_user_oauth2_tokens` with a
valid
`refresh_token` and an expired `access_token` for this `(user_id,
database_id)` pair,
created by `OAuth2StoreTokenCommand.run()` at
`superset/commands/database/oauth2.py:48-58`, which stores
`refresh_token=token_response.get("refresh_token")`.
3. When `get_oauth2_access_token` runs at
`superset/utils/oauth2.py:107-119`, it loads the
token row, sees that `access_token` is expired, and since
`token.refresh_token` is truthy
at line 118, it calls `refresh_oauth2_token(config, database_id, user_id,
db_engine_spec)`
at line 119.
4. In parallel, invoke the OAuth2 callback API `/api/v1/database/...`
handled in
`superset/databases/api.py:20-22`, which constructs
`OAuth2StoreTokenCommand(parameters)`
and calls `command.run()`. This command deletes any existing token via
`DatabaseUserOAuth2TokensDAO.delete([existing])` at
`superset/commands/database/oauth2.py:41-47` and creates a new token row with
`refresh_token=token_response.get("refresh_token")` at lines 48-57; for
providers that do
not return a refresh token, this new row has `refresh_token=None`.
5. Back in `refresh_oauth2_token` at `superset/utils/oauth2.py:48-71`, after
acquiring the
`DistributedLock`, the code re-queries the token (`token =
db.session.query(DatabaseUserOAuth2Tokens)...one_or_none()` at lines 65-69)
and only
checks `if token is None: return None` at lines 70-71, not whether
`token.refresh_token`
is still present; in the concurrent scenario, this re-query can return the
new row with
`refresh_token=None`.
6. `refresh_oauth2_token` then unconditionally calls
`db_engine_spec.get_oauth2_fresh_token(config, token.refresh_token)` at
`superset/utils/oauth2.py:73-77`. With `token.refresh_token` now `None`, the
engine spec
can raise its `oauth2_exception`, triggering the handler at lines 78-87 that
logs the
error, deletes the token row (`db.session.delete(token)`), flushes, and
re-raises, causing
the calling request's database connection attempt to fail and unnecessarily
deleting the
newly stored token.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Futils%2Foauth2.py%0A%2A%2ALine%3A%2A%2A%20143%3A150%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20After%20re-querying%20the%20token%20under%20the%20lock%2C%20the%20code%20only%20checks%20whether%20the%20row%20exists%2C%20but%20not%20whether%20%60refresh_token%60%20is%20still%20present.%20Since%20%60refresh_token%60%20is%20nullable%20and%20can%20legitimately%20be%20%60None%60%2C%20this%20can%20call%20%60get_oauth2_fresh_token%60%20with%20a%20null%20token%20and%20trigger%20an%20OAuth%20error%20path%20that%20deletes%20the%20row%20unnecessarily.%20Add%20a%20post-requery%20guard%20for%20missing%20%60refresh_token%60%20%28and%20return%20%60None%60%20without%20refreshing%29%20before%20invoking%20the%20engine%20spec.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20Ho
w%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Futils%2Foauth2.py%0A%2A%2ALine%3A%2A%2A%20143%3A150%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20After%20re-querying%20the%20token%20under%20the%20lock%2C%20the%20code%20only%20checks%20whether%20the%20row%20exists%2C%20but%20not%20whether%20%60refresh_token%60%20is%20still%20present.%20Since%20%60refresh_token%60%20is%20nullable%20and%
20can%20legitimately%20be%20%60None%60%2C%20this%20can%20call%20%60get_oauth2_fresh_token%60%20with%20a%20null%20token%20and%20trigger%20an%20OAuth%20error%20path%20that%20deletes%20the%20row%20unnecessarily.%20Add%20a%20post-requery%20guard%20for%20missing%20%60refresh_token%60%20%28and%20return%20%60None%60%20without%20refreshing%29%20before%20invoking%20the%20engine%20spec.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/oauth2.py
**Line:** 143:150
**Comment:**
*Incorrect Condition Logic: After re-querying the token under the lock,
the code only checks whether the row exists, but not whether `refresh_token` is
still present. Since `refresh_token` is nullable and can legitimately be
`None`, this can call `get_oauth2_fresh_token` with a null token and trigger an
OAuth error path that deletes the row unnecessarily. Add a post-requery guard
for missing `refresh_token` (and return `None` without refreshing) before
invoking the engine spec.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40071&comment_hash=0cee6dd3144601665d64af98df8d7c344b34911c5bae74e07faf52ec96c9ab8c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40071&comment_hash=0cee6dd3144601665d64af98df8d7c344b34911c5bae74e07faf52ec96c9ab8c&reaction=dislike'>👎</a>
--
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]
