feat(auth): Skip OAUTH provider selection - #41635
Conversation
Code Review Agent Run #36a495Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41635 +/- ##
==========================================
- Coverage 64.41% 64.06% -0.36%
==========================================
Files 2668 2774 +106
Lines 147198 155683 +8485
Branches 33949 35210 +1261
==========================================
+ Hits 94824 99739 +4915
- Misses 50657 53965 +3308
- Partials 1717 1979 +262
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The PR description mentions an |
|
Sorry. Copy&paste error. Fixed. |
|
Thanks for clearing up the description. |
|
I am not really familiar with this testing framework so I do not mind if you take over. I will check how you did it and try to do it properly next time... |
The new OAuth provider-selection tests were inserted in place of the stored_metrics fixture, deleting it and breaking test_raise_for_access_guest_user_ok and other downstream tests that depend on it.
|
Pushed a fix. The new OAuth tests had landed right on top of the |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| assert sm | ||
|
|
||
|
|
||
| def test_superset_oauth_view_oauth_oauth_single_provider( |
There was a problem hiding this comment.
Suggestion: The test name contains a duplicated oauth, which is an obvious naming typo and makes the test harder to identify consistently. [typo]
Severity Level: Minor 🧹
- ⚠️ Test discovery and execution remain unaffected.
- ⚠️ Only test readability and searchability are reduced.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| 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) |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ OAuth configuration initialization regressions can pass CI.
- ⚠️ Single-provider login coverage only tests an injected provider map.
- ⚠️ Production authentication setup remains insufficiently validated.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
Code Review Agent Run #90639bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
This PR introduces functionality to skip provider selection screen if there is only one OAUTH provider and forwards request directly to the only OAUTH provider.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Provider selection should not be visible when only one provider used.
ADDITIONAL INFORMATION