[PM-20032] Give option to skip token refresh on fullSync (#14423)

* Give option to skip token refresh on fullSync

* Fix listener

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<SyncOptions>;
+    }[] = [
+      {
+        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<FullSyncFinishedMessage>();
+      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({

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<FullSyncMessage>("doFullSync");
 
@@ -60,9 +61,20 @@ export class ForegroundSyncService extends CoreSyncService {
     );
   }
 
-  async fullSync(forceSync: boolean, allowThrowOnError: boolean = false): Promise<boolean> {
+  async fullSync(
+    forceSync: boolean,
+    allowThrowOnErrorOrOptions?: boolean | SyncOptions,
+  ): Promise<boolean> {
     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);
       }
 

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",

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<void> {
     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,

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<Date>(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<boolean>;
   abstract fullSync(forceSync: boolean, allowThrowOnError?: boolean): Promise<boolean>;
 
   async getLastSync(): Promise<Date> {

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<InternalMasterPasswordServiceAbstraction>;
+  let accountService: MockProxy<AccountService>;
+  let apiService: MockProxy<ApiService>;
+  let domainSettingsService: MockProxy<DomainSettingsService>;
+  let folderService: MockProxy<InternalFolderService>;
+  let cipherService: MockProxy<CipherService>;
+  let keyService: MockProxy<KeyService>;
+  let collectionService: MockProxy<CollectionService>;
+  let messageSender: MockProxy<MessageSender>;
+  let policyService: MockProxy<InternalPolicyService>;
+  let sendService: MockProxy<InternalSendService>;
+  let logService: MockProxy<LogService>;
+  let keyConnectorService: MockProxy<KeyConnectorService>;
+  let stateService: MockProxy<StateService>;
+  let providerService: MockProxy<ProviderService>;
+  let folderApiService: MockProxy<FolderApiServiceAbstraction>;
+  let organizationService: MockProxy<InternalOrganizationServiceAbstraction>;
+  let sendApiService: MockProxy<SendApiService>;
+  let userDecryptionOptionsService: MockProxy<UserDecryptionOptionsServiceAbstraction>;
+  let avatarService: MockProxy<AvatarService>;
+  let logoutCallback: jest.Mock<Promise<void>, [logoutReason: LogoutReason, userId?: UserId]>;
+  let billingAccountProfileStateService: MockProxy<BillingAccountProfileStateService>;
+  let tokenService: MockProxy<TokenService>;
+  let authService: MockProxy<AuthService>;
+  let stateProvider: MockProxy<StateProvider>;
+
+  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);
+    });
+  });
+});

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<boolean> {
+  override async fullSync(
+    forceSync: boolean,
+    allowThrowOnErrorOrOptions?: boolean | SyncOptions,
+  ): Promise<boolean> {
+    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 {
+      if (!skipTokenRefresh) {
         await this.apiService.refreshIdentityToken();
+      }
       const response = await this.apiService.getSync();
 
       await this.syncProfile(response.profile);

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<boolean>;
+
+  /**
+   * @deprecated Use the overload taking {@link SyncOptions} instead.
    */
   abstract fullSync(forceSync: boolean, allowThrowOnError?: boolean): Promise<boolean>;