Skip to content

Commit

Permalink
Capture and instrument cache errors (#7021)
Browse files Browse the repository at this point in the history
- Add CacheError.
- Capture and throw cache errors as CacheError.
- Instrument the number of tokens in the cache if CacheError occurs.
  • Loading branch information
konstantin-msft committed Apr 10, 2024
1 parent 17a0bcb commit b7c2309
Show file tree
Hide file tree
Showing 14 changed files with 489 additions and 30 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Capture and instrument cache errors #7021",
"packageName": "@azure/msal-browser",
"email": "kshabelko@microsoft.com",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Capture and instrument cache errors #7021",
"packageName": "@azure/msal-common",
"email": "kshabelko@microsoft.com",
"dependentChangeType": "patch"
}
50 changes: 49 additions & 1 deletion lib/msal-browser/src/cache/BrowserCacheManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import {
IPerformanceClient,
StaticAuthorityOptions,
CacheHelpers,
StoreInCache,
CacheError,
} from "@azure/msal-common";
import { CacheOptions } from "../config/Configuration";
import {
Expand Down Expand Up @@ -79,6 +81,8 @@ export class BrowserCacheManager extends CacheManager {
protected temporaryCacheStorage: IWindowStorage<string>;
// Logger instance
protected logger: Logger;
// Telemetry perf client
protected performanceClient?: IPerformanceClient;

// Cookie life calculation (hours * minutes * seconds * ms)
protected readonly COOKIE_LIFE_MULTIPLIER = 24 * 60 * 60 * 1000;
Expand All @@ -88,7 +92,8 @@ export class BrowserCacheManager extends CacheManager {
cacheConfig: Required<CacheOptions>,
cryptoImpl: ICrypto,
logger: Logger,
staticAuthorityOptions?: StaticAuthorityOptions
staticAuthorityOptions?: StaticAuthorityOptions,
performanceClient?: IPerformanceClient
) {
super(clientId, cryptoImpl, logger, staticAuthorityOptions);
this.cacheConfig = cacheConfig;
Expand All @@ -107,6 +112,8 @@ export class BrowserCacheManager extends CacheManager {
this.migrateCacheEntries();
this.createKeyMaps();
}

this.performanceClient = performanceClient;
}

/**
Expand Down Expand Up @@ -1855,6 +1862,47 @@ export class BrowserCacheManager extends CacheManager {
);
return this.saveCacheRecord(cacheRecord);
}

/**
* saves a cache record
* @param cacheRecord {CacheRecord}
* @param storeInCache {?StoreInCache}
* @param correlationId {?string} correlation id
*/
async saveCacheRecord(
cacheRecord: CacheRecord,
storeInCache?: StoreInCache,
correlationId?: string
): Promise<void> {
try {
await super.saveCacheRecord(
cacheRecord,
storeInCache,
correlationId
);
} catch (e) {
if (
e instanceof CacheError &&
this.performanceClient &&
correlationId
) {
try {
const tokenKeys = this.getTokenKeys();

this.performanceClient.addFields(
{
cacheRtCount: tokenKeys.refreshToken.length,
cacheIdCount: tokenKeys.idToken.length,
cacheAtCount: tokenKeys.accessToken.length,
},
correlationId
);
} catch (e) {}
}

throw e;
}
}
}

export const DEFAULT_BROWSER_CACHE_MANAGER = (
Expand Down
7 changes: 5 additions & 2 deletions lib/msal-browser/src/controllers/StandardController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,8 @@ export class StandardController implements IController {
this.config.cache,
this.browserCrypto,
this.logger,
buildStaticAuthorityOptions(this.config.auth)
buildStaticAuthorityOptions(this.config.auth),
this.performanceClient
)
: DEFAULT_BROWSER_CACHE_MANAGER(
this.config.auth.clientId,
Expand All @@ -227,7 +228,9 @@ export class StandardController implements IController {
this.config.auth.clientId,
nativeCacheOptions,
this.browserCrypto,
this.logger
this.logger,
undefined,
this.performanceClient
);

// Initialize the token cache
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,9 @@ export class UnknownOperatingContextController implements IController {
this.config.auth.clientId,
this.config.cache,
this.browserCrypto,
this.logger
this.logger,
undefined,
this.performanceClient
)
: DEFAULT_BROWSER_CACHE_MANAGER(
this.config.auth.clientId,
Expand Down
95 changes: 95 additions & 0 deletions lib/msal-browser/test/cache/BrowserCacheManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ import {
CredentialType,
ProtocolMode,
CacheHelpers,
CacheError,
CacheErrorCodes,
CacheManager,
CacheRecord,
PerformanceEvent,
} from "@azure/msal-common";
import {
BrowserCacheLocation,
Expand All @@ -46,6 +51,7 @@ import { BrowserCacheManager } from "../../src/cache/BrowserCacheManager";
import { BrowserStateObject } from "../../src/utils/BrowserProtocolUtils";
import { base64Decode } from "../../src/encode/Base64Decode";
import { getDefaultPerformanceClient } from "../utils/TelemetryUtils";
import { BrowserPerformanceClient } from "../../src/telemetry/BrowserPerformanceClient";

describe("BrowserCacheManager tests", () => {
let cacheConfig: Required<CacheOptions>;
Expand Down Expand Up @@ -1564,6 +1570,95 @@ describe("BrowserCacheManager tests", () => {
).toEqual(testVal);
});
});

describe("saveCacheRecord", () => {
it("saveCacheRecord re-throws and captures telemetry", (done) => {
const cacheError = new CacheError(
CacheErrorCodes.cacheQuotaExceededErrorCode
);
const testAppConfig = {
auth: {
clientId: TEST_CONFIG.MSAL_CLIENT_ID,
},
};
const perfClient = new BrowserPerformanceClient(
testAppConfig
);

const testAccessToken =
CacheHelpers.createAccessTokenEntity(
"homeAccountId",
"environment",
TEST_TOKENS.ACCESS_TOKEN,
"client-id",
"tenantId",
"openid",
1000,
1000,
browserCrypto.base64Decode,
500,
AuthenticationScheme.BEARER,
"oboAssertion"
);

const cacheManager = new BrowserCacheManager(
TEST_CONFIG.MSAL_CLIENT_ID,
cacheConfig,
browserCrypto,
logger,
undefined,
perfClient
);
cacheManager.setAccessTokenCredential(testAccessToken);

sinon
.stub(CacheManager.prototype, "saveCacheRecord")
.throws(cacheError);

// @ts-ignore
const callbackId = perfClient.addPerformanceCallback(
(events: PerformanceEvent[]) => {
expect(events.length).toEqual(1);
const event = events[0];
expect(event.name).toBe("test-measurement");
expect(event.correlationId).toEqual(
"test-correlation-id"
);
expect(event.success).toBeFalsy();
expect(event.errorCode).toEqual(
CacheErrorCodes.cacheQuotaExceededErrorCode
);
expect(event.cacheIdCount).toEqual(0);
expect(event.cacheRtCount).toEqual(0);
expect(event.cacheAtCount).toEqual(1);
// @ts-ignore
perfClient.removePerformanceCallback(callbackId);
done();
}
);

const measurement = perfClient.startMeasurement(
"test-measurement",
"test-correlation-id"
);

cacheManager
.saveCacheRecord(
new CacheRecord(),
undefined,
"test-correlation-id"
)
.then(() => {
throw new Error(
"saveCacheRecord should have thrown"
);
})
.catch((e) => {
expect(e).toBeInstanceOf(CacheError);
measurement.end({ success: false }, e);
});
});
});
});
});

Expand Down
77 changes: 58 additions & 19 deletions lib/msal-common/src/cache/CacheManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import { getAliasesFromStaticSources } from "../authority/AuthorityMetadata";
import { StaticAuthorityOptions } from "../authority/AuthorityOptions";
import { TokenClaims } from "../account/TokenClaims";
import { IPerformanceClient } from "../telemetry/performance/IPerformanceClient";
import { CacheError, CacheErrorCodes } from "../error/CacheError";

/**
* Interface class which implement cache storage functions used by MSAL to perform validity checks, and store tokens.
Expand Down Expand Up @@ -501,39 +502,77 @@ export abstract class CacheManager implements ICacheManager {

/**
* saves a cache record
* @param cacheRecord
* @param cacheRecord {CacheRecord}
* @param storeInCache {?StoreInCache}
* @param correlationId {?string} correlation id
*/
async saveCacheRecord(
cacheRecord: CacheRecord,
storeInCache?: StoreInCache
storeInCache?: StoreInCache,
correlationId?: string
): Promise<void> {
if (!cacheRecord) {
throw createClientAuthError(
ClientAuthErrorCodes.invalidCacheRecord
);
}

if (!!cacheRecord.account) {
this.setAccount(cacheRecord.account);
}
try {
if (!!cacheRecord.account) {
this.setAccount(cacheRecord.account);
}

if (!!cacheRecord.idToken && storeInCache?.idToken !== false) {
this.setIdTokenCredential(cacheRecord.idToken);
}
if (!!cacheRecord.idToken && storeInCache?.idToken !== false) {
this.setIdTokenCredential(cacheRecord.idToken);
}

if (!!cacheRecord.accessToken && storeInCache?.accessToken !== false) {
await this.saveAccessToken(cacheRecord.accessToken);
}
if (
!!cacheRecord.accessToken &&
storeInCache?.accessToken !== false
) {
await this.saveAccessToken(cacheRecord.accessToken);
}

if (
!!cacheRecord.refreshToken &&
storeInCache?.refreshToken !== false
) {
this.setRefreshTokenCredential(cacheRecord.refreshToken);
}
if (
!!cacheRecord.refreshToken &&
storeInCache?.refreshToken !== false
) {
this.setRefreshTokenCredential(cacheRecord.refreshToken);
}

if (!!cacheRecord.appMetadata) {
this.setAppMetadata(cacheRecord.appMetadata);
if (!!cacheRecord.appMetadata) {
this.setAppMetadata(cacheRecord.appMetadata);
}
} catch (e: unknown) {
this.commonLogger?.error(`CacheManager.saveCacheRecord: failed`);
if (e instanceof Error) {
this.commonLogger?.errorPii(
`CacheManager.saveCacheRecord: ${e.message}`,
correlationId
);

if (
e.name === "QuotaExceededError" ||
e.name === "NS_ERROR_DOM_QUOTA_REACHED" ||
e.message.includes("exceeded the quota")
) {
this.commonLogger?.error(
`CacheManager.saveCacheRecord: exceeded storage quota`,
correlationId
);
throw new CacheError(
CacheErrorCodes.cacheQuotaExceededErrorCode
);
} else {
throw new CacheError(e.name, e.message);
}
} else {
this.commonLogger?.errorPii(
`CacheManager.saveCacheRecord: ${e}`,
correlationId
);
throw new CacheError(CacheErrorCodes.cacheUnknownErrorCode);
}
}
}

Expand Down
44 changes: 44 additions & 0 deletions lib/msal-common/src/error/CacheError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License.
*/

import * as CacheErrorCodes from "./CacheErrorCodes";
export { CacheErrorCodes };

export const CacheErrorMessages = {
[CacheErrorCodes.cacheQuotaExceededErrorCode]:
"Exceeded cache storage capacity.",
[CacheErrorCodes.cacheUnknownErrorCode]:
"Unexpected error occurred when using cache storage.",
};

/**
* Error thrown when there is an error with the cache
*/
export class CacheError extends Error {
/**
* Short string denoting error
*/
errorCode: string;

/**
* Detailed description of error
*/
errorMessage: string;

constructor(errorCode: string, errorMessage?: string) {
const message =
errorMessage ||
(CacheErrorMessages[errorCode]
? CacheErrorMessages[errorCode]
: CacheErrorMessages[CacheErrorCodes.cacheUnknownErrorCode]);

super(`${errorCode}: ${message}`);
Object.setPrototypeOf(this, CacheError.prototype);

this.name = "CacheError";
this.errorCode = errorCode;
this.errorMessage = message;
}
}

0 comments on commit b7c2309

Please sign in to comment.