CherryPick/Auth/PM-36080 (#20452) (#20463)

* Auth/PM-36080 (#20452)

* PM-36080 - Adjust logout behavior

* PM-36080 - On init, reconcile tokens with active sessions

* PM-36080 - TokenSvc - document determineStorageLocation

(cherry picked from commit 4d3d999eea)

* merge recent build workflow update from main

---------

Co-authored-by: Amy Galles <9685081+AmyLGalles@users.noreply.github.com>
Co-authored-by: Vince Grassia <593223+vgrassia@users.noreply.github.com>

.github/workflows/build-browser.yml

@@ -582,8 +582,8 @@ jobs:
         id: retrieve-secrets
         uses: bitwarden/gh-actions/get-keyvault-secrets@main
         with:
-          keyvault: "bitwarden-ci"
-          secrets: "crowdin-api-token"
+          keyvault: "gh-clients"
+          secrets: "CROWDIN-API-TOKEN"
 
       - name: Log out from Azure
         uses: bitwarden/gh-actions/azure-logout@main
@@ -592,61 +592,10 @@ jobs:
         uses: crowdin/github-action@8818ff65bfc4322384f983ea37e3926948c11745 # v2.15.0
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-          CROWDIN_API_TOKEN: ${{ steps.retrieve-secrets.outputs.crowdin-api-token }}
+          CROWDIN_API_TOKEN: ${{ steps.retrieve-secrets.outputs.CROWDIN-API-TOKEN }}
           CROWDIN_PROJECT_ID: "268134"
         with:
           config: apps/browser/crowdin.yml
           crowdin_branch_name: main
           upload_sources: true
           upload_translations: false
-
-
-  check-failures:
-    name: Check for failures
-    if: always()
-    runs-on: ubuntu-22.04
-    permissions:
-      contents: read
-      id-token: write
-    needs:
-      - setup
-      - locales-test
-      - build-source
-      - build
-      - build-safari
-      - crowdin-push
-    steps:
-      - name: Check if any job failed
-        if: |
-          github.event_name != 'pull_request_target'
-          && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/rc' || github.ref == 'refs/heads/hotfix-rc-browser')
-          && contains(needs.*.result, 'failure')
-        run: exit 1
-
-      - name: Log in to Azure
-        if: failure()
-        uses: bitwarden/gh-actions/azure-login@main
-        with:
-          subscription_id: ${{ secrets.AZURE_SUBSCRIPTION_ID }}
-          tenant_id: ${{ secrets.AZURE_TENANT_ID }}
-          client_id: ${{ secrets.AZURE_CLIENT_ID }}
-
-      - name: Retrieve secrets
-        id: retrieve-secrets
-        if: failure()
-        uses: bitwarden/gh-actions/get-keyvault-secrets@main
-        with:
-          keyvault: "bitwarden-ci"
-          secrets: "devops-alerts-slack-webhook-url"
-
-      - name: Log out from Azure
-        if: failure()
-        uses: bitwarden/gh-actions/azure-logout@main
-
-      - name: Notify Slack on failure
-        uses: act10ns/slack@44541246747a30eb3102d87f7a4cc5471b0ffb7d # v2.1.0
-        if: failure()
-        env:
-          SLACK_WEBHOOK_URL: ${{ steps.retrieve-secrets.outputs.devops-alerts-slack-webhook-url }}
-        with:
-          status: ${{ job.status }}

apps/browser/src/background/main.background.ts

@@ -1653,9 +1653,11 @@ export default class MainBackground {
     // This is here instead of in the InitService b/c we don't plan for
     // side effects to run in the Browser InitService.
     const accounts = await firstValueFrom(this.accountService.accounts$);
+    const userIds = Object.keys(accounts) as UserId[];
+    await this.tokenService.cleanupTokenStorage(userIds);
 
     const setUserKeyInMemoryPromises = [];
-    for (const userId of Object.keys(accounts) as UserId[]) {
+    for (const userId of userIds) {
       // For each acct, we must await the process of setting the user key in memory
       // if the auto user key is set to avoid race conditions of any code trying to access
       // the user key from mem.
@@ -1869,7 +1871,7 @@ export default class MainBackground {
     const needStorageReseed = await this.needsStorageReseed(userBeingLoggedOut);
 
     await this.stateService.clean({ userId: userBeingLoggedOut });
-    await this.tokenService.clearAccessToken(userBeingLoggedOut);
+    await this.tokenService.clearTokens(userBeingLoggedOut);
     await this.accountService.clean(userBeingLoggedOut);
 
     await this.stateEventRunnerService.handleEvent("logout", userBeingLoggedOut);

apps/cli/src/service-container/service-container.ts

@@ -1117,7 +1117,7 @@ export class ServiceContainer {
     await this.stateEventRunnerService.handleEvent("logout", userId as UserId);
 
     await this.stateService.clean({ userId: userId });
-    await this.tokenService.clearAccessToken(userId);
+    await this.tokenService.clearTokens(userId);
     await this.accountService.clean(userId as UserId);
     await this.accountService.switchAccount(null);
     process.env.BW_SESSION = undefined;
@@ -1137,6 +1137,9 @@ export class ServiceContainer {
     await this.i18nService.init();
     this.twoFactorService.init();
 
+    const accounts = await firstValueFrom(this.accountService.accounts$);
+    await this.tokenService.cleanupTokenStorage(Object.keys(accounts) as UserId[]);
+
     // If a user has a BW_SESSION key stored in their env (not process.env.BW_SESSION),
     // this should set the user key to unlock the vault on init.
     // TODO: ideally, we wouldn't want to do this here but instead only for commands that require the vault to be unlocked

apps/desktop/src/app/app.component.ts

@@ -737,7 +737,7 @@ export class AppComponent implements OnInit, OnDestroy {
       await this.stateEventRunnerService.handleEvent("logout", userBeingLoggedOut);
 
       await this.stateService.clean({ userId: userBeingLoggedOut });
-      await this.tokenService.clearAccessToken(userBeingLoggedOut);
+      await this.tokenService.clearTokens(userBeingLoggedOut);
       await this.accountService.clean(userBeingLoggedOut);
 
       // HACK: Wait for the user logging outs authentication status to transition to LoggedOut

apps/desktop/src/app/services/init.service.ts

@@ -4,6 +4,7 @@ import { firstValueFrom } from "rxjs";
 import { AbstractThemingService } from "@bitwarden/angular/platform/services/theming/theming.service.abstraction";
 import { WINDOW } from "@bitwarden/angular/services/injection-tokens";
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { TokenService } from "@bitwarden/common/auth/abstractions/token.service";
 import { TwoFactorService } from "@bitwarden/common/auth/two-factor";
 import { EventUploadService as EventUploadServiceAbstraction } from "@bitwarden/common/dirt/event-logs";
 import { EventUploadService } from "@bitwarden/common/dirt/event-logs/services/event-upload.service";
@@ -50,6 +51,7 @@ export class InitService {
     private encryptService: EncryptService,
     private userAutoUnlockKeyService: UserAutoUnlockKeyService,
     private accountService: AccountService,
+    private tokenService: TokenService,
     private versionService: VersionService,
     private sshAgentService: SshAgentService,
     private autofillService: DesktopAutofillService,
@@ -70,8 +72,11 @@ export class InitService {
       await this.migrationRunner.waitForCompletion(); // Desktop will run migrations in the main process
 
       const accounts = await firstValueFrom(this.accountService.accounts$);
+      const userIds = Object.keys(accounts) as UserId[];
+      await this.tokenService.cleanupTokenStorage(userIds);
+
       const setUserKeyInMemoryPromises = [];
-      for (const userId of Object.keys(accounts) as UserId[]) {
+      for (const userId of userIds) {
         // For each acct, we must await the process of setting the user key in memory
         // if the auto user key is set to avoid race conditions of any code trying to access
         // the user key from mem.

apps/web/src/app/app.component.ts

@@ -265,7 +265,7 @@ export class AppComponent implements OnDestroy, OnInit {
     await this.searchService.clearIndex(userId);
     this.authService.logOut(async () => {
       await this.stateService.clean({ userId: userId });
-      await this.tokenService.clearAccessToken(userId);
+      await this.tokenService.clearTokens(userId);
       await this.accountService.clean(userId);
       await this.accountService.switchAccount(null);
 

apps/web/src/app/core/init.service.ts

@@ -4,6 +4,7 @@ import { firstValueFrom } from "rxjs";
 import { AbstractThemingService } from "@bitwarden/angular/platform/services/theming/theming.service.abstraction";
 import { WINDOW } from "@bitwarden/angular/services/injection-tokens";
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { TokenService } from "@bitwarden/common/auth/abstractions/token.service";
 import { TwoFactorService } from "@bitwarden/common/auth/two-factor";
 import { EventUploadService as EventUploadServiceAbstraction } from "@bitwarden/common/dirt/event-logs";
 import { EventUploadService } from "@bitwarden/common/dirt/event-logs/services/event-upload.service";
@@ -16,6 +17,7 @@ import { ServerNotificationsService } from "@bitwarden/common/platform/server-no
 import { ContainerService } from "@bitwarden/common/platform/services/container.service";
 import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner";
 import { UserAutoUnlockKeyService } from "@bitwarden/common/platform/services/user-auto-unlock-key.service";
+import { UserId } from "@bitwarden/common/types/guid";
 import { TaskService } from "@bitwarden/common/vault/tasks";
 import { KeyService as KeyServiceAbstraction } from "@bitwarden/key-management";
 
@@ -35,6 +37,7 @@ export class InitService {
     private encryptService: EncryptService,
     private userAutoUnlockKeyService: UserAutoUnlockKeyService,
     private accountService: AccountService,
+    private tokenService: TokenService,
     private versionService: VersionService,
     private ipcService: IpcService,
     private sdkLoadService: SdkLoadService,
@@ -48,6 +51,9 @@ export class InitService {
       await this.sdkLoadService.loadAndInit();
       await this.migrationRunner.run();
 
+      const accounts = await firstValueFrom(this.accountService.accounts$);
+      await this.tokenService.cleanupTokenStorage(Object.keys(accounts) as UserId[]);
+
       const activeAccount = await firstValueFrom(this.accountService.activeAccount$);
       if (activeAccount) {
         // If there is an active account, we must await the process of setting the user key in memory

libs/common/src/auth/abstractions/token.service.ts

@@ -40,6 +40,18 @@ export abstract class TokenService {
    */
   abstract clearTokens(userId?: UserId): Promise<void>;
 
+  /**
+   * Ensures token storage is consistent with account state on app init.
+   * For each provided user id without an access token, clears all remaining tokens
+   * from all storage layers (disk and secure storage).
+   *
+   * A locked account always has an access token on disk, so this only affects
+   * accounts that have been logged out. Safe to call on every startup — a no-op
+   * when storage is already consistent.
+   * @param userIds - The user ids to check and clean up.
+   */
+  abstract cleanupTokenStorage(userIds: UserId[]): Promise<void>;
+
   /**
    * Sets the access token in memory or disk based on the given vaultTimeoutAction and vaultTimeout
    * and the user id read off the access token. The other storage location is always cleared to

libs/common/src/auth/services/token.service.spec.ts

@@ -2770,6 +2770,88 @@ describe("TokenService", () => {
     });
   });
 
+  describe("cleanupTokenStorage", () => {
+    it("calls clearTokens for a user id with no access token", async () => {
+      // Arrange
+      const userId = "userId" as UserId;
+      tokenService.getAccessToken = jest.fn().mockResolvedValue(null);
+      tokenService.clearTokens = jest.fn().mockResolvedValue(undefined);
+
+      // Act
+      await tokenService.cleanupTokenStorage([userId]);
+
+      // Assert
+      expect(tokenService.clearTokens).toHaveBeenCalledWith(userId);
+    });
+
+    it("does not call clearTokens for a user id with an access token (locked user)", async () => {
+      // Arrange
+      const userId = "userId" as UserId;
+      tokenService.getAccessToken = jest.fn().mockResolvedValue("accessToken");
+      tokenService.clearTokens = jest.fn().mockResolvedValue(undefined);
+
+      // Act
+      await tokenService.cleanupTokenStorage([userId]);
+
+      // Assert
+      expect(tokenService.clearTokens).not.toHaveBeenCalled();
+    });
+
+    it("only clears tokens for user ids without an access token when given a mixed list", async () => {
+      // Arrange
+      const loggedOutUserId = "loggedOutUserId" as UserId;
+      const lockedUserId = "lockedUserId" as UserId;
+      tokenService.getAccessToken = jest.fn().mockImplementation((userId: UserId) => {
+        return Promise.resolve(userId === lockedUserId ? "accessToken" : null);
+      });
+      tokenService.clearTokens = jest.fn().mockResolvedValue(undefined);
+
+      // Act
+      await tokenService.cleanupTokenStorage([loggedOutUserId, lockedUserId]);
+
+      // Assert
+      expect(tokenService.clearTokens).toHaveBeenCalledTimes(1);
+      expect(tokenService.clearTokens).toHaveBeenCalledWith(loggedOutUserId);
+    });
+
+    it("calls clearTokens for a user id with an undefined access token", async () => {
+      // Arrange
+      const userId = "userId" as UserId;
+      tokenService.getAccessToken = jest.fn().mockResolvedValue(undefined);
+      tokenService.clearTokens = jest.fn().mockResolvedValue(undefined);
+
+      // Act
+      await tokenService.cleanupTokenStorage([userId]);
+
+      // Assert
+      expect(tokenService.clearTokens).toHaveBeenCalledWith(userId);
+    });
+
+    it("calls clearTokens for a user id with no tokens at all (safe no-op)", async () => {
+      // Arrange - simulate a userId that has no access token and no other tokens
+      const userId = "userId" as UserId;
+      tokenService.getAccessToken = jest.fn().mockResolvedValue(null);
+      tokenService.clearTokens = jest.fn().mockResolvedValue(undefined);
+
+      // Act
+      await tokenService.cleanupTokenStorage([userId]);
+
+      // Assert - clearTokens is still called; it is a no-op when nothing is stored
+      expect(tokenService.clearTokens).toHaveBeenCalledWith(userId);
+    });
+
+    it("does not call clearTokens when given an empty user id list", async () => {
+      // Arrange
+      tokenService.clearTokens = jest.fn().mockResolvedValue(undefined);
+
+      // Act
+      await tokenService.cleanupTokenStorage([]);
+
+      // Assert
+      expect(tokenService.clearTokens).not.toHaveBeenCalled();
+    });
+  });
+
   describe("Two Factor Token methods", () => {
     describe("setTwoFactorToken", () => {
       it("sets the email and two factor token when there hasn't been a previous record (initializing the record)", async () => {

libs/common/src/auth/services/token.service.ts

@@ -958,6 +958,15 @@ export class TokenService implements TokenServiceAbstraction {
     ]);
   }
 
+  async cleanupTokenStorage(userIds: UserId[]): Promise<void> {
+    for (const userId of userIds) {
+      const accessToken = await this.getAccessToken(userId);
+      if (accessToken == null) {
+        await this.clearTokens(userId);
+      }
+    }
+  }
+
   // jwthelper methods
   // ref https://github.com/auth0/angular-jwt/blob/master/src/angularJwt/services/jwt.js
 
@@ -1148,6 +1157,17 @@ export class TokenService implements TokenServiceAbstraction {
     return await firstValueFrom(this.singleUserStateProvider.get(userId, storageLocation).state$);
   }
 
+  /**
+   * Determines where tokens should be stored based on vault timeout settings.
+   *
+   * | Vault Timeout Action | Vault Timeout   | Token Storage        |
+   * |----------------------|-----------------|----------------------|
+   * | Lock                 | Any             | Disk / Secure Storage|
+   * | Log Out              | Never           | Disk / Secure Storage|
+   * | Log Out              | Any other value | Memory only          |
+   *
+   * Memory-only tokens are cleared when the app closes.
+   */
   private async determineStorageLocation(
     vaultTimeoutAction: VaultTimeoutAction,
     vaultTimeout: VaultTimeout,