Correct OIDServiceDiscovery's NSCoding implementation (#721)

* Improve NSCoding implementation.

* Test new and old encodings.

* Ensure that we're handling the old encoding without errors.

* No need to supply an empty failure string.

* Provide forward compatibility.

Author: petea

Source/AppAuthCore/OIDServiceConfiguration.m

@@ -185,8 +185,17 @@ - (nullable instancetype)initWithCoder:(NSCoder *)aDecoder {
     return nil;
   }
 
-  OIDServiceDiscovery *discoveryDocument = [aDecoder decodeObjectOfClasses:[NSSet setWithObjects:[OIDServiceDiscovery class], [NSArray class], nil]
-                                                                  forKey:kDiscoveryDocumentKey];
+  NSSet<Class> *allowedClasses = [NSSet setWithArray:@[[OIDServiceDiscovery class],
+                                                       // The following classes are required in
+                                                       // order to support secure decoding of the
+                                                       // old OIDServiceDiscovery encoding.
+                                                       [NSDictionary class],
+                                                       [NSArray class],
+                                                       [NSString class],
+                                                       [NSNumber class],
+                                                       [NSNull class]]];
+  OIDServiceDiscovery *discoveryDocument = [aDecoder decodeObjectOfClasses:allowedClasses
+                                                                    forKey:kDiscoveryDocumentKey];
 
   return [self initWithAuthorizationEndpoint:authorizationEndpoint
                                tokenEndpoint:tokenEndpoint

Source/AppAuthCore/OIDServiceDiscovery.m

@@ -23,6 +23,10 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+/*! @brief The key for the @c discoveryDictionary property.
+ */
+static NSString *const kDiscoveryDictionaryKey = @"discoveryDictionary";
+
 /*! Field keys associated with an OpenID Connect Discovery Document. */
 static NSString *const kIssuerKey = @"issuer";
 static NSString *const kAuthorizationEndpointKey = @"authorization_endpoint";
@@ -192,7 +196,27 @@ + (BOOL)supportsSecureCoding {
 
 - (nullable instancetype)initWithCoder:(NSCoder *)aDecoder {
   NSError *error;
-  NSDictionary *dictionary = [[NSDictionary alloc] initWithCoder:aDecoder];
+  NSDictionary *dictionary;
+  if ([aDecoder containsValueForKey:kDiscoveryDictionaryKey]) {
+    // We're decoding a collection type (NSDictionary) from NSJSONSerialization's
+    // +JSONObjectWithData, so we need to include all classes that could potentially be contained
+    // within.
+    NSSet<Class> *allowedClasses = [NSSet setWithArray:@[[NSDictionary class],
+                                                         [NSArray class],
+                                                         [NSString class],
+                                                         [NSNumber class],
+                                                         [NSNull class]]];
+    dictionary = [aDecoder decodeObjectOfClasses:allowedClasses
+                                          forKey:kDiscoveryDictionaryKey];
+  } else {
+    // Decode using the old encoding which delegated to NSDictionary's encodeWithCoder:
+    // implementation:
+    //
+    // - (void)encodeWithCoder:(NSCoder *)aCoder {
+    //   [_discoveryDictionary encodeWithCoder:aCoder];
+    // }
+    dictionary = [[NSDictionary alloc] initWithCoder:aDecoder];
+  }
   self = [self initWithDictionary:dictionary error:&error];
   if (error) {
     return nil;
@@ -201,6 +225,8 @@ - (nullable instancetype)initWithCoder:(NSCoder *)aDecoder {
 }
 
 - (void)encodeWithCoder:(NSCoder *)aCoder {
+  [aCoder encodeObject:_discoveryDictionary forKey:kDiscoveryDictionaryKey];
+  // Provide forward compatibilty by continuing to add the old encoding.
   [_discoveryDictionary encodeWithCoder:aCoder];
 }
 

UnitTests/OIDServiceDiscoveryTests.m

@@ -30,6 +30,42 @@
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wgnu"
 
+// Define a subclass of @c OIDServiceDiscovery that provides the old NSCoding decoding
+// implementation.
+@interface OIDServiceDiscoveryOldDecoding : OIDServiceDiscovery
+@end
+
+@implementation OIDServiceDiscoveryOldDecoding
+
++ (BOOL)supportsSecureCoding {
+  return YES;
+}
+
+- (nullable instancetype)initWithCoder:(NSCoder *)aDecoder {
+  NSError *error;
+  NSDictionary *dictionary = [[NSDictionary alloc] initWithCoder:aDecoder];
+  self = [self initWithDictionary:dictionary error:&error];
+  if (error) {
+    return nil;
+  }
+  return self;
+}
+
+@end
+
+// Define a subclass of @c OIDServiceDiscovery that provides the old NSCoding encoding
+// implementation.
+@interface OIDServiceDiscoveryOldEncoding : OIDServiceDiscovery
+@end
+
+@implementation OIDServiceDiscoveryOldEncoding
+
+- (void)encodeWithCoder:(NSCoder *)coder {
+  [self.discoveryDictionary encodeWithCoder:coder];
+}
+
+@end
+
 /*! Testing URL used when testing URL conversions. */
 static NSString *const kTestURL = @"http://www.google.com/";
 
@@ -110,7 +146,7 @@ + (NSDictionary *)completeServiceDiscoveryDictionary {
     kJWKSURLKey : @"http://www.example.com/jwks",
     kRegistrationEndpointKey : @"Registration Endpoint",
     kEndSessionEndpointKey : @"https://www.example.com/logout",
-    kScopesSupportedKey : @"Scopes Supported",
+    kScopesSupportedKey : @[ @"scope1", @"scope2" ],
     kResponseTypesSupportedKey : @"Response Types Supported",
     kResponseModesSupportedKey : @"Response Modes Supported",
     kGrantTypesSupportedKey : @"Grant Types Supported",
@@ -411,10 +447,97 @@ - (void)testSecureCoding {
   OIDServiceDiscovery *discovery =
       [[OIDServiceDiscovery alloc] initWithDictionary:serviceDiscoveryDictionary
                                                              error:&error];
-  NSData *data = [NSKeyedArchiver archivedDataWithRootObject:discovery];
-  OIDServiceDiscovery *unarchived = [NSKeyedUnarchiver unarchiveObjectWithData:data];
+  NSData *data;
+  if (@available(iOS 11.0, macOS 10.13, tvOS 11.0, watchOS 4.0, *)) {
+    data = [NSKeyedArchiver archivedDataWithRootObject:discovery
+                                 requiringSecureCoding:YES
+                                                 error:&error];
+  } else {
+    data = [NSKeyedArchiver archivedDataWithRootObject:discovery];
+  }
+  
+  OIDServiceDiscovery *unarchived;
+  if (@available(iOS 11.0, macOS 10.13, tvOS 11.0, watchOS 4.0, *)) {
+    unarchived = [NSKeyedUnarchiver unarchivedObjectOfClass:[OIDServiceDiscovery class]
+                                                   fromData:data
+                                                      error:&error];
+  } else {
+    unarchived = [NSKeyedUnarchiver unarchiveObjectWithData:data];
+  }
+
+  XCTAssertEqualObjects(discovery.discoveryDictionary, unarchived.discoveryDictionary);
+}
 
-  XCTAssertEqualObjects(discovery.discoveryDictionary, unarchived.discoveryDictionary, @"");
+/*! @brief To ensure backward compatibility, test our ability to decode the old encoding of
+        @c OIDServiceDiscovery.
+ */
+- (void)testSecureCodingDecodeOld {
+  NSError *error;
+  NSDictionary *serviceDiscoveryDictionary = [[self class] completeServiceDiscoveryDictionary];
+  OIDServiceDiscoveryOldEncoding *discovery =
+      [[OIDServiceDiscoveryOldEncoding alloc] initWithDictionary:serviceDiscoveryDictionary
+                                                           error:&error];
+  NSData *data;
+  if (@available(iOS 11.0, macOS 10.13, tvOS 11.0, watchOS 4.0, *)) {
+    data = [NSKeyedArchiver archivedDataWithRootObject:discovery
+                                 requiringSecureCoding:YES
+                                                 error:&error];
+  } else {
+    data = [NSKeyedArchiver archivedDataWithRootObject:discovery];
+  }
+  
+  OIDServiceDiscovery *unarchived;
+  if (@available(iOS 11.0, macOS 10.13, tvOS 11.0, watchOS 4.0, *)) {
+    NSSet<Class> *allowedClasses = [NSSet setWithArray:@[[OIDServiceDiscovery class],
+                                                         [NSDictionary class],
+                                                         [NSArray class],
+                                                         [NSString class],
+                                                         [NSNumber class],
+                                                         [NSNull class]]];
+    unarchived = [NSKeyedUnarchiver unarchivedObjectOfClasses:allowedClasses
+                                                     fromData:data
+                                                        error:&error];
+  } else {
+    unarchived = [NSKeyedUnarchiver unarchiveObjectWithData:data];
+  }
+
+  XCTAssertEqualObjects(discovery.discoveryDictionary, unarchived.discoveryDictionary);
+}
+
+/*! @brief To ensure forward compatibility, test the ability of the old implementation to decode
+        the new encoding of @c OIDServiceDiscovery.
+ */
+- (void)testSecureCodingOldDecodeNew {
+  NSError *error;
+  NSDictionary *serviceDiscoveryDictionary = [[self class] completeServiceDiscoveryDictionary];
+  OIDServiceDiscoveryOldDecoding *discovery =
+      [[OIDServiceDiscoveryOldDecoding alloc] initWithDictionary:serviceDiscoveryDictionary
+                                                           error:&error];
+  NSData *data;
+  if (@available(iOS 11.0, macOS 10.13, tvOS 11.0, watchOS 4.0, *)) {
+    data = [NSKeyedArchiver archivedDataWithRootObject:discovery
+                                 requiringSecureCoding:YES
+                                                 error:&error];
+  } else {
+    data = [NSKeyedArchiver archivedDataWithRootObject:discovery];
+  }
+  
+  OIDServiceDiscovery *unarchived;
+  if (@available(iOS 11.0, macOS 10.13, tvOS 11.0, watchOS 4.0, *)) {
+    NSSet<Class> *allowedClasses = [NSSet setWithArray:@[[OIDServiceDiscoveryOldDecoding class],
+                                                         [NSDictionary class],
+                                                         [NSArray class],
+                                                         [NSString class],
+                                                         [NSNumber class],
+                                                         [NSNull class]]];
+    unarchived = [NSKeyedUnarchiver unarchivedObjectOfClasses:allowedClasses
+                                                     fromData:data
+                                                        error:&error];
+  } else {
+    unarchived = [NSKeyedUnarchiver unarchiveObjectWithData:data];
+  }
+  XCTAssertNil(error);
+  XCTAssertEqualObjects(discovery.discoveryDictionary, unarchived.discoveryDictionary);
 }
 
 /*! @brief Tests the NSCopying implementation by round-tripping an instance through the copying