diff --git a/apps/browser/src/platform/sync/foreground-sync.service.spec.ts b/apps/browser/src/platform/sync/foreground-sync.service.spec.ts index f5daff9381..34ee4fa0f7 100644 --- a/apps/browser/src/platform/sync/foreground-sync.service.spec.ts +++ b/apps/browser/src/platform/sync/foreground-sync.service.spec.ts @@ -8,6 +8,7 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service" import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { MessageListener, MessageSender } from "@bitwarden/common/platform/messaging"; import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { SyncOptions } from "@bitwarden/common/platform/sync/sync.service"; import { FakeStateProvider, mockAccountServiceWith } from "@bitwarden/common/spec"; import { SendApiService } from "@bitwarden/common/tools/send/services/send-api.service.abstraction"; import { InternalSendService } from "@bitwarden/common/tools/send/services/send.service.abstraction"; @@ -80,7 +81,72 @@ describe("ForegroundSyncService", () => { const fullSyncPromise = sut.fullSync(true, false); expect(sut.syncInProgress).toBe(true); - const requestId = getAndAssertRequestId({ forceSync: true, allowThrowOnError: false }); + const requestId = getAndAssertRequestId({ + forceSync: true, + options: { allowThrowOnError: false, skipTokenRefresh: false }, + }); + + // Pretend the sync has finished + messages.next({ successfully: true, errorMessage: null, requestId: requestId }); + + const result = await fullSyncPromise; + + expect(sut.syncInProgress).toBe(false); + expect(result).toBe(true); + }); + + const testData: { + input: boolean | SyncOptions | undefined; + normalized: Required; + }[] = [ + { + input: undefined, + normalized: { allowThrowOnError: false, skipTokenRefresh: false }, + }, + { + input: true, + normalized: { allowThrowOnError: true, skipTokenRefresh: false }, + }, + { + input: false, + normalized: { allowThrowOnError: false, skipTokenRefresh: false }, + }, + { + input: { allowThrowOnError: false }, + normalized: { allowThrowOnError: false, skipTokenRefresh: false }, + }, + { + input: { allowThrowOnError: true }, + normalized: { allowThrowOnError: true, skipTokenRefresh: false }, + }, + { + input: { allowThrowOnError: false, skipTokenRefresh: false }, + normalized: { allowThrowOnError: false, skipTokenRefresh: false }, + }, + { + input: { allowThrowOnError: true, skipTokenRefresh: false }, + normalized: { allowThrowOnError: true, skipTokenRefresh: false }, + }, + { + input: { allowThrowOnError: true, skipTokenRefresh: true }, + normalized: { allowThrowOnError: true, skipTokenRefresh: true }, + }, + { + input: { allowThrowOnError: false, skipTokenRefresh: true }, + normalized: { allowThrowOnError: false, skipTokenRefresh: true }, + }, + ]; + + it.each(testData)("normalize input $input options correctly", async ({ input, normalized }) => { + const messages = new Subject(); + messageListener.messages$.mockReturnValue(messages); + const fullSyncPromise = sut.fullSync(true, input); + expect(sut.syncInProgress).toBe(true); + + const requestId = getAndAssertRequestId({ + forceSync: true, + options: normalized, + }); // Pretend the sync has finished messages.next({ successfully: true, errorMessage: null, requestId: requestId }); @@ -97,7 +163,10 @@ describe("ForegroundSyncService", () => { const fullSyncPromise = sut.fullSync(false, false); expect(sut.syncInProgress).toBe(true); - const requestId = getAndAssertRequestId({ forceSync: false, allowThrowOnError: false }); + const requestId = getAndAssertRequestId({ + forceSync: false, + options: { allowThrowOnError: false, skipTokenRefresh: false }, + }); // Pretend the sync has finished messages.next({ @@ -118,7 +187,10 @@ describe("ForegroundSyncService", () => { const fullSyncPromise = sut.fullSync(true, true); expect(sut.syncInProgress).toBe(true); - const requestId = getAndAssertRequestId({ forceSync: true, allowThrowOnError: true }); + const requestId = getAndAssertRequestId({ + forceSync: true, + options: { allowThrowOnError: true, skipTokenRefresh: false }, + }); // Pretend the sync has finished messages.next({ diff --git a/apps/browser/src/platform/sync/foreground-sync.service.ts b/apps/browser/src/platform/sync/foreground-sync.service.ts index a6ed728185..ce776f5368 100644 --- a/apps/browser/src/platform/sync/foreground-sync.service.ts +++ b/apps/browser/src/platform/sync/foreground-sync.service.ts @@ -14,6 +14,7 @@ import { import { Utils } from "@bitwarden/common/platform/misc/utils"; import { StateProvider } from "@bitwarden/common/platform/state"; import { CoreSyncService } from "@bitwarden/common/platform/sync/internal"; +import { SyncOptions } from "@bitwarden/common/platform/sync/sync.service"; import { SendApiService } from "@bitwarden/common/tools/send/services/send-api.service.abstraction"; import { InternalSendService } from "@bitwarden/common/tools/send/services/send.service.abstraction"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; @@ -22,7 +23,7 @@ import { InternalFolderService } from "@bitwarden/common/vault/abstractions/fold import { FULL_SYNC_FINISHED } from "./sync-service.listener"; -export type FullSyncMessage = { forceSync: boolean; allowThrowOnError: boolean; requestId: string }; +export type FullSyncMessage = { forceSync: boolean; options: SyncOptions; requestId: string }; export const DO_FULL_SYNC = new CommandDefinition("doFullSync"); @@ -60,9 +61,20 @@ export class ForegroundSyncService extends CoreSyncService { ); } - async fullSync(forceSync: boolean, allowThrowOnError: boolean = false): Promise { + async fullSync( + forceSync: boolean, + allowThrowOnErrorOrOptions?: boolean | SyncOptions, + ): Promise { this.syncInProgress = true; try { + // Normalize options + const options = + typeof allowThrowOnErrorOrOptions === "boolean" + ? { allowThrowOnError: allowThrowOnErrorOrOptions, skipTokenRefresh: false } + : { + allowThrowOnError: allowThrowOnErrorOrOptions?.allowThrowOnError ?? false, + skipTokenRefresh: allowThrowOnErrorOrOptions?.skipTokenRefresh ?? false, + }; const requestId = Utils.newGuid(); const syncCompletedPromise = firstValueFrom( this.messageListener.messages$(FULL_SYNC_FINISHED).pipe( @@ -79,10 +91,10 @@ export class ForegroundSyncService extends CoreSyncService { }), ), ); - this.messageSender.send(DO_FULL_SYNC, { forceSync, allowThrowOnError, requestId }); + this.messageSender.send(DO_FULL_SYNC, { forceSync, options, requestId }); const result = await syncCompletedPromise; - if (allowThrowOnError && result.errorMessage != null) { + if (options.allowThrowOnError && result.errorMessage != null) { throw new Error(result.errorMessage); } diff --git a/apps/browser/src/platform/sync/sync-service.listener.spec.ts b/apps/browser/src/platform/sync/sync-service.listener.spec.ts index 51f97e9f87..9682e2cdb5 100644 --- a/apps/browser/src/platform/sync/sync-service.listener.spec.ts +++ b/apps/browser/src/platform/sync/sync-service.listener.spec.ts @@ -27,11 +27,18 @@ describe("SyncServiceListener", () => { const emissionPromise = firstValueFrom(listener); syncService.fullSync.mockResolvedValueOnce(value); - messages.next({ forceSync: true, allowThrowOnError: false, requestId: "1" }); + messages.next({ + forceSync: true, + options: { allowThrowOnError: false, skipTokenRefresh: false }, + requestId: "1", + }); await emissionPromise; - expect(syncService.fullSync).toHaveBeenCalledWith(true, false); + expect(syncService.fullSync).toHaveBeenCalledWith(true, { + allowThrowOnError: false, + skipTokenRefresh: false, + }); expect(messageSender.send).toHaveBeenCalledWith(FULL_SYNC_FINISHED, { successfully: value, errorMessage: null, @@ -45,11 +52,18 @@ describe("SyncServiceListener", () => { const emissionPromise = firstValueFrom(listener); syncService.fullSync.mockRejectedValueOnce(new Error("SyncError")); - messages.next({ forceSync: true, allowThrowOnError: false, requestId: "1" }); + messages.next({ + forceSync: true, + options: { allowThrowOnError: false, skipTokenRefresh: false }, + requestId: "1", + }); await emissionPromise; - expect(syncService.fullSync).toHaveBeenCalledWith(true, false); + expect(syncService.fullSync).toHaveBeenCalledWith(true, { + allowThrowOnError: false, + skipTokenRefresh: false, + }); expect(messageSender.send).toHaveBeenCalledWith(FULL_SYNC_FINISHED, { successfully: false, errorMessage: "SyncError", diff --git a/apps/browser/src/platform/sync/sync-service.listener.ts b/apps/browser/src/platform/sync/sync-service.listener.ts index b717152864..4274eafcf6 100644 --- a/apps/browser/src/platform/sync/sync-service.listener.ts +++ b/apps/browser/src/platform/sync/sync-service.listener.ts @@ -9,6 +9,7 @@ import { MessageSender, isExternalMessage, } from "@bitwarden/common/platform/messaging"; +import { SyncOptions } from "@bitwarden/common/platform/sync/sync.service"; import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { DO_FULL_SYNC } from "./foreground-sync.service"; @@ -34,15 +35,15 @@ export class SyncServiceListener { listener$(): Observable { return this.messageListener.messages$(DO_FULL_SYNC).pipe( filter((message) => isExternalMessage(message)), - concatMap(async ({ forceSync, allowThrowOnError, requestId }) => { - await this.doFullSync(forceSync, allowThrowOnError, requestId); + concatMap(async ({ forceSync, options, requestId }) => { + await this.doFullSync(forceSync, options, requestId); }), ); } - private async doFullSync(forceSync: boolean, allowThrowOnError: boolean, requestId: string) { + private async doFullSync(forceSync: boolean, options: SyncOptions, requestId: string) { try { - const result = await this.syncService.fullSync(forceSync, allowThrowOnError); + const result = await this.syncService.fullSync(forceSync, options); this.messageSender.send(FULL_SYNC_FINISHED, { successfully: result, errorMessage: null, diff --git a/libs/common/src/platform/sync/core-sync.service.ts b/libs/common/src/platform/sync/core-sync.service.ts index 1865ffb852..4020c75f76 100644 --- a/libs/common/src/platform/sync/core-sync.service.ts +++ b/libs/common/src/platform/sync/core-sync.service.ts @@ -28,6 +28,8 @@ import { StateService } from "../abstractions/state.service"; import { MessageSender } from "../messaging"; import { StateProvider, SYNC_DISK, UserKeyDefinition } from "../state"; +import { SyncOptions } from "./sync.service"; + const LAST_SYNC_DATE = new UserKeyDefinition(SYNC_DISK, "lastSync", { deserializer: (d) => (d != null ? new Date(d) : null), clearOn: ["logout"], @@ -55,6 +57,7 @@ export abstract class CoreSyncService implements SyncService { protected readonly stateProvider: StateProvider, ) {} + abstract fullSync(forceSync: boolean, syncOptions?: SyncOptions): Promise; abstract fullSync(forceSync: boolean, allowThrowOnError?: boolean): Promise; async getLastSync(): Promise { diff --git a/libs/common/src/platform/sync/default-sync.service.spec.ts b/libs/common/src/platform/sync/default-sync.service.spec.ts new file mode 100644 index 0000000000..ded06c8be6 --- /dev/null +++ b/libs/common/src/platform/sync/default-sync.service.spec.ts @@ -0,0 +1,199 @@ +import { mock, MockProxy } from "jest-mock-extended"; +import { of } from "rxjs"; + +import { CollectionService } from "@bitwarden/admin-console/common"; +import { + LogoutReason, + UserDecryptionOptions, + UserDecryptionOptionsServiceAbstraction, +} from "@bitwarden/auth/common"; +import { KeyService } from "@bitwarden/key-management"; + +import { Matrix } from "../../../spec/matrix"; +import { ApiService } from "../../abstractions/api.service"; +import { InternalOrganizationServiceAbstraction } from "../../admin-console/abstractions/organization/organization.service.abstraction"; +import { InternalPolicyService } from "../../admin-console/abstractions/policy/policy.service.abstraction"; +import { ProviderService } from "../../admin-console/abstractions/provider.service"; +import { Account, AccountService } from "../../auth/abstractions/account.service"; +import { AuthService } from "../../auth/abstractions/auth.service"; +import { AvatarService } from "../../auth/abstractions/avatar.service"; +import { TokenService } from "../../auth/abstractions/token.service"; +import { AuthenticationStatus } from "../../auth/enums/authentication-status"; +import { DomainSettingsService } from "../../autofill/services/domain-settings.service"; +import { BillingAccountProfileStateService } from "../../billing/abstractions"; +import { KeyConnectorService } from "../../key-management/key-connector/abstractions/key-connector.service"; +import { InternalMasterPasswordServiceAbstraction } from "../../key-management/master-password/abstractions/master-password.service.abstraction"; +import { SendApiService } from "../../tools/send/services/send-api.service.abstraction"; +import { InternalSendService } from "../../tools/send/services/send.service.abstraction"; +import { UserId } from "../../types/guid"; +import { CipherService } from "../../vault/abstractions/cipher.service"; +import { FolderApiServiceAbstraction } from "../../vault/abstractions/folder/folder-api.service.abstraction"; +import { InternalFolderService } from "../../vault/abstractions/folder/folder.service.abstraction"; +import { LogService } from "../abstractions/log.service"; +import { StateService } from "../abstractions/state.service"; +import { MessageSender } from "../messaging"; +import { StateProvider } from "../state"; + +import { DefaultSyncService } from "./default-sync.service"; +import { SyncResponse } from "./sync.response"; + +describe("DefaultSyncService", () => { + let masterPasswordAbstraction: MockProxy; + let accountService: MockProxy; + let apiService: MockProxy; + let domainSettingsService: MockProxy; + let folderService: MockProxy; + let cipherService: MockProxy; + let keyService: MockProxy; + let collectionService: MockProxy; + let messageSender: MockProxy; + let policyService: MockProxy; + let sendService: MockProxy; + let logService: MockProxy; + let keyConnectorService: MockProxy; + let stateService: MockProxy; + let providerService: MockProxy; + let folderApiService: MockProxy; + let organizationService: MockProxy; + let sendApiService: MockProxy; + let userDecryptionOptionsService: MockProxy; + let avatarService: MockProxy; + let logoutCallback: jest.Mock, [logoutReason: LogoutReason, userId?: UserId]>; + let billingAccountProfileStateService: MockProxy; + let tokenService: MockProxy; + let authService: MockProxy; + let stateProvider: MockProxy; + + let sut: DefaultSyncService; + + beforeEach(() => { + masterPasswordAbstraction = mock(); + accountService = mock(); + apiService = mock(); + domainSettingsService = mock(); + folderService = mock(); + cipherService = mock(); + keyService = mock(); + collectionService = mock(); + messageSender = mock(); + policyService = mock(); + sendService = mock(); + logService = mock(); + keyConnectorService = mock(); + stateService = mock(); + providerService = mock(); + folderApiService = mock(); + organizationService = mock(); + sendApiService = mock(); + userDecryptionOptionsService = mock(); + avatarService = mock(); + logoutCallback = jest.fn(); + billingAccountProfileStateService = mock(); + tokenService = mock(); + authService = mock(); + stateProvider = mock(); + + sut = new DefaultSyncService( + masterPasswordAbstraction, + accountService, + apiService, + domainSettingsService, + folderService, + cipherService, + keyService, + collectionService, + messageSender, + policyService, + sendService, + logService, + keyConnectorService, + stateService, + providerService, + folderApiService, + organizationService, + sendApiService, + userDecryptionOptionsService, + avatarService, + logoutCallback, + billingAccountProfileStateService, + tokenService, + authService, + stateProvider, + ); + }); + + const user1 = "user1" as UserId; + + describe("fullSync", () => { + beforeEach(() => { + accountService.activeAccount$ = of({ id: user1 } as Account); + Matrix.autoMockMethod(authService.authStatusFor$, () => of(AuthenticationStatus.Unlocked)); + apiService.getSync.mockResolvedValue( + new SyncResponse({ + profile: { + id: user1, + }, + folders: [], + collections: [], + ciphers: [], + sends: [], + domains: [], + policies: [], + }), + ); + Matrix.autoMockMethod(userDecryptionOptionsService.userDecryptionOptionsById$, () => + of({ hasMasterPassword: true } satisfies UserDecryptionOptions), + ); + stateProvider.getUser.mockReturnValue(mock()); + }); + + it("does a token refresh when option missing from options", async () => { + await sut.fullSync(true, { allowThrowOnError: false }); + + expect(apiService.refreshIdentityToken).toHaveBeenCalledTimes(1); + expect(apiService.getSync).toHaveBeenCalledTimes(1); + }); + + it("does a token refresh when boolean passed in", async () => { + await sut.fullSync(true, false); + + expect(apiService.refreshIdentityToken).toHaveBeenCalledTimes(1); + expect(apiService.getSync).toHaveBeenCalledTimes(1); + }); + + it("does a token refresh when skipTokenRefresh option passed in with false and allowThrowOnError also passed in", async () => { + await sut.fullSync(true, { allowThrowOnError: false, skipTokenRefresh: false }); + + expect(apiService.refreshIdentityToken).toHaveBeenCalledTimes(1); + expect(apiService.getSync).toHaveBeenCalledTimes(1); + }); + + it("does a token refresh when skipTokenRefresh option passed in with false by itself", async () => { + await sut.fullSync(true, { skipTokenRefresh: false }); + + expect(apiService.refreshIdentityToken).toHaveBeenCalledTimes(1); + expect(apiService.getSync).toHaveBeenCalledTimes(1); + }); + + it("does not do a token refresh when skipTokenRefresh passed in as true", async () => { + await sut.fullSync(true, { skipTokenRefresh: true }); + + expect(apiService.refreshIdentityToken).not.toHaveBeenCalled(); + expect(apiService.getSync).toHaveBeenCalledTimes(1); + }); + + it("does not do a token refresh when skipTokenRefresh passed in as true and allowThrowOnError also passed in", async () => { + await sut.fullSync(true, { allowThrowOnError: false, skipTokenRefresh: true }); + + expect(apiService.refreshIdentityToken).not.toHaveBeenCalled(); + expect(apiService.getSync).toHaveBeenCalledTimes(1); + }); + + it("does a token refresh when nothing passed in", async () => { + await sut.fullSync(true); + + expect(apiService.refreshIdentityToken).toHaveBeenCalledTimes(1); + expect(apiService.getSync).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/libs/common/src/platform/sync/default-sync.service.ts b/libs/common/src/platform/sync/default-sync.service.ts index a6b1b97464..faf54f1191 100644 --- a/libs/common/src/platform/sync/default-sync.service.ts +++ b/libs/common/src/platform/sync/default-sync.service.ts @@ -54,6 +54,7 @@ import { MessageSender } from "../messaging"; import { StateProvider } from "../state"; import { CoreSyncService } from "./core-sync.service"; +import { SyncOptions } from "./sync.service"; export class DefaultSyncService extends CoreSyncService { syncInProgress = false; @@ -102,7 +103,15 @@ export class DefaultSyncService extends CoreSyncService { ); } - override async fullSync(forceSync: boolean, allowThrowOnError = false): Promise { + override async fullSync( + forceSync: boolean, + allowThrowOnErrorOrOptions?: boolean | SyncOptions, + ): Promise { + const { allowThrowOnError = false, skipTokenRefresh = false } = + typeof allowThrowOnErrorOrOptions === "boolean" + ? { allowThrowOnError: allowThrowOnErrorOrOptions } + : (allowThrowOnErrorOrOptions ?? {}); + const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id))); this.syncStarted(); const authStatus = await firstValueFrom(this.authService.authStatusFor$(userId)); @@ -127,7 +136,9 @@ export class DefaultSyncService extends CoreSyncService { } try { - await this.apiService.refreshIdentityToken(); + if (!skipTokenRefresh) { + await this.apiService.refreshIdentityToken(); + } const response = await this.apiService.getSync(); await this.syncProfile(response.profile); diff --git a/libs/common/src/platform/sync/sync.service.ts b/libs/common/src/platform/sync/sync.service.ts index 967e4db27a..6ef62fc9cb 100644 --- a/libs/common/src/platform/sync/sync.service.ts +++ b/libs/common/src/platform/sync/sync.service.ts @@ -7,6 +7,26 @@ import { } from "../../models/response/notification.response"; import { UserId } from "../../types/guid"; +/** + * A set of options for configuring how a {@link SyncService.fullSync} call should behave. + */ +export type SyncOptions = { + /** + * A boolean dictating whether or not caught errors should be rethrown. + * `true` if they can be rethrown, `false` if they should not be rethrown. + * @default false + */ + allowThrowOnError?: boolean; + /** + * A boolean dictating whether or not to do a token refresh before doing the sync. + * `true` if the refresh can be skipped, likely because one was done soon before the call to + * `fullSync`. `false` if the token refresh should be done before getting data. + * + * @default false + */ + skipTokenRefresh?: boolean; +}; + /** * A class encapsulating sync operations and data. */ @@ -47,9 +67,12 @@ export abstract class SyncService { * as long as the current user is authenticated. If `false` it will only sync if either a sync * has not happened before or the last sync date for the active user is before their account * revision date. Try to always use `false` if possible. - * - * @param allowThrowOnError A boolean dictating whether or not caught errors should be rethrown. - * `true` if they can be rethrown, `false` if they should not be rethrown. + * @param syncOptions Options for customizing how the sync call should behave. + */ + abstract fullSync(forceSync: boolean, syncOptions?: SyncOptions): Promise; + + /** + * @deprecated Use the overload taking {@link SyncOptions} instead. */ abstract fullSync(forceSync: boolean, allowThrowOnError?: boolean): Promise;