Fix for missing appId in UpdateManager (#437)

Includes:
* Fix for missing appId in UpdateManager when not passing a device record
* Guards against empty app and player ids in api and updated tests
* Adding more guards against missing app id in API native call and updating tests
* Adding a TODO comment to appId in the Device Record class

Author: itrush

src/OneSignalApiBase.ts

@@ -1,6 +1,7 @@
 import Environment from './Environment';
 import SdkEnvironment from './managers/SdkEnvironment';
 import { Utils } from "./utils/Utils";
+import { OneSignalApiError, OneSignalApiErrorKind } from './errors/OneSignalApiError';
 
 type Headers = any[] & {[key: string]: any};
 type SupportedMethods = "GET" | "POST" | "PUT" | "DELETE";
@@ -22,7 +23,19 @@ export class OneSignalApiBase {
     return OneSignalApiBase.call('DELETE', action, data, headers);
   }
 
-  private static call(method: SupportedMethods, action: string, data: any, headers: Headers | undefined) {
+  private static call(method: SupportedMethods, action: string, data: any, headers: Headers | undefined): Promise<any> {
+    if (method === "GET") {
+      if (action.indexOf("players") > -1 && action.indexOf("app_id=") === -1) {
+        console.error("Calls to player api are not permitted without app_id");
+        return Promise.reject(new OneSignalApiError(OneSignalApiErrorKind.MissingAppId));
+      }
+    } else {
+      if (action.indexOf("players") > -1 && (!data || !data["app_id"])) {
+        console.error("Calls to player api are not permitted without app_id");
+        return Promise.reject(new OneSignalApiError(OneSignalApiErrorKind.MissingAppId));
+      }
+    }
+
     let callHeaders: any = new Headers();
     callHeaders.append('SDK-Version', `onesignal/web/${Environment.version()}`);
     callHeaders.append('Content-Type', 'application/json;charset=UTF-8');

src/OneSignalApiSW.ts

@@ -2,9 +2,11 @@ import { ServerAppConfig } from "./models/AppConfig";
 import { OneSignalApiBase } from "./OneSignalApiBase";
 import { SubscriptionStateKind } from "./models/SubscriptionStateKind";
 import Log from "./libraries/Log";
+import { Utils } from "./utils/Utils";
 
 export class OneSignalApiSW {
   static async downloadServerAppConfig(appId: string): Promise<ServerAppConfig> {
+    Utils.enforceAppId(appId);
     return await new Promise<ServerAppConfig>((resolve, _reject) => {
       resolve(OneSignalApiBase.get(`sync/${appId}/web`, null));
     });
@@ -16,6 +18,7 @@ export class OneSignalApiSW {
    */
   static getUserIdFromSubscriptionIdentifier(appId: string, deviceType: number, identifier: string): Promise<string> {
     // Calling POST /players with an existing identifier returns us that player ID
+    Utils.enforceAppId(appId);
     return OneSignalApiBase.post("players", {
       app_id: appId,
       device_type: deviceType,
@@ -34,6 +37,8 @@ export class OneSignalApiSW {
   }
 
   static updatePlayer(appId: string, playerId: string, options?: Object) {
+    Utils.enforceAppId(appId);
+    Utils.enforcePlayerId(playerId);
     return OneSignalApiBase.put(`players/${playerId}`, {app_id: appId, ...options});
   }
 }

src/OneSignalApiShared.ts

@@ -8,10 +8,14 @@ import Utils from "./utils/Utils";
 
 export default class OneSignalApiShared {
   static getPlayer(appId: string, playerId: string) {
+    Utils.enforceAppId(appId);
+    Utils.enforcePlayerId(playerId);
     return OneSignalApiBase.get(`players/${playerId}?app_id=${appId}`);
   }
 
   static updatePlayer(appId: string, playerId: string, options?: Object) {
+    Utils.enforceAppId(appId);
+    Utils.enforcePlayerId(playerId);
     return OneSignalApiBase.put(`players/${playerId}`, {app_id: appId, ...options});
   }
 
@@ -39,7 +43,9 @@ export default class OneSignalApiShared {
   }
 
   static async createUser(deviceRecord: DeviceRecord): Promise<string | null> {
-    const response = await OneSignalApiBase.post(`players`, deviceRecord.serialize());
+    const serializedDeviceRecord = deviceRecord.serialize();
+    Utils.enforceAppId(serializedDeviceRecord.app_id);
+    const response = await OneSignalApiBase.post(`players`, serializedDeviceRecord);
     if (response && response.success)
       return response.id;
     return null;
@@ -50,6 +56,7 @@ export default class OneSignalApiShared {
     emailProfile: EmailProfile,
     pushDeviceRecordId?: string
   ): Promise<string | null> {
+    Utils.enforceAppId(appConfig.appId);
     const emailRecord = new EmailDeviceRecord(emailProfile.emailAddress, emailProfile.emailAuthHash);
     emailRecord.appId = appConfig.appId;
     emailRecord.pushDeviceRecordId = pushDeviceRecordId;
@@ -66,6 +73,8 @@ export default class OneSignalApiShared {
     emailProfile: EmailProfile,
     pushDeviceRecordId?: string
   ): Promise<string | null> {
+    Utils.enforceAppId(appConfig.appId);
+    Utils.enforcePlayerId(emailProfile.emailId);
     const emailRecord = new EmailDeviceRecord(emailProfile.emailAddress, emailProfile.emailAuthHash);
     emailRecord.appId = appConfig.appId;
     emailRecord.pushDeviceRecordId = pushDeviceRecordId;
@@ -78,6 +87,8 @@ export default class OneSignalApiShared {
   }
 
   static async logoutEmail(appConfig: AppConfig, emailProfile: EmailProfile, deviceId: string): Promise<boolean> {
+    Utils.enforceAppId(appConfig.appId);
+    Utils.enforcePlayerId(deviceId);
     const response = await OneSignalApiBase.post(`players/${deviceId}/email_logout`, {
       app_id: appConfig.appId,
       parent_player_id: emailProfile.emailId,
@@ -95,7 +106,10 @@ export default class OneSignalApiShared {
     deviceRecord: DeviceRecord,
   ): Promise<string> {
     try {
-      const response = await OneSignalApiBase.post(`players/${userId}/on_session`, deviceRecord.serialize());
+      const serializedDeviceRecord = deviceRecord.serialize();
+      Utils.enforceAppId(serializedDeviceRecord.app_id);
+      Utils.enforcePlayerId(userId);
+      const response = await OneSignalApiBase.post(`players/${userId}/on_session`, serializedDeviceRecord);
       if (response.id) {
         // A new user ID can be returned
         return response.id;

src/managers/UpdateManager.ts

@@ -27,6 +27,7 @@ export class UpdateManager {
 
   private async createDeviceRecord(): Promise<PushDeviceRecord> {
     const deviceRecord = new PushDeviceRecord();
+    deviceRecord.appId = this.context.appConfig.appId;
     deviceRecord.subscriptionState = await MainHelper.getCurrentNotificationType();
     return deviceRecord;
   }

src/models/DeviceRecord.ts

@@ -8,14 +8,33 @@ import { Serializable } from './Serializable';
 import { SubscriptionStateKind } from './SubscriptionStateKind';
 import { OneSignalUtils } from "../utils/OneSignalUtils";
 
+export interface FlattenedDeviceRecord {
+  /* Old Parameters */
+  device_type: DeliveryPlatformKind;
+  language: string;
+  timezone: number;
+  device_os: number;
+  sdk: string;
+  notification_types: SubscriptionStateKind | undefined;
+
+  /* New Parameters */
+  delivery_platform: DeliveryPlatformKind;
+  browser_name: string;
+  browser_version: number;
+  operating_system: string;
+  operating_system_version: string;
+  device_platform: DevicePlatformKind;
+  device_model: string;
+  // TODO: Make it a required parameter
+  app_id?: string;
+}
 
 /**
  * Describes the fields of a OneSignal "player" device record.
  *
  * This is used when creating or modifying push and email records.
  */
 export abstract class DeviceRecord implements Serializable {
-  public appId: string;
   public deliveryPlatform: DeliveryPlatformKind;
   public language: string;
   public timezone: number;
@@ -26,9 +45,12 @@ export abstract class DeviceRecord implements Serializable {
   public devicePlatform: DevicePlatformKind;
   public deviceModel: string;
   public sdkVersion: string;
-  public subscriptionState: SubscriptionStateKind;
+  public appId: string | undefined;
+  public subscriptionState: SubscriptionStateKind | undefined;
 
   constructor() {
+    // TODO: Possible implementation for appId initialization
+    // this.appId = OneSignal.context.appConfig.appId;
     this.language = Environment.getLanguage();
     this.timezone = new Date().getTimezoneOffset() * -60;
     this.browserName = bowser.name;
@@ -39,7 +61,7 @@ export abstract class DeviceRecord implements Serializable {
     this.deviceModel = navigator.platform;
     this.sdkVersion = Environment.version().toString();
     this.deliveryPlatform = this.getDeliveryPlatform();
-    // Unimplemented properties are appId, deliveryPlatform, subscriptionState, and subscription
+    // Unimplemented properties are appId, subscriptionState, and subscription
   }
 
   getDevicePlatform(): DevicePlatformKind {
@@ -129,8 +151,8 @@ export abstract class DeviceRecord implements Serializable {
     }
   }
 
-  serialize() {
-    const serializedBundle: any = {
+  serialize(): FlattenedDeviceRecord {
+    const serializedBundle: FlattenedDeviceRecord = {
       /* Old Parameters */
       device_type: this.deliveryPlatform,
       language: this.language,

src/utils/Utils.ts

@@ -97,6 +97,18 @@ export class Utils {
     }
     return defaultValue;
   }
+
+  public static enforceAppId(appId: string | undefined | null): void {
+    if (!appId) {
+      throw new Error("App id cannot be empty");
+    }
+  }
+
+  public static enforcePlayerId(playerId: string | undefined | null): void {
+    if (!playerId) {
+      throw new Error("Player id cannot be empty");
+    }
+  }
 }
 
 export default Utils;

test/unit/managers/ServiceWorkerManager.ts

@@ -159,7 +159,7 @@ test('notification clicked - While page is opened in background', async t => {
   const workerMessageReplyBuffer = new WorkerMessengerReplyBuffer();
   OneSignal.context.workerMessenger = new WorkerMessenger(OneSignal.context, workerMessageReplyBuffer);
 
-  sandbox.stub(Event, 'trigger', function(event: string) {
+  sandbox.stub(Event, 'trigger').callsFake(function(event: string) {
     if (event === OneSignal.EVENTS.NOTIFICATION_CLICKED)
       t.pass();
   });
@@ -199,9 +199,12 @@ test('installWorker() installs worker A with the correct file name and query par
   t.is(await manager.getActiveState(), ServiceWorkerActiveState.None);
   await manager.installWorker();
   t.is(await manager.getActiveState(), ServiceWorkerActiveState.WorkerA);
-  t.true(navigator.serviceWorker.controller.scriptURL.endsWith(
-    `/Worker-A.js?appId=${OneSignal.context.appConfig.appId}`)
-  );
+  t.not(navigator.serviceWorker.controller, null);
+  if (navigator.serviceWorker.controller !== null) {
+    t.true(navigator.serviceWorker.controller.scriptURL.endsWith(
+      `/Worker-A.js?appId=${OneSignal.context.appConfig.appId}`)
+    );
+  }
 });
 
 test('installWorker() installs worker A when a third party service worker exists', async t => {
@@ -296,7 +299,7 @@ test("Service worker failed to install due to 404 on host page. Send notificatio
     .get(function(uri) {
       return uri.indexOf(workerPath) !== -1;
     })
-    .reply(404,  (uri, requestBody) => {
+    .reply(404,  (_uri: string, _requestBody: any) => {
       return {
         status: 404,
         statusText: "404 Not Found"
@@ -332,7 +335,7 @@ test("Service worker failed to install in popup. No handling.", async t => {
     .get(function(uri) {
       return uri.indexOf(workerPath) !== -1;
     })
-    .reply(404,  (uri, requestBody) => {
+    .reply(404,  (_uri: string, _requestBody: any) => {
       return {
         status: 404,
         statusText: "404 Not Found"

test/unit/managers/UpdateManager.ts

@@ -127,6 +127,23 @@ test("sendOnSessionUpdate triggers on_session for existing unsubscribed user if
   t.is(onSessionSpy.called, false);
 });
 
+test("sendOnSessionUpdate includes appId at all times", async t => {
+  const deviceId = Random.getRandomUuid();
+  sandbox.stub(Database, "getSubscription").resolves({ deviceId });
+  
+  OneSignal.context.sessionManager.setPageViewCount(1);
+  OneSignal.context.updateManager = new UpdateManager(OneSignal.context);
+  t.is(OneSignal.context.updateManager.onSessionAlreadyCalled(), false);
+
+  const onSessionApiSpy = sandbox.stub(OneSignalApiShared, "updateUserSession").resolves();
+  await OneSignal.context.updateManager.sendOnSessionUpdate();
+  t.is(onSessionApiSpy.calledOnce, true);
+  t.is(onSessionApiSpy.getCall(0).args.length, 2);
+  t.is(onSessionApiSpy.getCall(0).args[0], deviceId);
+  t.not(OneSignal.context.appConfig.appId, undefined);
+  t.is(onSessionApiSpy.getCall(0).args[1].appId, OneSignal.context.appConfig.appId);
+});
+
 test("sendPlayerCreate returns user id", async t => {
   const onCreateSpy = sandbox.stub(OneSignalApiShared, "createUser").resolves(Random.getRandomUuid());
 

test/unit/modules/subscriptionHelper.ts

@@ -1,82 +1,51 @@
 import '../../support/polyfills/polyfills';
 import test from 'ava';
 import { TestEnvironment, HttpHttpsEnvironment } from '../../support/sdk/TestEnvironment';
-import CookieSyncer from '../../../src/modules/CookieSyncer';
 import OneSignal from '../../../src/OneSignal';
-import MainHelper from '../../../src/helpers/MainHelper';
 import sinon from 'sinon';
 import SubscriptionHelper from '../../../src/helpers/SubscriptionHelper';
 import { SubscriptionManager } from '../../../src/managers/SubscriptionManager';
-import { AppConfig } from '../../../src/models/AppConfig';
-
-import Context from '../../../src/models/Context';
 import { SessionManager } from '../../../src/managers/SessionManager';
-import Random from '../../support/tester/Random';
 
-test.beforeEach(async t => {
+const sinonSandbox = sinon.sandbox.create();
+
+test.beforeEach(async () => {
   await TestEnvironment.initialize({
     httpOrHttps: HttpHttpsEnvironment.Https
   });
-
-  const appConfig = TestEnvironment.getFakeAppConfig();
-  appConfig.appId = Random.getRandomUuid();
-  OneSignal.context = new Context(appConfig);
+  TestEnvironment.mockInternalOneSignal();
 });
 
+test.afterEach(() => {
+  sinonSandbox.restore();
+})
+
 test('should not resubscribe user on subsequent page views if the user is already subscribed', async t => {
-  const isPushEnabledStub = sinon.stub(OneSignal, 'privateIsPushNotificationsEnabled').resolves(true);
-  const getSessionCountStub = sinon.stub(SessionManager.prototype, 'getPageViewCount').returns(2);
-  const subscribeSpy = sinon.spy(SubscriptionManager.prototype, 'subscribe');
+  sinonSandbox.stub(OneSignal, 'privateIsPushNotificationsEnabled').resolves(true);
+  sinonSandbox.stub(SessionManager.prototype, 'getPageViewCount').returns(2);
+  const subscribeSpy = sinonSandbox.spy(SubscriptionManager.prototype, 'subscribe');
 
   await SubscriptionHelper.registerForPush();
-
   t.true(subscribeSpy.notCalled);
-
-  subscribeSpy.restore();
-  isPushEnabledStub.restore();
-  getSessionCountStub.restore();
 });
 
 test('should subscribe user on subsequent page views if the user is not subscribed', async t => {
-  await TestEnvironment.initialize({
-    httpOrHttps: HttpHttpsEnvironment.Https
-  });
-
-  const appConfig = TestEnvironment.getFakeAppConfig();
-  appConfig.appId = Random.getRandomUuid();
-  OneSignal.context = new Context(appConfig);
-
-  const isPushEnabledStub = sinon.stub(OneSignal, 'isPushNotificationsEnabled').resolves(false);
-  const getSessionCountStub = sinon.stub(SessionManager.prototype, 'getPageViewCount').returns(2);
-  const subscribeStub = sinon.stub(SubscriptionManager.prototype, 'subscribe').resolves(null);
-
+  sinonSandbox.stub(OneSignal, 'isPushNotificationsEnabled').resolves(false);
+  sinonSandbox.stub(SessionManager.prototype, 'getPageViewCount').returns(2);
+  sinonSandbox.stub(SubscriptionManager.prototype, 'registerSubscription').resolves();
+  
+  const subscribeStub = sinonSandbox.stub(SubscriptionManager.prototype, 'subscribe').resolves(null);
   await SubscriptionHelper.registerForPush();
-
   t.true(subscribeStub.called);
-
-  subscribeStub.restore();
-  isPushEnabledStub.restore();
-  getSessionCountStub.restore();
 });
 
 test('should resubscribe an already subscribed user on first page view', async t => {
-  await TestEnvironment.initialize({
-    httpOrHttps: HttpHttpsEnvironment.Https
-  });
-
-  const appConfig = TestEnvironment.getFakeAppConfig();
-  appConfig.appId = Random.getRandomUuid();
-  OneSignal.context = new Context(appConfig);
-
-  const isPushEnabledStub = sinon.stub(OneSignal, 'isPushNotificationsEnabled').resolves(true);
-  const getSessionCountStub = sinon.stub(SessionManager.prototype, 'getPageViewCount').returns(1);
-  const subscribeStub = sinon.stub(SubscriptionManager.prototype, 'subscribe').resolves(null);
+  sinonSandbox.stub(OneSignal, 'isPushNotificationsEnabled').resolves(true);
+  sinonSandbox.stub(SessionManager.prototype, 'getPageViewCount').returns(1);
+  sinonSandbox.stub(SubscriptionManager.prototype, 'registerSubscription').resolves();
 
+  const subscribeStub = sinonSandbox.stub(SubscriptionManager.prototype, 'subscribe').resolves(null);
   await SubscriptionHelper.registerForPush();
 
   t.true(subscribeStub.called);
-
-  subscribeStub.restore();
-  isPushEnabledStub.restore();
-  getSessionCountStub.restore();
 });