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

Extract user object from form body #767

Closed pieperu closed 1 year ago

pieperu commented 1 year ago

Related to #766

This change pulls the User object from the Form on the Request sent from the apple Authorize endpoint.

This is then provided to the OAuthCreatingTicketContext so that the user properties can be retrieved using the ticket created event.

kevinchalet commented 1 year ago

Thanks for your PR 😃

Unfortunately, as @martincostello said, we deliberately stopped using this unsafe property for security reasons and it's unlikely we'll accept a PR that will expose it (not to mention that's changing the object passed to the context constructor is a behavior breaking change).

Closing.

pieperu commented 1 year ago

Hi @kevinchalet

Thanks for checking this over so quickly. I didn't get to poke anyone and explain :)

My intention wasn't for this to be merged but to demonstrate some odd behaviour.

Martin did mention that the raw payload would be available on the ticket created event, but upon checking the context, the user json object holds this format:

{
"access_token": string,
"expires_in”: string,
"id_token”: string,
“refresh_token”: string,
“token_type”: string
}

Instead of the User payload with the user's names which comes in the form.

Is this not misleading, whether you intend to surface it or not? Should it just be an empty JsonElement if you're not intending on exposing the expected user payload on this event/context?

pieperu commented 1 year ago

In terms of breaking behaviour @kevinchalet, yes absolutely, although the exact same properties are available on the Tokens property so you are just passing the same information in 2 different constructor params currently...

pieperu commented 1 year ago

So just to clarify @kevinchalet - If i wanted to access a user's GivenName and Surname, given that I only have 1 opportunity to capture it - I could not do so using this library, because you will never pass it on, even in the CreateTicket event?

If this is your stance on it, that's fair enough, but I need access to these values, so I just need to know if i need to fork or not :D

fipster-dev commented 1 year ago

Martin cleared everything up for me. I totally missed that the HTTP context was available in the event handler so i can pluck it straight from there.