From d0927ed7222d82e31b200836862779183e94c4b6 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Fri, 27 Aug 2021 22:00:20 +0000 Subject: [PATCH 1/5] [Identity] Allow Pii logging through MSAL --- sdk/identity/identity/CHANGELOG.md | 2 ++ sdk/identity/identity/review/identity.api.md | 1 + sdk/identity/identity/src/client/identityClient.ts | 6 ++++++ sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts | 2 +- sdk/identity/identity/src/msal/flows.ts | 1 + sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts | 2 +- sdk/identity/identity/src/msal/utils.ts | 4 +++- 7 files changed, 15 insertions(+), 3 deletions(-) diff --git a/sdk/identity/identity/CHANGELOG.md b/sdk/identity/identity/CHANGELOG.md index a45f25df2919..f8ff28f5170e 100644 --- a/sdk/identity/identity/CHANGELOG.md +++ b/sdk/identity/identity/CHANGELOG.md @@ -4,6 +4,8 @@ ### Features Added +- All of the credentials now include `allowPiiLogging` on their constructor options. If set to true, Personal Identifiable Information (PII) will be displayed in trace logs. + ### Breaking Changes ### Bugs Fixed diff --git a/sdk/identity/identity/review/identity.api.md b/sdk/identity/identity/review/identity.api.md index c4462db4fa91..07b829a31bf9 100644 --- a/sdk/identity/identity/review/identity.api.md +++ b/sdk/identity/identity/review/identity.api.md @@ -321,6 +321,7 @@ export { TokenCredential } // @public export interface TokenCredentialOptions extends CommonClientOptions { allowMultiTenantAuthentication?: boolean; + allowPiiLogging?: boolean; authorityHost?: string; } diff --git a/sdk/identity/identity/src/client/identityClient.ts b/sdk/identity/identity/src/client/identityClient.ts index c1590e9234cb..f2f4cfaa6cce 100644 --- a/sdk/identity/identity/src/client/identityClient.ts +++ b/sdk/identity/identity/src/client/identityClient.ts @@ -312,4 +312,10 @@ export interface TokenCredentialOptions extends CommonClientOptions { * If set to true, allows authentication flows to change the tenantId of the request if a different tenantId is received from a challenge or through a direct getToken call. */ allowMultiTenantAuthentication?: boolean; + + /** + * If set to true, Personal Identifiable Information (PII) will be displayed in trace logs. + * By default, this is disabled. + */ + allowPiiLogging?: boolean; } diff --git a/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts b/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts index a81004fba11e..b4fad91deaf7 100644 --- a/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts +++ b/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts @@ -39,7 +39,7 @@ export class MSALAuthCode extends MsalBrowser { }; this.msalConfig.system = { loggerOptions: { - loggerCallback: defaultLoggerCallback(this.logger, "Browser") + loggerCallback: defaultLoggerCallback(this.logger, options.allowPiiLogging, "Browser") } }; diff --git a/sdk/identity/identity/src/msal/flows.ts b/sdk/identity/identity/src/msal/flows.ts index f678af41d690..1c6e81b9998d 100644 --- a/sdk/identity/identity/src/msal/flows.ts +++ b/sdk/identity/identity/src/msal/flows.ts @@ -18,6 +18,7 @@ export interface MsalFlowOptions { authorityHost?: string; authenticationRecord?: AuthenticationRecord; disableAutomaticAuthentication?: boolean; + allowPiiLogging?: boolean; } /** diff --git a/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts b/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts index 3e8da5fccada..0963b080fb80 100644 --- a/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts +++ b/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts @@ -135,7 +135,7 @@ export abstract class MsalNode extends MsalBaseUtilities implements MsalFlow { system: { networkClient: this.identityClient, loggerOptions: { - loggerCallback: defaultLoggerCallback(options.logger) + loggerCallback: defaultLoggerCallback(options.logger, options.allowPiiLogging) } } }; diff --git a/sdk/identity/identity/src/msal/utils.ts b/sdk/identity/identity/src/msal/utils.ts index eafb02ab3597..89b127a1104b 100644 --- a/sdk/identity/identity/src/msal/utils.ts +++ b/sdk/identity/identity/src/msal/utils.ts @@ -85,12 +85,14 @@ export function getKnownAuthorities(tenantId: string, authorityHost: string): st */ export const defaultLoggerCallback: ( logger: CredentialLogger, + allowPiiLogging?: boolean, platform?: "Node" | "Browser" ) => msalCommon.ILoggerCallback = ( logger: CredentialLogger, + allowPiiLogging: boolean = false, platform: "Node" | "Browser" = isNode ? "Node" : "Browser" ) => (level, message, containsPii): void => { - if (containsPii) { + if (containsPii && !allowPiiLogging) { return; } switch (level) { From 4b5255e302c680be654f722d8d6d6850547ace9d Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Tue, 31 Aug 2021 21:38:03 +0000 Subject: [PATCH 2/5] cleanup after feedback from Ramya --- .../src/msal/browserFlows/msalAuthCode.ts | 2 +- sdk/identity/identity/src/msal/flows.ts | 2 +- sdk/identity/identity/src/msal/utils.ts | 15 ++++++++------- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts b/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts index b4fad91deaf7..53a09c89fa41 100644 --- a/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts +++ b/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts @@ -39,7 +39,7 @@ export class MSALAuthCode extends MsalBrowser { }; this.msalConfig.system = { loggerOptions: { - loggerCallback: defaultLoggerCallback(this.logger, options.allowPiiLogging, "Browser") + loggerCallback: defaultLoggerCallback(this.logger, options.allowPiiLogging) } }; diff --git a/sdk/identity/identity/src/msal/flows.ts b/sdk/identity/identity/src/msal/flows.ts index 1c6e81b9998d..88f1e77deeef 100644 --- a/sdk/identity/identity/src/msal/flows.ts +++ b/sdk/identity/identity/src/msal/flows.ts @@ -13,12 +13,12 @@ import { CredentialFlowGetTokenOptions } from "./credentials"; */ export interface MsalFlowOptions { logger: CredentialLogger; + allowPiiLogging?: boolean; clientId?: string; tenantId?: string; authorityHost?: string; authenticationRecord?: AuthenticationRecord; disableAutomaticAuthentication?: boolean; - allowPiiLogging?: boolean; } /** diff --git a/sdk/identity/identity/src/msal/utils.ts b/sdk/identity/identity/src/msal/utils.ts index 89b127a1104b..5727a13f9d2d 100644 --- a/sdk/identity/identity/src/msal/utils.ts +++ b/sdk/identity/identity/src/msal/utils.ts @@ -85,13 +85,14 @@ export function getKnownAuthorities(tenantId: string, authorityHost: string): st */ export const defaultLoggerCallback: ( logger: CredentialLogger, - allowPiiLogging?: boolean, - platform?: "Node" | "Browser" -) => msalCommon.ILoggerCallback = ( - logger: CredentialLogger, - allowPiiLogging: boolean = false, - platform: "Node" | "Browser" = isNode ? "Node" : "Browser" -) => (level, message, containsPii): void => { + allowPiiLogging?: boolean +) => msalCommon.ILoggerCallback = (logger: CredentialLogger, allowPiiLogging: boolean = false) => ( + level, + message, + containsPii +): void => { + const platform = isNode ? "Node" : "Browser"; + if (containsPii && !allowPiiLogging) { return; } From acbad36650103a3573ad66d1b94cebe9bd96a62c Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Tue, 31 Aug 2021 21:47:12 +0000 Subject: [PATCH 3/5] added missing MSAL option relate to piiLogging --- sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts b/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts index 0963b080fb80..54cb6ba910cf 100644 --- a/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts +++ b/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts @@ -135,6 +135,7 @@ export abstract class MsalNode extends MsalBaseUtilities implements MsalFlow { system: { networkClient: this.identityClient, loggerOptions: { + piiLoggingEnabled: options.allowPiiLogging, loggerCallback: defaultLoggerCallback(options.logger, options.allowPiiLogging) } } From 4c7be538cebe6e1c85beb825564f237989e23bc8 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Wed, 1 Sep 2021 01:06:51 +0000 Subject: [PATCH 4/5] renamed to enableUnsafeLogging --- sdk/identity/identity/CHANGELOG.md | 2 +- sdk/identity/identity/src/client/identityClient.ts | 4 ++-- .../identity/src/msal/browserFlows/msalAuthCode.ts | 2 +- sdk/identity/identity/src/msal/flows.ts | 2 +- sdk/identity/identity/src/msal/utils.ts | 13 ++++++------- 5 files changed, 11 insertions(+), 12 deletions(-) diff --git a/sdk/identity/identity/CHANGELOG.md b/sdk/identity/identity/CHANGELOG.md index f8ff28f5170e..98792aaf64ee 100644 --- a/sdk/identity/identity/CHANGELOG.md +++ b/sdk/identity/identity/CHANGELOG.md @@ -4,7 +4,7 @@ ### Features Added -- All of the credentials now include `allowPiiLogging` on their constructor options. If set to true, Personal Identifiable Information (PII) will be displayed in trace logs. +- All of the credentials now include `enableUnsafeLogging` on their constructor options. If set to true, Personal Identifiable Information (PII) will be displayed in trace logs. ### Breaking Changes diff --git a/sdk/identity/identity/src/client/identityClient.ts b/sdk/identity/identity/src/client/identityClient.ts index f2f4cfaa6cce..624ad6927672 100644 --- a/sdk/identity/identity/src/client/identityClient.ts +++ b/sdk/identity/identity/src/client/identityClient.ts @@ -314,8 +314,8 @@ export interface TokenCredentialOptions extends CommonClientOptions { allowMultiTenantAuthentication?: boolean; /** - * If set to true, Personal Identifiable Information (PII) will be displayed in trace logs. + * If set to true, personal data may be displayed in the trace logs depending on the authentication method. * By default, this is disabled. */ - allowPiiLogging?: boolean; + enableUnsafeLogging?: boolean; } diff --git a/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts b/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts index 53a09c89fa41..fa444c92ad3a 100644 --- a/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts +++ b/sdk/identity/identity/src/msal/browserFlows/msalAuthCode.ts @@ -39,7 +39,7 @@ export class MSALAuthCode extends MsalBrowser { }; this.msalConfig.system = { loggerOptions: { - loggerCallback: defaultLoggerCallback(this.logger, options.allowPiiLogging) + loggerCallback: defaultLoggerCallback(this.logger, options.enableUnsafeLogging) } }; diff --git a/sdk/identity/identity/src/msal/flows.ts b/sdk/identity/identity/src/msal/flows.ts index 88f1e77deeef..2b9714ecf910 100644 --- a/sdk/identity/identity/src/msal/flows.ts +++ b/sdk/identity/identity/src/msal/flows.ts @@ -13,7 +13,7 @@ import { CredentialFlowGetTokenOptions } from "./credentials"; */ export interface MsalFlowOptions { logger: CredentialLogger; - allowPiiLogging?: boolean; + enableUnsafeLogging?: boolean; clientId?: string; tenantId?: string; authorityHost?: string; diff --git a/sdk/identity/identity/src/msal/utils.ts b/sdk/identity/identity/src/msal/utils.ts index 5727a13f9d2d..92a925dee2f2 100644 --- a/sdk/identity/identity/src/msal/utils.ts +++ b/sdk/identity/identity/src/msal/utils.ts @@ -85,15 +85,14 @@ export function getKnownAuthorities(tenantId: string, authorityHost: string): st */ export const defaultLoggerCallback: ( logger: CredentialLogger, - allowPiiLogging?: boolean -) => msalCommon.ILoggerCallback = (logger: CredentialLogger, allowPiiLogging: boolean = false) => ( - level, - message, - containsPii -): void => { + enableUnsafeLogging?: boolean +) => msalCommon.ILoggerCallback = ( + logger: CredentialLogger, + enableUnsafeLogging: boolean = false +) => (level, message, containsPii): void => { const platform = isNode ? "Node" : "Browser"; - if (containsPii && !allowPiiLogging) { + if (containsPii && !enableUnsafeLogging) { return; } switch (level) { From 619218890f3a61677f404eb62f93e6f50faedcac Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Wed, 1 Sep 2021 01:10:11 +0000 Subject: [PATCH 5/5] renamed to enableUnsafeLogging (part 2, I decided to merge main in between) --- sdk/identity/identity/review/identity.api.md | 2 +- sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/identity/identity/review/identity.api.md b/sdk/identity/identity/review/identity.api.md index e38c2ae222c7..749db9f5c183 100644 --- a/sdk/identity/identity/review/identity.api.md +++ b/sdk/identity/identity/review/identity.api.md @@ -320,8 +320,8 @@ export { TokenCredential } // @public export interface TokenCredentialOptions extends CommonClientOptions { allowMultiTenantAuthentication?: boolean; - allowPiiLogging?: boolean; authorityHost?: string; + enableUnsafeLogging?: boolean; } // @public diff --git a/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts b/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts index 54cb6ba910cf..2bfc6e6332f2 100644 --- a/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts +++ b/sdk/identity/identity/src/msal/nodeFlows/nodeCommon.ts @@ -135,8 +135,8 @@ export abstract class MsalNode extends MsalBaseUtilities implements MsalFlow { system: { networkClient: this.identityClient, loggerOptions: { - piiLoggingEnabled: options.allowPiiLogging, - loggerCallback: defaultLoggerCallback(options.logger, options.allowPiiLogging) + piiLoggingEnabled: options.enableUnsafeLogging, + loggerCallback: defaultLoggerCallback(options.logger, options.enableUnsafeLogging) } } };