Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract user object from form body #767

Closed
wants to merge 1 commit into from

Conversation

pieperu
Copy link

@pieperu pieperu commented Mar 13, 2023

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
Copy link
Member

kevinchalet commented Mar 13, 2023

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
Copy link
Author

pieperu commented Mar 13, 2023

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
Copy link
Author

pieperu commented Mar 13, 2023

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
Copy link
Author

pieperu commented Mar 13, 2023

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants