Revert "Replace NSCoding with NSSecureCoding"#7863
Conversation
This reverts commit 34c4bdb.
charlotteliang
requested review from
maksymmalyhin and
paulb777
and removed request for
maksymmalyhin
paulb777
left a comment
paulb777
left a comment
There was a problem hiding this comment.
Are we missing a test that would have caught the regression?
|
The unit test didn't capture this. So we change Messaging from read/write to IIDTokenInfo to MessagingTokenInfo. We need a test like IID call deleteToken and then Messaging try to read the token. |
There was a problem hiding this comment.
LGTM.
BTW, I just realized that if we bump minimum supported macOS version to 10.13 then we don't need to use GULSecureCoding as modern NSKeyedUnarchiver API will be available for all supported OS (iOS, tvOS and watchOS versions already support the API).
Never mind, the NSKeyedUnarchiver API actually requires iOS 11+ (I misread the code) , so using GULSecureCoding is still a cleaner solution. We can consider extending GULSecureCoding to support setting the class for an unarchiver instance if we want to avoid global class override.
maksymmalyhin
left a comment
maksymmalyhin
left a comment
There was a problem hiding this comment.
Noticed one potential issue(see the comment). In either way I would suggest changes to the PR. Would you mind discussing before proceeding.
| @"Could not parse raw APNS Info while parsing unarchived token info: %@", error); | ||
| // TODO(chliangGoogle: Use the new API and secureCoding protocol. | ||
| @try { | ||
| [NSKeyedUnarchiver setClass:[FIRMessagingAPNSInfo class] |
There was a problem hiding this comment.
If we override the class globally then:
- we can still use
GULSecureCodingsince the override will affect it as well - if the FCM version is used with IID then
FIRInstanceIDAPNSInfoarchives will be unarchived toFIRMessagingAPNSInfoin IID as well. It will probably not cause any issue as long as the classes are identical due to Objective-C dynamic nature, but it looks a bit risky and not clean.
I think we should modify it either:
- override the class only for an
NSKeyedUnarchiverinstance, not globally. It's a safer option. - if we are fine with the global override, then the revert is not required. Just appending the global
[NSKeyedUnarchiver setClass:...]will be enough. It's simpler to implement.
There was a problem hiding this comment.
I suggest we made the change after I/O breaking change because Messaging already had significant amount of code change and at this point we want to ensure no unnecessary changes in so we can fully tested all the complicated scenarios with IID and Messaging.
maksymmalyhin
left a comment
maksymmalyhin
left a comment
There was a problem hiding this comment.
Discussed offline:
- the PR is a clean revert to the tested version, so it should be safe to do
- we will most likely postpone the NSSecureCoding update until after Firebase 8 to reduce risks
- Firebase 8 is going to include code to make sure IID and FCM are not installed in the app together, so setting the class globally will not be an issue
- A version after Firebase 8 will include NSSecureCoding change based on existing GULSecureCoding API
Coverage ReportAffected SDKs
Test Logs |
4 participants
Reverts #7831
Looks like this is the one breaks the Encoding/Decoding as the GULSecureCoding doesn't offer overwriting the class. Considering the code change is significant, it's less risk to revert this change and fix the secure coding on top of the original implementation. (preferably after breaking change and let the large refactor of token code settle for a while)