Skip to content
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

[Feature Request] Sign In With Apple #2884

Closed
harrisrobin opened this issue Nov 17, 2019 · 37 comments · Fixed by #2979
Closed

[Feature Request] Sign In With Apple #2884

harrisrobin opened this issue Nov 17, 2019 · 37 comments · Fixed by #2979

Comments

@harrisrobin
Copy link

Looks like Firebase now supports sign in with apple:
image

Would be great to support that here and update the docs! Let me know if there's anything we can do to help 😄

@Salakar
Copy link
Member

Salakar commented Nov 17, 2019

This is in the pipeline 👍 but will have to be done as part of v7.0.0, the OAuth flow needs a full rework as theres been several breaking changes and our implementation is now quite far behind the Web SDK. #2673 is a part of it, but there still a few bits to do.

v7.0.0 though won't be far away like v6 was, hopefully end of this year/early Q1 2020.

@seantimm
Copy link

Looking forward to this! The deadline for inclusion (from Apple) is April 2020: https://developer.apple.com/news/?id=09122019b

@mcjohnalds
Copy link

The deadline for existing apps is April 2020. Apple won't accept any new apps lacking sign in with Apple 😢

@mikehardy
Copy link
Collaborator

April is luckily still a fair bit away, that said I'm just now expanding my work project's login to other providers so I'm listening in on this if you need any testing etc - esp. if the modules ship source so you can depend on a commit hash and just do a build postinstall I can test whatever when this comes through

@mikehardy
Copy link
Collaborator

mikehardy commented Nov 22, 2019

Oh! I think I misunderstood you @mcjohnalds - do you mean to say it is already not possible to create a new app on the App Store with federated login that does not have apple? And it's just existing apps grandfathered to 2020?

Edited to add - yep - new apps already not allowed - ouch! from the link above

Starting today, new apps submitted to the App Store must follow these guidelines. Existing apps and app updates must follow them by April 2020.

Also edited to add, the loophole is you must also allow email auth, I believe, so you are not exclusively using third-party/social login:

Apps that exclusively use a third-party or social login service (such as Facebook Login, Google Sign-In, Sign in with Twitter, Sign In with LinkedIn, Login with Amazon, or WeChat Login) to set up or authenticate the user’s primary account with the app must also offer Sign in with Apple as an equivalent option.

@mbizplus
Copy link

@mikehardy yep - new apps already not allowed
My app rejected

Screen Shot 2019-11-26 at 10 11 49 PM

@mikehardy
Copy link
Collaborator

So, what happens when you enable email authentication? According to the text block in their document you would be allowed:

Apps that exclusively use a third-party or social login service (such as Facebook Login, Google Sign-In, Sign in with Twitter, Sign In with LinkedIn, Login with Amazon, or WeChat Login) to set up or authenticate the user’s primary account with the app must also offer Sign in with Apple as an equivalent option.

@HeribertoAlves
Copy link

HeribertoAlves commented Nov 26, 2019

@mikehardy My app uses email authentication and was rejected.

edit: Mine offers phone authentication too.

@mikehardy
Copy link
Collaborator

mikehardy commented Nov 26, 2019

@HeribertoAlves that is terrible! Am I just being dense in my reading of their own rules? I would appeal immediately with that passage quoted and state explicitly that your backend provider still has the integration listed as beta, and you can't rely on beta software. I could rant (uselessly of course) on the app store gatekeeper monopoly thing but the text seems so clear to me here.

Please note that my intention here is constructive: I am literally about to attempt this same approval process. So I might be denied as well and I'm motivated to fix it but will likely need help. Who on this thread has Obj-C or Android or OAuth skills? I'm strong on Android but not on ObjC. OAuth is relatively new for me but the underlying APIs exist anyway so should be possible, and there's an existing PR to base off of to get the feature done and acceptable for merge if Apple won't approve for now.

@nhahn
Copy link

nhahn commented Nov 28, 2019

I have had a similar issue with getting my app to past review from Apple -- even when adding in email and phone authentication they still rejected it because it had "3rd party auth providers without login with apple". I monkey-patched together a working version of expo-apple-authentication and @react-native-firebase/auth that you can find here: https://gist.github.com/nhahn/010305dcff1404bfdeb6e2174b74e5a6

You can apply the patches to your code using patch-package and it should hopefully work

@mikehardy
Copy link
Collaborator

@nhahn I just read your gist, the expo module source, and the apple website related doc - that looks viable - fantastic - a couple questions if you have time?

  • is there any traction for adding the nonce patch to expo-apple-signin? I didn't see it raised in an issue or PR and it doesn't seem to be in their source at all. Is this something no one else will need?

  • does this only work on iOS? I will know the answer to this as soon as I actually try it, I just only see iOS code in the expo-apple-signin repo

@mikehardy
Copy link
Collaborator

Reading even more deeply: seems supported at the native level (i.e., via expo-apple-signin) only for iOS and only iOS13+ even: docs say those symbols used in expo-apple-signin are only for iOS13+ https://developer.apple.com/documentation/authenticationservices/asauthorizationcontroller

But I don't see in your example App.tsx that you are checking platform or whether it's available via https://github.com/expo/expo/blob/master/docs/pages/versions/unversioned/sdk/apple-authentication.md#appleauthenticationisavailableasync

mostly I'm just trying to confirm if my understanding is correct

If so, this is a great way to get react-native-firebase apps that use social auth through the App Store Review, but we'll need to expand it a bit to be universal (e.g., by back-filling Android + ios<13 support via some other mechanism)

@nhahn
Copy link

nhahn commented Nov 28, 2019

  1. I haven't even tried to raise / submit an issue for the nonce on the expo-apple-signin repo. I'll probably go ahead and do that -- since it was a relatively simple change. I'm on holiday right now, and really just came up with this as a stop gap so I can get my app on the store.

  2. Yes -- this only works on iOS. The challenge with Android is the flow ends up being significantly different. I took a look at the Firebase Android Apple Auth guide, and while in our case we're getting the auth key and passing it along, their code recommends letting Firebase handle all of that (getting key -> passing it to firebase -> authing) using startActivityForSignInWithProvider. So that's definitely challenge / change from what the current state of affairs is in the Firebase auth package.

@mikehardy
Copy link
Collaborator

ok - well I am understanding it then - that's an obvious critical basis for moving forward :-). Thanks for replying on holiday!

Agreed on android flow being radically different, that was my take as well

This seems fine then to start, and via separate channel (the react-native-community Discord server) I pinged @brentvatne to see if he was interested in the nonce stuff as an expo-apple-signin patch, he'll respond here or there I'm sure and we'll see. Cheers!

@mikehardy
Copy link
Collaborator

I also pinged @Salakar via react-native-firebase Discord chat server to check the gist and see what he thought. I'll PR here in react-native-firebase repo on v5 and v6 branch pending feedback. I don't like making changes to v5 but my commitment there is to make sure the branch is still useful, and I'm still on v5 and implementing social auth now so I need it :-)

@mikehardy
Copy link
Collaborator

I have chatted with @brentvatne and:

  • I have a confirmed understanding of expo-apple-signin - It only does iOS and only iOS13+ - that is the strict requirement and it was urgent to make this work, so it only implements strict requirement at the moment. Should be fine by Apple

  • He's going to make a change where if you pass a nonce in, it will be carried through. But he wants it passed in vs generated in expo-apple-signin. Fair enough, it just moves the division of labor a little bit. For Expo things they have a lib function already, for react-native-firebase we'll generate a nonce and send it

This should be propposed functionality soon, I'll link related PRs here once expo-apple-signin has the change needed to build it on this side and test and @Salakar or @Ehesp has a look at the gist to make sure the RNFB part doesn't have a major issue

Cheers!

tsapeta pushed a commit to expo/expo that referenced this issue Nov 29, 2019
# Why

invertase/react-native-firebase#2884

# How

Based off of diff from https://gist.github.com/nhahn/010305dcff1404bfdeb6e2174b74e5a6 but less opinionated about the format of the nonce - we just pass the string along to the request.

# Test Plan

User has tested diff provided. I did not test this code.
@jonstuebe
Copy link

@mikehardy is there any way to use expo-apple-signin without the unimodules? Seems like a whole lot of extra bloatware for no reason (no offense to the expo team, they do great work 😁).

@mikehardy
Copy link
Collaborator

mikehardy commented Dec 1, 2019

@mikehardy is there any way to use expo-apple-signin without the unimodules? Seems like a whole lot of extra bloatware for no reason (no offense to the expo team, they do great work 😁).

Literally struggling with this same thing myself, existentially, while also having respect for the expo team. I just don't have unimodules yet and do I want to add it? Not particularly as I'm not sure how it's going to affect my plist with permissions etc (I think they merged a change for that but it's just not on my radar really whereas I've already perfected my react-native-permissions usage.)

So, basically no, not without a fork. I'm going to personally use it just for now because I need to launch social auth for iOS, but I also need a cross-platform apple signin solution and expo-apple-signin isn't that whereas an internal react-native-firebase implementation - using the firebase SDK APIs which are pretty rich for apple auth on android - could work. I'd add horsepower to that once the time is right on react-native-firebase v6 (a week or three?) but need to move now. Pragmatism with an eye towards the future.

@mikehardy
Copy link
Collaborator

@jonstuebe if I'm not mistaken, you tried a different more focused library (I see you github-forked it anyway). And it looked tempting. Any results to report? I have expo-apple-authentication more or less integrated but it's beastly and requires me to carry like 20 new patch-package patches.

@elirangoshen
Copy link

so only login in with email and password is ok if i get it right ? all the rest would get rejected?

@mikehardy
Copy link
Collaborator

Based on the other replies yes. I almost have a version 2 of the expo-apple version working locally (changed to conform to final form of the expo-apple PR + w/typescript support in react-native-firebase)

There will be a standalone react-native-firebase compatible version shortly as well. This is the current "stop work, do this" priority for me. I'll post a gist of what I'm using shortly, based on the gist from @nhahn

@mikehardy
Copy link
Collaborator

mikehardy commented Dec 3, 2019

For anyone following along, I updated the react-native-firebase@5.5.6 patch in my gist https://gist.github.com/mikehardy/83c9535d71cec4a8764bfda5a492c25f - I had neglected to carry the nonce-related changes to the android native side but when I went to test that today obviously it failed. I now have this submitted to the App Store, we'll see. Passes all my testing android+iOS at least.

@brentvatne
Copy link
Contributor

@Salakar - i was just joking, firebase is great even though it does tons of stuff :P that's one of the selling features!

@mikehardy - appreciate the feedback. we're going to look at documenting the minimal unimodule setup and making the minimal setup requirements as lean as possible for people that are more hesitant to install react-native-unimodules itself in their project.

@mikehardy
Copy link
Collaborator

App based on my gist is Apple approved as of today, with Facebook Google and Apple sign in. Share and enjoy until there is a formal release with the necessary changes

@Salakar Salakar modified the milestones: v7.0.0, v6.2.0 Dec 4, 2019
@Salakar
Copy link
Member

Salakar commented Dec 5, 2019

Hey everyone, we've just released invertase/react-native-apple-authentication for this, its up to the usual standard for our libraries.

The feature still needs implementing in RN Firebase Auth, which will be done this week for a v6.2.0 release (we can do this without any breaking changes and will revist the API in v7), but if you want to try it out now for v6 then I've made an example here with a v6.1.0 patch file included;

If you want to remain with the expo one + unimodules then that also works too, our one is aimed at being a pure React Native module with no added extras as we don't use unimodules. If you're using unimodules already for other stuff then it probably makes sense to stick with it.

@Salakar
Copy link
Member

Salakar commented Dec 8, 2019

Hey all, v6.2.0 release is out with this in. I've also written up a guide for integrating this with @invertase/react-native-apple-authentication here.

@MorgadoKnivet
Copy link

MorgadoKnivet commented Dec 8, 2019

@Salakar I have a problem.
[auth/invalid-credential] The supplied auth credential is malformed or has expired.

More details...

#2979 (comment)

I didn't know where to put the error, link here, sorry about anything.

@mikehardy
Copy link
Collaborator

mikehardy commented Dec 8, 2019

Still polishing up my work project before I make a PR to react-native-firebase v5.x.x but I confirm that if you use the 'react-native-firebase@5.5.6' patch from my gist above (and linked here https://gist.github.com/mikehardy/83c9535d71cec4a8764bfda5a492c25f) it integrates fine with @invertase/react-native-apple-authentication by basically just changing the symbol names and removing the sha256 stuff, instead just using the 'nonce' property that comes back on the credential returned from apple sign-in

The PR will actually even be simpler as @Salakar shows in his v6 support released in 6.2.0 that instead of plumbing the nonce all the way through I can just use the otherwise unused 'secret' field: https://github.com/invertase/react-native-firebase/pull/2979/files

@mikehardy
Copy link
Collaborator

My PR for the v5.x.x branch is up as #2980 - our plan is to merge it and actually do a release. It will likely be a v5.6.0 because it requires react-native 0.60 and higher in order to get Xcode 11 support, which is how iOS 13 is supported. That's a hard requirement for apple sign-in so if you want it, there's no way around it.

In the meantime people can depend on this commit hash if they want it in their projects, and just follow the firebase instructions from invertase/react-native-apple-authentication (with only change being a different way to import firebase for v5 vs v6) and it works

I won't be commenting here anymore, everything's out there to make it work I believe, and you can subscribe in the relevant places if you need status

package.json dependency until merged/published:

    "react-native-firebase": "git+https://github.com/mikehardy/react-native-firebase.git#c3d73b95892b9d65b485e141780e9d824c3730f6",

@damandeepsingh93
Copy link

Is this done for Android as well ?

@mikehardy
Copy link
Collaborator

mikehardy commented Jan 30, 2020

@damandeepsingh93 no it is not. The two solutions I'm aware of (expo or invertase apple auth for react native) are both iOS only. In my experience (my own app's user base) there is almost no value in implementing it at all (except Apple says you have to), and for android in particular zero, so I am certainly not motivated at least, vs other things on my roadmap. Any support would have to come from the community as PRs

@skizzo
Copy link

skizzo commented Apr 19, 2020

Since I can't get react-native-firebase v6 to work properly no matter what I try, I'll give @mikehardy 's solution a shot. Thanks!

@mikehardy
Copy link
Collaborator

@skizzo it's all merged in now, works pretty much out of the box with RNFBv5.6.x and the invertase react-native-apple-authentication package, it's what I'm still using (haven't had the time to upgrade to RNFBv6 yet 😮 )

@skizzo
Copy link

skizzo commented Apr 19, 2020

It does indeed work with v5.6.x, thanks for your hints!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment