dotMorten / WinUIEx

WinUI Extensions
https://dotmorten.github.io/WinUIEx
MIT License
572 stars 36 forks source link

Add BeforeProcessStart hook #141

Closed frederikprijck closed 8 months ago

frederikprijck commented 8 months ago

This PR is in a very draft state and serves as a way to demonstrate the functionality we are looking for in order to integrate WinUIEX. Nothing in here is final at all, but I figured it'd be good to have a conversation about whether this would be something you want to merge eventually, so I can work on getting it in better shape.

The issue Auth0 does not send back a state parameter when one is being passed to /logout. This means there is no signinId or applicationId available to ensure the correct instance is activated after being redirected back.

The consequence of that is that after logging out, a new instance is opened.

The solution Auth0's logout URL accepts a returnTo parameter that it uses to redirect back. If we append a state querystring parameter to the url inside returnTo, it works.

So the goal would be to call logout using a URL like this (ignoring correct URL encoding to keep it readable):

my.domain/logout?returnTo=myapp://callback?state=STATE_GOES_HERE;

To achieve that, we need to find a way to intercept between generating the state, and opening the browser, which seems to be impossible.

This PR adds a BeforeProcessStart hook to achieve that.

Because of the above, we also need to escape the state on logout (like was done before in WinUIEx, but reverted).

How to use it Using it would come down to:

public class WebAuthenticatorBrowser : IdentityModel.OidcClient.Browser.IBrowser
{
    public async Task<BrowserResult> InvokeAsync(BrowserOptions options, CancellationToken cancellationToken = default)
    {
        try
        {
            if (options.StartUrl.IndexOf("logout") > -1)
            {
                WinUIEx.WebAuthenticator.BeforeProcessStart = (uri) =>
                {
                    var query = System.Web.HttpUtility.ParseQueryString(uri.Query);
                    // The state QueryString as generated by WinUIEx
                    var state = query["state"];
                    // The original returnTo as configured externally
                    var returnTo = query["returnTo"];

                    UriBuilder returnToBuilder = new UriBuilder(returnTo);

                    // Get the original returnTo querystring params, so we can append state to it
                    var returnToQuery = System.Web.HttpUtility.ParseQueryString(new Uri(returnTo).Query);
                    // Append state as a querystring parameter to returnTo
                    // We need to escape it for it to be accepted
                    returnToQuery["state"] = Uri.EscapeDataString(state);
                    // Set the query again on the returnTo url
                    returnToBuilder.Query = returnToQuery.ToString();

                    // Update returnTo in the original query so that it now includes state
                    query["returnTo"] = returnToBuilder.Uri.ToString();

                    UriBuilder logoutUrlBuilder = new UriBuilder(uri);
                    // Set the query again on the logout url
                    logoutUrlBuilder.Query = query.ToString();

                    // Return the Uri so it can be used internally by WinUIEx to start the process and open the browser
                    return logoutUrlBuilder.Uri;
                };
            }

            var result = await WinUIEx.WebAuthenticator.AuthenticateAsync(new Uri(options.StartUrl), new Uri(options.EndUrl));

            var url = new RequestUrl(options.EndUrl)
                .Create(new Parameters(result.Properties));

            return new BrowserResult
            {
                Response = url,
                ResultType = BrowserResultType.Success
            };
        }
        catch (TaskCanceledException)
        {
            return new BrowserResult
            {
                ResultType = BrowserResultType.UserCancel,
                ErrorDescription = "Login canceled by the user."
            };
        }
    }
}

I understand there are all kinds of different ways to approach this (e.g. we could also pass the state to the hook, instead of getting the state again from the URL, or we can avoid a hook). I used a hook approach as that might be the most flexible and the least specific to our use-case.

Happy to hear your opinion on the use-case we have, the proposed way to solve it as well as any other solutions you would prefer.

Disclaimer: I am not realy familiar with WinUIEx, so this is my first stab at the source code. So there may be better ways to achieve this. But as I was modifying WinUIEx locally to debug the cause of the behavior I was seeing, I figured I could open a PR with the changes to discuss them.

dotMorten commented 8 months ago

Thank you for this, but I'm not really following the need for this in the first place. If you want to sign out, just throw away the tokens that were generated during sign in, and the app is no longer signed in. Or if you want to also revoke the token, just make a webrequest to the logout endpoint to cause the token to get revoked - I don't see the need for a redirect workflow for logout. I've actually never seen an oauth service before that does this?

frederikprijck commented 8 months ago

If you want to sign out, just throw away the tokens that were generated during sign in, and the app is no longer signed in.

We can throw away tokens, but that isn't realy logging the user out of the Identity Provider. There is still a cookie/session with the IDP and any subsequent call to login to the identity provider will reuse that same cookie/session and not prompt the user for credentials, which is weird and potentially a security concern (why is the user still logged in when they asked to be logged out?). So we want to ensure we open the browser, clear the cookie and close the browser unless the user explicitly asks to only logout locally, in which case we only throw away the tokens.

if you want to also revoke the token

This isn't about revoking any tokens, but just logging out from the IDP which are two totally unrelated things (a token is usable unrelated to the session at the IDP, we can kill the session but any valid token is still usable untill revoked, and we can revoke tokens while the session continues to stay active).

The functionality I describe is also according to the OIDC spec for RP Initiated logout (not OAuth), more specifically OpenID Provider Discovery Metadata, which says:

end_session_endpoint: URL at the OP to which an RP can perform a redirect to request that the End-User be logged out at the OP. This URL MUST use the https scheme and MAY contain port, path, and query parameter components.

I think the important bit here is the redirect here, which is what we are doing here and what caused this PR and why we rely on WebAuthenticator to also handle the logout and not just login.

Additionally, I do want to mention that this works fine on the WebAuthenticator built-in to MAUI on iOS, macOS and Android, but doesnt work with the WebAuthenticator of WinUIEx because it can not activate the correct instance. Because of this, I opened a PR to help bring the functionality closer to the WebAuthenticator built-into MAUI, even though I know this is specific for WinUI, it does show how WebAuthenticator behaves on other platforms.

dotMorten commented 8 months ago

logging out should also open a browser to clear the cookie

But that's in the browser. The app you're authenticating doesn't have cookies and is separate from the browser process. The webauthenticator is used to sign in the app - its purpose isn't really for also signing into a browser as well - that's just part of the oauth workflow.

Lastly WebAuthenticator doesn't provide any APIs for logout, and your PR doesn't add it either, so there's not really anything for it to do to react on in an activation event. You don't need WinUIEx to react to the startup call. You can just add your own check in the constructor, rather than relying on a callback.

Or put another way: WebAuthenticator isn't an API to handling arbitrary app launch arguments - there's already an API for that in the Windows App SDK that you can just use.

This means there is no signinId or applicationId available to ensure the correct instance is activated after being redirected back

You must have something from the redirect, or you won't be able to reactivate the correct application instance, or complete the task you're waiting for.

frederikprijck commented 8 months ago

WebAuthenticator starts the browser, and allows to start the session which ends up with a cookie in the browser. We should also have a way to clear the cookie in that same browser that got opened to login.

Or put another way: WebAuthenticator isn't an API to handling arbitrary app launch arguments - there's already an API for that in the Windows App SDK that you can just use.

I am not trying to use WebAuthenticator for that, I am using it for authentication based browser interactions, in this case to open the browser to logout, clear the cookies set by the same browser opened by WebAuthenticator, close again and re-activate the correct application again.

You must have something from the redirect, or you won't be able to reactivate the correct application instance, or complete the task you're waiting for.

That is what this PR is trying to solve. Not all IDP's accept state on the logout URL. On login, state is sent back, but not on logout. So there is nothing to reactivate this app. This PR allows users to hook into the process, add any queryparam to then use to reactivate the correct instance.

To be clear, these changes have been tested to solve the exact issue I am describing. So even though it does not add Logout APIs, I am 100% convinced this solves the issue described (even though I would argue it could benefit from reconsidering the implementation to fit WinUIEx).

Having said that, if you don't believe this is something you want to solve in WebAuthenticator, I understand and am happy to close the PR.

dotMorten commented 8 months ago

This PR allows users to hook into the process, add any queryparam to then use to reactivate the correct instance.

How though? When the app launches you have no state to append to the parameters. The activation launches an entirely new app instance - you need the app id to find the already running instance that initiated the request, forward the parameters to that instance, and shut the new instance down.

Having said that, if you don't believe this is something you want to solve in WebAuthenticator, I understand and am happy to close the PR.

It feels more like we're trying to shoe-horn generic app activation handling into WebAuthenticator. Perhaps there's a more broader idea behind helping dealing with app activation easier, but I don't think that would belong on the authenticator.

frederikprijck commented 8 months ago

How though?

Here is an example that uses WinUI, with some files in the WinUIExEx namespace that contain the patched versions of this PR. You can see how it works as expected. If you do a find+replace of WinUIExEx with WinUIEx (but not the namespace of my patched files) and drop the BeforeProcessStart, you will see how it behaves differently.

To move this to MAUI, all we need to do is replace this line: https://github.com/frederikprijck/winuiex-logout/blob/main/WinUIApp/WebAuthenticatorBrowser.cs#L55 with:

#if WINDOWS
    var result = await WinUIEx.WebAuthenticator.AuthenticateAsync(new Uri(options.StartUrl), new Uri(options.EndUrl));
#else
    var result = await WebAuthenticator.Default.AuthenticateAsync(new Uri(options.StartUrl), new Uri(options.EndUrl));
#endif

And everything works as expected on iOS, macOS, Android and Windows. You can see that, even for the built-in Authenticator, it works perfectly fine to logout through the authenticator's AuthenticateAsync. If needed, I can share a MAUI sample to show that as well.

I figured there is a difference in the WebAuthenticator in WinUIEx and those built into MAUI, mostly in terms of supporting Logout. That's why I opened this PR, now it's API is kinda a drop in replacement for the built-in WebAuthenticator to support logout, apart from the state, which I think is Windows specific anyway (the implementation in this PR might not be ideal, but reactivating the app after logout is windows specific and something that the built-in web authenticator for iOS, macOS and Android solve as well).

If we would introduce a logout-specific API, it would be different for the built-in WebAuthenticator as that's all done through the AuthenticateAsync. Even the official sample for the corresponding OIDC library use this: https://github.com/IdentityModel/IdentityModel.OidcClient.Samples/blob/main/Maui/MauiApp2/MauiApp2/MauiAuthenticationBrowser.cs#L12

Anyway, I believe this does not match with the intention of WinUIEx' WebAuthenticator and will close this PR. This will mean we will need a different solution on our side, I might look into if I can just use WinUIEx to open the browser instead, but it would mean the implementation for all platforms uses pure WebAuthenticator, but for windows it will not. That isn't the end of the world, as it will be hidden from our users anyway, just thought it would make sense to align the WebAuthenticator.

Let me know if you want to reconsider, I definitely prefer using WinUIEx's WebAuthentricator fully in our project and would be happy to help ship logout support that's compatible with the built-in WebAuthenticator in MAUI (without adding a dedicated API for logout).

Regardless, thanks for the time and discussion. I really appreciate WinUIEx and think it's a great addition to fill a gap in WinUI 🙏