You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/09/28 07:14:34 UTC
[PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
aminghadersohi opened a new pull request, #44765:
URL: https://github.com/apache/superset/pull/44765
### SUMMARY
Some databases (Snowflake, for example) reject an OAuth2 access token when the connection **logs in**. If the token store still holds that token as unexpired, the stored refresh token is never used: the user is sent back to the sign-in page even though a refresh would have worked.
`execute_with_oauth2_retry()` (#43623) handles exactly this, but it is only called by the new SQL executor and its Celery task. The paths that open connections elsewhere never use it:
- the SQL Lab API (`superset.sql_lab` β `Database.get_raw_connection()`),
- chart data (`Database.get_df()` β `get_raw_connection()`, and before that the default schema read for the chart's cache key through `get_inspector()`),
- metadata calls through `get_inspector()`.
This change routes opening the connection (`get_raw_connection()`) and the inspector (`get_inspector()`) through `execute_with_oauth2_retry()`:
- a rejected token triggers one forced refresh, then the connection is opened again;
- only the login is retried; work the caller does with the connection is never replayed;
- the wrapper steps aside when there is no signed-in user (the existing sign-in handling applies unchanged) or when an outer operation already owns the recovery (the new executor), so a token is exchanged at most once.
Engines that reject the token later, per statement (e.g. Google Sheets), are not covered by this change.
### TESTING INSTRUCTIONS
New unit tests in `tests/unit_tests/models/core_test.py`:
- `test_get_raw_connection_refreshes_a_token_rejected_at_login`
- `test_get_inspector_refreshes_a_token_rejected_at_login`
- `test_get_raw_connection_asks_to_sign_in_when_the_new_token_is_rejected`
- `test_get_raw_connection_does_not_replay_the_callers_block`
- `test_get_raw_connection_leaves_recovery_to_an_outer_retry`
- `test_get_raw_connection_without_oauth2_is_unchanged`
The first three fail without the change; the others guard behaviour that must not change. The existing `tests/unit_tests/models/`, `utils/oauth2_tests.py`, `sql_lab_test.py` and `db_engine_specs/` suites pass.
The same approach was verified against a live Snowflake account: the rejected token (error 390303) was exchanged once with the stored refresh token, the new token was persisted, and the connection was opened again with it, for both SQL Lab and chart data; without the change no exchange happened and the user was asked to sign in.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4154014391
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -239,6 +245,50 @@ def test_execute_with_oauth2_retry_forces_refresh_once(
db.session.expire.assert_called_once_with(token)
+@pytest.mark.parametrize("retry_fails", [False, True])
+@pytest.mark.parametrize("outer_active", [False, True])
+def test_execute_with_oauth2_retry_guards_both_attempts(
+ mocker: MockerFixture,
+ retry_fails: bool,
+ outer_active: bool,
+) -> None:
+ """Both attempts own connection recovery, and every exit restores the guard."""
+ mocker.patch(
+ "superset.utils.oauth2._oauth2_retry_active",
+ ContextVar("test_oauth2_retry_active", default=outer_active),
+ )
+ database = mocker.MagicMock(id=1)
+ database.is_oauth2_enabled.return_value = True
+ database.db_engine_spec.needs_oauth2.return_value = True
+ database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG
+ mocker.patch("superset.utils.oauth2.g").user.id = 2
+ db = mocker.patch("superset.utils.oauth2.db")
+ db.session.query().filter_by().one_or_none.return_value = None
+ refresh = mocker.patch(
+ "superset.utils.oauth2.refresh_oauth2_token", return_value="new-token"
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Weak refresh assertion</b></div>
<div id="fix">
The `refresh` mock is only checked with `assert_called_once()`; unlike `test_execute_with_oauth2_retry_forces_refresh_once` (lines 237-244), the call arguments are never verified. If `execute_with_oauth2_retry` passed the wrong config, user id, or `force` flag, this test would still pass. Assert the full call signature.
</div>
</div>
<small><i>Code Review Run #22acaa</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5916090990
Good back and forth here. Looks like 54572820a6 fixes the refused-refresh issue @gabotorresruiz flagged (the uncommitted-deletion race), with a test parametrized over both `get_raw_connection` and `get_inspector`. @gabotorresruiz mind taking another pass to confirm that closes it out for you?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4154202119
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -239,6 +245,50 @@ def test_execute_with_oauth2_retry_forces_refresh_once(
db.session.expire.assert_called_once_with(token)
+@pytest.mark.parametrize("retry_fails", [False, True])
+@pytest.mark.parametrize("outer_active", [False, True])
+def test_execute_with_oauth2_retry_guards_both_attempts(
+ mocker: MockerFixture,
+ retry_fails: bool,
+ outer_active: bool,
+) -> None:
+ """Both attempts own connection recovery, and every exit restores the guard."""
+ mocker.patch(
+ "superset.utils.oauth2._oauth2_retry_active",
+ ContextVar("test_oauth2_retry_active", default=outer_active),
+ )
+ database = mocker.MagicMock(id=1)
+ database.is_oauth2_enabled.return_value = True
+ database.db_engine_spec.needs_oauth2.return_value = True
+ database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG
+ mocker.patch("superset.utils.oauth2.g").user.id = 2
+ db = mocker.patch("superset.utils.oauth2.db")
+ db.session.query().filter_by().one_or_none.return_value = None
+ refresh = mocker.patch(
+ "superset.utils.oauth2.refresh_oauth2_token", return_value="new-token"
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion to verify the full call signature of the `refresh` mock is appropriate. Asserting the specific arguments (such as the configuration, user ID, and flags) ensures that the `execute_with_oauth2_retry` function correctly handles the token refresh process, preventing potential regressions where incorrect parameters might be passed.
**tests/unit_tests/utils/oauth2_tests.py**
```
refresh.assert_called_once_with(
database, DUMMY_OAUTH2_CONFIG, user_id=2, force=True
)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4119518731
##########
superset/models/core.py:
##########
@@ -1220,8 +1263,13 @@ def get_inspector(
catalog: str | None = None,
schema: str | None = None,
) -> Inspector:
- with self.get_sqla_engine(catalog=catalog, schema=schema) as engine:
- yield sqla.inspect(engine)
+ @contextmanager
+ def open_inspector() -> Iterator[Inspector]:
+ with self.get_sqla_engine(catalog=catalog, schema=schema) as engine:
+ yield sqla.inspect(engine)
Review Comment:
**Suggestion:** Only `sqla.inspect(engine)` is retried. Lazy metadata calls such as `inspector.get_columns()` can reject the token afterward, outside the retry.
**Assessment:** π `Major` Β· π `Occurrence: Sometimes` Β· π·οΈ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e1d5b276cf624e4a81b436c138ffe9d0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e1d5b276cf624e4a81b436c138ffe9d0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/core.py
**Line:** 1269:1269
**Comment:**
*Api Mismatch: Only `sqla.inspect(engine)` is retried. Lazy metadata calls such as `inspector.get_columns()` can reject the token afterward, outside the retry.
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%2F44765&comment_hash=98172d582738ff9e02d43f9411220146ae5975179818fb69f69b767411088543&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44765&comment_hash=98172d582738ff9e02d43f9411220146ae5975179818fb69f69b767411088543&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4140679361
##########
superset/utils/oauth2.py:
##########
@@ -268,6 +268,107 @@ def _refresh_oauth2_token_locked( # noqa: C901
return token.access_token
+@backoff.on_exception(
+ backoff.expo,
+ AcquireDistributedLockFailedException,
+ factor=0.1,
+ base=2,
+ max_tries=8,
Review Comment:
<!-- Bito Reply -->
The suggestion to extract the backoff configuration into a shared named constant is appropriate. Using a single source of truth for these parameters ensures consistency across the expired-token and forced-refresh paths, preventing potential drift if the retry policy is tuned in the future.
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected token
+ races for the non-blocking refresh lock. The losers must retry until the winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+ db.session.query().populate_existing().filter_by().one_or_none.return_value = token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
Review Comment:
<!-- Bito Reply -->
The reviewer's finding is correct regarding the behavior of the backoff decorator. When `raise_on_giveup=False` is set, the backoff decorator swallows the `AcquireDistributedLockFailedException` after the maximum number of retries and returns `None` instead of raising the exception. Consequently, the `except` block in `force_refresh_oauth2_token` is not triggered, and the fallback logic to read the committed access token is bypassed.
To address this, the forced-refresh helper should be configured to allow the exception to propagate after the retries are exhausted, ensuring the `except` block can handle the lock failure and proceed with the fallback token read.
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected token
+ races for the non-blocking refresh lock. The losers must retry until the winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+ db.session.query().populate_existing().filter_by().one_or_none.return_value = token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_returns_none_when_no_one_refreshed(
+ mocker: MockerFixture,
+) -> None:
+ """With the lock held and the stored token unchanged there is nothing to reuse."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="rejected-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ mocker.MagicMock(),
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result is None
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding the test `test_force_refresh_returns_none_when_no_one_refreshed` is based on a misunderstanding of the implementation. As you noted, `force_refresh_oauth2_token` does not use the backoff decorator directly; it calls `_retry_forced_refresh_until_lock_available` (which lacks the decorator) and then proceeds to `_read_refreshed_access_token` if the lock acquisition fails. The test correctly exercises the branch where the lock is held and no new token is found, confirming that the function returns `None` as expected in that scenario.
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected token
+ races for the non-blocking refresh lock. The losers must retry until the winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+ db.session.query().populate_existing().filter_by().one_or_none.return_value = token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_returns_none_when_no_one_refreshed(
+ mocker: MockerFixture,
+) -> None:
+ """With the lock held and the stored token unchanged there is nothing to reuse."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="rejected-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ mocker.MagicMock(),
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result is None
+
+
+def test_execute_with_oauth2_retry_survives_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A chart that loses the refresh race renders with the winner's token.
+
+ Without contention tolerance the lock failure is not an OAuth2 error, so it
+ escapes as an opaque `AcquireDistributedLockFailedException` and the chart fails.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ operation = mocker.Mock(side_effect=[RuntimeError("stale OAuth token"), "result"])
+ database = mocker.MagicMock()
+ database.id = 1
+ database.is_oauth2_enabled.return_value = True
+ database.db_engine_spec.needs_oauth2.return_value = True
+ database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG
+ mocker.patch("superset.utils.oauth2.g").user.id = 2
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ # The rejected token is read first, then the winner's committed replacement.
+ db.session.query().filter_by().one_or_none.side_effect = [
+ mocker.MagicMock(access_token="rejected-token"), # noqa: S106
+ mocker.MagicMock(access_token="winning-token"), # noqa: S106
+ ]
+
+ assert execute_with_oauth2_retry(database, operation) == "result"
+
+ assert operation.call_count == 2
+ database.start_oauth2_dance.assert_not_called()
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding the test failure is incorrect because `raise_on_giveup=False` is not applied to the forced-refresh helper. As noted, the helper (now `_retry_forced_refresh_until_lock_available`) allows the lock exception to propagate to the `except` block in `force_refresh_oauth2_token`, ensuring `_read_refreshed_access_token` is executed as intended. The updated tests in 54572820a6 correctly verify this behavior by asserting the expected number of lock attempts and confirming the fallback token read occurs.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4158794776
##########
superset/models/core.py:
##########
@@ -1260,8 +1303,14 @@ def get_inspector(
catalog: str | None = None,
schema: str | None = None,
) -> Inspector:
- with self.get_sqla_engine(catalog=catalog, schema=schema) as engine:
- yield sqla.inspect(engine)
+ @contextmanager
+ def open_inspector() -> Iterator[Inspector]:
+ with self.get_sqla_engine(catalog=catalog, schema=schema) as engine:
+ with check_for_oauth2(self):
Review Comment:
NIT, definitely not a blocker. I went looking for what this guard changes in the real path and could not find a case: `get_sqla_engine` already wraps its own `yield engine` in `check_for_oauth2`, so it catches both a rejection raised by `sqla.inspect()` and one thrown back in from the caller's block. I removed this line on `ed86e158` and re-ran the login-rejection, rejected-replacement-token, caller-block and schema-discovery scenarios against a real metadata database: identical outcomes, exactly one `start_oauth2_dance` in every case. The only test that fails without it is `test_get_inspector_does_not_replay_the_callers_block`, and that one stubs `get_sqla_engine` out with `nullcontext(engine)`, so it pins the guard rather than the behaviour.
Happy to keep it as cheap insurance, and the symmetry with `open_connection` reads well. Worth a short comment saying it is redundant with `get_sqla_engine` today, so a later reader does not take that test as proof the real path depends on it?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4137077763
##########
superset/utils/oauth2.py:
##########
@@ -322,12 +423,11 @@ def execute_with_oauth2_retry( # noqa: C901
database.db_engine_spec.engine,
)
try:
- access_token = refresh_oauth2_token(
+ access_token = force_refresh_oauth2_token(
Review Comment:
This block worries me a bit. Sending `get_raw_connection()` and `get_inspector()` through here also sends `OAuth2TokenRefreshError` through here, and that one is already terminal: `_refresh_oauth2_token_locked` raises it *after* deleting the token row and flushing on the caller's `db.session` without committing (`superset/utils/oauth2.py:230`). `needs_oauth2()` still answers `True` for it, since Snowflake lists `OAuth2TokenRefreshError` in `oauth2_exception` and the `BaseEngineSpec` default `OAuth2RedirectError` is its parent class. So we force a second exchange: `force_refresh_oauth2_token` opens its own `Session`, still sees the row the request's session deleted but has not committed, presents the same refused refresh token to the provider again, and its own `DELETE` then waits on the row lock that same request is holding.
I verified it on `796cbf1` against a Postgres metadata database with a Redis coordination backend, stored access token expired and the provider refusing the refresh:
* this branch: the refresh token is presented twice, then the request blocks on the row lock. It came back at all only because I had set `lock_timeout` on the test database; the Postgres default is `0`. The user gets an `OperationalError`, not the sign-in redirect.
* `ffe9a00`, same scenario: one exchange, `OAuth2RedirectError`, 0.01s.
`get_inspector()` behaves identically. The login-rejection case this PR targets is unaffected, because nothing deleted the row first. It is also invisible under the KeyValue lock backend, since `AcquireDistributedLock._acquire_kv` is `@transaction` wrapped and commits `db.session` on the way in, so only `DISTRIBUTED_COORDINATION_CONFIG` deployments see it.
Treating an already refused exchange as terminal fixes it, and all 125 tests in `oauth2_tests.py` and `core_test.py` still pass with it applied:
```python
if not is_oauth2_error:
raise
if isinstance(ex, OAuth2TokenRefreshError):
# the exchange already ran and the provider refused it
app.config["STATS_LOGGER"].incr("oauth2.forced_refresh.exchange_rejected")
database.start_oauth2_dance()
raise
```
A `test_get_raw_connection_does_not_re_exchange_a_refused_refresh_token` (stored token already expired, `get_oauth2_fresh_token` raising `oauth2_exception`, asserting one call and `OAuth2RedirectError`) would lock it in. The new tests miss it because they all start from a token the store still considers valid, so `get_oauth2_access_token` never refreshes before the login.
Or am I misreading the ordering here?
##########
superset/utils/oauth2.py:
##########
@@ -268,6 +268,107 @@ def _refresh_oauth2_token_locked( # noqa: C901
return token.access_token
+@backoff.on_exception(
+ backoff.expo,
+ AcquireDistributedLockFailedException,
+ factor=0.1,
+ base=2,
+ max_tries=8,
Review Comment:
Not a blocker, and the contention handling clearly works: 8 concurrent opens against a Postgres metadata database with a Redis lock gave me 8 of 8 rendering off a single exchange, at 0s, 1s and 3s of provider latency.
Recording the trade though. `factor=0.1, base=2, max_tries=8` is roughly 12.7s of waiting worst case, while `DistributedLock` is entered with `ttl_seconds=30`, so an exchange slower than the backoff window guarantees every loser waits out the whole window and *then* gets the sign-in dance anyway. At 20s of provider latency I measured 1 of 8 rendering, with the other 7 sitting in the backoff for about 12s first; before this change they failed immediately. Is the mismatch with the TTL deliberate, or worth moving the window closer to it?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5886680007
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5932110404
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4140678229
##########
superset/utils/oauth2.py:
##########
@@ -322,12 +423,11 @@ def execute_with_oauth2_retry( # noqa: C901
database.db_engine_spec.engine,
)
try:
- access_token = refresh_oauth2_token(
+ access_token = force_refresh_oauth2_token(
Review Comment:
Fixed in 54572820a6. `execute_with_oauth2_retry` now treats `OAuth2TokenRefreshError` as terminal: it increments `oauth2.forced_refresh.exchange_rejected`, starts the sign-in dance and re-raises, so it never opens the isolated session or presents the refused refresh token a second time. `test_connection_does_not_re_exchange_a_refused_refresh_token` (parametrized over `get_raw_connection` and `get_inspector`) starts from an expired stored token with the provider refusing the refresh. It asserts one exchange, the deletion flushed but not committed, no isolated `Session`, one `start_oauth2_dance` and an `OAuth2RedirectError`. Both cases fail with the guard removed.
##########
superset/utils/oauth2.py:
##########
@@ -268,6 +268,107 @@ def _refresh_oauth2_token_locked( # noqa: C901
return token.access_token
+@backoff.on_exception(
+ backoff.expo,
+ AcquireDistributedLockFailedException,
+ factor=0.1,
+ base=2,
+ max_tries=8,
Review Comment:
Fixed in 54572820a6. Both decorators read `OAUTH2_LOCK_BACKOFF_FACTOR`, `OAUTH2_LOCK_BACKOFF_BASE` and `OAUTH2_LOCK_BACKOFF_MAX_TRIES`, so the expired-token and forced-refresh paths can't drift apart.
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected token
+ races for the non-blocking refresh lock. The losers must retry until the winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+ db.session.query().populate_existing().filter_by().one_or_none.return_value = token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
Review Comment:
This finding's premise is wrong. `raise_on_giveup=False` is set only on `get_oauth2_access_token`. The forced-refresh helper (renamed `_retry_forced_refresh_until_lock_available` in 54572820a6, following your naming suggestion) omits it, so the lock exception reaches `force_refresh_oauth2_token`'s `except` and `_read_refreshed_access_token` runs. To pin that, 54572820a6 also asserts that the lock is attempted `OAUTH2_LOCK_BACKOFF_MAX_TRIES` times and that the fallback's token read happens.
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected token
+ races for the non-blocking refresh lock. The losers must retry until the winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+ db.session.query().populate_existing().filter_by().one_or_none.return_value = token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_returns_none_when_no_one_refreshed(
+ mocker: MockerFixture,
+) -> None:
+ """With the lock held and the stored token unchanged there is nothing to reuse."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="rejected-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ mocker.MagicMock(),
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result is None
Review Comment:
This finding's premise is wrong. `raise_on_giveup=False` is set only on `get_oauth2_access_token`. The forced-refresh helper (renamed `_retry_forced_refresh_until_lock_available` in 54572820a6, following your naming suggestion) omits it, so the lock exception reaches `force_refresh_oauth2_token`'s `except` and `_read_refreshed_access_token` runs. To pin that, 54572820a6 also asserts that the lock is attempted `OAUTH2_LOCK_BACKOFF_MAX_TRIES` times and that the fallback's token read happens.
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected token
+ races for the non-blocking refresh lock. The losers must retry until the winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+ db.session.query().populate_existing().filter_by().one_or_none.return_value = token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_returns_none_when_no_one_refreshed(
+ mocker: MockerFixture,
+) -> None:
+ """With the lock held and the stored token unchanged there is nothing to reuse."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="rejected-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ mocker.MagicMock(),
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result is None
+
+
+def test_execute_with_oauth2_retry_survives_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A chart that loses the refresh race renders with the winner's token.
+
+ Without contention tolerance the lock failure is not an OAuth2 error, so it
+ escapes as an opaque `AcquireDistributedLockFailedException` and the chart fails.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ operation = mocker.Mock(side_effect=[RuntimeError("stale OAuth token"), "result"])
+ database = mocker.MagicMock()
+ database.id = 1
+ database.is_oauth2_enabled.return_value = True
+ database.db_engine_spec.needs_oauth2.return_value = True
+ database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG
+ mocker.patch("superset.utils.oauth2.g").user.id = 2
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ # The rejected token is read first, then the winner's committed replacement.
+ db.session.query().filter_by().one_or_none.side_effect = [
+ mocker.MagicMock(access_token="rejected-token"), # noqa: S106
+ mocker.MagicMock(access_token="winning-token"), # noqa: S106
+ ]
+
+ assert execute_with_oauth2_retry(database, operation) == "result"
+
+ assert operation.call_count == 2
+ database.start_oauth2_dance.assert_not_called()
Review Comment:
This finding's premise is wrong. `raise_on_giveup=False` is set only on `get_oauth2_access_token`. The forced-refresh helper (renamed `_retry_forced_refresh_until_lock_available` in 54572820a6, following your naming suggestion) omits it, so the lock exception reaches `force_refresh_oauth2_token`'s `except` and `_read_refreshed_access_token` runs. To pin that, 54572820a6 also asserts that the lock is attempted `OAUTH2_LOCK_BACKOFF_MAX_TRIES` times and that the fallback's token read happens.
##########
superset/utils/oauth2.py:
##########
@@ -268,6 +268,107 @@ def _refresh_oauth2_token_locked( # noqa: C901
return token.access_token
+@backoff.on_exception(
+ backoff.expo,
+ AcquireDistributedLockFailedException,
+ factor=0.1,
+ base=2,
+ max_tries=8,
Review Comment:
The mismatch is deliberate, so I've left the window unchanged. The 30s `ttl_seconds` is a safety ceiling: it frees the lock if its holder dies mid-exchange. It isn't how long we expect an exchange to take. Waiting out the TTL would hold a web worker for about 30s on every losing chart request, and `backoff.expo` uses full jitter by default, so 12.7s is already the upper bound, not the typical wait. The expired-token path (`get_oauth2_access_token`) has used the same 12.7s window against the same 30s lock since before this PR. 54572820a6 moves both paths onto shared constants (`OAUTH2_LOCK_BACKOFF_*`), so they can't diverge if we retune later. A provider that takes 20s for a token exchange is pathological, and the loser falls back to the sign-in dance as it did before, so the bounded wait seems the better trade.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5906110908
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5884336285
Review feedback addressed in this PR β no fast-follow.
**@rebenitez1802 β π‘ Medium, concurrent dashboard charts lose the refresh race and fail with an opaque lock error β 796cbf1**
Confirmed exactly as described: `refresh_oauth2_token(force=True)` took the non-blocking `DistributedLock` with no backoff, so `LockAlreadyHeldException` fell through `execute_with_oauth2_retry`'s generic `except Exception` (`oauth2.forced_refresh.exchange_transient_failure`) and re-raised. Took your first option β mirror the contention tolerance `get_oauth2_access_token` already has β plus the store re-read as a floor:
- `_refresh_oauth2_token_forced` wraps the forced refresh in the same `@backoff.on_exception(backoff.expo, AcquireDistributedLockFailedException, factor=0.1, base=2, max_tries=8)` as the expired-token path. A loser retries until the winner commits and releases; its re-read under the lock then short-circuits on `rejected_access_token != token.access_token`, so a rotating refresh token is never exchanged twice.
- If the lock never frees within the window, `force_refresh_oauth2_token` falls back to `_read_refreshed_access_token`, which reads through an isolated `Session` so the winner's commit is visible regardless of the caller's snapshot, and returns it only when it actually replaced the rejected token. Counted as `oauth2.forced_refresh.lock_contended`.
- Losers therefore render with the winner's token instead of raising; when nobody refreshed, the result is still the normal sign-in dance, never a bare lock error.
Tests, including the one you specified:
- `tests/unit_tests/models/core_test.py::test_get_raw_connection_survives_a_lost_refresh_race` β end-to-end on the path this PR newly routes: `DistributedLock` raising `LockAlreadyHeldException`, asserting the connection opens with the refreshed token and no dance is started. Fails without the change with `LockAlreadyHeldException`.
- `oauth2_tests.py::test_force_refresh_waits_out_lock_contention` β contended then acquired: reuses the winner's token, no second exchange.
- `oauth2_tests.py::test_force_refresh_reads_committed_token_when_lock_never_frees` and `::test_force_refresh_returns_none_when_no_one_refreshed` β giveup fallback, both directions.
- `oauth2_tests.py::test_execute_with_oauth2_retry_survives_lock_contention` β the wrapper no longer surfaces the lock failure.
**CodeAnt inline, `superset/models/core.py:1269` β not valid, [replied with evidence](https://github.com/apache/superset/pull/44765#discussion_r4129978961)**
`sqla.inspect(engine)` logs in: SQLAlchemy 2.0.52 `Inspector._init_engine` calls `engine.connect().close()`, so the login is inside the retry (`test_get_inspector_refreshes_a_token_rejected_at_login`). Lazy calls reconnect outside it by design β retrying them would replay the caller's block, which `_open_with_oauth2_retry` deliberately never does.
Verification: `pre-commit run` clean on the touched files; `tests/unit_tests/utils/oauth2_tests.py`, `tests/unit_tests/models/`, `tests/unit_tests/sql_lab_test.py` β 564 passed (remaining failures are local missing optional drivers, `trino` / `sqlalchemy_bigquery`, and reproduce on an unmodified tree).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5928454352
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5865305903
<!-- Bito Reply -->
The flagged issue is correct. The current implementation of `_open_with_oauth2_retry` only wraps the initial connection opening, meaning subsequent metadata operations (like `inspector.get_columns()`) that occur outside this context will not benefit from the retry logic if they encounter a token rejection.
To resolve this, you should ensure that the retry logic encompasses the entire scope of the database interaction, not just the initial connection. You can achieve this by extending the `execute_with_oauth2_retry` wrapper to cover the metadata calls as well, or by refactoring the calling code to use the retry-enabled context manager for all operations that require a valid token.
I have checked the available PR comments, and there are no other comments to address at this time. Would you like me to proceed with implementing a fix for this issue?
**superset/models/core.py**
```
with ExitStack() as stack:
yield execute_with_oauth2_retry(
self,
lambda: stack.enter_context(open_context()),
)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4129978961
##########
superset/models/core.py:
##########
@@ -1220,8 +1263,13 @@ def get_inspector(
catalog: str | None = None,
schema: str | None = None,
) -> Inspector:
- with self.get_sqla_engine(catalog=catalog, schema=schema) as engine:
- yield sqla.inspect(engine)
+ @contextmanager
+ def open_inspector() -> Iterator[Inspector]:
+ with self.get_sqla_engine(catalog=catalog, schema=schema) as engine:
+ yield sqla.inspect(engine)
Review Comment:
Not a valid finding β the login this PR targets is inside the retry.
`sqla.inspect(engine)` does log in. SQLAlchemy 2.0.52 `Inspector._init_engine` (`sqlalchemy/engine/reflection.py`):
```python
def _init_engine(self, engine: Engine) -> None:
self.bind = self.engine = engine
engine.connect().close()
self._op_context_requires_connect = True
```
So the `engine.connect()` that presents the token runs inside `_open_with_oauth2_retry`, and a token rejected at login is refreshed and the inspector rebuilt. `tests/unit_tests/models/core_test.py::test_get_inspector_refreshes_a_token_rejected_at_login` covers exactly that.
The lazy calls you point at (`Inspector._operation_context` β `self.bind.connect()`) do reconnect outside the retry, but that is deliberate, not an oversight:
- they reconnect with a token that just authenticated successfully at `inspect()` time, so the login-rejection failure mode does not recur there;
- retrying them would mean replaying the caller's block. The wrapper cannot know whether that block is idempotent, so it only ever retries opening β see the `_open_with_oauth2_retry` docstring (`superset/models/core.py:1276-1283`) and `test_get_raw_connection_does_not_replay_the_callers_block`.
Engines that reject the token later, per statement (Google Sheets, for example), are out of scope and called out as such in the PR description.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5943098810
**Bito Automatic Review Skipped β PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5925814258
@rusackas Guard: fixed in 36af4d3300; `test_outer_oauth2_retry_does_not_refresh_again_when_login_is_rejected` reproduces two exchanges before the fix and passes after it.
Inspector parity: fixed in 36af4d3300; `test_get_inspector_asks_to_sign_in_when_the_new_token_is_rejected` verifies the sign-in redirect.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5937587018
@rusackas both of your points landed in `36af4d33`, which went in after your review, so the head has moved past the commit you blocked at.
On the retry guard you were right, and it was measurable. On `54572820` the SQL executor shape exchanged the token twice for one logical query and the request succeeded through a nested inner retry. On this head it exchanges once and returns the sign-in redirect instead. The guard is now set around the retried call and the whole thing sits inside `check_for_oauth2(database)`.
On the inspector, `open_inspector` picked up the `check_for_oauth2(self)` wrapping, with tests for both the rejected replacement token and an exception thrown back from the caller's block. Both of those fail against master.
I approved at `ed86e158`. The only red checks are the two `Analyze` jobs, failing with a CodeQL configuration error that hit several unrelated branches in the same hour.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4131480309
##########
superset/utils/oauth2.py:
##########
@@ -268,6 +268,107 @@ def _refresh_oauth2_token_locked( # noqa: C901
return token.access_token
+@backoff.on_exception(
+ backoff.expo,
+ AcquireDistributedLockFailedException,
+ factor=0.1,
+ base=2,
+ max_tries=8,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>duplicated backoff config</b></div>
<div id="fix">
The backoff config `factor=0.1, base=2, max_tries=8` is duplicated verbatim from `get_oauth2_access_token` (lines 92-94). If one retry policy is tuned (e.g. max_tries raised for a slow provider), the other silently diverges. Extract a shared named constant used by both decorators.
</div>
</div>
<small><i>Code Review Run #3adf73</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected token
+ races for the non-blocking refresh lock. The losers must retry until the winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+ db.session.query().populate_existing().filter_by().one_or_none.return_value = token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Failing lock-fallback test</b></div>
<div id="fix">
This test fails against the current implementation. `_refresh_oauth2_token_forced` is decorated with `@backoff.on_exception(..., raise_on_giveup=False)`, so when `DistributedLock` always raises `AcquireDistributedLockFailedException`, backoff swallows it and returns None β the `except` block in `force_refresh_oauth2_token` that calls `_read_refreshed_access_token` is never reached, so `result` is None, not "winning-token". The lock-contention fallback is dead code.
</div>
</div>
<small><i>Code Review Run #3adf73</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected token
+ races for the non-blocking refresh lock. The losers must retry until the winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+ db.session.query().populate_existing().filter_by().one_or_none.return_value = token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_returns_none_when_no_one_refreshed(
+ mocker: MockerFixture,
+) -> None:
+ """With the lock held and the stored token unchanged there is nothing to reuse."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="rejected-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ mocker.MagicMock(),
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result is None
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test passes for wrong reason</b></div>
<div id="fix">
This test passes but does not test what its name/docstring claim. Because the backoff decorator on `_refresh_oauth2_token_forced` swallows the lock exception, `_read_refreshed_access_token` is never called; `result is None` holds only because `force_refresh_oauth2_token` returns None directly. The 'stored token unchanged -> nothing to reuse' branch is never exercised.
</div>
</div>
<small><i>Code Review Run #3adf73</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected token
+ races for the non-blocking refresh lock. The losers must retry until the winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+ db.session.query().populate_existing().filter_by().one_or_none.return_value = token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_returns_none_when_no_one_refreshed(
+ mocker: MockerFixture,
+) -> None:
+ """With the lock held and the stored token unchanged there is nothing to reuse."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="rejected-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ mocker.MagicMock(),
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result is None
+
+
+def test_execute_with_oauth2_retry_survives_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A chart that loses the refresh race renders with the winner's token.
+
+ Without contention tolerance the lock failure is not an OAuth2 error, so it
+ escapes as an opaque `AcquireDistributedLockFailedException` and the chart fails.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ operation = mocker.Mock(side_effect=[RuntimeError("stale OAuth token"), "result"])
+ database = mocker.MagicMock()
+ database.id = 1
+ database.is_oauth2_enabled.return_value = True
+ database.db_engine_spec.needs_oauth2.return_value = True
+ database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG
+ mocker.patch("superset.utils.oauth2.g").user.id = 2
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ # The rejected token is read first, then the winner's committed replacement.
+ db.session.query().filter_by().one_or_none.side_effect = [
+ mocker.MagicMock(access_token="rejected-token"), # noqa: S106
+ mocker.MagicMock(access_token="winning-token"), # noqa: S106
+ ]
+
+ assert execute_with_oauth2_retry(database, operation) == "result"
+
+ assert operation.call_count == 2
+ database.start_oauth2_dance.assert_not_called()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Failing retry test</b></div>
<div id="fix">
This test fails: because the lock exception is swallowed by `@backoff.on_exception(..., raise_on_giveup=False)` on `_refresh_oauth2_token_forced`, `force_refresh_oauth2_token` returns None, so `execute_with_oauth2_retry` (oauth2.py:441-443) calls `database.start_oauth2_dance()` and `assert_not_called()` fails. The intended "reuse winner's token" path is never exercised.
</div>
</div>
<small><i>Code Review Run #3adf73</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5884357096
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 796cbf19d5174b82df2e354b984744b6ac34a07c |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6abb4df9efbc91000829bf54 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-44765--superset-docs-preview.netlify.app](https://deploy-preview-44765--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">π€</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEADAv-w5waQi-0HPcHUXg3LFDIro73mbw7yQixy5wgLQARaXfGzm7VbirhqNKsSFzqgkKS8pka8tBsq7PGtodXKIQ2BLjN72WuvZWORxgO8H) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44765--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5865234849
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5865380360
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44765?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `88.46154%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.16%. Comparing base ([`ffe9a00`](https://app.codecov.io/gh/apache/superset/commit/ffe9a00fc42635287310eab08ad664c00759c4fc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0174f01`](https://app.codecov.io/gh/apache/superset/commit/0174f01524d32f8f5af64e0594f7ef5c33f9a35c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44765?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/models/core.py](https://app.codecov.io/gh/apache/superset/pull/44765?src=pr&el=tree&filepath=superset%2Fmodels%2Fcore.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | 88.46% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/44765?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (ffe9a00) and HEAD (0174f01). Click for more details.
>
> <details><summary>HEAD has 31 uploads less than BASE</summary>
>
>| Flag | BASE (ffe9a00) | HEAD (0174f01) |
>|------|------|------|
>|python|28|1|
>|presto|5|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44765 +/- ##
===========================================
- Coverage 81.22% 57.16% -24.06%
===========================================
Files 2968 2968
Lines 179363 179381 +18
Branches 41597 41598 +1
===========================================
- Hits 145681 102542 -43139
- Misses 30971 75851 +44880
+ Partials 2711 988 -1723
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44765/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44765/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44765/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44765/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44765/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.65% <88.46%> (+0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44765/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.65% <88.46%> (-46.84%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44765/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44765/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44765?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4160487949
##########
superset/models/core.py:
##########
@@ -1260,8 +1303,14 @@ def get_inspector(
catalog: str | None = None,
schema: str | None = None,
) -> Inspector:
- with self.get_sqla_engine(catalog=catalog, schema=schema) as engine:
- yield sqla.inspect(engine)
+ @contextmanager
+ def open_inspector() -> Iterator[Inspector]:
+ with self.get_sqla_engine(catalog=catalog, schema=schema) as engine:
+ with check_for_oauth2(self):
Review Comment:
Addressed in 5d5a4a29e6: kept the defensive guard, documented its redundancy with get_sqla_engineβs own check_for_oauth2, and clarified that the stubbed test pins the guard rather than the real path.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44765:
URL: https://github.com/apache/superset/pull/44765
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5868225815
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44765:
URL: https://github.com/apache/superset/pull/44765#issuecomment-5865469175
About the CodeQL alert (`py/clear-text-logging-sensitive-data`, `superset/models/core.py:1546`): it's a false positive.
- **The sink** is existing code in `Database.is_oauth2_enabled()`: `logger.warning("Invalid OAuth2 client configuration for database %s", self)`. This PR doesn't change it. `Database.__repr__` returns `self.name` (the verbose name or `database_name`), so no credential is logged.
- **The "password" source** CodeQL reported is the new unit-test helper `_oauth2_database()` in `tests/unit_tests/models/core_test.py`. CodeQL's sensitive-data name heuristic classifies the name as a credential, then follows the returned `Database` through `get_raw_connection()`/`get_inspector()` into that log call. The SARIF code flow shows exactly this.
8f0f274 renames the helper to `_login_rejecting_database()`, with no behaviour change, so the check should clear without a manual dismissal.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(oauth2): refresh a token rejected when a connection opens [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4154200487
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -239,6 +245,50 @@ def test_execute_with_oauth2_retry_forces_refresh_once(
db.session.expire.assert_called_once_with(token)
+@pytest.mark.parametrize("retry_fails", [False, True])
+@pytest.mark.parametrize("outer_active", [False, True])
+def test_execute_with_oauth2_retry_guards_both_attempts(
+ mocker: MockerFixture,
+ retry_fails: bool,
+ outer_active: bool,
+) -> None:
+ """Both attempts own connection recovery, and every exit restores the guard."""
+ mocker.patch(
+ "superset.utils.oauth2._oauth2_retry_active",
+ ContextVar("test_oauth2_retry_active", default=outer_active),
+ )
+ database = mocker.MagicMock(id=1)
+ database.is_oauth2_enabled.return_value = True
+ database.db_engine_spec.needs_oauth2.return_value = True
+ database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG
+ mocker.patch("superset.utils.oauth2.g").user.id = 2
+ db = mocker.patch("superset.utils.oauth2.db")
+ db.session.query().filter_by().one_or_none.return_value = None
+ refresh = mocker.patch(
+ "superset.utils.oauth2.refresh_oauth2_token", return_value="new-token"
+ )
Review Comment:
Addressed in ed86e15890 (test(oauth2): assert refresh call arguments) by asserting the full refresh call signature.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org