Skip to content

Upgrade/Rollback Telemetry #7738

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

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Upgrade/rollback telemetry #7738",
"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": "Upgrade/rollback telemetry #7738",
"packageName": "@azure/msal-common",
"email": "[email protected]",
"dependentChangeType": "patch"
}
24 changes: 24 additions & 0 deletions lib/msal-browser/src/cache/BrowserCacheManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ import { getAccountKeys, getTokenKeys } from "./CacheHelpers.js";
import { EventType } from "../event/EventType.js";
import { EventHandler } from "../event/EventHandler.js";
import { clearHash } from "../utils/BrowserUtils.js";
import { version } from "../packageMetadata.js";

/**
* This class implements the cache storage interface for MSAL through browser local or session storage.
Expand Down Expand Up @@ -123,6 +124,29 @@ export class BrowserCacheManager extends CacheManager {

async initialize(correlationId: string): Promise<void> {
await this.browserStorage.initialize(correlationId);
this.trackVersionChanges(correlationId);
}

/**
* Tracks upgrades and downgrades for telemetry and debugging purposes
*/
private trackVersionChanges(correlationId: string): void {
const previousVersion = this.browserStorage.getItem(
StaticCacheKeys.VERSION
);
if (previousVersion) {
this.logger.info(
`MSAL.js was last initialized by version: ${previousVersion}`
);
this.performanceClient.addFields(
{ previousLibraryVersion: previousVersion },
correlationId
);
}

if (previousVersion !== version) {
this.browserStorage.setItem(StaticCacheKeys.VERSION, version);
}
}

/**
Expand Down
1 change: 1 addition & 0 deletions lib/msal-browser/src/utils/BrowserConstants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ export type TemporaryCacheKeys =
export const StaticCacheKeys = {
ACCOUNT_KEYS: "msal.account.keys",
TOKEN_KEYS: "msal.token.keys",
VERSION: "msal.version",
} as const;
export type StaticCacheKeys =
(typeof StaticCacheKeys)[keyof typeof StaticCacheKeys];
Expand Down
30 changes: 20 additions & 10 deletions lib/msal-browser/test/app/PublicClientApplication.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import {
CacheHelpers,
CacheManager,
ClientAuthErrorCodes,
CommonAuthorizationCodeRequest,
CommonAuthorizationUrlRequest,
CommonSilentFlowRequest,
Constants,
Expand Down Expand Up @@ -60,6 +59,7 @@ import {
BrowserConstants,
CacheLookupPolicy,
InteractionType,
StaticCacheKeys,
PlatformAuthConstants,
TemporaryCacheKeys,
WrapperSKU,
Expand All @@ -76,7 +76,6 @@ import { NavigationOptions } from "../../src/navigation/NavigationOptions.js";
import { EventMessage } from "../../src/event/EventMessage.js";
import { EventHandler } from "../../src/event/EventHandler.js";
import { SilentIframeClient } from "../../src/interaction_client/SilentIframeClient.js";
import { base64Encode } from "../../src/encode/Base64Encode.js";
import { FetchClient } from "../../src/network/FetchClient.js";
import {
BrowserAuthError,
Expand All @@ -89,10 +88,6 @@ import { RedirectClient } from "../../src/interaction_client/RedirectClient.js";
import { PopupClient } from "../../src/interaction_client/PopupClient.js";
import { SilentCacheClient } from "../../src/interaction_client/SilentCacheClient.js";
import { SilentRefreshClient } from "../../src/interaction_client/SilentRefreshClient.js";
import {
AuthorizationCodeRequest,
EndSessionRequest,
} from "../../src/index.js";
import { SilentAuthCodeClient } from "../../src/interaction_client/SilentAuthCodeClient.js";
import { BrowserCacheManager } from "../../src/cache/BrowserCacheManager.js";
import { PlatformAuthExtensionHandler } from "../../src/broker/nativeBroker/PlatformAuthExtensionHandler.js";
Expand All @@ -117,6 +112,9 @@ import {
TestTimeUtils,
} from "msal-test-utils";
import { INTERACTION_TYPE } from "../../src/utils/BrowserConstants.js";
import { version } from "../../src/packageMetadata.js";
import { AuthorizationCodeRequest } from "../../src/request/AuthorizationCodeRequest.js";
import { EndSessionRequest } from "../../src/request/EndSessionRequest.js";
import { BaseOperatingContext } from "../../src/operatingcontext/BaseOperatingContext.js";
import { PlatformAuthDOMHandler } from "../../src/broker/nativeBroker/PlatformAuthDOMHandler.js";
import { config } from "process";
Expand Down Expand Up @@ -832,7 +830,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
pca.handleRedirectPromise().catch((e) => {
expect(e).toMatchObject(testError);
expect(window.localStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(1);
expect(
window.sessionStorage.getItem(StaticCacheKeys.VERSION)
).toEqual(version); // Validate that the one item in sessionStorage is what we expect
done();
});
});
Expand Down Expand Up @@ -2104,7 +2105,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
expect(redirectClientSpy).toHaveBeenCalledTimes(1);
expect(browserStorage.isInteractionInProgress()).toBe(false);
expect(window.localStorage.length).toBe(0);
expect(window.sessionStorage.length).toBe(0);
expect(window.sessionStorage.length).toBe(1);
expect(
window.sessionStorage.getItem(StaticCacheKeys.VERSION)
).toEqual(version); // Validate that the one item in sessionStorage is what we expect
done();
});
});
Expand Down Expand Up @@ -5480,7 +5484,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
});
} catch (e) {
// Test that error was cached for telemetry purposes and then thrown
expect(window.sessionStorage).toHaveLength(1);
expect(window.sessionStorage).toHaveLength(2);
expect(
window.sessionStorage.getItem(StaticCacheKeys.VERSION)
).toEqual(version);
const failures = window.sessionStorage.getItem(
`server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}`
);
Expand Down Expand Up @@ -5536,7 +5543,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
await silentRequest3.catch(() => {});
// Test that error was cached for telemetry purposes and then thrown
expect(atsSpy).toHaveBeenCalledTimes(1);
expect(window.sessionStorage).toHaveLength(1);
expect(window.sessionStorage).toHaveLength(2);
expect(
window.sessionStorage.getItem(StaticCacheKeys.VERSION)
).toEqual(version);
const failures = window.sessionStorage.getItem(
`server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}`
);
Expand Down
86 changes: 82 additions & 4 deletions lib/msal-browser/test/cache/BrowserCacheManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@ import {
import { CacheOptions } from "../../src/config/Configuration.js";
import {
Constants,
PersistentCacheKeys,
CommonAuthorizationCodeRequest as AuthorizationCodeRequest,
ProtocolUtils,
Logger,
LogLevel,
AuthenticationScheme,
Expand All @@ -41,18 +39,18 @@ import {
import {
BrowserCacheLocation,
INTERACTION_TYPE,
InteractionType,
StaticCacheKeys,
TemporaryCacheKeys,
} from "../../src/utils/BrowserConstants.js";
import { CryptoOps } from "../../src/crypto/CryptoOps.js";
import { DatabaseStorage } from "../../src/cache/DatabaseStorage.js";
import { BrowserCacheManager } from "../../src/cache/BrowserCacheManager.js";
import { BrowserStateObject } from "../../src/utils/BrowserProtocolUtils.js";
import { base64Decode } from "../../src/encode/Base64Decode.js";
import { getDefaultPerformanceClient } from "../utils/TelemetryUtils.js";
import { BrowserPerformanceClient } from "../../src/telemetry/BrowserPerformanceClient.js";
import { CookieStorage } from "../../src/cache/CookieStorage.js";
import { EventHandler } from "../../src/event/EventHandler.js";
import { version } from "../../src/packageMetadata.js";

describe("BrowserCacheManager tests", () => {
let cacheConfig: Required<CacheOptions>;
Expand Down Expand Up @@ -140,6 +138,85 @@ describe("BrowserCacheManager tests", () => {
});
});

describe("initialize", () => {
it("sets MSAL version in localStorage if not already set", async () => {
const browserCacheManager = new BrowserCacheManager(
TEST_CONFIG.MSAL_CLIENT_ID,
{
...cacheConfig,
cacheLocation: BrowserCacheLocation.LocalStorage,
},
browserCrypto,
logger,
new StubPerformanceClient(),
new EventHandler()
);
await browserCacheManager.initialize(TEST_CONFIG.CORRELATION_ID);
expect(window.localStorage.getItem(StaticCacheKeys.VERSION)).toBe(
version
);
});

it("sets MSAL version in localStorage if previous version doesn't match", async () => {
window.localStorage.setItem(StaticCacheKeys.VERSION, "1.0.0");
const browserCacheManager = new BrowserCacheManager(
TEST_CONFIG.MSAL_CLIENT_ID,
{
...cacheConfig,
cacheLocation: BrowserCacheLocation.LocalStorage,
},
browserCrypto,
logger,
new StubPerformanceClient(),
new EventHandler()
);
await browserCacheManager.initialize(TEST_CONFIG.CORRELATION_ID);
expect(window.localStorage.getItem(StaticCacheKeys.VERSION)).toBe(
version
);
});

it("does not set MSAL version in localStorage if existing version already matches", async () => {
// First make sure the version gets set
const browserCacheManager1 = new BrowserCacheManager(
TEST_CONFIG.MSAL_CLIENT_ID,
{
...cacheConfig,
cacheLocation: BrowserCacheLocation.LocalStorage,
},
browserCrypto,
logger,
new StubPerformanceClient(),
new EventHandler()
);
await browserCacheManager1.initialize(TEST_CONFIG.CORRELATION_ID);
expect(window.localStorage.getItem(StaticCacheKeys.VERSION)).toBe(
version
);

const setSpy = jest.spyOn(Storage.prototype, "setItem");
const browserCacheManager2 = new BrowserCacheManager(
TEST_CONFIG.MSAL_CLIENT_ID,
{
...cacheConfig,
cacheLocation: BrowserCacheLocation.LocalStorage,
},
browserCrypto,
logger,
new StubPerformanceClient(),
new EventHandler()
);
await browserCacheManager2.initialize(TEST_CONFIG.CORRELATION_ID);
expect(window.localStorage.getItem(StaticCacheKeys.VERSION)).toBe(
version
);
expect(setSpy).not.toHaveBeenCalledWith(
StaticCacheKeys.VERSION,
expect.anything()
);
});
});

describe("Interface functions", () => {
let browserSessionStorage: BrowserCacheManager;
let authority: Authority;
Expand Down Expand Up @@ -251,6 +328,7 @@ describe("BrowserCacheManager tests", () => {
expect(browserLocalStorage.getKeys()).toEqual([
"msal.account.keys",
`msal.token.keys.${TEST_CONFIG.MSAL_CLIENT_ID}`,
StaticCacheKeys.VERSION,
msalCacheKey,
msalCacheKey2,
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
TemporaryCacheKeys,
ApiId,
BrowserConstants,
StaticCacheKeys,
} from "../../src/utils/BrowserConstants.js";
import * as BrowserCrypto from "../../src/crypto/BrowserCrypto.js";
import * as PkceGenerator from "../../src/crypto/PkceGenerator.js";
Expand All @@ -66,6 +67,7 @@ import * as BrowserUtils from "../../src/utils/BrowserUtils.js";
import { FetchClient } from "../../src/network/FetchClient.js";
import { TestTimeUtils } from "msal-test-utils";
import { PopupRequest } from "../../src/request/PopupRequest.js";
import { version } from "../../src/packageMetadata.js";

const testPopupWondowDefaults = {
height: BrowserConstants.POPUP_HEIGHT,
Expand Down Expand Up @@ -814,7 +816,10 @@ describe("PopupClient", () => {
});
} catch (e) {
// Test that error was cached for telemetry purposes and then thrown
expect(window.sessionStorage).toHaveLength(1);
expect(window.sessionStorage).toHaveLength(2);
expect(
window.sessionStorage.getItem(StaticCacheKeys.VERSION)
).toEqual(version);
const failures = window.sessionStorage.getItem(
`server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}`
);
Expand Down
17 changes: 14 additions & 3 deletions lib/msal-browser/test/interaction_client/RedirectClient.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import {
ApiId,
BrowserCacheLocation,
InteractionType,
StaticCacheKeys,
} from "../../src/utils/BrowserConstants.js";
import { base64Encode } from "../../src/encode/Base64Encode.js";
import { FetchClient } from "../../src/network/FetchClient.js";
Expand Down Expand Up @@ -87,6 +88,7 @@ import {
TestTimeUtils,
} from "msal-test-utils";
import { BrowserPerformanceClient } from "../../src/telemetry/BrowserPerformanceClient.js";
import { version } from "../../src/packageMetadata.js";

const cacheConfig = {
cacheLocation: BrowserCacheLocation.SessionStorage,
Expand Down Expand Up @@ -205,7 +207,10 @@ describe("RedirectClient", () => {
.then((response) => {
expect(response).toBe(null);
expect(window.localStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(1);
expect(
window.sessionStorage.getItem(StaticCacheKeys.VERSION)
).toEqual(version); // Validate that the one item in sessionStorage is what we expect
done();
});
});
Expand All @@ -223,7 +228,10 @@ describe("RedirectClient", () => {
.then((response) => {
expect(response).toBe(null);
expect(window.localStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(1);
expect(
window.sessionStorage.getItem(StaticCacheKeys.VERSION)
).toEqual(version); // Validate that the one item in sessionStorage is what we expect
done();
});
});
Expand All @@ -241,7 +249,10 @@ describe("RedirectClient", () => {
.then((response) => {
expect(response).toBe(null);
expect(window.localStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(0);
expect(window.sessionStorage.length).toEqual(1);
expect(
window.sessionStorage.getItem(StaticCacheKeys.VERSION)
).toEqual(version); // Validate that the one item in sessionStorage is what we expect
expect(window.location.hash).toEqual(
TEST_HASHES.TEST_SUCCESS_CODE_HASH_POPUP
);
Expand Down
Loading