aspnet / AspNetKatana

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

notification.AuthenticationTicket is null in OnAuthorizationCodeReceived with PKCE #436

Closed mg-arrow closed 8 months ago

mg-arrow commented 2 years ago

I am using latest OWIN 4.2 and have PKCE enabled.

Check settings below.

app.UseOpenIdConnectAuthentication(
                new OpenIdConnectAuthenticationOptions
                {
                    MetadataAddress = string.Format(
                        ConfigurationManager.AppSettings["ida:AadInstance"],
                        ConfigurationManager.AppSettings["ida:Tenant"],
                        ConfigurationManager.AppSettings["ida:SignUpSignInPolicyId"]),

                    ClientId = ConfigurationManager.AppSettings["ida:ClientId"],
                    RedirectUri = ConfigurationManager.AppSettings["ida:RedirectUri"],
                    PostLogoutRedirectUri = ConfigurationManager.AppSettings["ida:RedirectUri"],
                    Notifications = new OpenIdConnectAuthenticationNotifications
                    {
                        RedirectToIdentityProvider = OnRedirectToIdentityProvider,
                        AuthorizationCodeReceived = OnAuthorizationCodeReceived,
                        AuthenticationFailed = OnAuthenticationFailed,
                        SecurityTokenValidated = OnSecurityTokenValidated
                    },
                    TokenValidationParameters = new Microsoft.IdentityModel.Tokens.TokenValidationParameters
                    {
                        NameClaimType = "signInName"
                    },

                    Scope = ConfigurationManager.AppSettings["ida:IdTokenScopes"],

                    SignInAsAuthenticationType = "cookie",
                    ResponseMode = OpenIdConnectResponseMode.Query,
                    ResponseType = OpenIdConnectResponseType.Code,
                    RedeemCode = true,
                    UseTokenLifetime = false,
                    UsePkce = true,
                    SaveTokens = true
                });

Application is integrated with Azure B2C and user is redirected to Azure B2C for login. User is redirected back after successfull login and authorization code received as well.

Code verifier is populated and can be retrieved by using this line. notification.TokenEndpointRequest.Parameters.TryGetValue("code_verifier", out var codeVerifier);

I am able to get access token inside the AuthorizationCodeReceived by calling AcquireTokenByAuthorizationCode with some additional custom code.

However the notification.AuthenticationTicket is null.

There are few other observations:

That causes the Authentication to fail.

The same flow works fine with the same Azure B2C policies but without PKCE.

Tratcher commented 2 years ago

Can you share your OnAuthorizationCodeReceived code?

mg-arrow commented 2 years ago

Hi Chris,

Please find the OnAuthorizationCodeReceived below

    private Task OnAuthorizationCodeReceived(AuthorizationCodeReceivedNotification notification)
    {

        notification.TokenEndpointRequest.Parameters.TryGetValue("code_verifier", out var codeVerifier);

        if (codeVerifier != null)
        {

           // get access token for API calls
            var accessToken = getAccessToken(notification, codeVerifier);

            if (!accessToken.Equals(null))
            {
                var tokenClaim = new Claim("access_token", accessToken);
                **notification.AuthenticationTicket.Identity.AddClaim(tokenClaim);**

            }

           // Now, make sure a membership user is in sync with the currently logged in user
            var memberName = UpsertMembershipMember(notification.AuthenticationTicket.Identity);

        }

        return Task.FromResult(0);
    }

Fails on the highlighted line even after access token is received.

Tratcher commented 2 years ago
  • OnAuthorizationCodeReceived is firing before OnSecurityTokenValidated

This is expected in the code flow, since the code is needed to retrieve the token.

https://github.com/aspnet/AspNetKatana/blob/b32c437b05217f367c201e420f147acf115e0712/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L400-L401

notification.AuthenticationTicket is null

That's expected, the ticket isn't created until after OnAuthorizationCodeReceived, unless you create it yourself.

Have you tried customizing the AuthorizationCodeReceivedNotification.TokenEndpointRequest rather than retrieving the token yourself? https://github.com/aspnet/AspNetKatana/blob/b32c437b05217f367c201e420f147acf115e0712/src/Microsoft.Owin.Security.OpenIdConnect/Notifications/AuthorizationCodeReceivedNotification.cs#L42

Then you could do your custom logic later in the TokenResponseReceived or SecurityTokenValidated events.

Overall I recommend reading through the flow here so you can better understand the order of operations. https://github.com/aspnet/AspNetKatana/blob/b32c437b05217f367c201e420f147acf115e0712/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L413-L524

mg-arrow commented 2 years ago

Chris,

Thanks for your comment.

I am confused, it seems like you are handling this same thing in the library here:

ClaimsIdentity claimsIdentity = tokenEndpointUser.Identity as ClaimsIdentity; ticket = new AuthenticationTicket(claimsIdentity, properties);

     var securityTokenValidatedNotification = new SecurityTokenValidatedNotification<OpenIdConnectMessage, OpenIdConnectAuthenticationOptions>(Context, Options) 
     { 
         AuthenticationTicket = ticket, 
         ProtocolMessage = tokenEndpointResponse, 
     }; 

Why do I need as consumer of the library write similar code ?

Thanks

Tratcher commented 2 years ago

I don't think you do, you just need to put your code later in the flow. If you customize the TokenEndpointRequest with your extra paramters then the existing code should be able to retrieve the token for you, validate it, etc..

mg-arrow commented 2 years ago

Chris,

I've tried doing that but OnSecurityTokenValidated is not firing. I have breakpoint there but it does not hit it at all.

Your wrote:

put your code later in the flow --> where do you think this code suppose to be in the flow ?

I had the following in OnSecurityTokenValidated:

        var signedInUserID = notification.AuthenticationTicket.Identity.GetUserId();

which fails as well.

Thanks

mg-arrow commented 2 years ago

I think after reading again I hope I understand your recommendations:

You are saying I need to add code_verifier as parameter to TokenEndpointRequest in OnAuthorizationCodeReceived and get access token and rest of the flow in OnSecurityTokenValidated.

But it has another challenge:

getAcessToken method is using AuthorizationCodeReceivedNotification notification, which would not work with SecurityTokenValidatedNotification<OpenIdConnectMessage, OpenIdConnectAuthenticationOptions> notification.

This is how I am retrieving token now: AuthenticationResult result = cca.AcquireTokenByAuthorizationCode(accessScopes, notification.Code).WithExtraQueryParameters("code_verifier=" + codeVerifier).ExecuteAsync().Result;

And it works.

Thanks

mg-arrow commented 2 years ago

Chris,

I've just tried the following in OnAuthorizationCodeReceived method:

notification.TokenEndpointRequest.SetParameter("code_verifier", codeVerifier);

then calling:

AuthenticationResult result = cca.AcquireTokenByAuthorizationCode(accessScopes, notification.Code).ExecuteAsync().Result;

fails with invalid code verifier error.

AADB2C90183: The supplied code_verifier is invalid

Any advice ?

Tratcher commented 2 years ago

TokenEndpointRequest already has the code_verifier. You said you needed to add additional paramters, it looks like those are scopes? You might be able to add those to TokenEndpointRequest as well. Not sure what else AcquireTokenByAuthorizationCode is doing for you.

If AcquireTokenByAuthorizationCode isn't something you can get away from then you should at least set notification.TokenEndpointResponse with the retrieved token. You'd also disable RedeemCode since you've already handled that. The auth handler should be able to take the TokenEndpointResponse and then take care of the validation.

mg-arrow commented 2 years ago

Ok, here's the current status, removed RedeemCode line and removed the line to add code_verifier to TokenEndpointRequest .

This line fails:

AuthenticationResult result = cca.AcquireTokenByAuthorizationCode(accessScopes, notification.Code).ExecuteAsync().Result;

with AADB2C90183: The supplied code_verifier is invalid

But if I do

AuthenticationResult result = cca.AcquireTokenByAuthorizationCode(accessScopes, notification.Code).WithExtraQueryParameters("code_verifier=" + codeVerifier).ExecuteAsync().Result;

It works.

Scopes I am already passing, so looks like code verifier is the issue.

Please also check my questions above.

Is it easier to have quick meeting ?

Thanks

mg-arrow commented 2 years ago

Any advice ?

mg-arrow commented 2 years ago

Can you please respond to my previous message ?

Tratcher commented 2 years ago

AuthenticationResult result = cca.AcquireTokenByAuthorizationCode(accessScopes, notification.Code).WithExtraQueryParameters("code_verifier=" + codeVerifier).ExecuteAsync().Result;

That makes sense, you'd need the verifier to redeem the code. See also my prior comment:

If AcquireTokenByAuthorizationCode isn't something you can get away from then you should at least set notification.TokenEndpointResponse with the retrieved token. You'd also disable RedeemCode since you've already handled that. The auth handler should be able to take the TokenEndpointResponse and then take care of the validation.

mg-arrow commented 2 years ago

I have new update for you on the progress I was able to make.

  1. I am able to receive the acess token in both cases

Now I have new issue .

  1. I am now hitting the breakpoint in OnSecurityTokenValidated
  2. This code works properly as well

var signedInUserID = notification.AuthenticationTicket.Identity.GetUserId();

So, Authentication ticket is populated and I can get various claims from the token.

However if I check System.Web.HttpContext.Current.User.Identity.IsAuthenticated returns false.

I also tried manually populating it using this statement:

var claimsPrincipal = new ClaimsPrincipal(claimsIdentity);

It gets populated and now System.Web.HttpContext.Current.User.Identity.IsAuthenticated returns true inside the cs code.

But if I try to check the System.Web.HttpContext.Current.User.Identity.IsAuthenticated inside the cshtml template it still returns false.

What could be the reason for such behavior ?

Thanks

mg-arrow commented 2 years ago

Looks like we are pretty close, thanks for your help

Tratcher commented 2 years ago

It gets populated and now System.Web.HttpContext.Current.User.Identity.IsAuthenticated returns true inside the cs code.

But if I try to check the System.Web.HttpContext.Current.User.Identity.IsAuthenticated inside the cshtml template it still returns false.

cshtml's would be happening on a separate request. You need to make sure cookie auth middleware runs for that request so it can read the cookie and set the current user. Make sure CookieAuthenticationOptions.AuthenticationMode is set to Active, which is the default. You could also put a break point in the Cookie Auth Provider events to make sure it fires for that request.

theepansubramani commented 2 years ago

@Tratcher I pretty much stumbled across the same problem and only this github issue helped progress one level to get the access & id token.

But then I could see the notification.AuthenticationTicket is not getting set and has null.

Should I manually decode the id token and set the AuthenticationTicket with its claims?

theepansubramani commented 2 years ago

@mg-arrow Can you explain how you were able to get the access/id tokens using using Redeem option as you suggested without any custom code?

That might be helpful for me.

theepansubramani commented 2 years ago

I tried manually decoding and setting the AuthenticationTicket but then it ends up with the following error:

"OpenIdConnectMessage.Error was not null, indicating an error. Error: 'invalid_grant'. Error_Description (may be empty): 'error_description is null'. Error_Uri (may be empty): 'error_uri is null'."

AuthenticationFailed event is called straight after the AuthorizationCodeReceived where I have done the manual mapping

mg-arrow commented 2 years ago

Sorry, I did not have a chance to work on this issue before, was planning to go back to it next week.

I have different idea now how to implement it. Will Share if it works.

BTW PKCE is not that important for me because this is not SPA application, so it is not required.

justinjxzhang commented 2 years ago

If you're manually retrieving the Access Token in AuthorizationCodeReceived, have you tried setting the TokenEndpointResponse to the result of the manual call?

I do this (using IdentityModel to issue the request to the IdP)

// ...
AuthorizationCodeReceived = async n => {
    // other preamble stuff
    var idpClient = HttpClientFactory.Create(); // using IdentityModel and the token helpers
    var tokenResponse = await idpClient.RequestAuthorizationCodeTokenAsync(new AuthorizationCodeTokenRequest { ... });
    n.TokenEndpointResponse = new OpenIdConnectMessage(tokenResponse.Raw); // tokenResponse.Raw is a JSON string, not the encoded JWT
}
// ..

Then the middleware uses the provided token to line to populate the AuthenticationTicket: https://github.com/aspnet/AspNetKatana/blob/b32c437b05217f367c201e420f147acf115e0712/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L476

https://github.com/aspnet/AspNetKatana/blob/b32c437b05217f367c201e420f147acf115e0712/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L495

And then in SecurityTokenValidated you should have the AuthenticationTicket and the TokenEndpointResponse you set earlier (as ProtocolMessage) https://github.com/aspnet/AspNetKatana/blob/b32c437b05217f367c201e420f147acf115e0712/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L498-L501

Inside SecurityTokenValidated I hit the UserInfo endpoint and update the SecurityTokenValidatedNotification.AuthenticationTicket with extra claims as and where needed.

@theepansubramani That looks more like an error with what you're sending to the IdP and being rejected there; have you tried using something like Postman to retrieve a code with response_type=code to the authorization_endpoint ?

sivakumar715 commented 2 years ago

thanks @justinjxzhang for the update.

i have tried as mentioned above steps and able to see AuthenticationTicket and ProtocolMessage under SecurityTokenValidated . But, IDP authentication is not successful and repeatedly AuthorizationCodeReceived , SecurityTokenReceived, SecurityTokenValidated etc., events fired.

On further checking, IsPersistent property is false in the AuthenticationTicket object. please let us know if we miss anything.

image

fyi @theepansubramani.

justinjxzhang commented 2 years ago

Are you using something like Cookie Authentication? From what I understand the IsPersistent (in the context of Cookie Authentication) indicates that the cookie is persisted in the browser across browser opening/closings and not between independent requests.

Do you have a minimal repro of this, or at least your OpenIdConnectAuthenticationOptions? Also might pay to raise a new issue as we don't want to be hijacking @mg-arrow's issue as they don't seem related.