diff --git a/.github/workflows/build-browser.yml b/.github/workflows/build-browser.yml index 91d39a08197..28289b37aed 100644 --- a/.github/workflows/build-browser.yml +++ b/.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 }} diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 9aaa694250c..6e6ee552f1f 100644 --- a/apps/browser/src/background/main.background.ts +++ b/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); diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index ab605f7c97f..dda902251bf 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/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 diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index 76d03268db2..2b6baf3cf20 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/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 diff --git a/apps/desktop/src/app/services/init.service.ts b/apps/desktop/src/app/services/init.service.ts index 7299b2efc85..e728410087c 100644 --- a/apps/desktop/src/app/services/init.service.ts +++ b/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. diff --git a/apps/web/src/app/app.component.ts b/apps/web/src/app/app.component.ts index 005b1565a2c..ac5ee3ca4bc 100644 --- a/apps/web/src/app/app.component.ts +++ b/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); diff --git a/apps/web/src/app/core/init.service.ts b/apps/web/src/app/core/init.service.ts index 9fbd36c117d..d94a3b2d492 100644 --- a/apps/web/src/app/core/init.service.ts +++ b/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 diff --git a/libs/common/src/auth/abstractions/token.service.ts b/libs/common/src/auth/abstractions/token.service.ts index b3fbfbe4c29..4bae15efd53 100644 --- a/libs/common/src/auth/abstractions/token.service.ts +++ b/libs/common/src/auth/abstractions/token.service.ts @@ -40,6 +40,18 @@ export abstract class TokenService { */ abstract clearTokens(userId?: UserId): Promise; + /** + * 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; + /** * 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 diff --git a/libs/common/src/auth/services/token.service.spec.ts b/libs/common/src/auth/services/token.service.spec.ts index d7d54e6377d..0392093cc25 100644 --- a/libs/common/src/auth/services/token.service.spec.ts +++ b/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 () => { diff --git a/libs/common/src/auth/services/token.service.ts b/libs/common/src/auth/services/token.service.ts index 799a0f42f0e..e74d0348554 100644 --- a/libs/common/src/auth/services/token.service.ts +++ b/libs/common/src/auth/services/token.service.ts @@ -958,6 +958,15 @@ export class TokenService implements TokenServiceAbstraction { ]); } + async cleanupTokenStorage(userIds: UserId[]): Promise { + 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,