nhosoya / omniauth-apple

OmniAuth strategy for Sign In with Apple
MIT License
264 stars 101 forks source link

Encode spaces in “scope” as '%20' Instead of '+' #16

Closed karlentwistle closed 3 years ago

karlentwistle commented 4 years ago

When providing both email and name as scopes. For example "email name" the request URL is being incorrectly transformed with a + between the scope names rather than correctly using a %20.

Incorrect: name+email Correct: name%20email

This is the part of Faraday that performs the transform. I cannot see an obvious way to override or customize the behaviour. @sebfie helpfully diagnosed the issue and figured out the fix here https://github.com/nhosoya/omniauth-apple/issues/12#issuecomment-571690494.

I wondered if we could get this merged and released in the gem.

Closes #12

sebfie commented 4 years ago

\o/

jeffsawatzky commented 4 years ago

Not sure this fixes #13, as I've implemented that initializer fix in #12 (and now get an email) but first and last names are still nil.

karlentwistle commented 4 years ago

Ah sorry about that @jeffsawatzky i have updated the description.

jeffsawatzky commented 4 years ago

In fact I just tried with the + in the url instead of `%20% and apple's servers handle it fine and I get back email and name data just fine.

Seems like this issue has to do with something else.

jeffsawatzky commented 4 years ago

I don't think this is actually needed. I have successfully been able to get the email and name information together even with the +

The only issue I have now is that apple only returns name information on the first authorization: https://developer.apple.com/documentation/signinwithapplejs/configuring_your_webpage_for_sign_in_with_apple#3331292

and their token never seems to contain the name info.

karlentwistle commented 4 years ago

@jeffsawatzky I have just tested this again and I think I have discovered where the bug occurs however it's a bit strange.

Just to share my testing process.

What I've found is if I use a brand new account that has never authenticated against my app it doesn't matter if I use scope encoded with name+email or name%20email I get the user_info object with name and email returned after the first request.

However, the discrepancy occurs when I run through my testing process again by removing the app from "Apps & websites using Apple ID". At this point, if I use scope name+email then Apple stops returning the full user_info object even for the first request. However, if I use name%20email I always get back { user_info: {"email":"foo@bar.com","name":{"firstName":"Karl","lastName":"Entwistle"}} } for the first request.

Just to be clear on terminology when I say "first request" I mean the actual first request or the first request since the app was last removed in Apple ID.

nov commented 4 years ago

In my case, Apple always return name after revoking with name+email scope. Probably you should ask Apple about the behavior not here.

maxencehenneron commented 4 years ago

Apple only sends the information the first time a user authenticates with your app. If you need it later, you will have to save the data on your backend server. As far as I know, there aren't any endpoints available to retrieve this information after the initial login. To test, I usually remove my app from the list of authorized app on my apple account settings, as described earlier.

adamirowe commented 4 years ago

This is the only thing that got us working for Touch ID authentication on Safari. Would be great to see this get merged in to the official gem as we're having to fork it as of now @nhosoya.

karlentwistle commented 4 years ago

@adamirowe thanks for letting us know you're also experiencing the issue and its not just me 😅

In the meantime I opened a PR upstream in Faraday https://github.com/lostisland/faraday/pull/1125 so if you'd like to use the official "omniauth-apple" gem and you're able to use Faraday version 1.0.1 you can now configure Faraday to use the encoding Apple seems to require with the following config:

Faraday::Utils.default_space_encoding = '%20'
adamirowe commented 4 years ago

@adamirowe thanks for letting us know you're also experiencing the issue and its not just me 😅

In the meantime I opened a PR upstream in Faraday lostisland/faraday#1125 so if you'd like to use the official "omniauth-apple" gem and you're able to use Faraday version 1.0.1 you can now configure Faraday to use the encoding Apple requires with the following config:

Faraday::Utils.default_space_encoding = '%20'

Oh nice. Yeah, we ended up just forking this gem and cherry picking your commit over to it. Might look into upgrading faraday though after we get this out to prod.

Lockyy commented 4 years ago

@nhosoya Is there any chance of this being merged in? When the space is encoded as + instead of %20 I do get sent the user's details however in Safari they are unable to customize their name or select the private relay service in the Apple ID sheet.

Currently I'm monkey-patching this PR in as I am unable to update Faraday. For anyone who wants the monkey-patch:

module OmniAuth
  module Strategies
    module ScopeEncodingOverride
      def request_phase
        redirect client.auth_code.authorize_url({:redirect_uri => callback_url}.merge(authorize_params)).gsub(/\+/, '%20')
      end
    end
  end
end

OmniAuth::Strategies::Apple.include OmniAuth::Strategies::ScopeEncodingOverride
btalbot commented 4 years ago

I've been trying to reproduce this today and haven't been able to. Is this still an issue for anyone? Maybe apple updated their code to accept the + encoding for spaces or something. BTW, the + for spaces is a very old encoding and in no way invalid ... just pre-dates the %-url escaping.

matsubo commented 4 years ago

@btalbot I've encountered related to this issue. I hope this PR or https://github.com/looker/looker-sdk-ruby/pull/96 will be merged or I need to change Faraday's global behavior mentioned https://github.com/nhosoya/omniauth-apple/pull/16#issuecomment-606570337.

waissbluth commented 4 years ago

I've been trying to reproduce this today and haven't been able to. Is this still an issue for anyone?.

~@btalbot I am not being able to reproduce it either, but I am a facing a slightly similar issue whereby the email is empty when a user uses touch or face id. Everything works well when using a password, which involves a longer flow (for example, asking the user whether they would like to keep their email private).~

~I thought this would solve the issue I am having but it does not make a difference. Changing how the space is encoded does not affect this case (asking for email only does not make a difference either).~

EDIT: this is still a thing, but for me it only matters when using Touch or Face ID, and not when using email+password to sign in to Apple.

karlentwistle commented 4 years ago

I was checking the Apple documentation again today and realised it does specify the scope should be using percent encoding.

scope The amount of user information requested from Apple. Valid values are name and email. You can request one, both, or > > none. Use space separation and percent-encoding for multiple scopes; for example, "scope=name%20email".

https://developer.apple.com/documentation/sign_in_with_apple/sign_in_with_apple_js/incorporating_sign_in_with_apple_into_other_platforms#3332113

jeffsawatzky commented 3 years ago

Now that https://github.com/lostisland/faraday/pull/1125/files is out, perhaps this PR should be updated to use default_space_encoding in faraday?

karlentwistle commented 3 years ago

Since it's now possible to circumvent this issue by using Faraday::Utils.default_space_encoding = '%20' and this hasn't been merged for almost a year I will go ahead and close it.