codeant-ai-for-open-source[bot] commented on code in PR #41635:
URL: https://github.com/apache/superset/pull/41635#discussion_r3658135045
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -49,6 +53,94 @@ def test_security_manager(app_context: None) -> None:
assert sm
+def test_superset_oauth_view_oauth_oauth_single_provider(
Review Comment:
**Suggestion:** The test name contains a duplicated `oauth`, which is an
obvious naming typo and makes the test harder to identify consistently. [typo]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Test discovery and execution remain unaffected.
- โ ๏ธ Only test readability and searchability are reduced.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Run the test suite containing
`tests/unit_tests/security/manager_test.py:56`, which
defines `test_superset_oauth_view_oauth_oauth_single_provider`.
2. Pytest discovers and executes the test despite the duplicated `oauth`
token in its
function name.
3. The test invokes `SupersetOAuthView.login()` at
`tests/unit_tests/security/manager_test.py:76` and validates the behavior at
line 78; the
typo does not alter execution or assertions.
4. The issue is therefore limited to readability and naming consistency; no
functional
failure can be reproduced from the test name itself.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2f54994c3c6f406e877d2d46744d5891&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=2f54994c3c6f406e877d2d46744d5891&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 56:56
**Comment:**
*Typo: The test name contains a duplicated `oauth`, which is an obvious
naming typo and makes the test harder to identify consistently.
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%2F41635&comment_hash=afa865a1c258a830df5f92c96faa3ca26de40d0e861040068aeca049453482f7&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41635&comment_hash=afa865a1c258a830df5f92c96faa3ca26de40d0e861040068aeca049453482f7&reaction=dislike'>๐</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -49,6 +53,94 @@ def test_security_manager(app_context: None) -> None:
assert sm
+def test_superset_oauth_view_oauth_oauth_single_provider(
+ mocker: MockerFixture, app: Flask, app_context: None
+) -> None:
+ """
+ Test that SupersetOAuthView auto-selects the provider and delegates
+ to the parent login method when only a single OAuth provider is configured.
+ """
+ mock_provider = MagicMock()
+ mock_remotes = {"google": mock_provider}
+
+ mocker.patch.object(app.appbuilder.sm, "oauth_remotes", mock_remotes)
+
+ mock_super_login = mocker.patch(
+ "superset.views.auth.AuthOAuthView.login",
return_value="mocked_response"
+ )
+
+ oauth_view = SupersetOAuthView()
+
+ oauth_view.appbuilder = app.appbuilder
+
+ oauth_view.login()
+
+ mock_super_login.assert_called_once_with("google")
+
+
+def test_superset_oauth_view_oauth_multiple_providers(
+ mocker: MockerFixture, app: Flask, app_context: None
+) -> None:
+ """
+ Test that SupersetOAuthView does NOT auto-select when multiple OAuth
+ providers are configured, and instead calls parent login without provider.
+ """
+ mock_provider1 = MagicMock()
+ mock_provider2 = MagicMock()
+ mock_remotes = {"google": mock_provider1, "github": mock_provider2}
+
+ mocker.patch.object(app.appbuilder.sm, "oauth_remotes", mock_remotes)
+
+ mock_super_login = mocker.patch(
+ "superset.views.auth.AuthOAuthView.login",
return_value="mocked_response"
+ )
+
+ oauth_view = SupersetOAuthView()
+
+ oauth_view.appbuilder = app.appbuilder
+
+ oauth_view.login()
+
+ mock_super_login.assert_called_once_with(None)
+
+
+def test_security_manager_with_oauth_auth_type(
+ mocker: MockerFixture, app: Flask, app_context: None
+) -> None:
+ """
+ Test that SupersetSecurityManager login works correctly when
+ AUTH_TYPE is set to AUTH_OAUTH with a single provider.
+ """
+ from flask_appbuilder.security.manager import AUTH_OAUTH
+
+ app.config["AUTH_TYPE"] = AUTH_OAUTH
+ app.config["OAUTH_PROVIDERS"] = [
+ {
+ "name": "google",
+ "icon": "fa-google",
+ "token_key": "access_token",
+ "remote_app": {
+ "client_id": "test_client_id",
+ "client_secret": "test_client_secret",
+ "api_base_url": "https://accounts.google.com/o/oauth2/v2/auth";,
+ "client_kwargs": {"scope": "email profile"},
+ },
+ }
+ ]
+
+ mock_provider = mocker.Mock()
+ mock_remotes = {"google": mock_provider}
+ mocker.patch.object(app.appbuilder.sm, "oauth_remotes", mock_remotes)
Review Comment:
**Suggestion:** This test does not actually verify the `AUTH_TYPE` or
`OAUTH_PROVIDERS` configuration because it replaces
`app.appbuilder.sm.oauth_remotes` with a mock before invoking the view. It
would pass even if configuration-driven OAuth provider initialization were
broken; instantiate or configure the security manager through the real
configuration instead of bypassing it. [possible bug]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ ๏ธ OAuth configuration initialization regressions can pass CI.
- โ ๏ธ Single-provider login coverage only tests an injected provider map.
- โ ๏ธ Production authentication setup remains insufficiently validated.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Run `test_security_manager_with_oauth_auth_type` from
`tests/unit_tests/security/manager_test.py:107`.
2. The test sets `AUTH_TYPE` and a single `OAUTH_PROVIDERS` entry at
`tests/unit_tests/security/manager_test.py:116-129`, but does not construct
or
reinitialize the security manager from those settings.
3. At line 133, it replaces `app.appbuilder.sm.oauth_remotes` with
`{"google":
mock_provider}` before `SupersetOAuthView.login()` is called at line 139.
4. The view therefore observes the injected dictionary and delegates to
`AuthOAuthView.login("google")` at line 141, even if configuration-driven
creation of
`oauth_remotes` were broken.
5. Introduce a failure in the security manager's OAuth provider
initialization and rerun
this test; the patched `oauth_remotes` still allows the test to pass,
demonstrating that
the test does not cover the configuration-to-provider initialization path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=07f38efbbdfd4e93a236655308ce7c53&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=07f38efbbdfd4e93a236655308ce7c53&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 117:133
**Comment:**
*Possible Bug: This test does not actually verify the `AUTH_TYPE` or
`OAUTH_PROVIDERS` configuration because it replaces
`app.appbuilder.sm.oauth_remotes` with a mock before invoking the view. It
would pass even if configuration-driven OAuth provider initialization were
broken; instantiate or configure the security manager through the real
configuration instead of bypassing it.
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%2F41635&comment_hash=4be0a7b818341ec21861cbb08d46c99ed9149811659d181078cbc7eb06c17fde&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41635&comment_hash=4be0a7b818341ec21861cbb08d46c99ed9149811659d181078cbc7eb06c17fde&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
