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

Creating AccessToken from string does not populate relevant properties #187

Closed
aspyre opened this issue Sep 25, 2017 · 8 comments
Closed
Labels

Comments

@aspyre
Copy link

aspyre commented Sep 25, 2017

I'm using custom authorisation with the Uber iOS SDK, and having trouble creating the AccessToken in my iOS code. This is the response I get from my server with what appears to be a valid access token:

{
"access_token":"token here",
"expires_in":2592000,
"token_type":"Bearer",
"scope":"all_trips request",
"refresh_token":"refresh token here",
"last_authenticated":0
}

I then pass this to the AccessToken initialiser, like so:

let jsonString = //response from server as above
let accessToken = AccessToken(tokenString: jsonString)

My access token is created (ie. non-nil), but none of the relevant property are populated.

accessToken //non-nil
accessToken.expirationDate //nil
accessToken.refreshToken //nil
accessToken.tokenString //populated with the full text of jsonString

Digging through the AccessToken.swift source file of the Uber project, I find this:

@objc public init(tokenString: String) {
        super.init()
        self.tokenString = tokenString
}

This means refreshToken and expiration date will never be populated.

It also means that API attempts fail, as the API is expecting the tokenString field to contain the actual access_token text, not the full text.

I have verified that if I initialise the AccessToken like so, then API requests succeed (at least until the access token expires).

let accessTokenText = //access_token text only
let accessToken = AccessToken(tokenString: accessTokenText)
@aspyre
Copy link
Author

aspyre commented Sep 25, 2017

@edjiang
Copy link
Contributor

edjiang commented Sep 25, 2017

Hey @aspyre,

The tokenString is meant to be just the access token itself, as you observed. If you want to parse the JSON itself, I would suggest using the fact that the model conforms to the Decodable protocol, and pass in your JSON through that method.

let decoder = JSONDecoder()
decoder.dateDecodingStrategy = .secondsSince1970
let accessToken = try? decoder.decode(AccessToken.self, from: jsonData)
// If you need to convert a string to data, use String.data(using: .utf8)!

Looking at this question, I think it'll be a good idea to make it easier to initialize this object from JSON. I'll leave this issue open to address this.

@aspyre
Copy link
Author

aspyre commented Sep 27, 2017

Thanks @edjiang for the reply, makes sense and not sure why I didn't connect that I can just use the JSONDecoder. In previous versions of the UberRides SDK the token was created like so, which did in fact populate all the properties as expected:

AccessTokenFactory.createAccessTokenFromJSONString(jsonString)

So, I think I was expecting something similar here. I agree it would be a good idea to make it easier when initializing from JSON, as I expect this would be the primary use case.

@aspyre
Copy link
Author

aspyre commented Oct 3, 2017

To follow up on this, I believe there is still some work to be done. Two issues:

  1. The response from https://login.uber.com/oauth/v2/token contains the key "expires_in", but the CodingKey in AccessToken.swift is "expiration_date". So at the moment I have to manually update the response data to use this key instead.
  2. "expires_in" is not a date at all, but number of seconds until the access token expires. So again, this requires manually updating the response from https://login.uber.com/oauth/v2/token

Of course, these are not particularly complicated, but it does creating an AccessToken object from https://login.uber.com/oauth/v2/token not the trivial task that it used to be.

@edjiang
Copy link
Contributor

edjiang commented Oct 11, 2017

Thanks for reporting this. Definitely looks like some warts in here. I didn't realize that the implementation did #1, so I recommended that strategy in error. I'll get these cleaned up for you and fixed. :)

@edjiang edjiang added the bug label Oct 11, 2017
@aspyre
Copy link
Author

aspyre commented Oct 11, 2017

Thanks for the follow up!

edjiang added a commit that referenced this issue Nov 27, 2017
* Adds two new initializers for AccessToken: `init(tokenString:refreshToken:expirationDate:grantedScopes:)` and `init?(oauthDictionary:)`
* Makes `AccessTokenFactory` public
* Adds additional test coverage to assert more parameters from JSON deserialization, and test new getters.
* Adds comments to document `AccessToken`'s `NSCoding` behavior
* Removes the `Codable` protocol from `AccessToken` since it won't be used for JSON serialization / deserialization
edjiang added a commit that referenced this issue Nov 28, 2017
* Adds two new initializers for AccessToken: `init(tokenString:refreshToken:expirationDate:grantedScopes:)` and `init?(oauthDictionary:)`
* Makes `AccessTokenFactory` public
* Adds additional test coverage to assert more parameters from JSON deserialization, and test new getters.
* Adds comments to document `AccessToken`'s `NSCoding` behavior
* Removes the `Codable` protocol from `AccessToken` since it won't be used for JSON serialization / deserialization
@edjiang
Copy link
Contributor

edjiang commented Nov 28, 2017

@aspyre should be fixed in 0.8; let me know how it goes! https://github.com/uber/rides-ios-sdk/releases/tag/v0.8.0

@aspyre
Copy link
Author

aspyre commented Nov 28, 2017

@edjiang thanks for the note, and the fix! Looking good so far.

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

No branches or pull requests

2 participants