Make st.logout use end_session_endpoint if provided in OIDC config (V2)#12693
Make st.logout use end_session_endpoint if provided in OIDC config (V2)#12693kmcgrady merged 8 commits intostreamlit:developfrom
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. |
There was a problem hiding this comment.
Pull Request Overview
Adds support for OIDC provider logout by redirecting to the end_session_endpoint when available, and introduces infrastructure to expose selected OAuth tokens to app code via a new st.user.tokens API. Key changes include splitting user auth data and tokens into separate signed cookies, updating logout / callback flows, and adding token exposure and access utilities with accompanying tests.
- Implement provider-aware logout (uses end_session_endpoint with client_id, post_logout_redirect_uri, optional id_token_hint).
- Add tokens cookie, exposure configuration (expose_tokens), TokensProxy, and st.user.tokens accessor.
- Expand test coverage for logout flows, callback behavior, token exposure, and configuration parsing.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/streamlit/web/server/server_util.py | Adds constant for separate tokens cookie. |
| lib/streamlit/web/server/oauth_authlib_routes.py | Implements provider logout logic, splits auth vs token cookies, and augments callback handling. |
| lib/streamlit/web/server/browser_websocket_handler.py | Reads tokens cookie and filters exposed tokens per configuration. |
| lib/streamlit/user_info.py | Adds TokensProxy and st.user.tokens API; updates logout docstring. |
| lib/streamlit/auth_util.py | Adds get_expose_tokens_config helper. |
| lib/tests/streamlit/web/server/oauth_authlib_routes_test.py | Adds tests for logout behavior with/without end_session_endpoint and id_token_hint. |
| lib/tests/streamlit/user_info_test.py | Adds tests for TokensProxy and st.user.tokens behavior. |
| lib/tests/streamlit/auth_util_test.py | Adds tests for expose_tokens config parsing. |
| """ | ||
| auth_section = get_secrets_auth_section() | ||
| expose_tokens = auth_section.get("expose_tokens") | ||
|
|
||
| if expose_tokens is None: | ||
| return [] | ||
|
|
||
| if isinstance(expose_tokens, str): | ||
| return [expose_tokens] | ||
| if isinstance(expose_tokens, list): | ||
| return [str(token) for token in expose_tokens] | ||
| return [] | ||
|
|
||
|
|
There was a problem hiding this comment.
The function accepts arbitrary token labels which (when combined with later f"{token_type}_token" logic) allows exposing sensitive refresh_token values if the config includes "refresh". Consider restricting to an explicit allowlist (e.g. {"id", "access"}) or requiring an explicit opt-in flag for refresh tokens to reduce risk of leaking long-lived credentials.
| """ | |
| auth_section = get_secrets_auth_section() | |
| expose_tokens = auth_section.get("expose_tokens") | |
| if expose_tokens is None: | |
| return [] | |
| if isinstance(expose_tokens, str): | |
| return [expose_tokens] | |
| if isinstance(expose_tokens, list): | |
| return [str(token) for token in expose_tokens] | |
| return [] | |
| Only allows safe token types ("id", "access") by default. | |
| To expose "refresh" tokens, require explicit opt-in via expose_refresh_token = true. | |
| """ | |
| ALLOWED_TOKEN_TYPES = {"id", "access"} | |
| auth_section = get_secrets_auth_section() | |
| expose_tokens = auth_section.get("expose_tokens") | |
| expose_refresh_token = auth_section.get("expose_refresh_token", False) | |
| tokens: list[str] = [] | |
| if expose_tokens is None: | |
| tokens = [] | |
| elif isinstance(expose_tokens, str): | |
| tokens = [expose_tokens] | |
| elif isinstance(expose_tokens, list): | |
| tokens = [str(token) for token in expose_tokens] | |
| else: | |
| tokens = [] | |
| # Filter tokens to only allow those in the allowlist | |
| filtered_tokens = [token for token in tokens if token in ALLOWED_TOKEN_TYPES] | |
| # Only add "refresh" if explicit opt-in is set | |
| if expose_refresh_token and "refresh" in tokens: | |
| filtered_tokens.append("refresh") | |
| return filtered_tokens |
| except Exception as e: | ||
| _LOGGER.warning("Failed to get provider logout URL: %s", e) | ||
| return None |
There was a problem hiding this comment.
Broad except Exception masks unexpected errors (e.g. network/config issues) that could prevent proper logout diagnostics; narrow this to the specific exceptions you expect (e.g. json.JSONDecodeError, TypeError) or re-raise after logging for non-handled types.
| except Exception as e: | |
| _LOGGER.warning("Failed to get provider logout URL: %s", e) | |
| return None | |
| except (json.JSONDecodeError, TypeError, AttributeError) as e: | |
| _LOGGER.warning("Failed to get provider logout URL: %s", e) | |
| return None | |
| except Exception as e: | |
| _LOGGER.error("Unexpected error in provider logout URL: %s", e) | |
| raise |
|
|
||
| @property | ||
| def tokens(self) -> TokensProxy: | ||
| """Access exposed tokens via a dict-like object.""" |
There was a problem hiding this comment.
Expand the property docstring to document how to configure expose_tokens, list returned token name mappings (e.g. tokens.id -> id_token), and emphasize immutability and security considerations.
| """Access exposed tokens via a dict-like object.""" | |
| """ | |
| Access exposed authentication tokens via a dict-like object. | |
| Configuration: | |
| The tokens exposed here are controlled by the `expose_tokens` configuration | |
| option in your Streamlit app's settings. For example, you can specify which | |
| tokens to expose by setting `expose_tokens = ["id_token", "access_token"]` | |
| in your configuration file. | |
| Token name mappings: | |
| The returned object maps token names to their values. For example: | |
| - `tokens.id` or `tokens["id"]` returns the value of the `id_token` | |
| - `tokens.access` or `tokens["access"]` returns the value of the `access_token` | |
| The available keys depend on your authentication provider and configuration. | |
| Immutability and security: | |
| The returned `TokensProxy` object is immutable; tokens cannot be modified | |
| through this interface. Tokens are sensitive credentials—do not log, print, | |
| or share them. Handle with care to avoid accidental exposure. | |
| Returns | |
| ------- | |
| TokensProxy | |
| A dict-like object containing the exposed authentication tokens. | |
| """ |
## Describe your changes Store the OIDC auth response in a cookie and expose them to users for API calls Currently, from user perspective, `st.user` gets an extra field called "tokens" that is a dict of all the tokens that were made available - most often the triplet of acces_token, id_token, refresh_token This is useful for multiple use cases (see the issues below). To achieve (a) reliably, the tokens are stored in a separate cookie from user info. This is because cookies are limited to be <4096 bytes and I ran into that limit in my own testing already if I combined the two dictionaries. Additionally, both cookies can now be chunked so as a by-product, this now supports login systems with larger userdata. I have tested it to work in my own use case and it seems to be performing as intended. But it is just one use case and I may well be missing some others. so any proposals to improve this are very welcome. Originally, also contained st.user_refresh command, but that got it's own PR: #12696 ## GitHub Issue Link (if applicable) Closes #10378 Exposing access_token: #10378 Too large usedata for current login system: #12518 Need to provide id token hint at logout: #12144 (comment) -> PR #12693 Need for user refresh: #12043 -> PR #12696 ## Testing Plan - Unit tests present - Manually tested to work for my own use case --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: Ken McGrady <[email protected]>
1a5c65d to
4119a63
Compare
|
@jrieke @kmcgrady this is a follow-up to #12044 that also fixes logout to also properly log out from the oidc provider instead of just deleting the cookie. I just rebased it to the current development branch, so it should now be ready for review. It's probably better to take this on for you while you still remember the OIDC context :) |
|
Just for clarification: |
Yes, this is something that comes directly from OIDC server metadata. This does affect behavior of course, by properly logging the user out from the OIDC provider perspective as well. For instance, Frontegg (the provider we use) normally keeps it's on cookie and auto-logs you in when directed to login redirect with that cookie persent. Calling the logout properly clears it's cookie as well, meaning next time you log in, you are presented with a login screen. Which is the expected behavior (and my reason for this PR). I imagine it's the same for many other providers as well. |
SummaryThis PR implements OIDC-compliant logout functionality by redirecting users to the OAuth provider's
This addresses the issue where OIDC providers that maintain their own session cookies would auto-login users even after calling Code QualityStrengths
Minor Observations
if provider is None:
# See https://github.com/streamlit/streamlit/issues/13101
_LOGGER.warning(
"Missing provider for OAuth callback; this often indicates a stale "
"or replayed callback (for example, from browser back/forward "
"navigation).",
)
self.redirect_to_base()
return
Test CoverageStrengthsThe unit test coverage is comprehensive and well-structured:
Potential Test GapsThe following scenarios are not covered but would be good additions:
These are minor and the current tests adequately cover the main functionality. Backwards CompatibilityThis PR is backwards compatible:
Security & RiskSecurity Assessment
Risk Assessment
Recommendations
VerdictAPPROVED: This PR correctly implements OIDC-compliant logout functionality with proper fallback behavior, comprehensive unit tests, and good security practices. The change is backwards compatible and improves the user experience for OIDC providers that support This is an automated AI review. Please verify the feedback and use your judgment. |
4119a63 to
a0addf6
Compare
a0addf6 to
ec37ec3
Compare
| redirect_uri: str = auth_section["redirect_uri"] | ||
| if not redirect_uri.endswith("/oauth2callback"): | ||
| _LOGGER.warning("Redirect URI does not end with /oauth2callback") | ||
| return None |
There was a problem hiding this comment.
Do we need to include the port logic here @velochy from the other PR as well. Probably should consolidate and use redirect_uri = get_redirect_uri(auth_section), which handles that case, right?
There was a problem hiding this comment.
You are correct. Good catch :)
There was a problem hiding this comment.
Haha I try. I'll add the change :-). This should make the 1.53 release if I can get it merged today.
@velochy Do the changes look good to you?
There was a problem hiding this comment.
reluctant to comment on the removed dead code, but the other two changes look good :)
There was a problem hiding this comment.
Yea, we check provider is None twice. The first time is return so the dead code is unreachable (our static analysis found this)
## 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 - Added unit and e2e tests. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
|
Hello @velochy, firt of all, congrats on adding tokens in version 1.53! However, I encountered a 400 error when calling st.logout(): Okta configuration:
After reviewing the code, it appears that Two suggestions if I'm not mistaken:
|
|
@ppo-corp thank you for pointing these things out. Initially, the redirect URI-s were indeed different, but this led to #12144 (comment) as for making that end point configurable - it seems to not be worth the extra complexity unless there is a very clear need for it. Do you have a use case that the present system does not support at all? |
|
@velochy, thank you for your quick response. Adding this parameter (optional) would offer a little more flexibility on the application side, especially for teams that do not have control over the IdP configuration. Also, since this is a URL parameter that gives the application the choice of redirection, it would be interesting, in my opinion, if this value were not forced by Streamlit. |
|
Perhaps we could also add a parameter named |
Describe your changes
Currently, st.logout ignores end_session_endpoint (ESE) in the OIDC configuration.
This can cause issues, as some OIDC providers keep their own cookies and skip the login screen if they have an active session.
This change:
a) Writes the name of provider into the session cookie (so it would be clear which provider the user is currently logged into)
b) Makes st.logout redirect to ESE if one is provided
This is a revised version of #11901 to adress the issues raised in #12144 (comment)
Most notably:
This is a follow-up to #12044
GitHub Issue Link (if applicable)
Closes #11900
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.