Skip to content

Commit

Permalink
ensure sw is a OneSignal one before using it
Browse files Browse the repository at this point in the history
Created a new getOneSignalRegistration() function to guarantee
the service worker is OneSignal's. This encapsulates this so
consuming code doesn't have to account for this, and it was
found out most code was not.
  • Loading branch information
jkasten2 committed Sep 11, 2024
1 parent a828638 commit 9435030
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 33 deletions.
2 changes: 1 addition & 1 deletion src/shared/libraries/WorkerMessenger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ export class WorkerMessenger {
);

const workerRegistration =
await this.context?.serviceWorkerManager.getRegistration();
await this.context?.serviceWorkerManager.getOneSignalRegistration();
if (!workerRegistration) {
Log.error(
'`[Worker Messenger] [Page -> SW] Could not get ServiceWorkerRegistration to postMessage!',
Expand Down
36 changes: 25 additions & 11 deletions src/shared/managers/ServiceWorkerManager.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import Environment from '../helpers/Environment';
import { WorkerMessengerCommand } from '../libraries/WorkerMessenger';
import Path from '../models/Path';
import SdkEnvironment from './SdkEnvironment';
import Database from '../services/Database';
import { WindowEnvironmentKind } from '../models/WindowEnvironmentKind';
import Log from '../libraries/Log';
import OneSignalEvent from '../services/OneSignalEvent';
import ServiceWorkerRegistrationError from '../errors/ServiceWorkerRegistrationError';
Expand Down Expand Up @@ -35,18 +33,35 @@ export class ServiceWorkerManager {
this.config = config;
}

// Gets details on the OneSignal service-worker (if any)
public async getRegistration(): Promise<
ServiceWorkerRegistration | null | undefined
/**
* Gets the current ServiceWorkerRegistration in the scoped configured
* in OneSignal.
* WARNING: This might be a non-OneSignal service worker, use
* getOneSignalRegistration() instead if you need this guarantee.
*/
private async getRegistration(): Promise<
ServiceWorkerRegistration | undefined
> {
return await ServiceWorkerUtilHelper.getRegistration(
return ServiceWorkerUtilHelper.getRegistration(
this.config.registrationOptions.scope,
);
}

/**
* Gets the OneSignal ServiceWorkerRegistration reference, if it was registered
*/
public async getOneSignalRegistration(): Promise<
ServiceWorkerRegistration | undefined
> {
const state = await this.getActiveState();
if (state === ServiceWorkerActiveState.OneSignalWorker) {
return this.getRegistration();
}
return undefined;
}

public async getActiveState(): Promise<ServiceWorkerActiveState> {
const workerRegistration =
await this.context.serviceWorkerManager.getRegistration();
const workerRegistration = await this.getRegistration();
if (!workerRegistration) {
return ServiceWorkerActiveState.None;
}
Expand Down Expand Up @@ -163,8 +178,7 @@ export class ServiceWorkerManager {

private async haveParamsChanged(): Promise<boolean> {
// 1. No workerRegistration
const workerRegistration =
await this.context.serviceWorkerManager.getRegistration();
const workerRegistration = await this.getRegistration();
if (!workerRegistration) {
Log.info(
'[changedServiceWorkerParams] workerRegistration not found at scope',
Expand Down Expand Up @@ -382,7 +396,7 @@ export class ServiceWorkerManager {
ServiceWorkerRegistration | undefined | null
> {
if (!(await this.shouldInstallWorker())) {
return this.getRegistration();
return this.getOneSignalRegistration();
}

Log.info('Installing worker...');
Expand Down
15 changes: 4 additions & 11 deletions src/shared/managers/SubscriptionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,7 @@ export class SubscriptionManager {
}

const serviceWorkerRegistration =
await this.context.serviceWorkerManager.getRegistration();
await this.context.serviceWorkerManager.getOneSignalRegistration();
if (!serviceWorkerRegistration) return false;

// It's possible to get here in Safari 11.1+ version
Expand Down Expand Up @@ -834,17 +834,12 @@ export class SubscriptionManager {
};
}

const workerState =
await this.context.serviceWorkerManager.getActiveState();
const workerRegistration =
await this.context.serviceWorkerManager.getRegistration();
await this.context.serviceWorkerManager.getOneSignalRegistration();
const notificationPermission =
await this.context.permissionManager.getNotificationPermission(
this.context.appConfig.safariWebId,
);
const isWorkerActive =
workerState === ServiceWorkerActiveState.OneSignalWorker;

if (!workerRegistration) {
/* You can't be subscribed without a service worker registration */
return {
Expand All @@ -861,16 +856,14 @@ export class SubscriptionManager {
* const isPushEnabled = !!(
* pushSubscription &&
* deviceId &&
* notificationPermission === NotificationPermission.Granted &&
* isWorkerActive
* notificationPermission === NotificationPermission.Granted
* );
*/

const isPushEnabled = !!(
isValidPushSubscription &&
subscriptionToken &&
notificationPermission === NotificationPermission.Granted &&
isWorkerActive
notificationPermission === NotificationPermission.Granted
);

return {
Expand Down
4 changes: 2 additions & 2 deletions src/sw/helpers/ServiceWorkerUtilHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export default class ServiceWorkerUtilHelper {
// A relative scope is required as a domain can have none to many service workers installed.
static async getRegistration(
scope: string,
): Promise<ServiceWorkerRegistration | null | undefined> {
): Promise<ServiceWorkerRegistration | undefined> {
try {
// Adding location.origin to negate the effects of a possible <base> html tag the page may have.
const url = location.origin + scope;
Expand All @@ -17,7 +17,7 @@ export default class ServiceWorkerUtilHelper {
scope,
e,
);
return null;
return undefined;
}
}

Expand Down
15 changes: 7 additions & 8 deletions test/unit/managers/ServiceWorkerManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@ import sinon, { SinonSandbox, SinonStub } from 'sinon';
import nock from 'nock';
import { ServiceWorkerManager } from '../../../src/shared/managers/ServiceWorkerManager';
import { ServiceWorkerActiveState } from '../../../src/shared/helpers/ServiceWorkerHelper';
import {
TestEnvironment,
TestEnvironmentConfig,
} from '../../support/sdk/TestEnvironment';
import { TestEnvironment } from '../../support/sdk/TestEnvironment';
import Context from '../../../src/page/models/Context';
import SdkEnvironment from '../../../src/shared/managers/SdkEnvironment';
import { WindowEnvironmentKind } from '../../../src/shared/models/WindowEnvironmentKind';
Expand All @@ -25,7 +22,6 @@ import { ServiceWorkerRegistrationError } from '../../../src/shared/errors/Servi
import OneSignalUtils from '../../../src/shared/utils/OneSignalUtils';
import { MockServiceWorkerRegistration } from '../../support/mocks/service-workers/models/MockServiceWorkerRegistration';
import { MockServiceWorker } from '../../support/mocks/service-workers/models/MockServiceWorker';
import { ConfigIntegrationKind } from '../../../src/shared/models/AppConfig';
import Environment from '../../../src/shared/helpers/Environment';
import { MockServiceWorkerContainerWithAPIBan } from '../../support/mocks/service-workers/models/MockServiceWorkerContainerWithAPIBan';
import Path from '../../../src/shared/models/Path';
Expand Down Expand Up @@ -374,12 +370,14 @@ test('Service worker failed to install due to 404 on host page. Send notificatio

test('ServiceWorkerManager.getRegistration() returns valid instance when sw is registered', async (t) => {
await navigator.serviceWorker.register('/Worker.js');
const result = await OneSignal.context.serviceWorkerManager.getRegistration();
const result =
await OneSignal.context.serviceWorkerManager.getOneSignalRegistration();
t.truthy(result);
});

test('ServiceWorkerManager.getRegistration() returns undefined when sw is not registered ', async (t) => {
const result = await OneSignal.context.serviceWorkerManager.getRegistration();
const result =
await OneSignal.context.serviceWorkerManager.getOneSignalRegistration();
t.is(result, undefined);
});

Expand All @@ -395,6 +393,7 @@ test('ServiceWorkerManager.getRegistration() handles throws by returning null',
throw new Error('HTTP NOT SUPPORTED');
}),
);
const result = await OneSignal.context.serviceWorkerManager.getRegistration();
const result =
await OneSignal.context.serviceWorkerManager.getOneSignalRegistration();
t.is(result, null);
});

0 comments on commit 9435030

Please sign in to comment.