aspnet / AspNetKatana

Microsoft's OWIN implementation, the Katana project
Apache License 2.0
959 stars 331 forks source link

Form POST data is lost when the user is redirected to the Identity provider. #493

Closed dibyenduk closed 1 year ago

dibyenduk commented 1 year ago

I am using the ws federation authentication middleware for passive federated authentication and facing below issue - When an user posts a form to our application and he / she is not authenticated, they are redirected to the Identity server for authentication. Once the authentication is completed, the browser redirects the user to the original application endpoint using a GET request. This loses all the form post values and also incorrectly changes the request to a GET rather than a POST.

I have created Pull request https://github.com/aspnet/AspNetKatana/pull/492 to add a pre redirect hook event to the pipeline so that the application can handle the redirect rather than it getting handled by the ws federation authentication handler.

Explanation - In the OnRedirectingtoIdentityprovider event, I check if it is a post request and save the form values in cache. Once the request returns from the Identity server, I hook into the preredirect hook event and rehydrate the request with the form values from cache and mark that I have handled the redirect by calling HandleRedirect function for the pre redirect hook. In the InvokeAsync function, the handler will check if the application has handled the redirect themselves and will not execute the code to redirect to the application using a GET request. I have tested this on my end and it works fine.

Tratcher commented 1 year ago

The user was previously authenticated to receive the form, correct? This is mainly about handling the race condition of their auth expiring between retrieval and submission?

Can you show a code sample of how you're use these events?

I think you can already do what you want by using the SecurityTokenValidated event. https://github.com/aspnet/AspNetKatana/blob/00d999d5c8bd3e733d0417b87e22a2683f5ee93d/src/Microsoft.Owin.Security.WsFederation/WsFederationAuthenticationHandler.cs#L361 The only part you'd need to duplicate is https://github.com/aspnet/AspNetKatana/blob/00d999d5c8bd3e733d0417b87e22a2683f5ee93d/src/Microsoft.Owin.Security.WsFederation/WsFederationAuthenticationHandler.cs#L192

dibyenduk commented 1 year ago

We use single sign on mechanism of ADFS / Azure for the authentication and it is possible to post values from one application to the other application. In that case, the user is not authenticated for the second application and hence they are redirected to the Identity server.

When the redirection happens, all the values in form post are lost and the user is redirected after authentication to the url as a GET rather than a POST. Till the time the request reaches SecurityTokenValidated event, all the values are already lost so we save them on RedirectingToIdentityProvider event before it get redirected to the Identity server. Now even though the values are saved, wsfederationhandler invokeasync method will redirect to the original url as a GET rather than a POST. Code explanation below -

  1. On RedirectToIdentityProvider I save the form values -
private Task RedirectToIdentityProvider(RedirectToIdentityProviderNotification<> arg)
{
    if (httpRequest.HttpMethod != HttpMethod.Post.ToString()) return Task.CompletedTask;

    var requestId = HttpContext.Current.Items["RequestId"].ToString();

    arg.ProtocolMessage.SetParameter("wctx", string.Concat(wctx, "&", CookieName, "=", requestId));

    var formCollection = AuthRedirectionManager.GetFormValues(path, httpRequest);

    AuthRedirectionManager.SaveState(requestId, formCollection);
}
  1. Once authentication is done and the request is returned back to the application, On SecurityTokenValidated I get the requestId from the wctx context (as I had appended it before in RedirectingToIdentityProvider) and push the value in AuthenticationTicket Property.
private Task SecurityTokenValidated(SecurityTokenValidatedNotification<,> arg)
{
   var wctx = HttpContext.Current.Request.Params[WCTX];
   var requestId = AuthRedirectionManager.ExtractStateValue(wctx, CookieName);
   arg.AuthenticationTicket.Properties.Dictionary["RequestId"] = requestId; 
}
  1. In the WSFederation configuration, configure PreRedirectReceived as below
var wsFederationOptions = new WsFederationAuthenticationOptions()
{                
       Notifications = new WsFederationAuthenticationNotifications()
       {
            RedirectToIdentityProvider =  RedirectToIdentityProvider                    
            SecurityTokenValidated = SecurityTokenValidated,
            PreRedirectReceived =  PreRedirectReceived , 
        },
}
  1. In the handler, I get the posted form values and create a form post and write that in the response which sends the request a POST with the values that were saved earlier. I also mark that I have handled the redirection and wsfedhandler does not need to do the redirection.
private Task PreRedirectReceived(PreRedirectReceivedNotification<,> arg)
{
      // Get the request Id
       var requestId = authenticationTicket.Properties.Dictionary["RequestId"];

     // Get the stored form values for the request Id
       var data = AuthRedirectionManager.GetState(requestId);

      // Create a form to post with the values
       var html = AuthRedirectionManager.CreatePostForm(data);

     // Clean up the stored state
     AuthRedirectionManager.ResetState(requestId);

    // Write response
    HttpContext.Current.Response.Write(html);
    HttpContext.Current.Response.End();

    // Mark that I have handled the redirection and wsfederationhandler does not need to do it.
    arg.HandledRedirect(); 
}

I can be available on a call to explain the actual usage.

In my opinion, just with RedirectingToIdentityProvider and SecurityValidated event, application does not have the liberty to handle the redirect and this addition of hook will enable it.

Tratcher commented 1 year ago

Try putting that PreRedirectReceived logic into SecurityTokenValidated. You'll also need to call SignIn.

Writing to the response and calling End like that might prevent SignIn from correctly adding the auth cookie. Keep an eye on that.

dibyenduk commented 1 year ago

The InvokeReplyPathAsync would still be called even with the logic as it is part of the pipeline. If the authenticatedTicket has a value then it will do the Signin that you mention -

private async Task<bool> InvokeReplyPathAsync()
{
    .....
    if (ticket.Identity != null)
    {
                Request.Context.Authentication.SignIn(ticket.Properties, ticket.Identity);
    }
}

The problem with SecurityTokenValidated is that the below code will still run in InvokeRepyAsync() and will do a GET redirect to the url -

// Redirect back to the original secured resource, if any.
            if (!string.IsNullOrWhiteSpace(ticket.Properties.RedirectUri))
            {
                Response.Redirect(ticket.Properties.RedirectUri);
                return true;
            }

This is just my understanding. If you still think it will work then I will try it out and see. Let me know what you think.

Tratcher commented 1 year ago

If you use HandleResponse() in SecurityTokenValidated then it will skip the redirect: https://github.com/aspnet/AspNetKatana/blob/423f80e7f82708e3f78a596cf405a2a5595c12f1/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L926-L933

dibyenduk commented 1 year ago

With HandledResponse, it will clear out the AuthenticatedTicket identity & properties and not even call Request.Context.Authentication.SignIn(ticket.Properties, ticket.Identity).

private static AuthenticationTicket GetHandledResponseTicket()
        {
            return new AuthenticationTicket(null, new AuthenticationProperties(new Dictionary<string, string>() { { HandledResponse, "true" } }));
        }

Are you saying that we call the Request.SignIn() on SecurityTokenValidated and hence even if it does not call it in InvokeReplyPathAsync then it will work fine ?

I understand what you are saying now. I will try it out and see.

Though an API / event hook for the redirection as I have in the Pull request would be beneficial as well. It would be more straight forward as you just handle the redirection and mark arg.IsHandledRedirection() rather than the hack to do Signin on SecurityTokenValidated and mark HandledResponse = true.

Tratcher commented 1 year ago

We need to verify if this is possible with existing APIs before considering new ones. Note that these libraries are not under active development so even if the new callback was added there's no telling when it would ship.

dibyenduk commented 1 year ago

I changed the securitytokenvalidated as follows and it worked. Thanks for the help.

private Task SecurityTokenValidated(SecurityTokenValidatedNotification<WsFederationMessage, WsFederationAuthenticationOptions> arg)
{
......
......
                var html = AuthRedirectionManager.CreatePostForm(data);
                arg.OwinContext.Authentication.SignIn(arg.AuthenticationTicket.Properties, arg.AuthenticationTicket.Identity);
                arg.HandleResponse();
                HttpContext.Current.Response.Write(html);             
                return Task.CompletedTask;
}
dibyenduk commented 1 year ago

This issue can be closed as I found a workaround (mentioned above).