uber / rides-ios-sdk

Uber Rides iOS SDK (beta)
https://developer.uber.com/docs
MIT License
374 stars 124 forks source link

Feature/code authenticator #232

Closed vriznychok closed 3 years ago

vriznychok commented 6 years ago

I've discovered that in rides-ios-sdk loginManager.login() with LoginType.authorizationCode is not implemented properly, because even your example returns error if you try to use it. (error code 14, I will describe reasons below)

The problem is in class AccessTokenFactory. It is not setup to receive authorization code, it is setup to make AccessToken no matter what login type you use. Which is wrong, because .authorizationCode login flow doesn't need AccessToken, but it needs authorization code to proceed authorization.

I've changed AccessToken model so it contain authorizationCode variable to store authorizationCode for this type of login.

Also i've added method: public static func createAuthorizationCode(fromRedirectURL redirectURL: URL) throws -> AccessToken

to create empty instance of accessToken with filled authorizationCode variable.

Then AuthorizationCodeGrantAuthenticator returns AuthenticationCompletionHandler with AccessToken that contains only authorizationCode variable so developer could finish his authentication process via backend service.

My changes doesn't impact on all other login types and functionality of SDK, they just fix .authorizationCode login flow.

I've tested it in my app and it works fine, authentication is successful. Also I've tested it on rides-ios-sdk examples and it works fine too.

CLAassistant commented 6 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

vriznychok commented 6 years ago

Oh, i guess I've accidentally included 2 commits from master in this pull request. Could you please review only my commit and I will remake pull request if it fits, or with changes?

edjiang commented 6 years ago

Hey @vriznychok, thanks for the pull request :) I'm out of country right now, but have a few comments about your PR. Will respond in a bit.

vriznychok commented 6 years ago

Hey @edjiang are there any comments already?

edjiang commented 6 years ago

Yep! So here's the problem right now. Authorization code flow, as you mentioned, doesn't work out of the box and is poorly documented.

Why doesn't auth code flow work out of the box? The reason is that we require a server to handle part of the flow. The auth code exchange, as currently designed, requires a client secret that can't live on the iOS app. Thus, requiring a server.

There's an OAuth RFC detailing a solution - OAuth with PKCE, which I've pushed the identity team to implement. They're in the process of doing so, and I think once that's out we should be able to do the authorization code flow without a server.

In the meantime, if you want to implement Auth Code flow with this SDK, I would actually recommend that you do it this way:

  1. Set the callback URL of the authorization code flow to your server in your Info.plist:
    <dict>
        <key>UberCallbackURIType</key>
        <string>AuthorizationCode</string>
        <key>URIString</key>
        <string>https://example.com/oauth/consumer</string>
    </dict>
  2. In the server, do the token exchange and then redirect to [Your Bundle ID Here]://oauth/consumer with the access token as the query parameters access_token=[Access Token], token_type=bearer, and expires_in=[Expiration time in seconds as detailed in the OAuth RFC
  3. Use the token as necessary :)

This should make the SDK a bit more seamless and allow you to migrate to PKCE easily when that's released :)