[nitpick] The test explicitly sets cookie_secret on a new Application instance via self.get_app(), but the AsyncHTTPTestCase harness already applies this secret returned by get_app for handling secure cookies. You can remove this redundant assignment to simplify the test.

nit: Wouldn't it be possible to construct this from the configured redirect_uri (the url without /oauth2callback) which I think is required anyways? just protocol and host (or just origin) might be wrong in some setups. E.g. if app is deployed under an url path via a proxy.

This is a very good catch and calling it a nit really does not do it justice.
My guess is that without the fix, the code would actually fail under some deployment scenarios.
Thank you for noticing it :)

I think there might still be some deployment scenarios where the current get_origin_from_secrets implementation won't work here. Redirecting to the origin of our redirect_uri may not work in cases where the app isn't being served from the root path of its origin. For example, it's possible to set up an app served from https://my.streamlit.app/path/to/app with redirect url https://my.streamlit.app/path/to/app/oauth2callback for which the redirect after logout will break.

It might be fine to merge this change as-is and make a quick followup since the end_session_endpoint doesn't work at the moment anyway, but it'd be slightly preferable to fix this in this PR.

(This might change the correct way to approach my other comment)

nit: The graphite request for a docstring may be overkill, but it might be useful to tweak this name to be get_redirect_uri_origin_from_secrets or something similar that makes it more clear what origin corresponds to now that the function lives a bit further away from where it's used.

@vdonato just so I understand your idea correctly, would it work if I:

a) change the name like you recommend
b) move the fallback of redirect uri missing here
c) change the fallback to use server.baseUrlPath (if present) ?

Or is there something I'm still not thinking of or misunderstanding?

Thinking about this more, I think we might end up needing two separate helper functions since we'll still need this function to get the origin for the other/older use case in AuthCallbackHandler.get (we could probably just keep the _get_origin_from_secrets method where it was and define a new one for the logout redirect url).

The fallback code can stay where it is since passing the request to this helper doesn't seem ideal, and the main functional change we'll want to make is to build post_logout_redirect_uri using server.baseUrlPath so that we handle the case where an app isn't served from the root of its origin.

Another thing that I was worried about is whether we have to consider cases where the Streamlit server thinks the base path where the app is being served is one thing, but the browser thinks it's another (this scenario is somewhat contrived, but I think it's possible if you, for example, have a load balancer in front of your Streamlit instance that receives traffic for the app at one path and routes it to another). I don't think this is an issue, though, because the login URLs generated in user_info.py are set using server.baseUrlPath, so it seems like we already have an implicit assumption that this doesn't happen for the auth feature to work.

I think I'm getting confused now, so trying to lay down a clear plan again:

1. Revert the _get_origin_from_secrets as it was originally for oauthCallback
2. Instead of using redirect_uri from secrets to construct redirect for logout, just use server.baseUrlPath

Or did you mean something more elaborate still?

Yeah, I think that's more or less what needs to be done. We are essentially relying on the assumption that our redirect_uri is equal to {origin}/{server.baseUrlPath}/oauth2callback for things to work, but we seem to be making that assumption for login already, so I think it's safe to do the same here.

After reviewing the code a little, I decided to go for a slightly different solution, and instead of taking server.baseUrlPath I am just stripping /oauth2callback from redirect uri.

The main reason I believe this is the better approach is that for deployments that work at domain root, baseUrlPath might not be set to the correct external URL (as things will work without it), whereas redirect_uri is guaranteed to be the public url or login would fail.

I do have to admit this is a very unfortunate case of duplication, however. In the ideal world, one of those two configuration parameters should be deprecated (with my bet being the redirect uri being the better candidate for it), but this would require a whole process for something as widely used as streamlit, so it might be just a bit too much trouble for the value it would create.

Sounds reasonable to me! Agreed on being able to only have one of server.baseUrlPath/redirect_uri would be ideal, but changing things so that we only need one of these two config values is definitely outside of the scope of this PR.

I think there might still be some deployment scenarios where the current get_origin_from_secrets implementation won't work here. Redirecting to the origin of our redirect_uri may not work in cases where the app isn't being served from the root path of its origin. For example, it's possible to set up an app served from https://my.streamlit.app/path/to/app with redirect url https://my.streamlit.app/path/to/app/oauth2callback for which the redirect after logout will break.

It might be fine to merge this change as-is and make a quick followup since the end_session_endpoint doesn't work at the moment anyway, but it'd be slightly preferable to fix this in this PR.

(This might change the correct way to approach my other comment)