exirationTime is always updated and fix tests

* Always update exirationTime, and only updated createdAt if there is a new subscription (totally new or resubscribe after an unsubscribe)
* Updated PushManager mock to throw if changing applicationServerKey without first calling unsubscribe.
   - To simulate the exact behavior of the browser.
* Updated test to check for resubscribing with a new key, instead of just using the old key.

src/managers/ServiceWorkerManager.ts

@@ -392,6 +392,10 @@ export class ServiceWorkerManager {
   private async installAlternatingWorker() {
     const workerState = await this.getActiveState();
 
+    if (workerState === ServiceWorkerActiveState.ThirdParty) {
+      Log.info(`[Service Worker Installation] 3rd party service worker detected.`);
+    }
+
     const workerFullPath = ServiceWorkerHelper.getServiceWorkerHref(workerState, this.config);
     const installUrlQueryParams = Utils.encodeHashAsUriComponent({
       appId: this.context.appConfig.appId

src/managers/SubscriptionManager.ts

@@ -422,7 +422,7 @@ export class SubscriptionManager {
    *
    * If the VAPID key isn't present, undefined is returned instead of null.
    */
-  public getVapidKeyForBrowser(): ArrayBuffer {
+  public getVapidKeyForBrowser(): ArrayBuffer | undefined {
     // Specifically return undefined instead of null if the key isn't available
     let key = undefined;
 
@@ -471,7 +471,7 @@ export class SubscriptionManager {
     const existingPushSubscription = await pushManager.getSubscription();
 
     /* Record the subscription created at timestamp only if this is a new subscription */
-    let shouldRecordSubscriptionCreatedAt = !existingPushSubscription;
+    let isNewSubscription = !existingPushSubscription;
 
     /* Depending on the subscription strategy, handle existing subscription in various ways */
     switch (subscriptionStrategy) {
@@ -500,37 +500,30 @@ export class SubscriptionManager {
             Because of this, we should unsubscribe the user from push first and then resubscribe
             them.
           */
-          await existingPushSubscription.unsubscribe();
+
           /* We're unsubscribing, so we want to store the created at timestamp */
-          shouldRecordSubscriptionCreatedAt = true;
+          isNewSubscription = await SubscriptionManager.doPushUnsubscribe(existingPushSubscription);
         }
         break;
       case SubscriptionStrategyKind.SubscribeNew:
         /* Since we want a new subscription every time with this strategy, just unsubscribe. */
         if (existingPushSubscription) {
-          Log.debug('[Subscription Manager] Unsubscribing existing push subscription.');
-          await existingPushSubscription.unsubscribe();
+          isNewSubscription = await SubscriptionManager.doPushUnsubscribe(existingPushSubscription);
+          break;
         }
 
         // Always record the subscription if we're resubscribing
-        shouldRecordSubscriptionCreatedAt = true;
+        isNewSubscription = true;
         break;
     }
 
-    const subscriptionOptions: PushSubscriptionOptionsInit = {
-      userVisibleOnly: true,
-      applicationServerKey: this.getVapidKeyForBrowser() ? this.getVapidKeyForBrowser() : undefined
-    };
-
     // Actually subscribe the user to push
-    const newPushSubscription = await SubscriptionManager.doPushSubscribe(pushManager, subscriptionOptions);
+    const [newPushSubscription, createdNew] =
+      await SubscriptionManager.doPushSubscribe(pushManager, this.getVapidKeyForBrowser());
 
-    if (shouldRecordSubscriptionCreatedAt) {
-      const bundle = await Database.getSubscription();
-      bundle.createdAt = new Date().getTime();
-      bundle.expirationTime = newPushSubscription.expirationTime;
-      await Database.setSubscription(bundle);
-    }
+    // Update saved create and expired times
+    const didCreateNewSubscription = isNewSubscription || createdNew;
+    await SubscriptionManager.updateSubscriptionTime(didCreateNewSubscription, newPushSubscription.expirationTime);
 
     // Create our own custom object from the browser's native PushSubscription object
     const pushSubscriptionDetails = RawPushSubscription.setFromW3cSubscription(newPushSubscription);
@@ -541,25 +534,49 @@ export class SubscriptionManager {
     return pushSubscriptionDetails;
   }
 
+  private static async updateSubscriptionTime(updateCreatedAt: boolean, expirationTime: number | null): Promise<void> {
+    const bundle = await Database.getSubscription();
+    if (updateCreatedAt) {
+      bundle.createdAt = new Date().getTime();
+    }
+    bundle.expirationTime = expirationTime;
+    await Database.setSubscription(bundle);
+  }
+
+  private static async doPushUnsubscribe(pushSubscription: PushSubscription): Promise<boolean> {
+    Log.debug('[Subscription Manager] Unsubscribing existing push subscription.');
+    const result = await pushSubscription.unsubscribe();
+    Log.debug(`[Subscription Manager] Unsubscribing existing push subscription result: ${result}`);
+    return result;
+  }
+
   // Subscribes the ServiceWorker for a pushToken.
   // If there is an error doing so unsubscribe from existing and try again
   //    - This handles subscribing to new server VAPID key if it has changed.
+  // return type - [PushSubscription, createdNewPushSubscription(boolean)]
   private static async doPushSubscribe(
     pushManager: PushManager,
-    subscriptionOptions: PushSubscriptionOptionsInit)
-    :Promise<PushSubscription> {
+    applicationServerKey: ArrayBuffer | undefined)
+    :Promise<[PushSubscription, boolean]> {
+
+    const subscriptionOptions: PushSubscriptionOptionsInit = {
+      userVisibleOnly: true,
+      applicationServerKey: applicationServerKey
+    };
     Log.debug('[Subscription Manager] Subscribing to web push with these options:', subscriptionOptions);
     try {
-      return await pushManager.subscribe(subscriptionOptions);
+      return [await pushManager.subscribe(subscriptionOptions), false];
     } catch (e) {
       if (e.name == "InvalidStateError") {
         // This exception is thrown if the key for the existing applicationServerKey is different,
         //    so we must unregister first.
         // In Chrome, e.message contains will be the following in this case for reference;
         // Registration failed - A subscription with a different applicationServerKey (or gcm_sender_id) already exists;
         //    to change the applicationServerKey, unsubscribe then resubscribe.
-        await (await pushManager.getSubscription()).unsubscribe();
-        return await pushManager.subscribe(subscriptionOptions);
+        Log.warn("[Subscription Manager] Couldn't re-subscribe due to applicationServerKey changing, " +
+          "unsubscribe and attempting to subscribe with new key.", e);
+        await SubscriptionManager.doPushUnsubscribe(await pushManager.getSubscription());
+        return [await pushManager.subscribe(subscriptionOptions), true];
       }
       else
         throw e; // If some other error, bubble the exception up

test/support/mocks/service-workers/models/PushManager.ts

@@ -1,13 +1,12 @@
 import PushSubscription from './PushSubscription';
 import PushSubscriptionOptions from './PushSubscriptionOptions';
-import { arrayBufferToBase64 } from "../../../../../src/utils/Encoding";
 
 /**
  * The PushManager interface of the Push API provides a way to receive notifications from
  * third-party servers as well as request URLs for push notifications.
  */
 export default class PushManager {
-  private subscription: PushSubscription;
+  private subscription: PushSubscription | null;
 
   constructor() {
     this.subscription = null;
@@ -18,7 +17,7 @@ export default class PushManager {
    * PushSubscription object containing details of an existing subscription. If no existing
    * subscription exists, this resolves to a null value.
    */
-  public async getSubscription(): Promise<PushSubscription> {
+  public async getSubscription(): Promise<PushSubscription | null> {
     return this.subscription;
   }
 
@@ -36,9 +35,16 @@ export default class PushManager {
    * service worker does not have an existing subscription.
    */
   public async subscribe(options: PushSubscriptionOptions): Promise<PushSubscription> {
-    if (!this.subscription) {
-      this.subscription = new PushSubscription(this, options);
+    if (this.subscription) {
+      if (this.subscription.options.applicationServerKey != options.applicationServerKey) {
+        // Simulate browser throwing if you don't unsubscribe first if applicationServerKey has changed.
+        throw {
+          name: "InvalidStateError",
+          message: "Can not change keys without calling unsubscribe first!"
+        };
+      }
     }
+    this.subscription = new PushSubscription(this, options);
     return this.subscription;
   }
 

test/support/mocks/service-workers/models/PushSubscription.ts

@@ -71,7 +71,7 @@ export default class PushSubscription {
    * Returns an ArrayBuffer which contains the client's public key, which can then be sent to a
    * server and used in encrypting push message data.
    */
-  getKey(param): ArrayBufferLike {
+  getKey(param: string): ArrayBufferLike | null {
     switch (param) {
       case 'p256dh':
         return this.p256dhArray.buffer;

test/unit/managers/SubscriptionManager.ts

@@ -28,7 +28,7 @@ import { OneSignalUtils } from '../../../src/utils/OneSignalUtils';
 import { Subscription } from "../../../src/models/Subscription";
 import { PushDeviceRecord } from "../../../src/models/PushDeviceRecord";
 
-const sandbox: SinonSandbox= sinon.sandbox.create();;
+const sandbox: SinonSandbox= sinon.sandbox.create();
 
 test.beforeEach(async t => {
   await TestEnvironment.initialize({
@@ -149,7 +149,7 @@ test('uses globally shared VAPID public key for Firefox', async t => {
   );
 });
 
-test('resubscribe-existing strategy uses existing subscription options', async t => {
+test('resubscribe-existing strategy uses new subscription applicationServerKey', async t => {
   const initialVapidKeys = generateVapidKeys();
   const subsequentVapidKeys = generateVapidKeys();
 
@@ -158,21 +158,30 @@ test('resubscribe-existing strategy uses existing subscription options', async t
     applicationServerKey: base64ToUint8Array(initialVapidKeys.uniquePublic).buffer,
   };
 
+  let unsubscribeSpy: sinon.SinonSpy;
+
   await testCase(
     t,
     BrowserUserAgent.ChromeMacSupported,
     subsequentVapidKeys.uniquePublic,
     subsequentVapidKeys.sharedPublic,
     SubscriptionStrategyKind.ResubscribeExisting,
-    async (pushManager, subscriptionManager) => {
-      // Create an initial subscription, so subsequent subscription attempts re-use this initial
-      // subscription's options
+    async (pushManager, _subscriptionManager) => {
+      // Create an initial subscription, so subsequent subscription attempts logic is tested
       await pushManager.subscribe(initialSubscriptionOptions);
+      // And spy on PushManager.unsubscribe(), because we expect the existing subscription to be unsubscribed
+      unsubscribeSpy = sandbox.spy(PushSubscription.prototype, 'unsubscribe');
     },
-    async (pushManager, pushManagerSubscribeSpy) => {
-      // The subscription options used should be identical to our initial subscription's options
+    async (_pushManager, pushManagerSubscribeSpy) => {
+      const newSubscriptionOptions: PushSubscriptionOptions = {
+        userVisibleOnly: true,
+        applicationServerKey: base64ToUint8Array(subsequentVapidKeys.uniquePublic).buffer,
+      };
+      // The subscription options used should always use the new subscription's options
       const calledSubscriptionOptions = pushManagerSubscribeSpy.getCall(0).args[0];
-      t.deepEqual(calledSubscriptionOptions, initialSubscriptionOptions);
+      t.deepEqual(calledSubscriptionOptions, newSubscriptionOptions);
+      // Unsubscribe should have been called
+      t.true(unsubscribeSpy.calledOnce);
     }
   );
 });
@@ -221,8 +230,6 @@ test(
         t.true(unsubscribeSpy.calledOnce);
       }
     );
-
-    unsubscribeSpy.restore();
   }
 );