Unarchiving#466
Conversation
…archivedObjectOfClass:fromData:error:] in OIDServiceDiscovery.
|
Andrew, can you sign the CLA? |
|
But I remember for sure I've signed everything needed during this pr. Should I undergo the procedure with each and every pr? |
|
Thanks for referencing the existing CLA, no need to do it again 😅 |
|
I searched the history of pull requests and I found #315 which was trying to fix the same error. Finally after discussion and investigation the PR was closed (see my comment in related issue 479). @AndrewMcDrew I suspect you're using |
| NSError *error; | ||
| NSDictionary *dictionary = [[NSDictionary alloc] initWithCoder:aDecoder]; | ||
| NSDictionary *dictionary = nil; | ||
| if (@available(iOS 11.0, *)) { |
There was a problem hiding this comment.
I think it is safe to replace the old encoding/decoding implementation for every versions of iOS. Also this will break backward compatibility for old archives... we need to find way to support unarchiving the old way too.
| XCTAssertEqualObjects(discovery.discoveryDictionary, unarchived.discoveryDictionary, @""); | ||
|
|
||
| if (@available(iOS 11.0, *)) { | ||
| data = [NSKeyedArchiver archivedDataWithRootObject:discovery requiringSecureCoding:YES error:&error]; |
There was a problem hiding this comment.
This breaks the build for older versions of other platforms where the NSKeyedArchiver/NSKeyedUnarchiver are not available.
Need more platform checks:
if (@available(iOS 11, macCatalyst 13, macOS 10.13, tvOS 11, *)) {
...
}
Thanks for mentioning that PR. It closes with words
So as you can see I am still using the very same sample in the test but inserted an array inside as a real discovery doc would have. The root object of the sample is still a dictionary. But it fails without a fix. On the other hand -- yes indeed I didn't thought about the data backward compatibility. That's a serious issue, I'm not sure I'm able to handle it now. |
Codecov Report
@@ Coverage Diff @@
## master #466 +/- ##
==========================================
- Coverage 72.2% 72.19% -0.02%
==========================================
Files 63 63
Lines 5400 5423 +23
==========================================
+ Hits 3899 3915 +16
- Misses 1501 1508 +7
Continue to review full report at Codecov.
|
|
So. I've dived a bit into how to unarchive the data archived using the previous version of AppAuth. Used the idea expressed here. But it doesn't work. It looks like it's possible to add some ugly conditional logic out of the AppAuth where unarchiving starts to determine how it was archived and choose the appropriate method there -- either a new one or deprecated in the case we have an old archive. But this leaves the deprecated line in the code so why to bother if it anyway there to stay? One more thing for anybody with the same problem at hand. Actually we can use our fork because we are just introducing OIDC login into the app so we aren't troubled with the data backward compatibility. But it can lead to the headaches in the future -- if AppAuth changes sth significantly in that area, then we will face some merges into our fork. So we are either going with the risk of Apple removing the old method altogether or AppAuth making some big changes. Decided that AppAuth risk is higher. So sad but true, not the first attempt in this repo to bring that new methods and get rid of deprecations. |
5 participants
In our project to preserve auth credentials between network sessions we archive the obtained during the successful login
OIDAuthStateobject and store it in the keychain. We used to use+[NSKeyedUnarchiver unarchiveObjectWithData:]to unarchive it back for the next session. But this method is deprecated started from the iOS 12 so we tried to make use of recommended+[NSKeyedUnarchiver unarchivedObjectOfClass:fromData:error:]which brought us to the crush in the depth of AppAuth. The thing is about a dictionary encapsulated byOIDServiceDiscovery. Despite confirming toNSSecureCodingit uses-[NSDictionary initWithCoder:]. It's ok and works and tests are green -- only because test's dictionary is a bit artificial. It contains strings only as a values. But in our real deployment it is full of nested collections. An attempt to use-[NSDictionary initWithCoder:]with such a dictionary fails and setsNSKeyedUnarchiver'serrorproperty to the appropriate error. So this pr improves the test dictionary with such an example and fixes such a bug trying to maintain the compatibility with the previous versions.Please note though -- I wasn't able to run the tests on the iOS 10 to check if I broke sth. Obviously it's about AuthenticationServices framework which didn't exist at the moment. Our project is runnable on it too. Looks like this thing is a bit beyond my skills. Here is what XCode greets me with.