diff --git a/apps/browser/src/platform/services/browser-task-scheduler.service.spec.ts b/apps/browser/src/platform/services/browser-task-scheduler.service.spec.ts index 345a2f0839..ba97ca8a8b 100644 --- a/apps/browser/src/platform/services/browser-task-scheduler.service.spec.ts +++ b/apps/browser/src/platform/services/browser-task-scheduler.service.spec.ts @@ -4,7 +4,6 @@ import { BehaviorSubject, Observable } from "rxjs"; import { ScheduledTaskNames } from "@bitwarden/common/platform/enums/scheduled-task-name.enum"; import { ConsoleLogService } from "@bitwarden/common/platform/services/console-log.service"; import { GlobalState, StateProvider } from "@bitwarden/common/platform/state"; -import { UserId } from "@bitwarden/common/types/guid"; import { flushPromises, triggerOnAlarmEvent } from "../../autofill/spec/testing-utils"; @@ -37,15 +36,10 @@ function setupGlobalBrowserMock(overrides: Partial = {}) { ...overrides, }; } -const userUuid = "user-uuid" as UserId; -function getAlarmNameMock(taskName: string) { - return `${taskName}__${userUuid}`; -} describe("BrowserTaskSchedulerService", () => { const callback = jest.fn(); const delayInMinutes = 2; - let activeUserIdMock$: BehaviorSubject; let activeAlarmsMock$: BehaviorSubject; let logService: MockProxy; let stateProvider: MockProxy; @@ -73,14 +67,12 @@ describe("BrowserTaskSchedulerService", () => { }), ]; activeAlarmsMock$ = new BehaviorSubject(activeAlarms); - activeUserIdMock$ = new BehaviorSubject(userUuid); logService = mock(); globalStateMock = mock>({ state$: mock>(), update: jest.fn((callback) => callback([], {} as any)), }); stateProvider = mock({ - activeUserId$: activeUserIdMock$, getGlobal: jest.fn(() => globalStateMock), }); browserTaskSchedulerService = new BrowserTaskSchedulerServiceImplementation( @@ -126,7 +118,7 @@ describe("BrowserTaskSchedulerService", () => { ); expect(chrome.alarms.create).toHaveBeenCalledWith( - getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout), + ScheduledTaskNames.loginStrategySessionTimeout, { delayInMinutes }, expect.any(Function), ); @@ -144,43 +136,6 @@ describe("BrowserTaskSchedulerService", () => { expect(chrome.alarms.create).not.toHaveBeenCalled(); }); - it("clears a scheduled alarm if a user-specific alarm for the same task is being registered", async () => { - const mockAlarm = mock({ - name: ScheduledTaskNames.loginStrategySessionTimeout, - }); - chrome.alarms.get = jest - .fn() - .mockImplementation((name, callback) => - callback(name === ScheduledTaskNames.loginStrategySessionTimeout ? mockAlarm : undefined), - ); - - await browserTaskSchedulerService.setTimeout( - ScheduledTaskNames.loginStrategySessionTimeout, - delayInMinutes * 60 * 1000, - ); - - expect(chrome.alarms.clear).toHaveBeenCalledWith( - ScheduledTaskNames.loginStrategySessionTimeout, - expect.any(Function), - ); - }); - - it("creates an alarm that is not associated with a user", async () => { - activeUserIdMock$.next(undefined); - chrome.alarms.get = jest.fn().mockImplementation((_name, callback) => callback(undefined)); - - await browserTaskSchedulerService.setTimeout( - ScheduledTaskNames.loginStrategySessionTimeout, - delayInMinutes * 60 * 1000, - ); - - expect(chrome.alarms.create).toHaveBeenCalledWith( - ScheduledTaskNames.loginStrategySessionTimeout, - { delayInMinutes }, - expect.any(Function), - ); - }); - describe("when the task is scheduled to be triggered in less than 1 minute", () => { const delayInMs = 45000; @@ -203,7 +158,7 @@ describe("BrowserTaskSchedulerService", () => { ); expect(chrome.alarms.create).toHaveBeenCalledWith( - getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout), + ScheduledTaskNames.loginStrategySessionTimeout, { delayInMinutes: 0.5 }, expect.any(Function), ); @@ -218,7 +173,7 @@ describe("BrowserTaskSchedulerService", () => { ); expect(browser.alarms.create).toHaveBeenCalledWith( - getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout), + ScheduledTaskNames.loginStrategySessionTimeout, { delayInMinutes: 1 }, ); }); @@ -234,7 +189,7 @@ describe("BrowserTaskSchedulerService", () => { await flushPromises(); expect(chrome.alarms.clear).toHaveBeenCalledWith( - getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout), + ScheduledTaskNames.loginStrategySessionTimeout, expect.any(Function), ); }); @@ -263,18 +218,23 @@ describe("BrowserTaskSchedulerService", () => { ); expect(chrome.alarms.create).toHaveBeenCalledWith( - `${getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout)}__0`, - { periodInMinutes: 0.5 }, + `${ScheduledTaskNames.loginStrategySessionTimeout}__0`, + { periodInMinutes: 0.6666666666666666, delayInMinutes: 0.5 }, expect.any(Function), ); expect(chrome.alarms.create).toHaveBeenCalledWith( - `${getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout)}__1`, - { periodInMinutes: 0.6666666666666666 }, + `${ScheduledTaskNames.loginStrategySessionTimeout}__1`, + { periodInMinutes: 0.6666666666666666, delayInMinutes: 0.6666666666666666 }, expect.any(Function), ); expect(chrome.alarms.create).toHaveBeenCalledWith( - `${getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout)}__2`, - { periodInMinutes: 0.8333333333333333 }, + `${ScheduledTaskNames.loginStrategySessionTimeout}__2`, + { periodInMinutes: 0.6666666666666666, delayInMinutes: 0.8333333333333333 }, + expect.any(Function), + ); + expect(chrome.alarms.create).toHaveBeenCalledWith( + `${ScheduledTaskNames.loginStrategySessionTimeout}__3`, + { periodInMinutes: 0.6666666666666666, delayInMinutes: 1 }, expect.any(Function), ); }); @@ -315,7 +275,7 @@ describe("BrowserTaskSchedulerService", () => { ); expect(chrome.alarms.create).toHaveBeenCalledWith( - getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout), + ScheduledTaskNames.loginStrategySessionTimeout, { periodInMinutes, delayInMinutes: 0.5 }, expect.any(Function), ); @@ -329,7 +289,7 @@ describe("BrowserTaskSchedulerService", () => { ); expect(chrome.alarms.create).toHaveBeenCalledWith( - getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout), + ScheduledTaskNames.loginStrategySessionTimeout, { periodInMinutes, delayInMinutes: periodInMinutes }, expect.any(Function), ); @@ -380,27 +340,6 @@ describe("BrowserTaskSchedulerService", () => { }); describe("triggering a task", () => { - it("clears an non user-based alarm if a separate user-based alarm has been set up", async () => { - jest.useFakeTimers(); - activeUserIdMock$.next(undefined); - const delayInMs = 10000; - chrome.alarms.get = jest - .fn() - .mockImplementation((_name, callback) => callback(mock())); - - await browserTaskSchedulerService.setTimeout( - ScheduledTaskNames.loginStrategySessionTimeout, - delayInMs, - ); - jest.advanceTimersByTime(delayInMs); - await flushPromises(); - - expect(chrome.alarms.clear).toHaveBeenCalledWith( - ScheduledTaskNames.loginStrategySessionTimeout, - expect.any(Function), - ); - }); - it("triggers a task when an onAlarm event is triggered", () => { const alarm = mock({ name: ScheduledTaskNames.loginStrategySessionTimeout, @@ -425,7 +364,7 @@ describe("BrowserTaskSchedulerService", () => { }); expect(chrome.alarms.clear).toHaveBeenCalledWith( - getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout), + ScheduledTaskNames.loginStrategySessionTimeout, expect.any(Function), ); }); @@ -438,7 +377,7 @@ describe("BrowserTaskSchedulerService", () => { }); expect(browser.alarms.clear).toHaveBeenCalledWith( - getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout), + ScheduledTaskNames.loginStrategySessionTimeout, ); }); }); diff --git a/apps/browser/src/platform/services/browser-task-scheduler.service.ts b/apps/browser/src/platform/services/browser-task-scheduler.service.ts index 1ea024521f..ecbf2a5f9c 100644 --- a/apps/browser/src/platform/services/browser-task-scheduler.service.ts +++ b/apps/browser/src/platform/services/browser-task-scheduler.service.ts @@ -58,8 +58,7 @@ export class BrowserTaskSchedulerServiceImplementation this.validateRegisteredTask(taskName); const delayInMinutes = delayInMs / 1000 / 60; - const alarmName = await this.getActiveUserAlarmName(taskName); - await this.scheduleAlarm(alarmName, { + await this.scheduleAlarm(taskName, { delayInMinutes: this.getUpperBoundDelayInMinutes(delayInMinutes), }); @@ -67,8 +66,8 @@ export class BrowserTaskSchedulerServiceImplementation // The alarm previously scheduled will be used as a backup in case the setTimeout fails. if (delayInMinutes < 1) { return globalThis.setTimeout(async () => { - await this.clearScheduledAlarm(alarmName); - await this.triggerTask(alarmName); + await this.clearScheduledAlarm(taskName); + await this.triggerTask(taskName); }, delayInMs); } } @@ -90,16 +89,15 @@ export class BrowserTaskSchedulerServiceImplementation this.validateRegisteredTask(taskName); const intervalInMinutes = intervalInMs / 1000 / 60; - const alarmName = await this.getActiveUserAlarmName(taskName); const initialDelayInMinutes = initialDelayInMs ? initialDelayInMs / 1000 / 60 : intervalInMinutes; if (intervalInMinutes < 1) { - return this.setupSteppedIntervalAlarms(taskName, alarmName, intervalInMs); + return this.setupSteppedIntervalAlarms(taskName, intervalInMs); } - await this.scheduleAlarm(alarmName, { + await this.scheduleAlarm(taskName, { periodInMinutes: this.getUpperBoundDelayInMinutes(intervalInMinutes), delayInMinutes: this.getUpperBoundDelayInMinutes(initialDelayInMinutes), }); @@ -112,28 +110,28 @@ export class BrowserTaskSchedulerServiceImplementation * api does not support intervals less than 1 minute. * * @param taskName - The name of the task, separate of any user id. - * @param alarmName - The name of the alarm to create, could contain a user id. * @param intervalInMs - The interval in milliseconds. */ private async setupSteppedIntervalAlarms( taskName: ScheduledTaskName, - alarmName: string, intervalInMs: number, ): Promise { const alarmMinDelayInMinutes = this.getAlarmMinDelayInMinutes(); const intervalInMinutes = intervalInMs / 1000 / 60; - const numberOfAlarmsToCreate = Math.ceil(1 / intervalInMinutes); + const numberOfAlarmsToCreate = Math.ceil(Math.ceil(1 / intervalInMinutes) / 2) + 1; + const steppedAlarmPeriodInMinutes = alarmMinDelayInMinutes + intervalInMinutes; for (let alarmIndex = 0; alarmIndex < numberOfAlarmsToCreate; alarmIndex++) { - const steppedAlarmName = `${alarmName}__${alarmIndex}`; - const periodInMinutes = alarmMinDelayInMinutes + intervalInMinutes * alarmIndex; + const steppedAlarmName = `${taskName}__${alarmIndex}`; + + const delayInMinutes = this.getUpperBoundDelayInMinutes( + alarmMinDelayInMinutes + intervalInMinutes * alarmIndex, + ); - // We need to clear alarms based on the task name as well as the - // user-based alarm name to ensure duplicate alarms are not created. - await this.clearScheduledAlarm(`${taskName}__${alarmIndex}`); await this.clearScheduledAlarm(steppedAlarmName); await this.scheduleAlarm(steppedAlarmName, { - periodInMinutes: this.getUpperBoundDelayInMinutes(periodInMinutes), + periodInMinutes: steppedAlarmPeriodInMinutes, + delayInMinutes, }); } @@ -147,7 +145,7 @@ export class BrowserTaskSchedulerServiceImplementation return; } - await this.triggerTask(alarmName, intervalInMinutes); + await this.triggerTask(taskName, intervalInMinutes); }, intervalInMs); return intervalId; @@ -168,8 +166,7 @@ export class BrowserTaskSchedulerServiceImplementation return; } - const alarmName = await this.getActiveUserAlarmName(taskName); - await this.clearScheduledAlarm(alarmName); + await this.clearScheduledAlarm(taskName); } /** @@ -226,14 +223,6 @@ export class BrowserTaskSchedulerServiceImplementation return; } - // We should always prioritize user-based alarms over non-user-based alarms. If a non-user-based alarm - // exists when the user-based alarm is being created, we want to clear the non-user-based alarm. - const taskName = this.getTaskFromAlarmName(alarmName); - const existingTaskBasedAlarm = await this.getAlarm(taskName); - if (existingTaskBasedAlarm) { - await this.clearScheduledAlarm(taskName); - } - await this.createAlarm(alarmName, createInfo); await this.setActiveAlarm(alarmName, createInfo); } @@ -331,43 +320,10 @@ export class BrowserTaskSchedulerServiceImplementation if (handler) { handler(); } - - // We should always prioritize user-based alarms over non-user-based alarms. As a result, if a triggered - // alarm is not user-based, we want to verify if the alarm should continue to exist. If an alarm exists - // for the same task that is user-based, we want to clear the non-user-based alarm. - if (alarmName === taskName) { - const taskName = this.getTaskFromAlarmName(alarmName); - const existingUserBasedAlarm = await this.getAlarm(taskName); - if (existingUserBasedAlarm) { - await this.clearScheduledAlarm(taskName); - } - } } /** - * Gets the active user id from state. - */ - private async getActiveUserId(): Promise { - return await firstValueFrom(this.stateProvider.activeUserId$); - } - - /** - * Gets the active user alarm name by appending the active user id to the task name. - * - * @param taskName - The task name to append the active user id to. - */ - private async getActiveUserAlarmName(taskName: ScheduledTaskName): Promise { - const activeUserId = await this.getActiveUserId(); - if (!activeUserId) { - return taskName; - } - - return `${taskName}__${activeUserId}`; - } - - /** - * Parses and returns the task name from an alarm name. If the alarm name - * contains a user id, it will return the task name without the user id. + * Parses and returns the task name from an alarm name. * * @param alarmName - The alarm name to parse. */