AppAuth-iOS icon indicating copy to clipboard operation
AppAuth-iOS copied to clipboard

Unarchive with unarchivedObjectOfClass:fromData:error: fails with Google IdP

Open julienbodet opened this issue 5 years ago • 15 comments

Describe the bug I replaced the deprecated archiving/unarchiving method calls in the example. unarchivedObjectOfClass:fromData:error: instead of unarchivedObjectWithData: for unarchiving.

While running the app, OIDAuthorizationRequest.m:147 asserts:

2019-11-18 22:40:44.589307-0500 Example-iOS_ObjC[9060:362850] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'The response_type "(null)" isn't supported. AppAuth only supports the "code" or "code id_token" response_type.'

By commenting this assert, I get the following error from the unarchive method:

Error Domain=NSCocoaErrorDomain Code=4864 "value for key 'NS.objects' was of unexpected class 'NSArray'. Allowed classes are '{(
    OIDServiceDiscovery
)}'." UserInfo={NSDebugDescription=value for key 'NS.objects' was of unexpected class 'NSArray'. Allowed classes are '{(
    OIDServiceDiscovery
)}'.}

This seems related to the bug described in #466.

To Reproduce Steps to reproduce the behavior:

  1. Run example app
  2. Authorize
  3. Kill app
  4. Open and see error

Smartphone (please complete the following information):

  • Device: iPhone 11 Simulator
  • OS: 13.0

julienbodet avatar Nov 19 '19 03:11 julienbodet

Please ignore the referencing pull request above, it's an error and unrelated.

julienbodet avatar Nov 24 '19 00:11 julienbodet

Pull request #315 already discussed the same error. The conclusion given by @WilliamDenniss was:

Steve and I spent a bit of time debugging this. Our conclusion is that the root object of your discovery doc JSON was likely an array, and not a dictionary.

Based on this issue, the previous conclusion made might be wrong and using unarchivedObjectOfClass:fromData:error: to unarchive the auth state object could have caused the error observed in #315.

julienbodet avatar Nov 24 '19 00:11 julienbodet

Hi everyone, @julienbodet, @s2ler, @AndrewMcDrew,

My company is developing a brand new app using AppAuth. And we faced the same issue while using NSKeyedUnarchiver.unarchivedObject(ofClass:from:).

we've read the various threads (https://github.com/openid/AppAuth-iOS/pull/315, https://github.com/openid/AppAuth-iOS/pull/466) related to that issue in this repo but could not find a nice workaround or a PR that fixes the issue.

Can you confirm that for now, there is no other way than using the Apple deprecated function NSKeyedUnarchiver.unarchiveObject(with:) ?

Thanks.

twittemb avatar Jul 07 '20 15:07 twittemb

From what I can tell, the problem has to do with the implementation of NSSecureCoding; while unarchiving, the code is listing allowed classes, but that doesn't match what's actually encoded in the archive.

You can work around this by manually creating an NSKeyedUnarchiver instance, setting its requiresSecureCoding property to false, and using that to unarchive the blob.

davedelong avatar Aug 13 '20 15:08 davedelong

Hey all, wanted to say that I'm running into the exact same issue. Not entirely sure what's going on, but I'm noticing that in OIDAuthorizationRequest's initWithCoder (circa lines 221-258) the OIDServiceConfiguration seems to be decoded correctly but all of the subsequent NSString*s are being decoded as nil, which is triggering the assert as described above.

@WilliamDenniss, does this sound like a bug in AppAuth?

sashaweiss avatar Dec 19 '20 20:12 sashaweiss

In line with what others have reported, I'm able to get get it to work by using NSKeyedArchiver.unarchiveTopLevelObjectWithData() instead of NSKeyedArchiver.unarchivedObject(ofClass:,from:).

Specifically, I have a class AuthState that packages an OIDAuthState and an OIDServiceConfiguration to serialize them together:

class AuthState: NSSecureCoding {
    let state: OIDAuthState
    let config: OIDServiceConfiguration
    
    <implementation of init, NSSecureCoding>
}

let serializedAuthState: Data = try NSKeyedArchiver.archivedData(withRootObject: anAuthState, requiringSecureCoding: true)

// Decodes incorrectly, hits `NSAssert` on `nil` value for `response_type` as described above.
let authState: AuthState = try NSKeyedUnarchiver.unarchivedObject(ofClass: AuthState.self, from: serializedAuthState)

// Succeeds
let authState: AuthState = try NSKeyedUnarchiver.unarchiveTopLevelObjectWithData(serializedAuthState) as! AuthState

As far as I can tell the implementation in AppAuth for those types is good, but obviously we're missing something. It's especially interesting to me that the configuration property decodes correctly (per previous comment), and then all the NSString*s come back as nil.

For the moment I'm going to work around this with the workaround as above, but given that method is also deprecated (although not yet removed as of iOS 14.3) a fix here will have some urgency.

sashaweiss avatar Dec 19 '20 21:12 sashaweiss

For the moment I'm going to work around this with the workaround as above, but given that method is also deprecated (although not yet removed as of iOS 14.3) a fix here will have some urgency.

Agree. At the very least, the documentation and the sample code should be updated

below avatar Feb 25 '21 11:02 below

Still no release where this is fixed ???.... Damn.....

tychop avatar Apr 22 '21 13:04 tychop

sometimes the unArchive oauth return nil

NSKeyedUnarchiver.unarchiveObject(with: archivedAuthState) as? OIDAuthState

fukemy avatar Jul 03 '21 08:07 fukemy

Hi, I'm new here.

I fixed the problem by changing in flie : OIDServiceConfiguration.m line : 188

OIDServiceDiscovery *discoveryDocument = [aDecoder decodeObjectOfClass:[OIDServiceDiscovery class]
                                                                  forKey:kDiscoveryDocumentKey];

to

OIDServiceDiscovery *discoveryDocument = [aDecoder decodeObjectOfClasses:[NSSet setWithObjects:[OIDServiceDiscovery class], [NSDictionary class], [NSArray class], nil]
                                                                  forKey:kDiscoveryDocumentKey];

Wish to help.

zhuyinli avatar Sep 16 '21 17:09 zhuyinli

Thanks for the fix @zhuyinli! I can confirm it works for me.

naquin avatar Sep 30 '21 09:09 naquin

can it be available in newest version? Thanks

fukemy avatar Sep 30 '21 10:09 fukemy

I just ran into this problem, and confirmed that the fix works for me too (thanks @zhuyinli!)

What needs to happen to get this fix into a release?

iainmerrick avatar Dec 13 '21 13:12 iainmerrick

Hi there, I have some bug with this package on macOS target: After stored data with methods: let encodedData = try NSKeyedArchiver.archivedData(withRootObject: authState!, requiringSecureCoding: false) defaults.set(encodedData, forKey: kAuthorizerKey) and unachieved with method: if let decoded = defaults.object(forKey: kAuthorizerKey) as? Data { do { let auth = try NSKeyedUnarchiver.unarchiveObject(with: decoded) as? OIDAuthState all work fine. But last method is deprecated. When I replaced last method with: let auth = try NSKeyedUnarchiver.unarchivedObject(ofClass: OIDAuthState.self, from: decoded) this failed with message in debugger area: *** Assertion failure in -[OIDAuthorizationRequest initWithConfiguration:clientId:clientSecret:scope:redirectURL:responseType:state:nonce:codeVerifier:codeChallenge:codeChallengeMethod:additionalParameters:], OIDAuthorizationRequest.m:147 2022-02-07 12:34:31.759062+0200 TestApp[23341:1292333] The response_type "(null)" isn't supported. AppAuth only supports the "code" or "code id_token" response_type.

Does anyone know how to fix this? Thanks!

Al-Kuka avatar Feb 07 '22 10:02 Al-Kuka

Hello,

All users of this library need to persist OIDAuthState, so it would be great if the library would support serialization of this class out of the box.

Today, developers are bound to use deprecated NSCoding apis, because NSSecureCoding support is broken. The Swift compiler does not allow to silence deprecation warnings. This is not a great situation.

In this repository, there are several pull requests that attempt at fixing this issue, at various levels of completion, and all blocked at some stage or another.

Clearly community contributions are unable to fix this problem.

May I suggest that some core contributor would act as a benevolent contributor, take this issue seriously, and provides a reliable serialization solution for OIDAuthState? I can't point at anybody, of course, but maybe @petea could provide some guidance?

groue avatar Jun 16 '22 09:06 groue