richardfogaca commented on PR #42644: URL: https://github.com/apache/superset/pull/42644#issuecomment-5194663722
Richard's agent here: Thanks for the thorough OAuth2 hardening. I reviewed `69a0873` and found **two blocking correctness issues** plus **one non-blocking clarification**. ## Blocking ### 1. Refresh sanitization can lose the re-authentication signal **Current flow** 1. `refresh_oauth2_token()` catches an engine-declared OAuth exception. 2. It replaces that exception with `OAuth2TokenRefreshError`. 3. `check_for_oauth2()` asks the engine spec to classify the replacement. Engine specs whose `oauth2_exception` or `needs_oauth2()` recognizes only the original vendor exception will therefore skip `start_oauth2_dance()`. **Impact** The token is deleted, but re-authentication may not start. If the replacement reaches browser or MCP consumers, it also lacks the authorization URL and tab metadata they require. **Requested change** Preserve the re-authentication decision before sanitizing the exception, or make the sanitized refresh marker unconditionally trigger the OAuth dance. Please add an end-to-end regression test using an engine spec with a vendor-specific OAuth exception. --- ### 2. Observability failures can escape after the token is committed The new decorator order commits the token before event logging and metrics finish. **Impact** - A custom event logger failure can return `500` after the one-shot callback successfully persisted the token. - A StatsD failure can replace the original sanitized OAuth exception. - Retrying the callback may not be safe because the authorization code is single-use. The current event-log test only raises `SQLAlchemyError`, which `DBEventLogger` already catches and suppresses. It does not prove isolation from arbitrary custom logger failures. **Requested change** Make both callback observability hooks best-effort, and cover: - an arbitrary event-logger exception after a successful token write; - a metric failure on the success path; - a metric failure while handling an OAuth error. This also encompasses the existing StatsD review thread. ## Non-blocking clarification ### 3. Clarify the API-response redaction guarantee The existing provider-denial path passes the callback’s arbitrary `error` value into `OAuth2Error.extra`, and the existing API test expects that value in the response. This predates the PR, so I would not block on it independently. However, the PR description should either: - narrow the guarantee to provider exception and response-body details; or - sanitize this legacy field and document the resulting API change. Could we address the two blockers before merging and clarify the intended redaction boundary? -- 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]
