firebase / firebase-ios-sdk

Firebase SDK for Apple App Development
https://firebase.google.com
Apache License 2.0
5.63k stars 1.48k forks source link

Share auth state between app and app extension on iOS sometimes fails #8445

Closed unxavi closed 9 months ago

unxavi commented 3 years ago

Step 1: Describe your environment

Step 2: Describe the problem

We are using Firebase Auth on an iOS app that uses an App Extension. I followed the documentation here https://firebase.google.com/docs/auth/ios/single-sign-on#share_auth_state_between_apps

The documentation states:

You can use either a keychain access group or an app group.

Since I'm already using app group for other things (subscription status for example) I decide to go with app groups.

Everything works fine, we can retrieve the user on the extension. I did not activate the entitlemente for Keychain Sharing, only for app group.

But I'm seeing so far for 2 users that we get this error:

Fatal error: Error changing user access group An error occurred when accessing the keychain. The NSLocalizedFailureReasonErrorKey field in the NSError.userInfo dictionary will contain more information about the error encountered

On:

The rest of the devices are working normally

Steps to reproduce:

What happened? How can we make the problem occur?

When the app starts, we run this code at the beginning, at this point the extension is not running yet:

if FirebaseApp.app() == nil {
    FirebaseApp.configure()
}
do {
    try Auth.auth().useUserAccessGroup(appGroup)
} catch {
    let message = "Error changing user access group \(error.localizedDescription)"
    fatalError(message)
}

The variable appGroup contains a String on the form group.APP-BUNDLE-ID as this is recommeded by Apple for iOS apps. Source https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_security_application-groups

For iOS, format the identifier as follows:

group.

App groups also act as keychain access groups.

At this point, all is normal, we are not receiving any crash.

Later on the user sign-in in the app.

Finally the user enable the extension, at this point we run the following code:

if FirebaseApp.app() == nil {
    FirebaseApp.configure()
}
do {
    try Auth.auth().useUserAccessGroup(appGroup)
    Crashlytics.crashlytics().setUserID(Auth.auth().currentUser?.uid ?? "N/A")
} catch {
    let message = "Error changing user access group \(error.localizedDescription)"
    fatalError(message)
}

Is that this point where the fatalError triggers the error, but not always, only for this 2 users.

I'm throwing a fatalError because I need the user to be authenticated and I need to share the auth data. I guess there is nothing wrong with that.

With all this information, I've got a couple of questions:

From Apple docs I get the when using app groups, we are also using keychain sharing, so we shouldn't need keychain sharing also https://developer.apple.com/documentation/security/keychain_services/keychain_items/sharing_access_to_keychain_items_among_a_collection_of_apps

Starting in iOS 8, when an app belongs to an app group, it can also use this mechanism to share keychain items.

using an app group enables additional data sharing beyond keychain items. You might want this extra sharing, or might already be using an app group for this purpose, and thus not need to add keychain access groups.

Like I said, I can't reproduce this every time, I have never been able to reproduce it myself. Anything that I can help you, let me know. I can only see on crashlytics that the fatalError is happening on the app extension for 2 user, which I don't know if they are able to recover or not. (Not sure how I could know)

morganchen12 commented 3 years ago

Are you able to print the full error, including the userInfo?

unxavi commented 3 years ago

@morganchen12 thanks a lot for your answer. uhmm I guess no, all I've is that log from my fatalError(message) which logs the error localized description (maybe this is not the best?). Which then gets logged into Crashlytics.

Would you like that I change my code to: try! Auth.auth().useUserAccessGroup(appGroup) and see what gets logged to crashlytics? do you have any other code suggestion that could help us debug more this error?

I'm not sure what you mean with userInfo I don't see this field anywhere. I'm opening to change my code to see if we can get more info.

morganchen12 commented 3 years ago

You should be able to change the message line to

let message = "Error changing user access group \(error)"

and it will include all of the error details.

unxavi commented 3 years ago

@morganchen12 thanks again, I will go ahead and change it for next release since I can't reproduce it on my devices. It will take some days to make the release and get apple approval and finally rollout. If you can keep this issue open I'll come back when I get more details.

Thanks for your help

unxavi commented 3 years ago

@morganchen12 fiinally I can get back to you with the Information.

Fatal error: Error changing user access group to group.APP-BUNDLE-ID error: Error Domain=FIRAuthErrorDomain Code=17995 "An error occurred when accessing the keychain. The NSLocalizedFailureReasonErrorKey field in the NSError.userInfo dictionary will contain more information about the error encountered" UserInfo={FIRAuthErrorUserInfoNameKey=ERROR_KEYCHAIN_ERROR, NSLocalizedFailureReason=SecItemCopyMatching (-25308), NSLocalizedDescription=An error occurred when accessing the keychain. The NSLocalizedFailureReasonErrorKey field in the NSError.userInfo dictionary will contain more information about the error encountered}

Just in case it helps, these are new installations, with Firebase 8.4.0. It doesn't happen to every user, happens around to 8% of the users. The group ID that I get on the logs seems correct and I have the entitlement on the project for app groups.

How do you see?

morganchen12 commented 3 years ago

Thanks @unxavi. The OSStatus of the error you've posted, -25308, is errSecInteractionNotAllowed, and is most likely caused by the extension not having access to your app's keychain or trying to access the keychain when the device is locked (under certain keychain protection levels). See this Apple developer thread for more details.

It's not immediately clear to me the best way of resolving this issue, but sharing the app and extension keychain seems like a good start.

unxavi commented 3 years ago

@morganchen12 thanks for your answer, I have been digging myself and I believe is device lock, because I've the feeling that this devices recover over time and are able to init correctly.

I'm not sure either how we could address this problem. About your comment:

sharing the app and extension keychain seems like a good start.

Isn't that what we are already doing with the share app group? I believe that we are sharing the keychain, or are you saying to drop app group and go only with keychain sharing entitlement? I believe we could face the same.

Or is it possible that you are suggesting another thing?

Thanks a lot.

unxavi commented 3 years ago

@morganchen12 is also my understanding that Firebase Auth uses kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly and that would mean that in order for the keychain to return this error, the device would need to rebooted and not unlocked while the extension is initializing, right? after the first unlock this should not happen?

Follow up question, is kSecAttrAccessible value something that we can choose?

Thanks for your time.

morganchen12 commented 3 years ago

You're correct, the app group should take care of keychain access between the app and extension. Firebase sets the accessibility attribute here, and I'm not sure if it can be overridden or manually set to another value.

If you need the app extension to be able to access auth state from the user access group, your best bet for now while we evaluate solutions on the SDK side is to fork Firebase. Otherwise, you can handle specifically the -25308 error and gracefully retry when the device unlocks instead of calling fatalError.

unxavi commented 3 years ago

@morganchen12 thanks for your answer again. I feel like missing a piece of the puzzle.

So far, Firebase Auth uses kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly so once the device is unlocked the first time after a reboot the error errSecInteractionNotAllowed should appear.

My app extension is a Network extension NEPacketTunnelProvider which on my test never runs before the device is unlock. I've been testing and I'm not able to reproduce this error.

So I'm not finding how could it be possible to have this locked keychain if it unlocks after the first time the device is unlocked and the extension does not runs after the device is unlocked. 🤯

morganchen12 commented 3 years ago

Unfortunately I don't have a good answer for your last question, though I agree it's very confusing behavior. Extensions are not guaranteed to wait until the first unlock before running--for example, notification extensions will run whenever a notification arrives, which may be before the device is first unlocked for certain kinds of notifications. You may find more success asking on the Apple developer forums or StackOverflow.

You may also be able to derive some more insights through creative crash breadcrumb usage.

unxavi commented 3 years ago

@morganchen12 I'm thinking on to handle the -25308 but I got a couple of questions:

  1. What would be the effect when try Auth.auth().useUserAccessGroup(appGroup) fails? That Auth.auth().currentUser would return nil? Something else? To help identify when the UserAccessGroup has fail.

you said:

gracefully retry when the device unlocks

Do you know if we can get a callback to this? I mean, do we know when the device has been unlock?

Thanks again. Hope you have a good weekl

unxavi commented 3 years ago

maybe a better question would be, how do I know for sure that I need to retry without holding the value on a flag or something like that

morganchen12 commented 3 years ago

@rosalyntan can probably better answer your first question.

There's no callback for when the device unlocks. A flow that might work:

I'm not sure there's a good way to do this without writing a flag somewhere.

unxavi commented 3 years ago

@morganchen12 @rosalyntan I've done the following, maybe is wrong:

How does that sound?

morganchen12 commented 3 years ago

That sounds fine. You may end up retrying more often than the approach I mentioned, but extra retries should not cause any significant issues.

paulb777 commented 9 months ago

Closing for staleness. Please comment or open a new issue to continue the discussion.