From 8cdc94076e9cccda9199bf48a4b7e2fb4eabc1b7 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Wed, 27 Mar 2024 17:46:56 -0400 Subject: [PATCH] Auth/PM-7092 - Fix CLI login via API key not working due to TokenService changes (#8499) * PM-7092 - Fix CLI login via API key not working (it apparently receives an undefined refresh token which was rejected by setTokens) * PM-7092 - Fix base login strategy tests * PM-7092 - per discucssion with jake, refactor setTokens to accept optional refresh token instead of exposing setRefreshToken as public. --- .../login-strategies/login.strategy.spec.ts | 2 +- .../common/login-strategies/login.strategy.ts | 2 +- .../src/auth/abstractions/token.service.ts | 7 +++--- .../src/auth/services/token.service.spec.ts | 24 ++++++++----------- .../common/src/auth/services/token.service.ts | 12 ++++++---- libs/common/src/services/api.service.ts | 2 +- .../vault-timeout-settings.service.ts | 2 +- 7 files changed, 26 insertions(+), 25 deletions(-) diff --git a/libs/auth/src/common/login-strategies/login.strategy.spec.ts b/libs/auth/src/common/login-strategies/login.strategy.spec.ts index ed40797df5..42541808c8 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.spec.ts @@ -186,9 +186,9 @@ describe("LoginStrategy", () => { expect(tokenService.setTokens).toHaveBeenCalledWith( accessToken, - refreshToken, mockVaultTimeoutAction, mockVaultTimeout, + refreshToken, ); expect(stateService.addAccount).toHaveBeenCalledWith( diff --git a/libs/auth/src/common/login-strategies/login.strategy.ts b/libs/auth/src/common/login-strategies/login.strategy.ts index eef5626493..8e927c2cc4 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.ts @@ -182,9 +182,9 @@ export abstract class LoginStrategy { // User id will be derived from the access token. await this.tokenService.setTokens( tokenResponse.accessToken, - tokenResponse.refreshToken, vaultTimeoutAction as VaultTimeoutAction, vaultTimeout, + tokenResponse.refreshToken, // Note: CLI login via API key sends undefined for refresh token. ); await this.stateService.addAccount( diff --git a/libs/common/src/auth/abstractions/token.service.ts b/libs/common/src/auth/abstractions/token.service.ts index d2358314d7..18366c5f1b 100644 --- a/libs/common/src/auth/abstractions/token.service.ts +++ b/libs/common/src/auth/abstractions/token.service.ts @@ -10,17 +10,18 @@ export abstract class TokenService { * Note 2: this method also enforces always setting the access token and the refresh token together as * we can retrieve the user id required to set the refresh token from the access token for efficiency. * @param accessToken The access token to set. - * @param refreshToken The refresh token to set. - * @param clientIdClientSecret The API Key Client ID and Client Secret to set. * @param vaultTimeoutAction The action to take when the vault times out. * @param vaultTimeout The timeout for the vault. + * @param refreshToken The optional refresh token to set. Note: this is undefined when using the CLI Login Via API Key flow + * @param clientIdClientSecret The API Key Client ID and Client Secret to set. + * * @returns A promise that resolves when the tokens have been set. */ setTokens: ( accessToken: string, - refreshToken: string, vaultTimeoutAction: VaultTimeoutAction, vaultTimeout: number | null, + refreshToken?: string, clientIdClientSecret?: [string, string], ) => Promise; diff --git a/libs/common/src/auth/services/token.service.spec.ts b/libs/common/src/auth/services/token.service.spec.ts index 63c581910a..8e8ed08853 100644 --- a/libs/common/src/auth/services/token.service.spec.ts +++ b/libs/common/src/auth/services/token.service.spec.ts @@ -991,6 +991,7 @@ describe("TokenService", () => { refreshToken, VaultTimeoutAction.Lock, null, + null, ); // Assert await expect(result).rejects.toThrow("User id not found. Cannot save refresh token."); @@ -1854,7 +1855,7 @@ describe("TokenService", () => { // Act // Note: passing a valid access token so that a valid user id can be determined from the access token - await tokenService.setTokens(accessTokenJwt, refreshToken, vaultTimeoutAction, vaultTimeout, [ + await tokenService.setTokens(accessTokenJwt, vaultTimeoutAction, vaultTimeout, refreshToken, [ clientId, clientSecret, ]); @@ -1901,7 +1902,7 @@ describe("TokenService", () => { tokenService.setClientSecret = jest.fn(); // Act - await tokenService.setTokens(accessTokenJwt, refreshToken, vaultTimeoutAction, vaultTimeout); + await tokenService.setTokens(accessTokenJwt, vaultTimeoutAction, vaultTimeout, refreshToken); // Assert expect((tokenService as any)._setAccessToken).toHaveBeenCalledWith( @@ -1933,9 +1934,9 @@ describe("TokenService", () => { // Act const result = tokenService.setTokens( accessToken, - refreshToken, vaultTimeoutAction, vaultTimeout, + refreshToken, ); // Assert @@ -1952,32 +1953,27 @@ describe("TokenService", () => { // Act const result = tokenService.setTokens( accessToken, - refreshToken, vaultTimeoutAction, vaultTimeout, + refreshToken, ); // Assert - await expect(result).rejects.toThrow("Access token and refresh token are required."); + await expect(result).rejects.toThrow("Access token is required."); }); - it("should throw an error if the refresh token is missing", async () => { + it("should not throw an error if the refresh token is missing and it should just not set it", async () => { // Arrange - const accessToken = "accessToken"; const refreshToken: string = null; const vaultTimeoutAction = VaultTimeoutAction.Lock; const vaultTimeout = 30; + (tokenService as any).setRefreshToken = jest.fn(); // Act - const result = tokenService.setTokens( - accessToken, - refreshToken, - vaultTimeoutAction, - vaultTimeout, - ); + await tokenService.setTokens(accessTokenJwt, vaultTimeoutAction, vaultTimeout, refreshToken); // Assert - await expect(result).rejects.toThrow("Access token and refresh token are required."); + expect((tokenService as any).setRefreshToken).not.toHaveBeenCalled(); }); }); diff --git a/libs/common/src/auth/services/token.service.ts b/libs/common/src/auth/services/token.service.ts index a1dc7ecf21..dd011eb40b 100644 --- a/libs/common/src/auth/services/token.service.ts +++ b/libs/common/src/auth/services/token.service.ts @@ -149,13 +149,13 @@ export class TokenService implements TokenServiceAbstraction { async setTokens( accessToken: string, - refreshToken: string, vaultTimeoutAction: VaultTimeoutAction, vaultTimeout: number | null, + refreshToken?: string, clientIdClientSecret?: [string, string], ): Promise { - if (!accessToken || !refreshToken) { - throw new Error("Access token and refresh token are required."); + if (!accessToken) { + throw new Error("Access token is required."); } // get user id the access token @@ -166,7 +166,11 @@ export class TokenService implements TokenServiceAbstraction { } await this._setAccessToken(accessToken, vaultTimeoutAction, vaultTimeout, userId); - await this.setRefreshToken(refreshToken, vaultTimeoutAction, vaultTimeout, userId); + + if (refreshToken) { + await this.setRefreshToken(refreshToken, vaultTimeoutAction, vaultTimeout, userId); + } + if (clientIdClientSecret != null) { await this.setClientId(clientIdClientSecret[0], vaultTimeoutAction, vaultTimeout, userId); await this.setClientSecret(clientIdClientSecret[1], vaultTimeoutAction, vaultTimeout, userId); diff --git a/libs/common/src/services/api.service.ts b/libs/common/src/services/api.service.ts index b6c2ab5c22..6306eb1e28 100644 --- a/libs/common/src/services/api.service.ts +++ b/libs/common/src/services/api.service.ts @@ -1780,9 +1780,9 @@ export class ApiService implements ApiServiceAbstraction { await this.tokenService.setTokens( tokenResponse.accessToken, - tokenResponse.refreshToken, vaultTimeoutAction as VaultTimeoutAction, vaultTimeout, + tokenResponse.refreshToken, ); } else { const error = await this.handleError(response, true, true); diff --git a/libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts b/libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts index 4eb9e77699..a8afc63297 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts +++ b/libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts @@ -52,7 +52,7 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA await this.stateService.setVaultTimeoutAction(action); - await this.tokenService.setTokens(accessToken, refreshToken, action, timeout, [ + await this.tokenService.setTokens(accessToken, action, timeout, refreshToken, [ clientId, clientSecret, ]);