-
Notifications
You must be signed in to change notification settings - Fork 4k
Support new auth features in Starlette #13571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
|
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for OIDC end_session_endpoint logout flow to Streamlit's authentication system, enabling proper logout from OAuth providers. The implementation refactors common auth logic into shared utility functions and adds the provider field to user cookies to support provider-specific logout URLs.
Changes:
- Introduced three new shared functions in
auth_util.py:get_validated_redirect_uri(),get_origin_from_redirect_uri(), andbuild_logout_url()to centralize redirect URI handling and logout URL construction - Updated both Tornado and Starlette auth routes to use the shared utilities and implement provider logout URL retrieval via
_get_provider_logout_url() - Added
providerfield to user cookies in both auth callback handlers to enable logout URL construction - Enhanced OIDC mock server with end_session_endpoint support and added E2E test for the logout flow
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/streamlit/auth_util.py |
Added three shared utility functions for redirect URI validation, origin extraction, and logout URL building with proper docstrings and type hints |
lib/streamlit/web/server/oauth_authlib_routes.py |
Refactored to use shared auth utilities, removed duplicate redirect URI and origin extraction logic, and integrated build_logout_url() |
lib/streamlit/web/server/starlette/starlette_auth_routes.py |
Implemented provider logout URL retrieval, added cookie value extraction helper, and integrated shared auth utilities for consistency |
lib/tests/streamlit/auth_util_test.py |
Added comprehensive unit tests for the three new utility functions covering various edge cases and port placeholder substitution |
lib/tests/streamlit/web/server/starlette/starlette_auth_routes_test.py |
Updated tests for refactored origin extraction logic and added extensive tests for provider logout URL retrieval and redirect behavior |
e2e_playwright/shared/oidc_mock_server.py |
Enhanced mock OIDC server to include end_session_endpoint in metadata and handle logout redirects |
e2e_playwright/auth.py |
Added logout button and st.logout() call to test script for E2E testing |
e2e_playwright/auth_test.py |
Added E2E test verifying complete logout flow with end_session_endpoint redirect |
lib/tests/streamlit/web/server/starlette/starlette_auth_routes_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!
…auth-logic-in-starlette
SummaryThis PR implements auth-related features from PR #12693 for the new Starlette implementation. The key changes include:
Code QualityThe code quality is excellent and follows Streamlit's best practices: ✅ Type annotations: All new functions are fully typed with proper return type hints Specific highlights:
Test CoverageThe test coverage is comprehensive and well-structured: Unit Tests (
|
lib/streamlit/auth_util.py
Outdated
| if id_token: | ||
| logout_params["id_token_hint"] = id_token | ||
|
|
||
| return f"{end_session_endpoint}?{urlencode(logout_params)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: This looks to always include a ?, will this break if the endpoint already has existing query params in it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's usually a clean URL, but I've updated the code to handle existing query params defensively for non-standard providers (on tornado and starlette side)
e2e_playwright/auth_test.py
Outdated
| """ | ||
| get_button(app, button_label).click() | ||
| # Wait for OAuth redirect chain to complete and return to app root | ||
| app.wait_for_url(re.compile(r"http://localhost:\d+/$")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is using localhost resilient enough for our testing harness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think localhost is somewhat fine, its also used in the AUTH_SECRETS_TEMPLATE. But I made it more explicit by using the actual part the app is running on.
…auth-logic-in-starlette
Describe your changes
Implementing auth-related features from #12693 and #12693 for the new Starlette implementation. This also applies some refactoring for better reuse and adds an e2e test.
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.