1
0
mirror of https://github.com/bitwarden/browser.git synced 2024-11-21 11:35:34 +01:00

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.
This commit is contained in:
Jared Snider 2024-03-27 17:46:56 -04:00 committed by GitHub
parent d9bec7f984
commit 8cdc94076e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 26 additions and 25 deletions

View File

@ -186,9 +186,9 @@ describe("LoginStrategy", () => {
expect(tokenService.setTokens).toHaveBeenCalledWith( expect(tokenService.setTokens).toHaveBeenCalledWith(
accessToken, accessToken,
refreshToken,
mockVaultTimeoutAction, mockVaultTimeoutAction,
mockVaultTimeout, mockVaultTimeout,
refreshToken,
); );
expect(stateService.addAccount).toHaveBeenCalledWith( expect(stateService.addAccount).toHaveBeenCalledWith(

View File

@ -182,9 +182,9 @@ export abstract class LoginStrategy {
// User id will be derived from the access token. // User id will be derived from the access token.
await this.tokenService.setTokens( await this.tokenService.setTokens(
tokenResponse.accessToken, tokenResponse.accessToken,
tokenResponse.refreshToken,
vaultTimeoutAction as VaultTimeoutAction, vaultTimeoutAction as VaultTimeoutAction,
vaultTimeout, vaultTimeout,
tokenResponse.refreshToken, // Note: CLI login via API key sends undefined for refresh token.
); );
await this.stateService.addAccount( await this.stateService.addAccount(

View File

@ -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 * 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. * 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 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 vaultTimeoutAction The action to take when the vault times out.
* @param vaultTimeout The timeout for the vault. * @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. * @returns A promise that resolves when the tokens have been set.
*/ */
setTokens: ( setTokens: (
accessToken: string, accessToken: string,
refreshToken: string,
vaultTimeoutAction: VaultTimeoutAction, vaultTimeoutAction: VaultTimeoutAction,
vaultTimeout: number | null, vaultTimeout: number | null,
refreshToken?: string,
clientIdClientSecret?: [string, string], clientIdClientSecret?: [string, string],
) => Promise<void>; ) => Promise<void>;

View File

@ -991,6 +991,7 @@ describe("TokenService", () => {
refreshToken, refreshToken,
VaultTimeoutAction.Lock, VaultTimeoutAction.Lock,
null, null,
null,
); );
// Assert // Assert
await expect(result).rejects.toThrow("User id not found. Cannot save refresh token."); await expect(result).rejects.toThrow("User id not found. Cannot save refresh token.");
@ -1854,7 +1855,7 @@ describe("TokenService", () => {
// Act // Act
// Note: passing a valid access token so that a valid user id can be determined from the access token // 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, clientId,
clientSecret, clientSecret,
]); ]);
@ -1901,7 +1902,7 @@ describe("TokenService", () => {
tokenService.setClientSecret = jest.fn(); tokenService.setClientSecret = jest.fn();
// Act // Act
await tokenService.setTokens(accessTokenJwt, refreshToken, vaultTimeoutAction, vaultTimeout); await tokenService.setTokens(accessTokenJwt, vaultTimeoutAction, vaultTimeout, refreshToken);
// Assert // Assert
expect((tokenService as any)._setAccessToken).toHaveBeenCalledWith( expect((tokenService as any)._setAccessToken).toHaveBeenCalledWith(
@ -1933,9 +1934,9 @@ describe("TokenService", () => {
// Act // Act
const result = tokenService.setTokens( const result = tokenService.setTokens(
accessToken, accessToken,
refreshToken,
vaultTimeoutAction, vaultTimeoutAction,
vaultTimeout, vaultTimeout,
refreshToken,
); );
// Assert // Assert
@ -1952,32 +1953,27 @@ describe("TokenService", () => {
// Act // Act
const result = tokenService.setTokens( const result = tokenService.setTokens(
accessToken, accessToken,
refreshToken,
vaultTimeoutAction, vaultTimeoutAction,
vaultTimeout, vaultTimeout,
refreshToken,
); );
// Assert // 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 // Arrange
const accessToken = "accessToken";
const refreshToken: string = null; const refreshToken: string = null;
const vaultTimeoutAction = VaultTimeoutAction.Lock; const vaultTimeoutAction = VaultTimeoutAction.Lock;
const vaultTimeout = 30; const vaultTimeout = 30;
(tokenService as any).setRefreshToken = jest.fn();
// Act // Act
const result = tokenService.setTokens( await tokenService.setTokens(accessTokenJwt, vaultTimeoutAction, vaultTimeout, refreshToken);
accessToken,
refreshToken,
vaultTimeoutAction,
vaultTimeout,
);
// Assert // Assert
await expect(result).rejects.toThrow("Access token and refresh token are required."); expect((tokenService as any).setRefreshToken).not.toHaveBeenCalled();
}); });
}); });

View File

@ -149,13 +149,13 @@ export class TokenService implements TokenServiceAbstraction {
async setTokens( async setTokens(
accessToken: string, accessToken: string,
refreshToken: string,
vaultTimeoutAction: VaultTimeoutAction, vaultTimeoutAction: VaultTimeoutAction,
vaultTimeout: number | null, vaultTimeout: number | null,
refreshToken?: string,
clientIdClientSecret?: [string, string], clientIdClientSecret?: [string, string],
): Promise<void> { ): Promise<void> {
if (!accessToken || !refreshToken) { if (!accessToken) {
throw new Error("Access token and refresh token are required."); throw new Error("Access token is required.");
} }
// get user id the access token // get user id the access token
@ -166,7 +166,11 @@ export class TokenService implements TokenServiceAbstraction {
} }
await this._setAccessToken(accessToken, vaultTimeoutAction, vaultTimeout, userId); 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) { if (clientIdClientSecret != null) {
await this.setClientId(clientIdClientSecret[0], vaultTimeoutAction, vaultTimeout, userId); await this.setClientId(clientIdClientSecret[0], vaultTimeoutAction, vaultTimeout, userId);
await this.setClientSecret(clientIdClientSecret[1], vaultTimeoutAction, vaultTimeout, userId); await this.setClientSecret(clientIdClientSecret[1], vaultTimeoutAction, vaultTimeout, userId);

View File

@ -1780,9 +1780,9 @@ export class ApiService implements ApiServiceAbstraction {
await this.tokenService.setTokens( await this.tokenService.setTokens(
tokenResponse.accessToken, tokenResponse.accessToken,
tokenResponse.refreshToken,
vaultTimeoutAction as VaultTimeoutAction, vaultTimeoutAction as VaultTimeoutAction,
vaultTimeout, vaultTimeout,
tokenResponse.refreshToken,
); );
} else { } else {
const error = await this.handleError(response, true, true); const error = await this.handleError(response, true, true);

View File

@ -52,7 +52,7 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA
await this.stateService.setVaultTimeoutAction(action); await this.stateService.setVaultTimeoutAction(action);
await this.tokenService.setTokens(accessToken, refreshToken, action, timeout, [ await this.tokenService.setTokens(accessToken, action, timeout, refreshToken, [
clientId, clientId,
clientSecret, clientSecret,
]); ]);