aspnet-contrib / AspNet.Security.OAuth.Providers

OAuth 2.0 social authentication providers for ASP.NET Core
Apache License 2.0
2.35k stars 533 forks source link

User object not returned after Apple Authorize #765

Closed pieperu closed 1 year ago

pieperu commented 1 year ago

Describe the bug

Hi. I’m trying to retrieve the givenname and surname claims from the apple signin flow. My understanding is that this is only returned the very first time a user authenticates, and is returned in the same payload as the id_token, and would like something like this:

state=xxxstatexxx
&code=xxxxx.x.xx.xxxxx
&id_token=xxxxsomeidtokenxxxx
&user={"name":{"firstName":"Some","lastName":"User"},"email":"testemail@outlook.com"}

I’ve been diving around trying to work out why I can’t surface them in my Identity Server External Identity /callback endpoint and I wanted to check that the issue wasn’t in this Apple OAuth library.

I can see that as of 6.10.0 that the email is no longer taken from the User payload coming back from the apple authorise response, but also it seems that the user payload is somewhat discarded. I can see here:

https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/875fb025081e45d345f0acf1f215586c1f797dce/src/AspNet.Security.OAuth.Apple/AppleAuthenticationHandler.cs#L104

That the User object is not included in the created ticket at all and doesn't appear to be available anywhere else. Could this be an oversight?

When creating the AuthenticationTicket, this User isn’t provided at all:

https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/875fb025081e45d345f0acf1f215586c1f797dce/src/AspNet.Security.OAuth.Apple/AppleAuthenticationHandler.cs#L108

Could it be that these could be included in the context.Properties to be included in the AuthenticationTicket?

Side note, I notice that the entire root payload is passed into the OAuthCreatingTIcketContext, should it be this instead:

tokens.Response.RootElement.GetString("user")

var context = new OAuthCreatingTicketContext(principal, properties, Context, Scheme, Options, Backchannel, tokens, tokens.Response.RootElement.GetString("user"));

To return the user json object? As is expected by the OAuthCreaingTicketContext from the Owin repo:

https://github.com/pruiz/Owin.OAuthGeneric/blob/d1acc2983fd9624f3da576aaaf759ca615b1405d/Owin.OAuthGeneric/OAuthCreatingTicketContext.cs#L47

Steps To reproduce

Using Identity Server (Duende) logging in with Apple, the payload returned to the callback endpoint does not include any names (givenname or surname). It does not appear to be surfaced anywhere.

Expected behaviour

One first login, that in the callback endpoint we will have access to the User payload returned by apple's Authorize endpoint on first login.

Actual behaviour

None of this comes back to the Identity Server endpoint/

System information

dotnet 7 Duende Identity Server Latest version of the apple oauth package

Additional context

For privacy reasons Apple does not provide an endpoint to retrieve users names after the initial login. The only opportunity to capture this information is on first sign in, so we need to ensure this is returned to the callback endpoint.

martincostello commented 1 year ago

That’s correct in that the library does not use this information since 6.0.10. It’s also correct that there is exactly one opportunity to get these values from Apple.

The use of the user object was removed in response to https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/security/advisories/GHSA-3893-h8qg-6h5f.

If you still want to access the raw payload, you can use the ticket creation event to do so. As the values can be spoofed as noted in the advisory, you should not use the data to make security-related decisions (e.g. authorisation checks).

fipster-dev commented 1 year ago

Thanks for the super quick response Martin, and the info. Much appreciated.

I'll look into using the ticket creation event as you've suggested 🙇‍♂️

pieperu commented 1 year ago

Hey Martin, a followup when you have a mo.

I may have misunderstood when you said

If you still want to access the raw payload, you can use the ticket creation event to do so.

By raw payload did you mean the payload that was passed from apple in the request form?

Looking into using that ticket creation event I noticed that the value coming through for the User object on the context was pretty much the same as the Token param that gets passed into that same context.

I had created a draft PR

767

Just to show that a change would be needed to surface that user object on the context in the event.

But just for clarity - Is surfacing the User payload that apples POSTs totally off the cards, even if it's on this context? (Therefore making GivenName and Surname totally inaccessible)

martincostello commented 1 year ago

Sorry - I confused the properties variable, which is passed to you via the event's context, and the parameters variable (containing the user), which is not.

However, the user is still ultimately still available to you if you really really want to access it via the HttpContext. You can see where we get it from the put into parameters below:

https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/f8e13868c8d9c3428b738206acbdcbe196ca7564/src/AspNet.Security.OAuth.Apple/AppleAuthenticationHandler.cs#L159-L173

You could use similar logic in your event handler to grab the user parameter of the current HTTP request to do what you would like to do with the values available.

pieperu commented 1 year ago

@martincostello thank you so much for your help on this! You are a god send :)

I'd been looking at this so long I totally missed the HTTP Context was available in the event handler 🤦

Problem solved!