codeant-ai-for-open-source[bot] commented on code in PR #37398:
URL: https://github.com/apache/superset/pull/37398#discussion_r2721947637
##########
superset/utils/oauth2.py:
##########
@@ -96,10 +99,20 @@ def refresh_oauth2_token(
user_id=user_id,
database_id=database_id,
):
- token_response = db_engine_spec.get_oauth2_fresh_token(
- config,
- token.refresh_token,
- )
+ try:
+ token_response = db_engine_spec.get_oauth2_fresh_token(
+ config,
+ token.refresh_token,
+ )
+ except Exception:
+ # If token refresh failed, delete the invalid token to prevent
retry loops
+ logger.warning(
+ "OAuth2 token refresh failed for user=%s db=%s, deleting
invalid token",
+ user_id,
+ database_id,
+ )
+ db.session.delete(token)
+ return None
Review Comment:
**Suggestion:** The broad `except Exception` swallows all errors from the
OAuth2 refresh call, deletes the stored token even for transient/network/server
errors, and prevents OAuth2-specific exceptions (like `oauth2_exception` used
by engine specs such as GSheets) from propagating to `check_for_oauth2`, which
can stop the OAuth2 dance from being triggered correctly; instead, only delete
and re-raise for OAuth2-specific exceptions and let other exceptions bubble up
without deleting the token. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ⚠️ GSheets OAuth2 token refresh flow may delete valid tokens.
- ❌ Users forced to re-authorize after transient network errors.
- ⚠️ OAuth2 dance (start_oauth2_dance) may not trigger.
- ⚠️ Token retry loops prevented but at data loss cost.
```
</details>
```suggestion
except db_engine_spec.oauth2_exception as ex:
# Token is no longer valid for OAuth2; delete it and trigger
normal OAuth2 flow
logger.warning(
"OAuth2 token refresh failed for user=%s db=%s, deleting
invalid token",
user_id,
database_id,
)
db.session.delete(token)
raise ex
except Exception:
# For non-OAuth2-specific failures, keep the token and surface
the error
logger.warning(
"OAuth2 token refresh failed for user=%s db=%s",
user_id,
database_id,
)
raise
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure an OAuth2 token exists in DB for a user+database: see
DatabaseUserOAuth2Tokens
query at superset/utils/oauth2.py:70-75 which returns token
(get_oauth2_access_token).
2. Trigger token refresh path by calling get_oauth2_access_token such that
token.refresh_token is present (call site returns refresh path at
superset/utils/oauth2.py:81-82). This happens during a normal DB request
using an
OAuth2-enabled Database (e.g., GSheets integration).
3. Execution enters refresh_oauth2_token at superset/utils/oauth2.py:90 and
attempts
db_engine_spec.get_oauth2_fresh_token at superset/utils/oauth2.py:102-106.
4. If get_oauth2_fresh_token raises a transient/network/server error
(non-OAuth2-specific
exception), the current code hits the broad except Exception at
superset/utils/oauth2.py:107-115 which logs and unconditionally deletes the
stored token
(db.session.delete(token)), causing the token to be removed even though the
failure may be
recoverable. Subsequent calls will find no token
(superset/utils/oauth2.py:70-76) and
users will be forced to re-authorize unnecessarily.
5. Additionally, if an engine-specific OAuth2 sentinel exception (e.g., an
oauth2-specific
exception class defined by a DB engine spec like GSheets) should instead
propagate to
check_for_oauth2 to trigger start_oauth2_dance, that signal is swallowed by
the broad
except, preventing the proper OAuth2 dance flow in check_for_oauth2
(check_for_oauth2 uses
database.db_engine_spec.needs_oauth2 at superset/utils/oauth2.py:224-228).
6. The proposed change narrows exception handling to delete-and-rethrow only
for
OAuth2-specific errors (db_engine_spec.oauth2_exception) and re-raise
non-OAuth2
exceptions without deleting the token, preventing token loss on transient
failures.
```
</details>
<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:** 107:115
**Comment:**
*Logic Error: The broad `except Exception` swallows all errors from the
OAuth2 refresh call, deletes the stored token even for transient/network/server
errors, and prevents OAuth2-specific exceptions (like `oauth2_exception` used
by engine specs such as GSheets) from propagating to `check_for_oauth2`, which
can stop the OAuth2 dance from being triggered correctly; instead, only delete
and re-raise for OAuth2-specific exceptions and let other exceptions bubble up
without deleting the token.
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.
```
</details>
--
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]
