-
Notifications
You must be signed in to change notification settings - Fork 1.6k
PR: Auth for watchOS #4632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PR: Auth for watchOS #4632
Changes from all commits
8f570e2
59ac3e5
d6036af
aaf7cde
29d77cf
3e4a150
195f67c
144e076
d770ee3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,7 +82,12 @@ - (FIRUser *)getStoredUserForAccessGroup:(NSString *)accessGroup | |
query[(__bridge id)kSecAttrAccount] = kSharedKeychainAccountValue; | ||
|
||
NSData *data = [self.keychainServices getItemWithQuery:query error:outError]; | ||
#if TARGET_OS_WATCH | ||
NSError *error; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know what sort of errors are potentially expected here, and where we should be checking it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should refactor it to using GULSecureCoding eventually (may be a part of a follow up PR). No existing data migration will be required because the archive file format is the same for the secure coding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't do anything to this. According to the documentation:
while the deprecated method throws an exception if data is not a valid archive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SG - @maksymmalyhin we can refactor after the fact since it's not directly related to watchOS. |
||
NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingFromData:data error:&error]; | ||
#else | ||
NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingWithData:data]; | ||
#endif | ||
FIRUser *user = [unarchiver decodeObjectOfClass:[FIRUser class] forKey:kStoredUserCoderKey]; | ||
|
||
return user; | ||
|
@@ -100,10 +105,17 @@ - (BOOL)setStoredUser:(FIRUser *)user | |
query[(__bridge id)kSecAttrService] = projectIdentifier; | ||
query[(__bridge id)kSecAttrAccount] = kSharedKeychainAccountValue; | ||
|
||
#if TARGET_OS_WATCH | ||
NSKeyedArchiver *archiver = [[NSKeyedArchiver alloc] initRequiringSecureCoding:false]; | ||
#else | ||
NSMutableData *data = [NSMutableData data]; | ||
NSKeyedArchiver *archiver = [[NSKeyedArchiver alloc] initForWritingWithMutableData:data]; | ||
#endif | ||
[archiver encodeObject:user forKey:kStoredUserCoderKey]; | ||
[archiver finishEncoding]; | ||
#if TARGET_OS_WATCH | ||
NSData *data = archiver.encodedData; | ||
#endif | ||
|
||
return [self.keychainServices setItem:data withQuery:query error:outError]; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that something manual that you set up, or are you saying that's how it'll work automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see: #4621 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@renkelvin can you please take a look at this bit specifically?
I'm comfortable merging the email/password sign in changes now, I'm not sure about the email link and will defer to @renkelvin for Auth's view of this (see the comment for the full solution and context).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Maybe add a link to #4621 (comment) to make it easier to other developers?