Page MenuHomePhabricator

[Tunnelbroker] invalidate bad FCM tokens
ClosedPublic

Authored by kamil on Jul 31 2024, 2:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 10:26 AM
Unknown Object (File)
Thu, Dec 19, 10:26 AM
Unknown Object (File)
Thu, Dec 19, 10:25 AM
Unknown Object (File)
Thu, Dec 19, 10:25 AM
Unknown Object (File)
Fri, Dec 6, 7:41 PM
Unknown Object (File)
Fri, Nov 29, 6:13 AM
Unknown Object (File)
Fri, Nov 29, 12:08 AM
Unknown Object (File)
Thu, Nov 28, 11:11 PM
Subscribers

Details

Summary

ENG-8498.
ENG-8824.

Keyserver code is here and here.

This is implemented according to the Detect invalid token responses from the FCM backend section in the Firebase docs.

Unfortunately, for INVALID_ARGUMENT there are no detailed docs of how the error might look like, the only information is a description of each ErrorCode. This is a common issue with Firebase, see e.g. here.

We had two options:

  1. Invalidate token for all INVALID_ARGUMENT errors - this error could be caused by multiple things e.g., a badly created HTTP request can invalidate the token.
  2. Check if the error has a message about an invalid registration token (this is a bit experimental).

The returned error looks like this:

{
  "error": {
    "code": 400,
    "message": "The registration token is not a valid FCM registration token",
    "status": "INVALID_ARGUMENT",
    "details": [
      {
        "@type": "type.googleapis.com/google.firebase.fcm.v1.FcmError",
        "errorCode": "INVALID_ARGUMENT"
      }
    ]
  }
}

Based on what node SDK is doing on a keyserver this approach is basically the same, so I implemented option 2 but I am open for changing this.

Depends on D12946

Test Plan

Manually malform device token for Android in DDB, try sending notif to this device and verify that flag was flipped and the message was sent to the device (both online and offline device).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Jul 31 2024, 2:59 AM
kamil edited the summary of this revision. (Show Details)
kamil added a reviewer: tomek.

About option 1 vs option 2: I think it is much worse if we have a false-negative token invalidation than a false-positive.
False-positive means that we think that a token is invalid when it is valid. In this case, we would ask a client to send a new token. If we have a good validation of notifications on Tunnelbroker, then it is unlikely to cause serious issues.
False-negative means that we think that a token is valid when it isn't. We won't be able to send any notif and don't have a way of recovering from it.

About option 1 vs option 2: I think it is much worse if we have a false-negative token invalidation than a false-positive.
False-positive means that we think that a token is invalid when it is valid. In this case, we would ask a client to send a new token. If we have a good validation of notifications on Tunnelbroker, then it is unlikely to cause serious issues.
False-negative means that we think that a token is valid when it isn't. We won't be able to send any notif and don't have a way of recovering from it.

Makes sense, updated to option 1

bartek added inline comments.
services/tunnelbroker/src/websockets/session.rs
500–516 ↗(On Diff #42992)

Not sure what is more readable

This revision is now accepted and ready to land.Jul 31 2024, 5:56 AM

rebase before landing

services/tunnelbroker/src/websockets/session.rs
500–516 ↗(On Diff #42992)

This does not work, we try to execute something like Result<_, fcm::error::Error> | Result<_, fcm::error::Error> which is not correct

This revision was automatically updated to reflect the committed changes.