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

Add logs to msal-browser #3004

Merged
merged 15 commits into from
Mar 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Add instrumentation to msal-browser (#3004)",
"packageName": "@azure/msal-browser",
"email": "joarroyo@microsoft.com",
"dependentChangeType": "patch"
}
33 changes: 33 additions & 0 deletions lib/msal-browser/src/cache/BrowserCacheManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ export class BrowserCacheManager extends CacheManager {
* @param value
*/
setAccount(account: AccountEntity): void {
this.logger.verbose("BrowserCacheManager.setAccount called");
const key = account.generateAccountKey();
this.setItem(key, JSON.stringify(account));
}
Expand All @@ -197,6 +198,7 @@ export class BrowserCacheManager extends CacheManager {
* @param idTokenKey
*/
getIdTokenCredential(idTokenKey: string): IdTokenEntity | null {
this.logger.verbose("BrowserCacheManager.getIdTokenCredential called");
const value = this.getItem(idTokenKey);
if (!value) {
return null;
Expand All @@ -209,6 +211,7 @@ export class BrowserCacheManager extends CacheManager {

const idToken: IdTokenEntity = CacheManager.toObject(new IdTokenEntity(), parsedIdToken);
if (IdTokenEntity.isIdTokenEntity(idToken)) {
this.logger.verbose("BrowserCacheManager.getIdTokenCredential: cache hit");
return idToken;
}
return null;
Expand All @@ -219,6 +222,7 @@ export class BrowserCacheManager extends CacheManager {
* @param idToken
*/
setIdTokenCredential(idToken: IdTokenEntity): void {
this.logger.verbose("BrowserCacheManager.setIdTokenCredential called");
const idTokenKey = idToken.generateCredentialKey();
this.setItem(idTokenKey, JSON.stringify(idToken));
}
Expand All @@ -228,6 +232,7 @@ export class BrowserCacheManager extends CacheManager {
* @param key
*/
getAccessTokenCredential(accessTokenKey: string): AccessTokenEntity | null {
this.logger.verbose("BrowserCacheManager.getAccessTokenCredential called");
const value = this.getItem(accessTokenKey);
if (!value) {
return null;
Expand All @@ -239,6 +244,7 @@ export class BrowserCacheManager extends CacheManager {

const accessToken: AccessTokenEntity = CacheManager.toObject(new AccessTokenEntity(), parsedAccessToken);
if (AccessTokenEntity.isAccessTokenEntity(accessToken)) {
this.logger.verbose("BrowserCacheManager.getAccessTokenCredential: cache hit");
return accessToken;
}
return null;
Expand All @@ -249,6 +255,7 @@ export class BrowserCacheManager extends CacheManager {
* @param accessToken
*/
setAccessTokenCredential(accessToken: AccessTokenEntity): void {
this.logger.verbose("BrowserCacheManager.setAccessTokenCredential called");
const accessTokenKey = accessToken.generateCredentialKey();
this.setItem(accessTokenKey, JSON.stringify(accessToken));
}
Expand All @@ -258,6 +265,7 @@ export class BrowserCacheManager extends CacheManager {
* @param refreshTokenKey
*/
getRefreshTokenCredential(refreshTokenKey: string): RefreshTokenEntity | null {
this.logger.verbose("BrowserCacheManager.getRefreshTokenCredential called");
const value = this.getItem(refreshTokenKey);
if (!value) {
return null;
Expand All @@ -269,6 +277,7 @@ export class BrowserCacheManager extends CacheManager {

const refreshToken: RefreshTokenEntity = CacheManager.toObject(new RefreshTokenEntity(), parsedRefreshToken);
if (RefreshTokenEntity.isRefreshTokenEntity(refreshToken)) {
this.logger.verbose("BrowserCacheManager.getRefreshTokenCredential: cache hit");
return refreshToken;
}
return null;
Expand All @@ -279,6 +288,7 @@ export class BrowserCacheManager extends CacheManager {
* @param refreshToken
*/
setRefreshTokenCredential(refreshToken: RefreshTokenEntity): void {
this.logger.verbose("BrowserCacheManager.setRefreshTokenCredential called");
const refreshTokenKey = refreshToken.generateCredentialKey();
this.setItem(refreshTokenKey, JSON.stringify(refreshToken));
}
Expand All @@ -288,6 +298,7 @@ export class BrowserCacheManager extends CacheManager {
* @param appMetadataKey
*/
getAppMetadata(appMetadataKey: string): AppMetadataEntity | null {
this.logger.verbose("BrowserCacheManager.getAppMetadata called");
const value = this.getItem(appMetadataKey);
if (!value) {
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In each of the getters should we also log whether or not we successfully found something in the cache?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I added cache hit logs, and it makes the logs a lot longer. If we discover it creates too much noise, I'm happy to remove them later.

Expand All @@ -300,6 +311,7 @@ export class BrowserCacheManager extends CacheManager {

const appMetadata: AppMetadataEntity = CacheManager.toObject(new AppMetadataEntity(), parsedMetadata);
if (AppMetadataEntity.isAppMetadataEntity(appMetadataKey, appMetadata)) {
this.logger.verbose("BrowserCacheManager.getAppMetadata: cache hit");
return appMetadata;
}
return null;
Expand All @@ -310,6 +322,7 @@ export class BrowserCacheManager extends CacheManager {
* @param appMetadata
*/
setAppMetadata(appMetadata: AppMetadataEntity): void {
this.logger.verbose("BrowserCacheManager.setAppMetadata called");
const appMetadataKey = appMetadata.generateAppMetadataKey();
this.setItem(appMetadataKey, JSON.stringify(appMetadata));
}
Expand All @@ -319,6 +332,7 @@ export class BrowserCacheManager extends CacheManager {
* @param serverTelemetryKey
*/
getServerTelemetry(serverTelemetryKey: string): ServerTelemetryEntity | null {
this.logger.verbose("BrowserCacheManager.getServerTelemetry called");
const value = this.getItem(serverTelemetryKey);
if (!value) {
return null;
Expand All @@ -330,6 +344,7 @@ export class BrowserCacheManager extends CacheManager {

const serverTelemetryEntity = CacheManager.toObject(new ServerTelemetryEntity(), parsedMetadata);
if (ServerTelemetryEntity.isServerTelemetryEntity(serverTelemetryKey, serverTelemetryEntity)) {
this.logger.verbose("BrowserCacheManager.getServerTelemetry: cache hit");
return serverTelemetryEntity;
}
return null;
Expand All @@ -341,19 +356,22 @@ export class BrowserCacheManager extends CacheManager {
* @param serverTelemetry
*/
setServerTelemetry(serverTelemetryKey: string, serverTelemetry: ServerTelemetryEntity): void {
this.logger.verbose("BrowserCacheManager.setServerTelemetry called");
this.setItem(serverTelemetryKey, JSON.stringify(serverTelemetry));
}

/**
*
*/
getAuthorityMetadata(key: string) : AuthorityMetadataEntity | null {
this.logger.verbose("BrowserCacheManager.getAuthorityMetadata called");
const value = this.internalStorage.getItem(key);
if (!value) {
return null;
}
const parsedMetadata = this.validateAndParseJson(value);
if (parsedMetadata && AuthorityMetadataEntity.isAuthorityMetadataEntity(key, parsedMetadata)) {
this.logger.verbose("BrowserCacheManager.getAuthorityMetadata: cache hit");
return CacheManager.toObject(new AuthorityMetadataEntity(), parsedMetadata);
}
return null;
Expand All @@ -374,6 +392,7 @@ export class BrowserCacheManager extends CacheManager {
* @param entity
*/
setAuthorityMetadata(key: string, entity: AuthorityMetadataEntity): void {
this.logger.verbose("BrowserCacheManager.setAuthorityMetadata called");
this.internalStorage.setItem(key, JSON.stringify(entity));
}

Expand All @@ -382,6 +401,7 @@ export class BrowserCacheManager extends CacheManager {
* @param throttlingCacheKey
*/
getThrottlingCache(throttlingCacheKey: string): ThrottlingEntity | null {
this.logger.verbose("BrowserCacheManager.getThrottlingCache called");
const value = this.getItem(throttlingCacheKey);
if (!value) {
return null;
Expand All @@ -394,6 +414,7 @@ export class BrowserCacheManager extends CacheManager {

const throttlingCache = CacheManager.toObject(new ThrottlingEntity(), parsedThrottlingCache);
if (ThrottlingEntity.isThrottlingEntity(throttlingCacheKey, throttlingCache)) {
this.logger.verbose("BrowserCacheManager.getThrottlingCache: cache hit");
return throttlingCache;
}
return null;
Expand All @@ -405,6 +426,7 @@ export class BrowserCacheManager extends CacheManager {
* @param throttlingCache
*/
setThrottlingCache(throttlingCacheKey: string, throttlingCache: ThrottlingEntity): void {
this.logger.verbose("BrowserCacheManager.setThrottlingCache called");
this.setItem(throttlingCacheKey, JSON.stringify(throttlingCache));
}

Expand All @@ -414,8 +436,10 @@ export class BrowserCacheManager extends CacheManager {
* @param key
*/
getTemporaryCache(cacheKey: string, generateKey?: boolean): string | null {
this.logger.verbose("BrowserCacheManager.getTemporaryCache called");
const key = generateKey ? this.generateCacheKey(cacheKey) : cacheKey;
if (this.cacheConfig.storeAuthStateInCookie) {
this.logger.verbose("BrowserCacheManager.getTemporaryCache: storeAuthStateInCookies set to true, retrieving from cookies");
const itemCookie = this.getItemCookie(key);
if (itemCookie) {
return itemCookie;
Expand All @@ -441,6 +465,7 @@ export class BrowserCacheManager extends CacheManager {

this.temporaryCacheStorage.setItem(key, value);
if (this.cacheConfig.storeAuthStateInCookie) {
this.logger.verbose("BrowserCacheManager.setTemporaryCache: storeAuthStateInCookie set to true, setting item cookie");
this.setItemCookie(key, value);
}
}
Expand All @@ -454,6 +479,7 @@ export class BrowserCacheManager extends CacheManager {
this.browserStorage.removeItem(key);
this.temporaryCacheStorage.removeItem(key);
if (this.cacheConfig.storeAuthStateInCookie) {
this.logger.verbose("BrowserCacheManager.removeItem: storeAuthStateInCookie is true, clearing item cookie");
this.clearItemCookie(key);
}
return true;
Expand Down Expand Up @@ -644,6 +670,7 @@ export class BrowserCacheManager extends CacheManager {
* @param account
*/
updateCacheEntries(state: string, nonce: string, authorityInstance: string): void {
this.logger.verbose("BrowserCacheManager.updateCacheEntries called");
// Cache the request state
const stateCacheKey = this.generateStateKey(state);
this.setTemporaryCache(stateCacheKey, state, false);
Expand All @@ -662,6 +689,7 @@ export class BrowserCacheManager extends CacheManager {
* @param state
*/
resetRequestCache(state: string): void {
this.logger.verbose("BrowserCacheManager.resetRequestCache called");
// check state and remove associated cache items
this.getKeys().forEach(key => {
if (!StringUtils.isEmpty(state) && key.indexOf(state) !== -1) {
Expand All @@ -686,6 +714,7 @@ export class BrowserCacheManager extends CacheManager {
* @param stateString
*/
cleanRequestByState(stateString: string): void {
this.logger.verbose("BrowserCacheManager.cleanRequestByState called");
// Interaction is completed - remove interaction status.
if (stateString) {
const stateKey = this.generateStateKey(stateString);
Expand All @@ -701,6 +730,7 @@ export class BrowserCacheManager extends CacheManager {
* @param interactionType
*/
cleanRequestByInteractionType(interactionType: InteractionType): void {
this.logger.verbose("BrowserCacheManager.cleanRequestByInteractionType called");
this.getKeys().forEach((key) => {
if (key.indexOf(TemporaryCacheKeys.REQUEST_STATE) === -1) {
return;
Expand All @@ -719,6 +749,8 @@ export class BrowserCacheManager extends CacheManager {
}

cacheCodeRequest(authCodeRequest: CommonAuthorizationCodeRequest, browserCrypto: ICrypto): void {
this.logger.verbose("BrowserCacheManager.cacheCodeRequest called");

const encodedValue = browserCrypto.base64Encode(JSON.stringify(authCodeRequest));
this.setTemporaryCache(TemporaryCacheKeys.REQUEST_PARAMS, encodedValue, true);
}
Expand All @@ -727,6 +759,7 @@ export class BrowserCacheManager extends CacheManager {
* Gets the token exchange parameters from the cache. Throws an error if nothing is found.
*/
getCachedRequest(state: string, browserCrypto: ICrypto): CommonAuthorizationCodeRequest {
this.logger.verbose("BrowserCacheManager.getCachedRequest called");
// Get token request from cache and parse as TokenExchangeParameters.
const encodedTokenRequest = this.getTemporaryCache(TemporaryCacheKeys.REQUEST_PARAMS, true);
if (!encodedTokenRequest) {
Expand Down
16 changes: 10 additions & 6 deletions lib/msal-browser/src/interaction_handler/RedirectHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,19 @@ export class RedirectHandler extends InteractionHandler {
* @param urlNavigate
*/
async initiateAuthRequest(requestUrl: string, params: RedirectParams): Promise<void> {
this.authModule.logger.verbose("RedirectHandler.initiateAuthRequest called");
// Navigate if valid URL
if (!StringUtils.isEmpty(requestUrl)) {
// Cache start page, returns to this page after redirectUri if navigateToLoginRequestUrl is true
if (params.redirectStartPage) {
this.authModule.logger.verbose("RedirectHandler.initiateAuthRequest: redirectStartPage set to true, caching start page");
this.browserStorage.setTemporaryCache(TemporaryCacheKeys.ORIGIN_URI, params.redirectStartPage, true);
}

// Set interaction status in the library.
this.browserStorage.setTemporaryCache(TemporaryCacheKeys.INTERACTION_STATUS_KEY, BrowserConstants.INTERACTION_IN_PROGRESS_VALUE, true);
this.browserStorage.cacheCodeRequest(this.authCodeRequest, this.browserCrypto);
this.authModule.logger.infoPii("Navigate to:" + requestUrl);
this.authModule.logger.infoPii("RedirectHandler.initiateAuthRequest: Navigate to:" + requestUrl);
const navigationOptions: NavigationOptions = {
apiId: ApiId.acquireTokenRedirect,
timeout: params.redirectTimeout,
Expand All @@ -51,27 +53,27 @@ export class RedirectHandler extends InteractionHandler {

// If onRedirectNavigate is implemented, invoke it and provide requestUrl
if (typeof params.onRedirectNavigate === "function") {
this.authModule.logger.verbose("Invoking onRedirectNavigate callback");
this.authModule.logger.verbose("RedirectHandler.initiateAuthRequest: Invoking onRedirectNavigate callback");
const navigate = params.onRedirectNavigate(requestUrl);

// Returning false from onRedirectNavigate will stop navigation
if (navigate !== false) {
this.authModule.logger.verbose("onRedirectNavigate did not return false, navigating");
this.authModule.logger.verbose("RedirectHandler.initiateAuthRequest: onRedirectNavigate did not return false, navigating");
await params.navigationClient.navigateExternal(requestUrl, navigationOptions);
return;
} else {
this.authModule.logger.verbose("onRedirectNavigate returned false, stopping navigation");
this.authModule.logger.verbose("RedirectHandler.initiateAuthRequest: onRedirectNavigate returned false, stopping navigation");
return;
}
} else {
// Navigate window to request URL
this.authModule.logger.verbose("Navigating window to navigate url");
this.authModule.logger.verbose("RedirectHandler.initiateAuthRequest: Navigating window to navigate url");
await params.navigationClient.navigateExternal(requestUrl, navigationOptions);
return;
}
} else {
// Throw error if request URL is empty.
this.authModule.logger.info("Navigate url is empty");
this.authModule.logger.info("RedirectHandler.initiateAuthRequest: Navigate url is empty");
throw BrowserAuthError.createEmptyNavigationUriError();
}
}
Expand All @@ -81,6 +83,8 @@ export class RedirectHandler extends InteractionHandler {
* @param hash
*/
async handleCodeResponse(locationHash: string, state: string, authority: Authority, networkModule: INetworkModule, clientId?: string): Promise<AuthenticationResult> {
this.authModule.logger.verbose("RedirectHandler.handleCodeResponse called");

// Check that location hash isn't empty.
if (StringUtils.isEmpty(locationHash)) {
throw BrowserAuthError.createEmptyHashError(locationHash);
Expand Down