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

Messaging: deleteToken no longer deletes token correctly #8491

Closed
helenaford opened this issue Aug 4, 2021 · 19 comments · Fixed by #8513
Closed

Messaging: deleteToken no longer deletes token correctly #8491

helenaford opened this issue Aug 4, 2021 · 19 comments · Fixed by #8513
Assignees
Milestone

Comments

@helenaford
Copy link

Step 0: Are you in the right place?

  • For issues or feature requests related to the code in this repository
    file a Github issue.
    • If this is a feature request please use the Feature Request template.
  • For general technical questions, post a question on StackOverflow
    with the firebase tag.
  • For general (non-iOS) Firebase discussion, use the firebase-talk
    google group.
  • For backend issues, console issues, and other non-SDK help that does not fall under one
    of the above categories, reach out to
    Firebase Support.
  • Once you've read this section and determined that your issue is appropriate for
    this repository, please delete this section.

[REQUIRED] Step 1: Describe your environment

  • Xcode version: 12.5
  • Firebase SDK version: 8.5.0
  • Installation method: CocoaPods (select one)
  • Firebase Component: Messaging (Auth, Core, Database, Firestore, Messaging, Storage, etc)

[REQUIRED] Step 2: Describe the problem

FlutterFire: Issue: firebase/flutterfire#6766
RNFB: Issue invertase/react-native-firebase#5570 P/R invertase/react-native-firebase#5571

Steps to reproduce:

Call deleteTokenWithCompletion and inspect the token from tokenWithCompletion and see that it has not changed and is still valid.

From debugging, I can see that when the installations id is refreshed via installations.deleteWithCompletion, the token will always refresh when calling messaging.deleteTokenWithCompletion

This behaviour can be replicated both in FlutterFire and RNFB. My question is what is the correct solution/method to refresh an FCM token?

I've looked through the changelog and the documentation, and it's still not 100% clear to me on how the installations id should be managed in relation to the FCM token.

Any help or guidance on this would be greatly appreciated.

Relevant Code:

By changing the following code, additional requests to deleteTokenWithCompletion will refresh the token:
From:

[[FIRMessaging messaging] deleteTokenWithCompletion:^(NSError *_Nullable error) {
    if (error) {
      [RNFBSharedUtils rejectPromiseWithNSError:reject error:error];
    } else {
      resolve([NSNull null]);
    }
  }];

To:

[[FIRInstallations installations] deleteWithCompletion:^(NSError * _Nullable error) {
    if (error) {
      [RNFBSharedUtils rejectPromiseWithNSError:reject error:error];
    } else {
      resolve([NSNull null]);
    }
  }];
@mikehardy
Copy link
Contributor

More than anything, I am just very eagerly listening here for feedback.

@helenaford to be clear - do you mean that if you never delete installations token, you never get a fresh messaging token? I believe that is clear. However, do you also mean that if you delete installations token once, then you get a fresh messaging token each time you delete messaging token after even if you never delete installations token again ?

I had a hunch that this was - perhaps, maybe - related to old installations prior to the change where Messaging token moved from IID to Installations. However! Testing reports in the react-native-firebase repo indicate this happens with people that have only used react-native-firebase since after that change. Stated differently: This should be reproducible on fresh quickstarts that have never had an installation before.

Separately - I'm curious if using Installations delete vs Messaging delete will have some knock-on side-effect like resetting the in-app-messaging "run only once on this install" triggers. It seems to me that it should have that effect, but is it wanted? Stated differently: are the messaging token and the installations token effectively equal? Should deleting one delete the other? The answer to that might be an opinion vs a technical requirement?

@helenaford
Copy link
Author

helenaford commented Aug 4, 2021

@mikehardy yes, you only have to delete the installations token once, and then it will be ok again. And, yeah it's not about old installations as I can reliably reproduce this issue by deleting the app, and reinstalling it - it resets the state back to the deleteToken not invalidating the token

I actually think it could be todo with the timing of when the Firebase App instance is initialized as I tried deleting the installations id in AppDelegate.m as the app is launching after:

    if ([FIRApp defaultApp] == nil) {
        [FIRApp configure];
    }
    
    // try to delete the installations token here
    [[FIRInstallations installations] deleteWithCompletion.....

with no luck sometimes. So maybe there's a race condition? But it's very confusing because the initial FCM token that is generated works - messages can be sent to the device via firebase-admin.

In terms of the side-effects of deleting the installations id, I was worried about the same thing. I can confirm it doesn't affect the auth token, but not sure about thein-app-messaging token. I'd guess it would change too. I don't see the harm if it did change, also I think most use-cases of deleting a token on the client are when a user wants to log out or delete their account.

@mikehardy
Copy link
Contributor

I'm tracking an unresolved issue with regard to the exact in-app-messaging "run once per device" scenario I mentioned (it's the source of the scenario I used here) - invertase/react-native-firebase#5535 - but the user never reported findings, so it's just an open question to me.

@charlotteliang
Copy link
Contributor

Can you share with us the debugLog? We should figure out why deleteToken is broken.

@mikehardy
Copy link
Contributor

mikehardy commented Aug 4, 2021

@chliangGoogle for the avoidance of doubt, can you specify exactly what you mean by "debugLog" and how exactly to obtain it?

If it is what I think you mean, it is (for example) what you would see if you added -FIRDebugEnabled as a build [edited: runtime, see below] argument in the Xcode build scheme, then launched the app on a real device via Xcode and watched the Xcode console?

(if there was some document that clearly explained the process that would actually be helpful - I only know what I typed above from digging through different documents over the last couple years, yet it is something I need to request from people myself - as I support react-native-firebase - fairly frequently, and I have to type it out each time 😅 - plus I may be getting it wrong)

@charlotteliang
Copy link
Contributor

Yes. To enable debug logging set the following application argument: -FIRDebugEnabled (see http://goo.gl/RfcP7r)

@helenaford
Copy link
Author

helenaford commented Aug 5, 2021

@chliangGoogle thanks I can try and get this information over soon. But, not sure if it will help that much - as it can be repeated in both FlutterFire and RNFB (I don't have a native iOS app I can test with), it seems the issue is within the native iOS SDK itself. If you could also take a look into it internally, it'd be much appreciated.

@charlotteliang
Copy link
Contributor

We are able to reproduce, I'm reaching out to our team for further investigation.

@spundun
Copy link

spundun commented Aug 10, 2021

@chliangGoogle do we know if only a subset of iOS os-versions are affected? Or specific SDK versions are affected?

@charlotteliang
Copy link
Contributor

This is introduced with the big refactor in an effort to remove instanceID dependency in I/O release v8.0.0

@spundun
Copy link

spundun commented Aug 10, 2021

IIUC, this bug is triggered under the following conditions. The conditions itself seem rare in production but common during manual QA testing of an app. @chliangGoogle correct me if I'm wrong.

  1. Using SDK v8.0.0 - v 8.6.0 (2021-04 - 2021-08)
  2. Re-install app and launch app
  3. race-condition triggered, authService(used by deleteToken... api) cached old checkin-credentials with some probability
  4. deleteToken... is called.

Now if any event between 3 and 4 resets the cache in authService class then this bug goes away(on top of the probabilistic setting of the stale cache in the first place).

Nor sure about all the triggers to reset that cache but I presume the cache is deleted from the memory as the user shuffles through apps, right? Also, I expect the bug to go away if the phone restarts.

Also, I am assuming that typically there will be a large time window(days) between the reinstall and deleteToken events.

@paulb777 paulb777 added this to the 8.6.0 - M102 milestone Aug 10, 2021
@spundun
Copy link

spundun commented Aug 11, 2021

@helenaford / @mikehardy (or any other developer stumbling on this) could you share your use cases for using delete token? I'm trying to understand whether this API is serving the use-cases the developers expect it to serve, beyond the scope of just this bug, more generally. Feel free to tag me.

@helenaford
Copy link
Author

@spundun most use-cases of deleting a token on the client are when a user wants to log out or delete their account.

@mikehardy
Copy link
Contributor

Exactly that.

If you have an app that registers the token for use in user-specific notifications, then you should as a best practice stop the flow of notifications to that token when a user logs out of the app, so that messages for the user are never delivered again

Yes on the backend the token shouldn't be used any more either so there are related processes that should occur to unsubscribe from topics and remove the token from any user correlations, but any of those may lose a race or fail to happen if the user is offline etc.

Token delete is local and should work every single time without fail, no?

@spundun
Copy link

spundun commented Aug 13, 2021

Thanks @helenaford and @mikehardy . I suspected that would be the expected use case.

I'd recommend you use the "deleting the installation id" flow that @helenaford posted above for that purpose.

I'd love to know if that flow doesn't address your use case. Please feel free to reply back with any concerns or other use cases you might think of where you'd want to use deleteToken.

Personally, I'd like us to reconsider if it makes sense at all to have this deleteToken api. I think it adds a lot of confusion for the developer and seems to be adding unnecessary complexity to the API and the system.

@mikehardy
Copy link
Contributor

There was some concern above that messaging and in-app-messaging would share the installation ID, and there may be a case where a user only wants to pop up a FIAM once per install, but supports multiple users and wants to keep FCM user targeted.

That was our hesitation on deleting installation token vs messaging

But perhaps they are the same token already? Insight there very appreciated

@charlotteliang
Copy link
Contributor

@mikehardy the token is only used by Messaging, in-app-messaging does NOT use token and only the installations ID, and even if you delete the installation ID, the new one should function as expected for in-app-messaging continue on. So you should be fine.

@mikehardy
Copy link
Contributor

Ironically, the firebase-android-sdk documents their Installations.delete API with:

This call may cause Firebase Cloud Messaging, Firebase Remote Config, Firebase Predictions, or Firebase In-App Messaging to not function properly.

https://firebase.google.com/docs/reference/android/com/google/firebase/installations/FirebaseInstallations#public-taskvoid-delete

That implies something much more serious, and I guess I'm really surprised that iOS doesn't have similar side effects given that this deletes the installation on the backend completely since it is also indicated to be useful for GDPR compliance:

https://firebase.google.com/docs/reference/android/com/google/firebase/installations/FirebaseInstallations

provides a API to perform GDPR-compliant deletion of a Firebase installation.

I guess I'm just confused how we can still have all the non-messaging items behave correctly for an app installation if we delete the entire installation id/token vs just making sure that messages are no longer targeted at perhaps the wrong user 🤔

@charlotteliang
Copy link
Contributor

@mikehardy I think the communication was misunderstood. What I meant is when you delete installations and get a new installation ID and new FCM token, then everything should function as expected. I was under the impression this was what you were asking. This helps you to resolve the issue and reset the state to back to normal.

If you deleted the installations and not getting a new installations ID and a FCM token, backend does a GDPR-compliant deletion and you should not expect anything is functional because the info has been wiped clean.

@firebase firebase locked and limited conversation to collaborators Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.