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

Overhaul client to better conform to OAuth2 spec #6

Merged
merged 12 commits into from Dec 28, 2012

Conversation

Projects
None yet
5 participants
@conradev
Member

conradev commented Nov 27, 2012

I made a slew of changes to the client, ones that I hope push it in the right direction.

Changes Made

  • I updated the library to use ARC.
  • I removed AFOAuthAccount. I made this decision because an account abstraction only makes sense for the password grant type. The password grant type is the only grant type with any sort of account identifier (i.e. username) in the request or response. The response from the token endpoint only returns information about the token, and the best representation for this is a 'credential' object. Thus, all authentication success handlers are now given a AFOAuthCredential object. The removal of the account class also prompted me to rename AFOauthAccountCredential to AFOAuthCredential.
  • I transformed the client_id and client_secret values into readonly properties on the client object. This removes the need to pass these values as arguments repeatedly, when there is no need to. They are fixed and unchanging values. This mimics the behavior in AFOAuth1Client, where the key and secret properties are treated in much the same manner.
  • I removed the check for token expiration in the success callback of the OAuth token requests. An expired credential would never be returned from a request to the token endpoint. In addition to this, the success callback also now checks for the error field and calls the failure callback if appropriate.
  • I removed the secret property from the credential object, because it is intrinsic to the client, and not the credential. I added the tokenType property because it provides critical information on how the access token should be used.
  • I added the header Accept: application/json to each OAuth request, because all responses should be formatted in JSON, according to the spec. For this reason, AFJSONRequestOperation is also registered as an HTTP operation class by default.
  • Support for the authorization_code and client_credentials grant types was added.
  • Simple keychain persistence was added to AFOAuthCredential.
  • This encompasses the features in #2, #3 (?), #4, #5 and #7

conradev added some commits Nov 27, 2012

Update client to conform to OAuth2 spec
This commit also includes a bunch of other changes.

The only non-functional change made to the library is that the code has
been updated to support ARC.

The first change made to the library was to remove AFOAuthAccount. This
decision was made because an account abstraction only makes sense for
the `password` grant type. The `password` grant type is the only grant
type with any sort of account identifier in the request or response. The
response from the token endpoint only returns information about the
token, and the best representation for this is a 'credential' object.
Thus, all authentication success handlers are now given a
`AFOAuthCredential` object. The removal of the account class also
prompted `AFOauthAccountCredential` to be renamed to `AFOAuthCredential`

The second change made to the library was to make the `client_id` and
`client_secret` values into readonly properties on the client object.
This removes the need to pass these values as parameters repeatedly,
when there is no need to. They are fixed and unchanging values. This
mimics the behavior in `AFOAuth1Client`, where the `key` and `secret`
properties were treated in much the same manner.

The third change to the library was to remove the check for token
expiration in the success callback for sending an OAuth token request.
While checking for an expired credential and renewing it is a good idea,
an expired credential would never be returned from a request to the
token endpoint. In addition to this, the success callback also now
checks for an `error` field and calls the `failure` callback if it
exists.

The fourth change made to the library was to the credential object. The
`secret` property was removed from the object, because it is intrinsic
to the client, not the credential. The `tokenType` property was added
because it provides critical information on how the access token should
be used.

The fifth change was to add the header `Accept: application/json` to
each OAuth request, because all responses should be formatted in JSON,
according to the spec. For this reason, the `AFJSONRequestOperation` is
also registered as an HTTP operation class. The `parameterEncoding`
property is left untouched, at the default value of
`AFFormURLParameterEncoding`.

Lastly, support for the `authorization_code` grant type was added, as
per the OAuth spec.
@kylef

This comment has been minimized.

Show comment
Hide comment
@kylef

kylef Nov 28, 2012

Member

Nice work, good timing too. Switching to this branch because I need scopes.

One issue: The operation variable on line 152 (and again on 171) shadows its the other operation.

AFHTTPRequestOperation *operation = [self HTTPRequestOperationWithRequest:mutableRequest success:^(AFHTTPRequestOperation *operation, id responseObject) {

This prevents compilation with warnings are errors settings.

Member

kylef commented Nov 28, 2012

Nice work, good timing too. Switching to this branch because I need scopes.

One issue: The operation variable on line 152 (and again on 171) shadows its the other operation.

AFHTTPRequestOperation *operation = [self HTTPRequestOperationWithRequest:mutableRequest success:^(AFHTTPRequestOperation *operation, id responseObject) {

This prevents compilation with warnings are errors settings.

@conradev

This comment has been minimized.

Show comment
Hide comment
@conradev

conradev Nov 28, 2012

Member

Thank you @kylef for pointing that out! I don't know why I didn't see it, even though I have -Wall and -Wextra set. Fixed.

Member

conradev commented Nov 28, 2012

Thank you @kylef for pointing that out! I don't know why I didn't see it, even though I have -Wall and -Wextra set. Fixed.

Add basic keychain persistence
Uses NSCoding to encode the credentials
@conradev

This comment has been minimized.

Show comment
Hide comment
@conradev

conradev Dec 3, 2012

Member

I added basic keychain persistence, based on NSCoding, to store AFOAuthCredential objects in the keychain. The feature is disabled unless _SECURITY_SECITEM_H_ is defined, much like reachability and MIME type detection in AFHTTPClient.

This fixes #7

Member

conradev commented Dec 3, 2012

I added basic keychain persistence, based on NSCoding, to store AFOAuthCredential objects in the keychain. The feature is disabled unless _SECURITY_SECITEM_H_ is defined, much like reachability and MIME type detection in AFHTTPClient.

This fixes #7

conradev added some commits Dec 3, 2012

Use old refresh token if necessary
From Section 6 of RFC 6749:

"The authorization server MAY issue a new refresh token, in which case
the client MUST discard the old refresh token and replace it with the
new refresh token."

The client was not falling back on the old refresh token in the new
credential object, if none was specified in the response.
@ay8s

This comment has been minimized.

Show comment
Hide comment
@ay8s

ay8s Dec 7, 2012

Great work @conradev. Definitely useful additions, making use of the authorization_code implementation which works perfectly. Would love to see this merged in.

ay8s commented Dec 7, 2012

Great work @conradev. Definitely useful additions, making use of the authorization_code implementation which works perfectly. Would love to see this merged in.

@dennisreimann

This comment has been minimized.

Show comment
Hide comment
@dennisreimann

dennisreimann Dec 10, 2012

Is there an official statement about the changes? I'd also like to see them pulled in, but this repo hasn't been updated for months now...

dennisreimann commented Dec 10, 2012

Is there an official statement about the changes? I'd also like to see them pulled in, but this repo hasn't been updated for months now...

@conradev

This comment has been minimized.

Show comment
Hide comment
@conradev

conradev Dec 10, 2012

Member

Hopefully @mattt will get around to looking at this.

For now, I will maintain my fork.

Member

conradev commented Dec 10, 2012

Hopefully @mattt will get around to looking at this.

For now, I will maintain my fork.

@kylef

This comment has been minimized.

Show comment
Hide comment
Member

kylef commented Dec 15, 2012

@conradev

This comment has been minimized.

Show comment
Hide comment
@conradev

conradev Dec 28, 2012

Member

As per AFNetworking 1.1, the class constructor now returns instancetype (09f2f5c)

Funny observation: this repository has the third most number of stars, and is the second most out of date of the AFNetworking repositories

Member

conradev commented Dec 28, 2012

As per AFNetworking 1.1, the class constructor now returns instancetype (09f2f5c)

Funny observation: this repository has the third most number of stars, and is the second most out of date of the AFNetworking repositories

@mattt mattt merged commit 09f2f5c into AFNetworking:master Dec 28, 2012

@mattt

This comment has been minimized.

Show comment
Hide comment
@mattt

mattt Dec 28, 2012

Contributor

An apology is in order to everyone who was inconvenienced by my mismanagement of this repository. It was irresponsible of me to let this languish for so long, and I humbly apologize for all of the hassle anyone faced in trying to use this in the meantime.

@conradev, your work on this pull request is superb. Thank you for your hard work on this. I've merged this in and tagged it as the 0.1.0 release.

Contributor

mattt commented Dec 28, 2012

An apology is in order to everyone who was inconvenienced by my mismanagement of this repository. It was irresponsible of me to let this languish for so long, and I humbly apologize for all of the hassle anyone faced in trying to use this in the meantime.

@conradev, your work on this pull request is superb. Thank you for your hard work on this. I've merged this in and tagged it as the 0.1.0 release.

@ay8s

This comment has been minimized.

Show comment
Hide comment
@ay8s

ay8s Dec 28, 2012

No apology needed mattt. Great work as ever 👍

Also, thanks @conradev!

ay8s commented Dec 28, 2012

No apology needed mattt. Great work as ever 👍

Also, thanks @conradev!

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