appfeel / node-pushnotifications

Push notifications for GCM, APNS, MPNS, AMZ (automatic detection from device token)
MIT License
544 stars 125 forks source link

iOS Simulator Push Tokens incorrectly identified as GCM tokens #188

Closed kvr-guestlogix closed 1 year ago

kvr-guestlogix commented 1 year ago

Since Xcode 14, iOS 16 simulators running on Apple Silicon now have the ability to register and receive push notifications. However, getPushMethodByRegId() is not capable for correctly identifying these tokens because they are more than 64 characters in length, and instead associates them as GCM tokens.

As far as I know, it looks like simulator tokens are 160 characters long, but this is based solely on my own testing and I can't confirm the exact format.

Is there any way to force the library to send via APN only? (I only have APN settings set in the constructor).

nsakaimbo commented 1 year ago

Can confirm this is an issue that we're experiencing as well. At the very least, perhaps an isAlwaysUseAPN setting (just like the isAlwaysUseFCM attribute) might be helpful here.

miqmago commented 1 year ago

Hi there, the issue here is with the length of the token. The triage of the method used to send push notifications is decided from token length. If it is longer than 64 characters (32 bytes) it is considered to be GCM, otherwise it is APNS (little more complex as there are other methods like web push and amazon). In simulators token is 160 characters long.

Problem is that android does not specify the length of the token and it seems it could be variable from 152 up to 4k, which includes 160! https://firebase.google.com/docs/reference/android/com/google/firebase/messaging/FirebaseMessaging#getToken() https://groups.google.com/g/android-gcm/c/q2PzJTP71TY/m/DNX4lvDu5X4J

At this point, the solution that comes to my mind right now is to evaluate if the token is fully composed of [0-9A-Z], its length is 64 or 160. But as per Apple documentation in https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1622958-application it seems that

deviceToken A globally unique token that identifies this device to APNs. Send this token to the server that you use to generate remote notifications. Your server must pass this token unmodified back to APNs when sending those remote notifications.

APNs device tokens are of variable length. Do not hard-code their size.

Additionally we could support a regId object with the property type so we could pass a regId like this:

{
  "id": "6ABF..."
  "type": "apn"
}

Also to me it seems that the length is associated on if it is a release or development token, but I'm not sure

In my opinion I think that we should deprecate string tokens, sending a warning to the user like "use at your own risk" and start using object tokens.

This is delicate issue as could break things... @alex-friedl what do you think?

alex-friedl commented 1 year ago

@miqmago If I understand correctly you suggest to drop the "auto-detect" of string tokens and have user's pass objects similar to the one you outlined in the comment, i.e.

{ "token": "string", "type": "string" }

where type is an enum of all supported providers, i.e. apn, gcm, web etc.?

If that's your suggestion I am all in favor of it, as it would get rid of these cases where the token is ambigious and be future-proof to changes on the provider side, as it would be agnostic of the token format.

The breaking change would not be a big issue, as we would of course release it as v 2.0 of the lib, therefore not breaking any users with 1.x

miqmago commented 1 year ago

@alex-friedl that's correct. I've already been working on it. To avoid breaking changes on this release I've made it compatible with actual cases (string). I've also added a "deprecated" warning so we can remove string support in future releases. I've defined the object like this:

{ "id": "string", "type": "string" }

As the web token it also uses object, the getPushMethod will be like this:

  getPushMethodByRegId(regId) {
    if (typeof regId === 'object' && (!regId.type || !regId.id)) {
      return { regId, pushMethod: WEB_METHOD };
    }

    if (typeof regId === 'object' && regId.id && regId.type) {
      return {
        regId: regId.id,
        pushMethod: this.settings.isAlwaysUseFCM ? GCM_METHOD : regId.type,
      };
    }

    // TODO: deprecated, plan removal of all cases below in future releases
    // and review test cases
    if (this.settings.isAlwaysUseFCM) {
      return { regId, pushMethod: GCM_METHOD };
    }

    if (regId.substring(0, 4) === 'http') {
      return { regId, pushMethod: WNS_METHOD };
    }

    if (/^(amzn[0-9]*.adm)/i.test(regId)) {
      return { regId, pushMethod: ADM_METHOD };
    }

    if (
      (regId.length === 64 || regId.length === 160) &&
      /^[a-fA-F0-9]+$/.test(regId)
    ) {
      return { regId, pushMethod: APN_METHOD };
    }

    if (regId.length > 64) {
      return { regId, pushMethod: GCM_METHOD };
    }

    return { regId, pushMethod: UNKNOWN_METHOD };
  }

This way we can publish update in 2.1 and new release 3.0 with breaking changes. I will try publishing it next days, probably 2.1 is easy as it does not need to review all test cases...