Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug causing temporary cache not to be cleared & turn on return-await lint rule #6678

Merged
merged 9 commits into from
Nov 14, 2023
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Fix bug causing temporary cache not to be cleared #6676",
"packageName": "@azure/msal-browser",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Turn on return-await lint rule #6678",
"packageName": "@azure/msal-common",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Turn on return-await lint rule #6678",
"packageName": "@azure/msal-node",
"email": "[email protected]",
"dependentChangeType": "patch"
}
2 changes: 1 addition & 1 deletion lib/msal-browser/src/cache/DatabaseStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export class DatabaseStorage<T> implements IAsyncStorage<T> {
*/
private async validateDbIsOpen(): Promise<void> {
if (!this.dbOpen) {
return await this.open();
return this.open();
}
}

Expand Down
8 changes: 3 additions & 5 deletions lib/msal-browser/src/controllers/ControllerFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export async function createV3Controller(
await standard.initialize();

const controller = await import("./StandardController");
return await controller.StandardController.createController(standard);
return controller.StandardController.createController(standard);
}

export async function createController(
Expand All @@ -34,12 +34,10 @@ export async function createController(
teamsApp.getConfig().auth.supportsNestedAppAuth
) {
const controller = await import("./NestedAppAuthController");
return await controller.NestedAppAuthController.createController(
teamsApp
);
return controller.NestedAppAuthController.createController(teamsApp);
} else if (standard.isAvailable()) {
const controller = await import("./StandardController");
return await controller.StandardController.createController(standard);
return controller.StandardController.createController(standard);
} else {
// Since neither of the actual operating contexts are available keep the UnknownOperatingContextController
return null;
Expand Down
4 changes: 2 additions & 2 deletions lib/msal-browser/src/controllers/StandardController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -923,10 +923,10 @@ export class StandardController implements IController {
);
atbcMeasurement.discard();
}
return response;
return await response;
} else if (request.nativeAccountId) {
if (this.canUseNative(request, request.nativeAccountId)) {
return this.acquireTokenNative(
return await this.acquireTokenNative(
{
...request,
correlationId,
Expand Down
2 changes: 1 addition & 1 deletion lib/msal-browser/src/crypto/CryptoOps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export class CryptoOps implements ICrypto {
* Removes all cryptographic keys from IndexedDB storage
*/
async clearKeystore(): Promise<boolean> {
return await this.cache.clear();
return this.cache.clear();
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/msal-browser/src/crypto/SignedHttpRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,6 @@ export class SignedHttpRequest {
* @returns If keys are properly deleted
*/
async removeKeys(publicKeyThumbprint: string): Promise<boolean> {
return await this.cryptoOps.removeTokenBindingKey(publicKeyThumbprint);
return this.cryptoOps.removeTokenBindingKey(publicKeyThumbprint);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ export abstract class BaseInteractionClient {
this.logger.verbose(
"Creating discovered authority with request authority"
);
return await AuthorityFactory.createDiscoveredInstance(
return AuthorityFactory.createDiscoveredInstance(
requestAuthority,
this.config.system.networkClient,
this.browserStorage,
Expand All @@ -295,7 +295,7 @@ export abstract class BaseInteractionClient {
this.logger.verbose(
"Creating discovered authority with configured authority"
);
return await AuthorityFactory.createDiscoveredInstance(
return AuthorityFactory.createDiscoveredInstance(
this.config.auth.authority,
this.config.system.networkClient,
this.browserStorage,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ export class NativeInteractionClient extends BaseInteractionClient {
reqTimestamp
);
this.browserStorage.setInteractionInProgress(false);
return result;
return await result;
} catch (e) {
this.browserStorage.setInteractionInProgress(false);
throw e;
Expand Down Expand Up @@ -533,7 +533,7 @@ export class NativeInteractionClient extends BaseInteractionClient {
if (!request.keyId) {
throw createClientAuthError(ClientAuthErrorCodes.keyIdMissing);
}
return await popTokenGenerator.signPopToken(
return popTokenGenerator.signPopToken(
response.access_token,
request.keyId,
shrParameters
Expand Down
2 changes: 1 addition & 1 deletion lib/msal-browser/src/interaction_client/PopupClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ export class PopupClient extends StandardInteractionClient {
this.browserCrypto,
validRequest.state
);
return nativeInteractionClient.acquireToken({
return await nativeInteractionClient.acquireToken({
...validRequest,
state: userRequestState,
prompt: undefined, // Server should handle the prompt, ideally native broker can do this part silently
Expand Down
6 changes: 3 additions & 3 deletions lib/msal-browser/src/interaction_client/RedirectClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ export class RedirectClient extends StandardInteractionClient {
this.logger.verbose(
"NavigateToLoginRequestUrl set to false, handling response"
);
return this.handleResponse(
return await this.handleResponse(
serverParams,
serverTelemetryManager
);
Expand Down Expand Up @@ -313,7 +313,7 @@ export class RedirectClient extends StandardInteractionClient {

// If navigateInternal implementation returns false, handle the hash now
if (!processHashOnRedirect) {
return this.handleResponse(
return await this.handleResponse(
serverParams,
serverTelemetryManager
);
Expand Down Expand Up @@ -483,7 +483,7 @@ export class RedirectClient extends StandardInteractionClient {
this.logger,
this.performanceClient
);
return await interactionHandler.handleCodeResponse(serverParams, state);
return interactionHandler.handleCodeResponse(serverParams, state);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class SilentAuthCodeClient extends StandardInteractionClient {
);

// Handle auth code parameters from request
return invokeAsync(
return await invokeAsync(
interactionHandler.handleCodeResponseFromServer.bind(
interactionHandler
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ export abstract class StandardInteractionClient extends BaseInteractionClient {
userAuthority,
requestAzureCloudOptions || this.config.auth.azureCloudOptions
);
return await invokeAsync(
return invokeAsync(
AuthorityFactory.createDiscoveredInstance.bind(AuthorityFactory),
PerformanceEvents.AuthorityFactoryCreateDiscoveredInstance,
this.logger,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export async function initiateAuthRequest(
throw createBrowserAuthError(BrowserAuthErrorCodes.emptyNavigateUri);
}
if (navigateFrameWait) {
return await invokeAsync(
return invokeAsync(
loadFrame,
PerformanceEvents.SilentHandlerLoadFrame,
logger,
Expand Down
2 changes: 1 addition & 1 deletion lib/msal-browser/src/naa/BridgeProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ export class BridgeProxy implements IBridgeProxy {
}
);

return promise;
return await promise;
} catch (error) {
window.console.log(error);
throw error;
Expand Down
130 changes: 130 additions & 0 deletions lib/msal-browser/test/interaction_client/RedirectClient.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ describe("RedirectClient", () => {
NavigationClient.prototype,
"navigateExternal"
).mockResolvedValue(true);
jest.spyOn(
NavigationClient.prototype,
"navigateInternal"
).mockResolvedValue(true);

// @ts-ignore
browserStorage = pca.browserStorage;
Expand Down Expand Up @@ -288,6 +292,132 @@ describe("RedirectClient", () => {
});
});

it("cleans temporary cache and re-throws error thrown by handleResponse when loginRequestUrl == current url", (done) => {
browserStorage.setInteractionInProgress(true);
browserStorage.setTemporaryCache(
TemporaryCacheKeys.ORIGIN_URI,
window.location.href,
true
);
const statekey = browserStorage.generateStateKey(
TEST_STATE_VALUES.TEST_STATE_REDIRECT
);
browserStorage.setTemporaryCache(
statekey,
TEST_STATE_VALUES.TEST_STATE_REDIRECT,
true
);

jest.spyOn(
RedirectClient.prototype,
<any>"handleResponse"
).mockRejectedValue("Error in handleResponse");
redirectClient
.handleRedirectPromise(
TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT
)
.catch((e) => {
expect(e).toEqual("Error in handleResponse");
expect(window.localStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(0);
done();
});
});

it("cleans temporary cache and re-throws error thrown by handleResponse after clientside navigation to loginRequestUrl", (done) => {
jest.spyOn(
NavigationClient.prototype,
"navigateInternal"
).mockResolvedValue(false); // Client-side navigation

browserStorage.setInteractionInProgress(true);
browserStorage.setTemporaryCache(
TemporaryCacheKeys.ORIGIN_URI,
window.location.href + "/differentPath",
true
);
const statekey = browserStorage.generateStateKey(
TEST_STATE_VALUES.TEST_STATE_REDIRECT
);
browserStorage.setTemporaryCache(
statekey,
TEST_STATE_VALUES.TEST_STATE_REDIRECT,
true
);

jest.spyOn(
RedirectClient.prototype,
<any>"handleResponse"
).mockRejectedValue("Error in handleResponse");
redirectClient
.handleRedirectPromise(
TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT
)
.catch((e) => {
expect(e).toEqual("Error in handleResponse");
expect(window.localStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(0);
done();
});
});

it("cleans temporary cache and re-throws error thrown by handleResponse when navigateToLoginRequestUrl is false", (done) => {
browserStorage.setInteractionInProgress(true);
browserStorage.setTemporaryCache(
TemporaryCacheKeys.ORIGIN_URI,
window.location.href + "/differentPath",
true
);
const statekey = browserStorage.generateStateKey(
TEST_STATE_VALUES.TEST_STATE_REDIRECT
);
browserStorage.setTemporaryCache(
statekey,
TEST_STATE_VALUES.TEST_STATE_REDIRECT,
true
);

jest.spyOn(
RedirectClient.prototype,
<any>"handleResponse"
).mockRejectedValue("Error in handleResponse");
redirectClient = // @ts-ignore
redirectClient = new RedirectClient(
{
// @ts-ignore
...pca.config,
auth: {
// @ts-ignore
...pca.config.auth,
navigateToLoginRequestUrl: false,
},
},
browserStorage,
//@ts-ignore
pca.browserCrypto,
//@ts-ignore
pca.logger,
//@ts-ignore
pca.eventHandler,
//@ts-ignore
pca.navigationClient,
//@ts-ignore
pca.performanceClient,
//@ts-ignore
pca.nativeInternalStorage
);
redirectClient
.handleRedirectPromise(
TEST_HASHES.TEST_SUCCESS_CODE_HASH_REDIRECT
)
.catch((e) => {
expect(e).toEqual("Error in handleResponse");
expect(window.localStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(0);
done();
});
});

it("gets hash from cache and processes response", async () => {
const stateString = TEST_STATE_VALUES.TEST_STATE_REDIRECT;
const browserCrypto = new CryptoOps(new Logger({}));
Expand Down
2 changes: 1 addition & 1 deletion lib/msal-common/src/client/RefreshTokenClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export class RefreshTokenClient extends BaseClient {
// if the app is part of the family, retrive a Family refresh token if present and make a refreshTokenRequest
if (isFOCI) {
try {
return invokeAsync(
return await invokeAsync(
this.acquireTokenWithCachedRefreshToken.bind(this),
PerformanceEvents.RefreshTokenClientAcquireTokenWithCachedRefreshToken,
this.logger,
Expand Down
2 changes: 1 addition & 1 deletion lib/msal-common/src/client/SilentFlowClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ export class SilentFlowClient extends BaseClient {
checkMaxAge(authTime, request.maxAge);
}

return await ResponseHandler.generateAuthenticationResult(
return ResponseHandler.generateAuthenticationResult(
this.cryptoUtils,
this.authority,
cacheRecord,
Expand Down
2 changes: 1 addition & 1 deletion lib/msal-common/src/crypto/PopTokenGenerator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export class PopTokenGenerator {
? new UrlString(resourceRequestUri)
: undefined;
const resourceUrlComponents = resourceUrlString?.getUrlComponents();
return await this.cryptoUtils.signJwt(
return this.cryptoUtils.signJwt(
{
at: payload,
ts: TimeUtils.nowSeconds(),
Expand Down
2 changes: 1 addition & 1 deletion lib/msal-common/src/response/ResponseHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ export class ResponseHandler {
this.logger.warning(
"Account used to refresh tokens not in persistence, refreshed tokens will not be stored in the cache"
);
return ResponseHandler.generateAuthenticationResult(
return await ResponseHandler.generateAuthenticationResult(
this.cryptoObj,
authority,
cacheRecord,
Expand Down
Loading