PR: Auth for watchOS #4632

hohteri · 2020-01-08T18:28:23Z

PR for FR #4621

Tested with email & password, Apple ID and email link sign ins. Please note that both Apple ID and email link sign in work only on a real device.

Please see FR for further discussion and sample code for using email link sign in.

renkelvin · 2020-01-13T18:37:53Z

Thanks @hohteri for you contribution! We'll review and get back to you. Since it's a non-trivial change, we may need more time on the watchOS support.

ryanwilson

Apologies for the long delay, and thank you so much for the contribution - this is great! A few comments but most of them are small. I'll work with you through getting this up and running.

Firebase/Auth/Source/Auth/FIRAuth.m

ryanwilson · 2020-02-04T22:53:16Z

Firebase/Auth/Source/AuthProvider/GameCenter/FIRGameCenterAuthProvider.m

@@ -78,6 +79,9 @@ + (void)getCredentialWithCompletion:(FIRGameCenterCredentialCallback)completion
                                                         timestamp:timestamp
                                                       displayName:displayName];
         completion(credential, nil);
+#else
+           completion(nil, nil);


This is purely because this PR doesn't support game center, right?

If that's the case, can you please construct an error and return it in the completion block? A helper method to generate the error can go at the bottom of this header: https://github.com/firebase/firebase-ios-sdk/blob/master/Firebase/Auth/Source/Utilities/FIRAuthErrorUtils.h

and the implementation in the associated .m file.

For the error code itself, please use FIRAuthErrorCodeInternalError

Apple has deprecated playerID property on watchOS 6 and the suggested replacement teamPlayerID is not available on watchOS.

Firebase/Auth/Source/SystemService/FIRAuthAPNSTokenManager.h

Firebase/Auth/Source/SystemService/FIRAuthNotificationManager.m

ryanwilson · 2020-02-04T23:03:26Z

Firebase/Auth/Source/SystemService/FIRAuthStoredUserManager.m

  NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingWithData:data];
+#else
+  NSError *error;


Do you know what sort of errors are potentially expected here, and where we should be checking it?

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.

I didn't do anything to this. According to the documentation:

This method throws an error if data isn’t a valid keyed archive.

while the deprecated method throws an exception if data is not a valid archive.

SG - @maksymmalyhin we can refactor after the fact since it's not directly related to watchOS.

ryanwilson · 2020-02-10T17:49:20Z

Hey @hohteri - let me know if you wanted to continue on with this or someone on our side should finish it up! No rush 😄 thanks!

hohteri · 2020-02-10T17:55:41Z

I will finish it up. 👍

ryanwilson

Just a few more things! Thanks again

ryanwilson · 2020-02-11T20:53:57Z

Firebase/Auth/Source/SystemService/FIRAuthStoredUserManager.m

  NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingWithData:data];
+#else
+  NSError *error;


SG - @maksymmalyhin we can refactor after the fact since it's not directly related to watchOS.

Firebase/Auth/Source/Utilities/FIRAuthErrorUtils.h

Firebase/Auth/Source/Utilities/FIRAuthErrorUtils.m

Firebase/Auth/Source/Utilities/FIRAuthInternalErrors.h

Firebase/Auth/Source/SystemService/FIRAuthNotificationManager.m

ryanwilson · 2020-02-11T21:01:55Z

Firebase/Auth/Source/Auth/FIRAuth.m

+#if TARGET_OS_WATCH
+    /*
+     The sign in can't happen fully in-app, i.e. watchOS does not support universal links but watchOS has a mail client that will open this link appropriately
+     in a browser. From the browser you trigger a cloud function which sends a push notification to complete the sign-in.


Is that something manual that you set up, or are you saying that's how it'll work automatically?

Please see: #4621 (comment)

@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).

This looks good to me. Maybe add a link to #4621 (comment) to make it easier to other developers?

Firebase/Auth/Source/Auth/FIRAuth.m

jostster · 2020-03-05T21:39:44Z

Any headway with this PR?

ryanwilson

So sorry for the delay - thanks for your patience on this one. LGTM and just want the Auth team to look at the email link sign in specifically since it's not just a simple SDK change.

ryanwilson · 2020-03-18T18:03:37Z

Firebase/Auth/Source/Auth/FIRAuth.m

+#if TARGET_OS_WATCH
+    /*
+     The sign in can't happen fully in-app, i.e. watchOS does not support universal links but watchOS has a mail client that will open this link appropriately
+     in a browser. From the browser you trigger a cloud function which sends a push notification to complete the sign-in.


@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).

ryanwilson · 2020-04-23T19:41:35Z

Hey @hohteri - hope you're doing well and sorry for the big delay again.

The Auth directory was restructured so this will need a manual merge. If you'd like to do it, please do and we can get this merged as soon as possible afterwards. If you're not willing/able to (no worries at all if that's the case, you've done a lot so far!) then we're more than happy to continue it, keeping your existing commits and fixing the conflicts ourselves.

The one thing I think should be left out for the time being is the email sign in since it requires a cloud function. I'd prefer we evaluate this separately and come up with a more official suggestion for folks if that's okay with you!

Let us know how you'd like to continue with this and I'll try my best to get this looked at quicker :) thanks again!

hohteri · 2020-04-23T19:49:21Z

No worries at all. I’ve been using my own fork meanwhile but I’d assume others would appreciate a quick merge. If you think you can do the required changes before the weekend please do so. I’m not able to look into it this week.

…

On Apr 23, 2020, at 10:41 PM, Ryan Wilson ***@***.***> wrote: Hey @hohteri - hope you're doing well and sorry for the big delay again. The Auth directory was restructured so this will need a manual merge. If you'd like to do it, please do and we can get this merged as soon as possible afterwards. If you're not willing/able to (no worries at all if that's the case, you've done a lot so far!) then we're more than happy to continue it, keeping your existing commits and fixing the conflicts ourselves. The one thing I think should be left out for the time being is the email sign in since it requires a cloud function. I'd prefer we evaluate this separately and come up with a more official suggestion for folks if that's okay with you! Let us know how you'd like to continue with this and I'll try my best to get this looked at quicker :) thanks again! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

ryanwilson · 2020-05-11T13:26:09Z

Just to update here - tried to rebase on master but it ended up quite messy... will give it another go this week.

ryanwilson · 2020-05-12T21:54:03Z

I re-implemented your changes along with some other additions/modifications on master in #5585 - I still need to test it and make sure CI tests are happy

I added you as a co-author on the commit to make sure you got credit but for whatever reason it's not picking up your username. Is there a different email set up with your GitHub account that will help it link? I want to make sure you get credit for it!

hohteri · 2020-05-13T02:44:54Z

I made my email address visible in my profile. Hope it helps!

ryanwilson · 2020-05-13T12:33:27Z

I made my email address visible in my profile. Hope it helps!

That worked, thank you!

This is a redo of #5585, which was a re-implementation of #4632. This ignores any changes to GoogleUtilities, those will be done in a later PR to create a common KeyedArchiver utility. Co-authored-by: Harri Hohteri <[email protected]>

ryanwilson · 2020-08-12T18:23:24Z

Closing in favour of the more up to date #6260, where @hohteri is marked as a co-author.

* watchOS support for FirebaseAuth. This is a redo of #5585, which was a re-implementation of #4632. This ignores any changes to GoogleUtilities, those will be done in a later PR to create a common KeyedArchiver utility. Co-authored-by: Harri Hohteri <[email protected]> * Fixed lint errors, added GHA tests. * Enabled GHA for PRs as well. Co-authored-by: Harri Hohteri <[email protected]>

hohteri and others added 6 commits January 8, 2020 19:12

Auth compiles for watchOS

8f570e2

Fix Auth to have Firestore compiling

59ac3e5

Do not check for open in app for watchOS

d6036af

Fix typo

aaf7cde

Add watchos targets for auth podspecs

29d77cf

Fix styling

3e4a150

googlebot added the cla: yes label Jan 8, 2020

Remove whitespaces

195f67c

paulb777 assigned renkelvin Jan 8, 2020

paulb777 added the api: auth label Jan 8, 2020

ryanwilson requested changes Feb 4, 2020

View reviewed changes

Update based on PR feedback

144e076

hohteri requested a review from ryanwilson February 10, 2020 19:42

ryanwilson reviewed Feb 11, 2020

View reviewed changes

Update based on second round of PR feedback

d770ee3

hohteri requested a review from ryanwilson February 18, 2020 07:13

paulb777 assigned ryanwilson Mar 5, 2020

ryanwilson reviewed Mar 18, 2020

View reviewed changes

ryanwilson mentioned this pull request May 14, 2020

watchOS support for FirebaseAuth #5585

Closed

ryanwilson mentioned this pull request Aug 12, 2020

watchOS support for FirebaseAuth. #6260

Merged

ryanwilson closed this Aug 12, 2020

firebase locked and limited conversation to collaborators Sep 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Auth for watchOS #4632

PR: Auth for watchOS #4632

hohteri commented Jan 8, 2020

renkelvin commented Jan 13, 2020

ryanwilson left a comment

ryanwilson Feb 4, 2020

hohteri Feb 10, 2020

ryanwilson Feb 4, 2020

maksymmalyhin Feb 4, 2020

hohteri Feb 10, 2020 •

edited

Loading

ryanwilson Feb 11, 2020

ryanwilson commented Feb 10, 2020

hohteri commented Feb 10, 2020

ryanwilson left a comment

ryanwilson Feb 11, 2020

ryanwilson Feb 11, 2020

hohteri Feb 15, 2020

ryanwilson Mar 18, 2020

renkelvin Mar 19, 2020

jostster commented Mar 5, 2020

ryanwilson left a comment

ryanwilson Mar 18, 2020

ryanwilson commented Apr 23, 2020

hohteri commented Apr 23, 2020 via email

ryanwilson commented May 11, 2020

ryanwilson commented May 12, 2020

hohteri commented May 13, 2020

ryanwilson commented May 13, 2020

ryanwilson commented Aug 12, 2020

PR: Auth for watchOS #4632

PR: Auth for watchOS #4632

Conversation

hohteri commented Jan 8, 2020

renkelvin commented Jan 13, 2020

ryanwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hohteri Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanwilson commented Feb 10, 2020

hohteri commented Feb 10, 2020

ryanwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jostster commented Mar 5, 2020

ryanwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanwilson commented Apr 23, 2020

hohteri commented Apr 23, 2020 via email

ryanwilson commented May 11, 2020

ryanwilson commented May 12, 2020

hohteri commented May 13, 2020

ryanwilson commented May 13, 2020

ryanwilson commented Aug 12, 2020

hohteri Feb 10, 2020 •

edited

Loading