Add support for dynamic client registration. #22

zamzterz · 2016-05-20T07:37:30Z

(Sorry for the large changeset.)

Similar implementation to the one for Android:

stores the registration response in the AuthState
constructs the registration request body as JSON
checks conditions on the registration response
adds implementation to the example app

This change is

iainmcgin · 2016-05-21T00:25:39Z

+@StevenEWright and +@WilliamDenniss for review

Previously, rebeckag (Rebecka Gulliksson) wrote…

Add support for dynamic client registration.

(Sorry for the large changeset.)

Similar implementation to the one for Android:

stores the registration response in the AuthState

constructs the registration request body as JSON

checks conditions on the registration response

adds implementation to the example app

Review status: 0 of 28 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

zamzterz · 2016-06-23T12:11:33Z

Any update on this?

StevenEWright · 2016-07-07T20:51:01Z

Reviewed 3 of 28 files at r1.
Review status: 3 of 28 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

Example/Source/AppAuthExampleViewController.m, line 182 [r2] (raw file):

    OIDRegistrationRequest *request =
            [[OIDRegistrationRequest alloc] initWithConfiguration:configuration
                                                     redirectURIs:@[redirectURI]

Nit... (sorry, my job! LOL)

https://google.github.io/styleguide/objcguide.xml?showone=Container_Literals#Container_Literals

"If the collection fits on one line, put a single space after the opening and before the closing brackets."

@[ redirectURI ]

Example/Source/AppAuthExampleViewController.m, line 184 [r2] (raw file):

                                                     redirectURIs:@[redirectURI]
                                                    responseTypes:nil
                                                       grantTypes:nil subjectType:nil

Nit... please one parameter per line, or all on one line.

Example/Source/AppAuthExampleViewController.m, line 193 [r2] (raw file):

                                               callback:^(OIDRegistrationResponse *_Nullable regResp,
                                                       NSError *_Nullable error) {
                                                   if (regResp) {

Block contents need only 2 spaces indent from "callback"

Example/Source/AppAuthExampleViewController.m, line 194 [r2] (raw file):

                                                       NSError *_Nullable error) {
                                                   if (regResp) {
                                                       [self setAuthState:[[OIDAuthState alloc] initWithRegistrationResponse:regResp]];

Only 2 spaces indent from start of "if"

Example/Source/AppAuthExampleViewController.m, line 236 [r2] (raw file):

- (void)doAuthWithoutCodeExchange:(OIDServiceConfiguration *)configuration :(NSString *) clientID {
    NSURL *redirectURI = [NSURL URLWithString:kRedirectURI];

Only 2 spaces from left.

Example/Source/AppAuthExampleViewController.m, line 239 [r2] (raw file):

    // builds authentication request
    OIDAuthorizationRequest *request =

Only +2 spaces from left.

Example/Source/AppAuthExampleViewController.m, line 240 [r2] (raw file):

    // builds authentication request
    OIDAuthorizationRequest *request =
            [[OIDAuthorizationRequest alloc] initWithConfiguration:configuration

Line continuations only +4 spaces.

Example/Source/AppAuthExampleViewController.m, line 276 [r2] (raw file):

  [OIDAuthorizationService discoverServiceConfigurationForIssuer:issuer
      completion:^(OIDServiceConfiguration *_Nullable configuration, NSError *_Nullable error) {

Unnecessary blank line after start of block.

Example/Source/AppAuthExampleViewController.m, line 290 [r2] (raw file):

      }];
    } else {
        [self doAuthWithAutoCodeExchange:configuration :kClientID];

Only +2 spaces from "if" alignment.

Example/Source/AppAuthExampleViewController.m, line 313 [r2] (raw file):

    if (!kClientID) {
      [self doClientRegistration:configuration : ^(OIDServiceConfiguration * configuration, NSString * clientID){
          [self doAuthWithoutCodeExchange:configuration :clientID];

Indentation seems consistently mixed between using 4 and 2 spaces for indentation. I'm going to stop here for now and let you address.

Comments from Reviewable

zamzterz · 2016-07-11T06:51:18Z

Example/Source/AppAuthExampleViewController.m, line 313 [r2] (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Indentation seems consistently mixed between using 4 and 2 spaces for indentation. I'm going to stop here for now and let you address.

Sorry about that, I've reformatted the added sections. Should be consistent with 2 spaces now (I hope! :))

Comments from Reviewable

codecov-io · 2016-11-22T10:32:54Z

Current coverage is 76.22% (diff: 75.67%)

Merging #22 into master will increase coverage by 0.03%

@@             master        #22   diff @@
==========================================
  Files            33         39     +6   
  Lines          1957       2355   +398   
  Methods         386        448    +62   
  Messages          0          0          
  Branches        101        123    +22   
==========================================
+ Hits           1491       1795   +304   
- Misses          424        503    +79   
- Partials         42         57    +15

Powered by Codecov. Last update ce74a55...7aa895a

WilliamDenniss · 2016-11-23T03:31:04Z

Reviewed 1 of 28 files at r1, 25 of 27 files at r3.
Review status: all files reviewed at latest revision, 40 unresolved discussions, some commit checks failed.

Source/AppAuth.h, line 31 at r3 (raw file):

#import "OIDRegistrationRequest.h"
#import "OIDRegistrationResponse.h"
#import "OIDResponseTypes.h"

is OIDClientMetadataParameters.h intentionally not included? OK if it is, just checking.

Source/OIDAuthorizationService.h, line 69 at r3 (raw file):

N
Align with the start of the first param (i.e. indent 4 more spaces)

Source/OIDAuthorizationService.h, line 135 at r3 (raw file):

callback:

sorry to be a pain, but can we call this completion and OIDRegistrationCompletion? I've realized that 'callback' is very generic and doesn't really help understanding, I know that's what we used elsewhere.

Source/OIDAuthorizationService.m, line 366 at r3 (raw file):

    return;
  }

can you set Xcode to strip trailing spaces?

Source/OIDAuthorizationService.m, line 413 at r3 (raw file):

      // not an OAuth error, just a generic server error
      NSError *returnedError =
      [OIDErrorUtilities errorWithCode:OIDErrorCodeServerError

this looks like it could fit on the previous line, but if not should be indented +4. Here and elsewhere

Source/OIDAuthorizationService.m, line 424 at r3 (raw file):

    NSError *jsonDeserializationError;
    NSDictionary<NSString *, NSObject <NSCopying> *> *json =
    [NSJSONSerialization JSONObjectWithData:data options:0 error:&jsonDeserializationError];

indent +4

Source/OIDAuthorizationService.m, line 437 at r3 (raw file):

                
    OIDRegistrationResponse *registrationResponse =
    [[OIDRegistrationResponse alloc] initWithRequest:request

indent

Source/OIDAuthorizationService.m, line 442 at r3 (raw file):

      // A problem occurred constructing the registration response from the JSON.
      NSError *returnedError =
      [OIDErrorUtilities errorWithCode:OIDErrorCodeRegistrationResponseConstructionError

and here

Source/OIDAuthState.m, line 192 at r3 (raw file):

    (nullable OIDAuthorizationResponse *)authorizationResponse
           tokenResponse:(nullable OIDTokenResponse *)tokenResponse
    registrationResponse:(nullable OIDRegistrationResponse *)registrationResponse {

I don't see this in the header as the new designated initializer.

Source/OIDAuthState.m, line 206 at r3 (raw file):

      
    if (registrationResponse) {
      [self updateWithRegistrationResponse:registrationResponse];

since this clears the auth state, shouldn't it be first?

Source/OIDClientMetadataParameters.h, line 31 at r3 (raw file):

extern NSString *const OIDApplicationTypeParam;

/*! @brief Parameter name for the redirect URI values.

doxygen comments are out of sync throughout

Source/OIDClientMetadataParameters.m, line 1 at r3 (raw file):

/*! @file OIDClientMetadataParameters.h

What's the design behind this file? I see in the Response class the consts are defined there instead. Are these shared? For AuthorizationRequest we don't even expose the param consts are part of the API (they exist only in the .m)

Source/OIDFieldMapping.m, line 117 at r3 (raw file):

      return value;
    }
    NSNumber *valueAsNumber = (NSNumber *) value;

no space before 'value' (NSNumber *)value

Source/OIDRegistrationRequest.h, line 98 at r3 (raw file):

Source/OIDRegistrationRequest.m, line 31 at r3 (raw file):

OIDSubjectTypeParam

For AuthorizationRequest we re-used the OAuth param names for serialization and building the URL. If you did that here would it mean you wouldn't need the MetadataParameters file?

Source/OIDRegistrationRequest.m, line 81 at r3 (raw file):

    _tokenEndpointAuthenticationMethod = [tokenEndpointAuthenticationMethod copy];
    _additionalParameters =
    [[NSDictionary alloc] initWithDictionary:additionalParameters copyItems:YES];

indent

Source/OIDRegistrationRequest.m, line 117 at r3 (raw file):

                                                 forKey:kSubjectTypeKey];
  NSString *tokenEndpointAuthenticationMethod =
  [aDecoder decodeObjectOfClass:[NSString class]

indent if it can't fit on the previous line with colon alignment

Source/OIDRegistrationRequest.m, line 122 at r3 (raw file):

                                                                   [NSString class] ]];
  NSDictionary *additionalParameters =
  [aDecoder decodeObjectOfClasses:additionalParameterCodingClasses

same

Source/OIDRegistrationRequest.m, line 129 at r3 (raw file):

indent this line +4 from the "self", and then align the colons on that

Source/OIDRegistrationRequest.m, line 136 at r3 (raw file):

- (void)encodeWithCoder:(NSCoder *)aCoder {
  [aCoder encodeObject:_configuration
                forKey:kConfigurationKey];

I think this can fit on one line, here and throughout this method

Source/OIDRegistrationRequest.m, line 157 at r3 (raw file):

  NSString *requestBody =
  [[NSString alloc] initWithData:request.HTTPBody encoding:NSUTF8StringEncoding];
  return [NSString stringWithFormat:@"<%@: %p, request: <URL: %@, HTTPBody: %@>>",

indent

Source/OIDRegistrationRequest.m, line 176 at r3 (raw file):

  NSURL *registrationRequestURL = _configuration.registrationEndpoint;
  NSMutableURLRequest *URLRequest =
  [[NSURLRequest requestWithURL:registrationRequestURL] mutableCopy];

indent

Source/OIDRegistrationResponse.m, line 96 at r3 (raw file):

    
    if ((_clientSecret && !_clientSecretExpiresAt) ||
        (!!_registrationClientURI != !!_registrationAccessToken)) {

indentation is correct, but move the OR operator to the second line

Source/OIDRegistrationResponse.m, line 97 at r3 (raw file):

    if ((_clientSecret && !_clientSecretExpiresAt) ||
        (!!_registrationClientURI != !!_registrationAccessToken)) {
      /* If client_secret is issued, client_secret_expires_at is REQUIRED,

use // style comments in the method, even for a long comment

Source/OIDServiceConfiguration.h, line 65 at r3 (raw file):

- (instancetype)initWithAuthorizationEndpoint:(NSURL *)authorizationEndpoint
                                tokenEndpoint:(NSURL *)tokenEndpoint
                          registrationEndpoint:(nullable NSURL *)registrationEndpoint;

to avoid a breaking change, can this be an additional constructor?

Source/OIDServiceConfiguration.m, line 47 at r3 (raw file):

- (instancetype)initWithAuthorizationEndpoint:(NSURL *)authorizationEndpoint
        tokenEndpoint:(NSURL *)tokenEndpoint
 registrationEndpoint:(nullable NSURL *)registrationEndpoint

indent 4 spaces and align columns on this line

Source/OIDTokenResponse.m, line 76 at r3 (raw file):

        [[OIDFieldMapping alloc] initWithName:@"_accessTokenExpirationDate"
                                         type:[NSDate class]
                                   conversion:[OIDFieldMapping dateSinceNowConversion]];

nice!

UnitTests/OIDRegistrationRequestTests.m, line 62 at r3 (raw file):

  
  OIDServiceConfiguration *config = [OIDServiceConfigurationTests testInstance];
  OIDRegistrationRequest *request = [[OIDRegistrationRequest alloc] initWithConfiguration:config

move to next line and +4. Then either line colons up to that line, or +4 indent the longest and line up on that one instead.

UnitTests/OIDRegistrationRequestTests.m, line 161 at r3 (raw file):

                                                               error:&error];

extra linebreak

UnitTests/OIDRegistrationResponseTests.m, line 67 at r3 (raw file):

  OIDRegistrationResponse *response = [[OIDRegistrationResponse alloc] initWithRequest:request
      parameters:@{
          OIDClientIDParam : kClientIDTestValue,

first param can go on the previous line (@{ OIDClient…), then line up as you have. here and below

Comments from Reviewable

zamzterz · 2016-11-23T08:29:05Z

Review status: 12 of 26 files reviewed at latest revision, 39 unresolved discussions.

Source/AppAuth.h, line 31 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> is `OIDClientMetadataParameters.h` intentionally not included? OK if it is, just checking.

Yep, I didn't think there was any reason to expose since it only contains OIDC parameters automagically handled by the SDK.

Source/OIDAuthorizationService.h, line 69 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> > N > Align with the start of the first param (i.e. indent 4 more spaces)

Done.

Source/OIDAuthorizationService.h, line 135 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> > callback: > > sorry to be a pain, but can we call this `completion` and `OIDRegistrationCompletion`? I've realized that 'callback' is very generic and doesn't really help understanding, I know that's what we used elsewhere. >

Done. However, this should be followed by the same change for the other methods in `OIDAuthorizationService` (for example`OIDDiscoveryCallback`, `OIDAuthorizationCallback`, `OIDTokenCallback`, `presentAuthorizationRequest:[...]:callback`, `performTokenRequest:[...]:callback`, etc), for a consistent API.

Source/OIDAuthorizationService.m, line 366 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> can you set Xcode to strip trailing spaces?

Done. (Sorry about that!)

Source/OIDAuthorizationService.m, line 413 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> this looks like it could fit on the previous line, but if not should be indented +4. Here and elsewhere

Done.

Source/OIDAuthorizationService.m, line 424 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> indent +4

Done.

Source/OIDAuthorizationService.m, line 437 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> indent

Done.

Source/OIDAuthorizationService.m, line 442 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> and here

Done.

Source/OIDAuthState.m, line 192 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> I don't see this in the header as the new designated initializer.

It is not exposed at all in the header by design. I think it is better if `AuthState` is initiated from the registration response only since you would probably *not* have an authorization response together with the registration response. The authorization response can be added later with `updateWithAuthorizationResponse`. I am however open to exposing this initializer in the header, but even then maybe not as the designated initializer?

Source/OIDAuthState.m, line 206 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> since this clears the auth state, shouldn't it be first?

That would definitely be more correct, done!

Source/OIDClientMetadataParameters.h, line 31 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> doxygen comments are out of sync throughout

Out of sync how?

Source/OIDClientMetadataParameters.m, line 1 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> What's the design behind this file? I see in the Response class the consts are defined there instead. Are these shared? For AuthorizationRequest we don't even expose the param consts are part of the API (they exist only in the .m)

All client metadata registered by the OP from the registration request should be echoed back to the client in the registration response, hence the response will (in most) cases contain all the parameters present in the request. So those are shared.

In addition the response will contain the metadata the OP issues (client_id, client_secret, etc.), which is why those parameters are only defined in the RegistrationResponse.m (they are exposed through the interface of that class).

The thought was to encapsulate all relevant client metadata parameters in OIDClientMetadataParameters.h to be able to share them between the request and the response. However, right now the only parameter explicitly handled is the token_endpoint_auth_method (in anticipation of the token endpoint client auth changeset).
(And this is also the reason why this header is not exposed through AppAuth.h, it probably isn't too useful for an user of the SDK.)

I think this design allows for greater flexibility in the case there are more parameters to come that should be explicitly handled by the SDK. Otherwise the parameter name would have to be duplicated in both the response and request class each time.

Source/OIDFieldMapping.m, line 117 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> no space before 'value' `(NSNumber *)value`

Done.

Source/OIDRegistrationRequest.h, line 98 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> >

Renamed for easier indentation, done.

Source/OIDRegistrationRequest.m, line 31 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> > OIDSubjectTypeParam > > For AuthorizationRequest we re-used the OAuth param names for serialization and building the URL. If you did that here would it mean you wouldn't need the MetadataParameters file?

Unfortunately not. As expanded above `MetadataParameters` is necessary due to the coupling between registration request and response. I have however changed the values to match the OAuth param names for consistency.

Source/OIDRegistrationRequest.m, line 81 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> indent

Done.

Source/OIDRegistrationRequest.m, line 117 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> indent if it can't fit on the previous line with colon alignment

Done.

Source/OIDRegistrationRequest.m, line 122 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> same

Done.

Source/OIDRegistrationRequest.m, line 129 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> > > indent this line +4 from the "self", and then align the colons on that

Done (by rename).

Source/OIDRegistrationRequest.m, line 136 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> I think this can fit on one line, here and throughout this method

Done.

Source/OIDRegistrationRequest.m, line 157 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> indent

Done.

Source/OIDRegistrationRequest.m, line 176 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> indent

Done.

Source/OIDRegistrationResponse.m, line 96 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> indentation is correct, but move the OR operator to the second line

Done.

Source/OIDRegistrationResponse.m, line 97 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> use // style comments in the method, even for a long comment

Done.

Source/OIDServiceConfiguration.h, line 65 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> to avoid a breaking change, can this be an additional constructor?

Done.

Source/OIDServiceConfiguration.m, line 47 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> indent 4 spaces and align columns on this line

Done.

UnitTests/OIDRegistrationRequestTests.m, line 62 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> move to next line and +4. Then either line colons up to that line, or +4 indent the longest and line up on that one instead.

Done.

UnitTests/OIDRegistrationRequestTests.m, line 161 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> extra linebreak

Done.

UnitTests/OIDRegistrationResponseTests.m, line 67 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> first param can go on the previous line (`@{ OIDClient…`), then line up as you have. here and below

Done.

Comments from Reviewable

WilliamDenniss · 2016-11-23T20:11:01Z

Review status: 12 of 26 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

Example/Source/AppAuthExampleViewController.m, line 276 at r2 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

> Unnecessary blank line after start of block.

please fix

Source/AppAuth.h, line 31 at r3 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

> Yep, I didn't think there was any reason to expose since it only contains OIDC parameters automagically handled by the SDK.

LGTM

Source/OIDAuthorizationService.h, line 135 at r3 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

> Done. However, this should be followed by the same change for the other methods in `OIDAuthorizationService` (for example`OIDDiscoveryCallback`, `OIDAuthorizationCallback`, `OIDTokenCallback`, `presentAuthorizationRequest:[...]:callback`, `performTokenRequest:[...]:callback`, etc), for a consistent API.

Agreed.

Source/OIDAuthorizationService.m, line 438 at r4 (raw file):

      return;
    }

Can we strip all these trailing spaces? Sorry to be a pain but our tools (and Reviewable) always points them out.

Source/OIDAuthState.m, line 192 at r3 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

> It is not exposed at all in the header by design. I think it is better if `AuthState` is initiated from the registration response only since you would probably *not* have an authorization response together with the registration response. The authorization response can be added later with `updateWithAuthorizationResponse`. I am however open to exposing this initializer in the header, but even then maybe not as the designated initializer?

hmm ok but this is the designated initializer right? You can declare it privately in the .m like this class does: https://github.com/openid/AppAuth-iOS/blob/master/Source/OIDServiceConfiguration.m#L44

That said, I don't really think it's a problem to just expose it. Maybe I am sitting on those three objects due to some out of band request processing, and just want to create an AuthState at that moment. My vote would be to expose.

Source/OIDAuthState.m, line 439 at r4 (raw file):

         additionalRefreshParameters:
    (nullable NSDictionary<NSString *, NSString *> *)additionalParameters {
  if ([self isTokenFresh]) {

can you rebase on origin/master to avoid these changes appearing here?

Source/OIDClientMetadataParameters.h, line 31 at r3 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

> Out of sync how?

My mistake

Source/OIDClientMetadataParameters.m, line 1 at r3 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

> All client metadata registered by the OP from the registration request should be echoed back to the client in the registration response, hence the response will (in most) cases contain all the parameters present in the request. So those are shared. > > In addition the response will contain the metadata the OP issues (`client_id`, `client_secret`, etc.), which is why those parameters are only defined in the `RegistrationResponse.m` (they are exposed through the interface of that class). > > The thought was to encapsulate all relevant client metadata parameters in `OIDClientMetadataParameters.h` to be able to share them between the request and the response. However, right now the only parameter explicitly handled is the `token_endpoint_auth_method` (in anticipation of the token endpoint client auth changeset). > (And this is also the reason why this header is not exposed through `AppAuth.h`, it probably isn't too useful for an user of the SDK.) > > I think this design allows for greater flexibility in the case there are more parameters to come that should be explicitly handled by the SDK. Otherwise the parameter name would have to be duplicated in both the response and request class each time. >

LGTM

Source/OIDRegistrationRequest.m, line 31 at r3 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

> Unfortunately not. As expanded above `MetadataParameters` is necessary due to the coupling between registration request and response. I have however changed the values to match the OAuth param names for consistency.

LGTM

UnitTests/OIDRegistrationRequestTests.m, line 161 at r4 (raw file):

                                                             options:kNilOptions
                                                               error:&error];

trailing spaces here and elsewhere

UnitTests/OIDRegistrationResponseTests.m, line 67 at r4 (raw file):

  OIDRegistrationResponse *response = [[OIDRegistrationResponse alloc] initWithRequest:request
      parameters:@{ OIDClientIDParam : kClientIDTestValue,
          OIDClientIDIssuedAtParam : @(kClientIDIssuedAtTestValue),

Align the 'O' with same on previous line

Comments from Reviewable

zamzterz · 2016-11-24T13:34:35Z

Review status: 6 of 26 files reviewed at latest revision, 15 unresolved discussions.

Example/Source/AppAuthExampleViewController.m, line 276 at r2 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> please fix

Done.

Source/OIDAuthorizationService.m, line 438 at r4 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> Can we strip all these trailing spaces? Sorry to be a pain but our tools (and Reviewable) always points them out.

Sorry, I thought XCode got them all in the last revision. But I've done another lap and hopefully got them all this time.

Source/OIDAuthState.m, line 192 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> hmm ok but this is the designated initializer right? You can declare it privately in the .m like this class does: https://github.com/openid/AppAuth-iOS/blob/master/Source/OIDServiceConfiguration.m#L44 > > That said, I don't really think it's a problem to just expose it. Maybe I am sitting on those three objects due to some out of band request processing, and just want to create an AuthState at that moment. My vote would be to expose.

You're right, it is the designated initializer. And I can agree that there might be a use case for it, so I've exposed it, done.

Source/OIDAuthState.m, line 439 at r4 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> can you rebase on origin/master to avoid these changes appearing here?

I've rebased it against latest master and it seems up to date. I don't see the refresh token changes in this PR?

Source/OIDClientMetadataParameters.h, line 31 at r3 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> My mistake

Ok :)

UnitTests/OIDRegistrationResponseTests.m, line 67 at r4 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

> Align the 'O' with same on previous line

Sorry, then I misunderstood the previous review comment. Since this will give too long lines I've reverted it back to the original formatting which at least left-aligns all rows.

Comments from Reviewable

WilliamDenniss · 2016-11-28T21:59:47Z

Bitrise is reporting the following failures:

▸ Building AppAuth/AppAuth [Debug]
▸ Check Dependencies
▸ Compiling OIDErrorUtilities.m
▸ Compiling OIDTokenUtilities.m
▸ Compiling OIDGrantTypes.m
▸ Compiling OIDRegistrationRequest.m

⚠️  /Users/vagrant/git/Source/OIDRegistrationRequest.m:72:17: convenience initializer should not invoke an initializer on 'super' [-Wobjc-designated-initializers]

  self = [super init];
                ^



⚠️  /Users/vagrant/git/Source/OIDRegistrationRequest.m:65:1: convenience initializer missing a 'self' call to another initializer [-Wobjc-designated-initializers]

- (nullable instancetype)initWithConfiguration:(OIDServiceConfiguration *)configuration
^



⚠️  /Users/vagrant/git/Source/OIDRegistrationRequest.m:50:17: method definition for 'initWithConfiguration:redirectURIs:responseTypes:grantTypes:subjectType:tokenEndpointAuthMethod:additionalParameters:' not found [-Wincomplete-implementation]

@implementation OIDRegistrationRequest
                ^


▸ Compiling OIDTokenRequest.m
▸ Compiling OIDAuthState+IOS.m
▸ Compiling OIDTokenResponse.m
▸ Compiling OIDScopeUtilities.m
▸ Compiling OIDAuthorizationResponse.m
▸ Compiling OIDServiceConfiguration.m
▸ Compiling OIDClientMetadataParameters.m
▸ Compiling OIDAuthState.m
▸ Compiling OIDAuthorizationService.m
▸ Compiling OIDAuthorizationUICoordinatorIOS.m
▸ Compiling OIDURLQueryComponent.m
▸ Compiling OIDFieldMapping.m
▸ Compiling OIDError.m
▸ Compiling OIDAuthorizationRequest.m
▸ Compiling OIDRegistrationResponse.m
▸ Compiling OIDAuthorizationService+IOS.m
▸ Compiling OIDResponseTypes.m
▸ Compiling OIDScopes.m
▸ Compiling OIDServiceDiscovery.m
▸ Building library libAppAuth.a
▸ Build Succeeded
▸ Building AppAuth/AppAuth [Debug]
▸ Check Dependencies
▸ Building AppAuth/AppAuthTests [Debug]
▸ Check Dependencies
▸ Processing UnitTestsInfo.plist
▸ Compiling OIDTokenResponseTests.m
▸ Compiling OIDURLQueryComponentTests.m
▸ Compiling OIDURLQueryComponentTestsIOS7.m
▸ Compiling OIDServiceDiscoveryTests.m
▸ Compiling OIDTokenRequestTests.m
▸ Compiling OIDAuthorizationResponseTests.m
▸ Compiling OIDAuthorizationRequestTests.m
▸ Compiling OIDGrantTypesTests.m
▸ Compiling OIDResponseTypesTests.m
▸ Compiling OIDRegistrationRequestTests.m

❌  /Users/vagrant/git/UnitTests/OIDRegistrationRequestTests.m:63:39: no visible @interface for 'OIDRegistrationRequest' declares the selector 'initWithConfiguration:redirectURIs:responseTypes:grantTypes:subjectType:tokenEndpointAuthenticationMethod:additionalParameters:'

      [[OIDRegistrationRequest alloc] initWithConfiguration:config
^


xcode test exit code: 65
xcode test failed, error: exit status 65
exit status 1

WilliamDenniss · 2016-11-28T22:08:23Z

Reviewed 4 of 14 files at r4, 19 of 19 files at r5.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Source/OIDAuthState.m, line 439 at r4 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

I've rebased it against latest master and it seems up to date. I don't see the refresh token changes in this PR?

OK, I think Readable is including it then.

Source/OIDRegistrationRequest.h, line 97 at r5 (raw file):

              grantTypes:(nullable NSArray<NSString *> *)grantTypes
             subjectType:(nullable NSString *)subjectType
 tokenEndpointAuthMethod:(nullable NSString *)tokenEndpointAuthMethod

The param is the spec is token_endpoint_auth_method is it? If so then this rename to tokenEndpointAuthMethod is definitely desirable. Our original goal of the AppAuth API is to directly map the spec into ObjC where possible (so things get camel case, but ideally it's a direct transposition).

UnitTests/OIDRegistrationResponseTests.m, line 67 at r4 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

Sorry, then I misunderstood the previous review comment.
Since this will give too long lines I've reverted it back to the original formatting which at least left-aligns all rows.

Sorry to be unclear, but this is what I was expecting:

parameters:@{ OIDClientIDParam : kClientIDTestValue,
              OIDClientIDIssuedAtParam : @(kClientIDIssuedAtTestValue),

Comments from Reviewable

StevenEWright · 2016-11-28T22:10:03Z

Reviewed 1 of 27 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Example/Source/AppAuthExampleViewController.m, line 182 at r2 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Nit... (sorry, my job! LOL)

https://google.github.io/styleguide/objcguide.xml?showone=Container_Literals#Container_Literals

"If the collection fits on one line, put a single space after the opening and before the closing brackets."

@[ redirectURI ]

LGTM

Example/Source/AppAuthExampleViewController.m, line 184 at r2 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Nit... please one parameter per line, or all on one line.

LGTM

Example/Source/AppAuthExampleViewController.m, line 193 at r2 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Block contents need only 2 spaces indent from "callback"

LGTM

Example/Source/AppAuthExampleViewController.m, line 194 at r2 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Only 2 spaces indent from start of "if"

LGTM

Example/Source/AppAuthExampleViewController.m, line 236 at r2 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Only 2 spaces from left.

LGTM

Example/Source/AppAuthExampleViewController.m, line 201 at r5 (raw file):

}

- (void)doAuthWithAutoCodeExchange:(OIDServiceConfiguration *)configuration :(NSString *)clientID {

Something seems wrong here with the client ID parameter name?

Source/OIDAuthorizationService.m, line 397 at r5 (raw file):

      // response, checks for that
      if (HTTPURLResponse.statusCode == 400) {
        NSError *jsonDeserializationError;

Plz CAP all acronyms & initialisms. (json --> JSON)

NSError *JSONDeserializationError;
NSDictionary<NSString *, NSObject *> *JSON =

Source/OIDAuthorizationService.m, line 399 at r5 (raw file):

        NSError *jsonDeserializationError;
        NSDictionary<NSString *, NSObject <NSCopying> *> *json =
        [NSJSONSerialization JSONObjectWithData:data options:0 error:&jsonDeserializationError];

Indent line continuation +4 sp.

Source/OIDAuthorizationService.m, line 425 at r5 (raw file):

    }

    NSError *jsonDeserializationError;

Same JSON comments.

Source/OIDAuthState.h, line 153 at r5 (raw file):

/*! @brief Creates an auth state from an registration response.
    @param response The registration response.

response -> registrationResponse

Source/OIDAuthState.h, line 159 at r5 (raw file):

/*! @brief Creates an auth state from an authorization, token and registration response.
    @param response The authorization response.

response -> authorizationResponse

Comments from Reviewable

zamzterz · 2016-11-29T11:52:52Z

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Example/Source/AppAuthExampleViewController.m, line 201 at r5 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Something seems wrong here with the client ID parameter name?

What do you mean?

Source/OIDAuthorizationService.m, line 397 at r5 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Plz CAP all acronyms & initialisms. (json --> JSON)

NSError *JSONDeserializationError;
NSDictionary<NSString *, NSObject *> *JSON =

This was actually copied straight from performTokenRequest, which does not capitalize those variable names. Should they be kept for consistency?

Source/OIDAuthorizationService.m, line 399 at r5 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Indent line continuation +4 sp.

Done.

Source/OIDAuthorizationService.m, line 425 at r5 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Same JSON comments.

Will change when above comment is resolved.

Source/OIDAuthState.h, line 153 at r5 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

response -> registrationResponse

Done.

Source/OIDAuthState.h, line 159 at r5 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

response -> authorizationResponse

Done.

Source/OIDRegistrationRequest.h, line 97 at r5 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

The param is the spec is token_endpoint_auth_method is it? If so then this rename to tokenEndpointAuthMethod is definitely desirable. Our original goal of the AppAuth API is to directly map the spec into ObjC where possible (so things get camel case, but ideally it's a direct transposition).

You're correct, so this (unintentional) change was for the better!

UnitTests/OIDRegistrationResponseTests.m, line 67 at r4 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

Sorry to be unclear, but this is what I was expecting:

parameters:@{ OIDClientIDParam : kClientIDTestValue,
              OIDClientIDIssuedAtParam : @(kClientIDIssuedAtTestValue),

Ok, I understand. But this will give lines that are too long in all cases but one. So I opted to revert to the original avoid inconsistent indentation.

Comments from Reviewable

zamzterz · 2016-11-29T11:53:57Z

There was a parameter name mismatch. Hopefully resolved now (I can't view the Bitrise results).

Comments from Reviewable

StevenEWright · 2016-11-29T16:24:59Z

Review status: 23 of 26 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Example/Source/AppAuthExampleViewController.m, line 201 at r5 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

What do you mean?

Please look carefully at the method signature:

(void)doAuthWithAutoCodeExchange:(OIDServiceConfiguration *)configuration :(NSString *)clientID {

There is no parameter name for the clientID parameter.

Comments from Reviewable

StevenEWright · 2016-11-29T16:28:25Z

Review status: 23 of 26 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Source/OIDAuthorizationService.m, line 397 at r5 (raw file):

Previously, rebeckag (Rebecka Gulliksson) wrote…

This was actually copied straight from performTokenRequest, which does not capitalize those variable names. Should they be kept for consistency?

Fine by me. Looks like there are several such issues in the codebase now. I can put together a PR To fix all of them as a follow up. @WilliamDenniss ?

Comments from Reviewable

zamzterz · 2016-11-30T07:09:03Z

Review status: 23 of 26 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Example/Source/AppAuthExampleViewController.m, line 201 at r5 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Please look carefully at the method signature:

(void)doAuthWithAutoCodeExchange:(OIDServiceConfiguration *)configuration :(NSString *)clientID {

There is no parameter name for the clientID parameter.

Done.

Comments from Reviewable

StevenEWright · 2016-12-01T16:41:40Z

Reviewed 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

WilliamDenniss · 2016-12-04T01:54:17Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Source/OIDAuthorizationService.m, line 397 at r5 (raw file):

Previously, StevenEWright (Steven E Wright) wrote…

Fine by me. Looks like there are several such issues in the codebase now. I can put together a PR To fix all of them as a follow up. @WilliamDenniss ?

SGTM

Source/OIDError.m, line 27 at r6 (raw file):

NSString *const OIDOAuthAuthorizationErrorDomain = @"org.openid.appauth.oauth_authorization";

NSString *const OIDOAuthRegistrationErrorDomain = @"org.openid.appauth.oauth_registration";

Need to add this to isOAuthErrorDomain WilliamDenniss@1c60fcc#diff-2dd226962951829f45ccdfd986b28602L40

Comments from Reviewable

WilliamDenniss · 2016-12-04T01:54:44Z

It's unfortunately that you can't view Bitrise, but I was able to get the same errors by building and running the tests manually from XCode.

Here are the errors:

Running the tests...
$ set -o pipefail && env "NSUnbufferedIO=YES" xcodebuild "-project" "AppAuth.xcodeproj" "-scheme" "AppAuth" "build" "test" "-destination" "platform=iOS Simulator,name=iPhone 6s Plus,OS=latest" "GCC_INSTRUMENT_PROGRAM_FLOW_ARCS=YES" "GCC_GENERATE_TEST_COVERAGE_FILES=YES" | xcpretty "--color" "--report" "html" "--output" "/Users/vagrant/deploy/xcode-test-results-AppAuth.html"

▸ Building AppAuth/AppAuth [Debug]
▸ Check Dependencies
▸ Compiling OIDErrorUtilities.m
▸ Compiling OIDTokenUtilities.m
▸ Compiling OIDGrantTypes.m
▸ Compiling OIDRegistrationRequest.m

⚠️  /Users/vagrant/git/Source/OIDRegistrationRequest.m:56:10: undeclared selector 'initWithConfiguration:redirectURIs:responseTypes:grantTypes:subjectType:tokenEndpointAuthenticationMethod:additionalParameters:' [-Wundeclared-selector]

        @selector(initWithConfiguration:
         ^


▸ Compiling OIDTokenRequest.m
▸ Compiling OIDAuthState+IOS.m
▸ Compiling OIDTokenResponse.m
▸ Compiling OIDScopeUtilities.m
▸ Compiling OIDAuthorizationResponse.m
▸ Compiling OIDServiceConfiguration.m
▸ Compiling OIDClientMetadataParameters.m
▸ Compiling OIDAuthState.m
▸ Compiling OIDAuthorizationService.m
▸ Compiling OIDAuthorizationUICoordinatorIOS.m
▸ Compiling OIDURLQueryComponent.m
▸ Compiling OIDFieldMapping.m
▸ Compiling OIDError.m
▸ Compiling OIDAuthorizationRequest.m
▸ Compiling OIDRegistrationResponse.m
▸ Compiling OIDAuthorizationService+IOS.m
▸ Compiling OIDResponseTypes.m
▸ Compiling OIDScopes.m
▸ Compiling OIDServiceDiscovery.m
▸ Building library libAppAuth.a
▸ Build Succeeded
▸ Building AppAuth/AppAuth [Debug]
▸ Check Dependencies
▸ Building AppAuth/AppAuthTests [Debug]
▸ Check Dependencies
▸ Processing UnitTestsInfo.plist
▸ Compiling OIDURLQueryComponentTests.m
▸ Compiling OIDTokenResponseTests.m
▸ Compiling OIDURLQueryComponentTestsIOS7.m
▸ Compiling OIDServiceDiscoveryTests.m
▸ Compiling OIDTokenRequestTests.m
▸ Compiling OIDAuthorizationResponseTests.m
▸ Compiling OIDAuthorizationRequestTests.m
▸ Compiling OIDGrantTypesTests.m
▸ Compiling OIDResponseTypesTests.m
▸ Compiling OIDRegistrationRequestTests.m

❌  /Users/vagrant/git/UnitTests/OIDRegistrationRequestTests.m:63:39: no visible @interface for 'OIDRegistrationRequest' declares the selector 'initWithConfiguration:redirectURIs:responseTypes:grantTypes:subjectType:tokenEndpointAuthenticationMethod:additionalParameters:'

      [[OIDRegistrationRequest alloc] initWithConfiguration:config
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


xcode test exit code: 65
xcode test failed, error: exit status 65
If you can't find the reason of the error in the log, please check the raw-xcodebuild-output.log
The log file is stored in $BITRISE_DEPLOY_DIR, and its full path
is available in the $BITRISE_XCODE_RAW_TEST_RESULT_TEXT_PATH environment variable
|                                                                              |
+---+---------------------------------------------------------------+----------+
| x | Unit test library (exit code: 1)                              | 86 sec   |
+---+---------------------------------------------------------------+----------+
| Issue tracker: https://github.com/bitrise-io/steps-xcode-test/issues         |
| Source: https://github.com/bitrise-io/steps-xcode-test                       |
+---+---------------------------------------------------------------+----------+

Here's a patch to fix those errors, and correct another issue with how dynamic registration errors are parsed:
WilliamDenniss@1c60fcc

WilliamDenniss · 2016-12-04T03:26:21Z

Is there a test server instance to try this out on?

I tried a node.js one (forked to work with AppAuth) https://github.com/WilliamDenniss/node-oidc-provider. The registration works, but token exchange doesn't as they require the client secret in a header which I believe #24 will address.

I'm keen to try a full end-to-end test.

zamzterz · 2016-12-05T09:14:47Z

Is there a test server instance to try this out on?

Not that I know of.
The test server I've been using (https://op1.test.inacademia.org/) also requires client authentication at the token endpoint, so I've just tested it together with #24.

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Source/OIDError.m, line 27 at r6 (raw file):

Previously, WilliamDenniss (William Denniss) wrote…

Need to add this to isOAuthErrorDomain WilliamDenniss@1c60fcc#diff-2dd226962951829f45ccdfd986b28602L40

Done.

Comments from Reviewable

WilliamDenniss · 2017-02-27T00:23:59Z

@panva fixed https://github.com/panva/node-oidc-provider following my feedback, so now it can be used!

To enable native app support in node-oidc-provider, oauthNativeApps: true needs to be added in example/settings.js of the sample, and in the AppAuth sample tokenEndpointAuthMethod set to "client_secret_post" in the OIDRegistrationRequest request (AppAuth only supports POST for client secrets at present).

With that, I was able to test this PR end-to-end. Yey!

Add support for dynamic client registration.

WilliamDenniss · 2017-02-27T16:52:15Z

LGTM

I rebased this, and merged. Added client secrets to the sample so it would work with dynamic registration in my end-to-end test.

iainmcgin assigned WilliamDenniss May 21, 2016

zamzterz mentioned this pull request May 24, 2016

Client authentication at the token endpoint #24

Closed

StevenEWright self-assigned this Jun 2, 2016

zamzterz mentioned this pull request Nov 22, 2016

Be more lenient about refresh tokens. #23

Merged

WilliamDenniss mentioned this pull request Dec 4, 2016

Sets the NSError localizedDescription in more cases. #65

Merged

Add support for dynamic client registration.

7aa895a

WilliamDenniss added a commit to WilliamDenniss/AppAuth-iOS that referenced this pull request Feb 27, 2017

Merge pull request openid#22 from zamzterz/oidc-dynreg

f001448

Add support for dynamic client registration.

WilliamDenniss added a commit to WilliamDenniss/AppAuth-iOS that referenced this pull request Feb 27, 2017

Merge pull request openid#22 from zamzterz/oidc-dynreg

6927c83

Add support for dynamic client registration.

WilliamDenniss added a commit to WilliamDenniss/AppAuth-iOS that referenced this pull request Feb 27, 2017

Merge pull request openid#22 from zamzterz/oidc-dynreg

2da9819

Add support for dynamic client registration.

WilliamDenniss closed this Feb 27, 2017

WilliamDenniss added a commit to WilliamDenniss/AppAuth-iOS that referenced this pull request Mar 8, 2017

Added missing files from openid#22 to macOS and tvOS targets.

bd18e96

Add support for dynamic client registration. #22

Add support for dynamic client registration. #22

Uh oh!

Conversation

zamzterz commented May 20, 2016 • edited by StevenEWright Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iainmcgin commented May 21, 2016

Add support for dynamic client registration.

Uh oh!

zamzterz commented Jun 23, 2016

Uh oh!

StevenEWright commented Jul 7, 2016

Uh oh!

zamzterz commented Jul 11, 2016

Uh oh!

codecov-io commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 76.22% (diff: 75.67%)

Uh oh!

WilliamDenniss commented Nov 23, 2016

Uh oh!

zamzterz commented Nov 23, 2016

Uh oh!

WilliamDenniss commented Nov 23, 2016

Uh oh!

zamzterz commented Nov 24, 2016

Uh oh!

WilliamDenniss commented Nov 28, 2016

Uh oh!

WilliamDenniss commented Nov 28, 2016

Uh oh!

StevenEWright commented Nov 28, 2016

Uh oh!

zamzterz commented Nov 29, 2016

Uh oh!

zamzterz commented Nov 29, 2016

Uh oh!

StevenEWright commented Nov 29, 2016

Uh oh!

StevenEWright commented Nov 29, 2016

Uh oh!

zamzterz commented Nov 30, 2016

Uh oh!

StevenEWright commented Dec 1, 2016

Uh oh!

WilliamDenniss commented Dec 4, 2016

Uh oh!

WilliamDenniss commented Dec 4, 2016

Uh oh!

WilliamDenniss commented Dec 4, 2016

Uh oh!

zamzterz commented Dec 5, 2016

Uh oh!

WilliamDenniss commented Feb 27, 2017

Uh oh!

WilliamDenniss commented Feb 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zamzterz commented May 20, 2016 •

edited by StevenEWright

Loading

codecov-io commented Nov 22, 2016 •

edited

Loading