-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
🔥 Access to FIRAuthErrorUserInfoUpdatedCredentialKey with Apple Sign In #3952
Comments
Interesting, I may have misunderstood the nature of the problem previously - frequently happens / always learning - and it looks like what's happening is that the native SDKs are providing an updated credential which you could use for a new call (as it will have an updated nonce) but it is (perhaps?) not passed through all the layers so you don't have access to it, so you can't use it from react-native-firebase? If that understanding is correct, can you imagine a patch that would fix it? Is likely a small change to the iOS code here to pass things through I think this is the code hit when link-with-credential blows up: https://github.com/invertase/react-native-firebase/blob/master/packages/auth/ios/RNFBAuth/RNFBAuthModule.m#L799 which delegates to react-native-firebase/packages/auth/ios/RNFBAuth/RNFBAuthModule.m Lines 971 to 978 in 4f15c83
which *uses this to extract parts: react-native-firebase/packages/auth/ios/RNFBAuth/RNFBAuthModule.m Lines 980 to 1045 in 4f15c83
which sends the extracted bits to react-native-firebase/packages/app/ios/RNFBApp/RNFBSharedUtils.m Lines 99 to 102 in 4f15c83
I think it's the "extract parts" bit. If you could check the NSError* in getJSError to see if it has the credential thing (I think it's an NSString* ?) you might be able to just add 'userInfoUpdatedCredentialKey' as a thing that gets passed around I took the time to find the things I think need to be changed but the hard part is not the change, it's the testing :-), and no one is more motivated then the reporter in my experience, so hopefully this is enough to get the basic code / string passing plumbed in then you can check it and see success? Then I strongly recommend https://github.com/ds300/patch-package to persist the change so you stay functional, and propose a PR (or just post the patch maybe) to fix it for good? |
Hey thanks! That's exactly what's going on. Let me see if I can get the error from native back to JSVille. |
After a few attempts, it looks like the credential is
I will check with the latest. |
Same with the latest unfortunately. I'll see if I can figure something else out as to why this is |
That's unexpected - perhaps you can set a breakpoint in Xcode on the actual firebase-ios-sdk return to inspect everything right there? Or have already tried that? At least this is an easy case to reproduce, but what a pain. |
Has there been any progress on this at all? Have spent the past few hours trying to resolve this by playing with the source but I'm not experienced in anyway in bridging so haven't had any luck. We currently have to display Apple Sign In twice to upgrade anon users so would appreciate a fix. |
I believe you @AppKidd and @timothystewart6 are the only interested parties at the moment. I'm still surprised that this isn't bridgeable based on the upstream firebase-ios-sdk issue. Your direct experience is obviously valuable as you're playing with the code but it seemed that the Obj-C should have the value coming back at some point, on their return object 🤔 |
Thanks @mikehardy. I'll try and have another look later this week. |
Hey, |
@One-Q Hey, sadly not. I spent a bit more time on it but I'm just not knowledgable enough in Objective-C to fully trace where the issue lies. Would really appreciate it if we could get a fix for this @mikehardy. |
I appreciate the effort truly - I think we're close here. My Obj-C isn't fantastic either, perhaps @Salakar has someone with better skills that can pick it up when they scan the queue for task distribution? If not I can give it a try but it isn't at the top of my queue at the moment (apologies) |
There's a free coffee on me for the person that gets to the bottom of this ☕🤓 |
Hmmm if There's also another secondary bug here which should probably also be fixed at the same time, the error code is coming through as |
firebase/firebase-ios-sdk#4434 (comment) was reported success, which gave me hope. Perhaps false hope, undetermined without a look I think |
thank you everyone! |
Hello 👋, to help manage issues we automatically close stale issues.
|
Hi @Salakar, are we any closer to seeing a fix for this in react-native-firebase? My understanding at the time was that the iOS SDK was exposing |
@AppKidd my understanding from the comments above was that it should be doing that but the people that investigated did not see it on the native side, so there's nothing to bridge. We have no positive confirmation here (yet?) that anyone was able to see the desired property from the firebase-ios-sdk |
I'm seeing a similar problem. In my case, I have already authenticated my user through Google, and when I try Facebook, I'm (expectedly) getting the duplicate credential error. But, as reported above, the conflicting email and credentials aren't being sent back in the userInfo object as part of the error response. It looks like the values are actually in the error response down in |
I have the same problem. I have absolutely no knowledge of ObjectiveC, but inserted a little log into
It seems that updated credential is not null (anymore)! Would be awesome, if this could be passed to JS! |
You join me in the "I suck at Objective-C" club then :-), no offense intended, but truthful for me at least. That was the first thing we needed I think - just confirmation it existed since a previous result was negative. If anyone posts a PR here that actually knows Objective-C 😅 I will happily merge. I think it will just be one layer up, in the promiseRejectAuthException method a value maybe just copied from one object to another? (then documented) |
My Objective-C is also pretty poor, but I know the credential could be added. I started digging into this, then realized that my JavaScript also isn't as good as it should be either! When you console.log() the error, you don't see all the fields. I figured out that some of them are actually present, you just need to know where to look for them. I was able to work around my specific issue (i.e. needing the duplicate credential email back), as I already had the email available. So, I have Google and Facebook working properly. I'm next adding Apple, so I figured I'd hit this issue head-on and have to resolve it one way or another. I'll post my results soon...thats my next task. :-) |
Okay, I digged further into this. I tried to add the I took a look how the flutter guys are solving this, and it seems that instead of passing the hole credential, they
Any thoughts on that? Edit: I implemented this solutions for test purposes and it seems to work! |
@henrik-d haha "the flutter guys" are also Invertase, who created this but since FlutterFire is newer it gets shiny new code where as react-native-firebase was coded up when this specific underlying stuff didn't exist. Which is to say: if it works in FlutterFire as a strategy it should work perfectly here, and if you have a coded solution that is fantastic! Post up a PR and I'll get my button-clicking finger ready to hit the merge button :-) |
@mikehardy Hehe, okay I didn't know that ;) I'm now thinking about the android side. We don't use Apple-Auth on Android in our project, but it should be the same issue there, shouldn't it? I also took a look into the tests, but it seems that social providers aren't tested (I assume because it would require heavy mocking)? |
Unsure on testing @henrik-d but I understand if there is no scaffolding built up for social auth testing that the burden of adding testing is quite high, I can accept a PR without coverage if you attest you've tested it (note the PR has an action that generates patches now, so for every PR you get a downloadable zip of patch-package format patches for testing - makes it easy!). Unsure on the Android side, I would assume it's the same, but similar to the testing, I won't make perfect the enemy of good. The react-native-apple-authentication library (which also Invertase created and maintained by me 😄 ) only just added Android support a week or two ago which shows you how important it is versus Apple (almost unimportant). The summary being: if you just PR an ios-only fix without test cover but that you have tested personally, I'll happily merge it :-) |
This is fixed on iOS now thanks to @henrik-d 🏆 - if anyone wants to take it on for Android please feel free. I merged the single-platform fix because I think Apple Sign On is much much much more prevalent as iOS only so it had standalone value |
Thanks @henrik-d @mikehardy - glad to see this is fixed. 👍 |
@mikehardy Thanks for your kind support. For sake of completeness, here is how I use it now:
|
Issue
Hello. I am integrating Apple Sign in now. It works great except for one small piece. It's the flow from Firebase Anonymous -> Link User -> User already exists in Firebase -> Create new account.
When calling the code below and the firebase user already exists, the call fails which I think is expected because I am trying to reuse the nonce that was used in the first call to link accounts. However I thought I should get an updated credential in the error response.
Error
I can successfully sign in with Apple if I sign in again, however the user need to authenticate to Apple again.
I found this issue and it looks the native side should return an updated credential key in this scenario that I can use on my next call without having to reauth with Apple.
error.userInfo[FIRAuthErrorUserInfoUpdatedCredentialKey]
returned in the error. Currentlyerror.userInfo
does not return this key. This key would prevent another request to sign in.firebase/firebase-ios-sdk#4434
I see you closed a similar issue but I think firebase has implemented the native side.
Thank you for your consideration.
Project Files
Javascript
Click To Expand
package.json
:(also saw the same issue on the latest version)
iOS
Click To Expand
ios/Podfile
:# N/A
AppDelegate.m
:// N/A
Android
Click To Expand
Have you converted to AndroidX?
android/gradle.settings
jetifier=true
for Android compatibility?jetifier
for react-native compatibility?android/build.gradle
:// N/A
android/app/build.gradle
:// N/A
android/settings.gradle
:// N/A
MainApplication.java
:// N/A
AndroidManifest.xml
:<!-- N/A -->
Environment
Click To Expand
react-native info
output:The text was updated successfully, but these errors were encountered: