[Identity] Allow Pii logging through MSAL #17321
Conversation
sadasant
requested review from
bterlson,
chradek,
schaabs,
witemple-msft and
xirzec
as code owners
ghost
added
the
| @@ -321,6 +321,7 @@ export { TokenCredential } | |||
| // @public | |||
| export interface TokenCredentialOptions extends CommonClientOptions { | |||
| allowMultiTenantAuthentication?: boolean; | |||
| allowPiiLogging?: boolean; | |||
There was a problem hiding this comment.
The name of this property is still being discussed.
There was a problem hiding this comment.
ok ok thank you! I’ll move it to draft then
There was a problem hiding this comment.
If this name is not decided yet, giving that we’re about to reach code complete this Friday, should we push this to the next milestone?
There was a problem hiding this comment.
I think PII is no longer used by any of our data standards. I believe "End User Identifiable Information" (EUII) is the new equivalent.
It also seems to fall under "Personal Data" in other places: https://www.microsoft.com/en-us/trust-center/privacy/customer-data-definitions
| @@ -18,6 +18,7 @@ export interface MsalFlowOptions { | |||
| authorityHost?: string; | |||
| authenticationRecord?: AuthenticationRecord; | |||
| disableAutomaticAuthentication?: boolean; | |||
| allowPiiLogging?: boolean; | |||
There was a problem hiding this comment.
where do we pass this on to the MSAL clients ?
Which part enables MSAL logging?
There was a problem hiding this comment.
It’s a bit odd to track, but the MSAL client sends us whether a log line has PII or not through their log callbacks. We receive that parameter here: https://github.com/Azure/azure-sdk-for-js/pull/17321/files#diff-b4fea3e025941e1a4d4d02990dc8c9a31d77db91dec08c490f2cf818fee97111R94-R95 (it’s the one named “containsPii”). So, MSAL always sends us logs regardless of PII, we decide whether to log them or not.
sdk/identity/identity/CHANGELOG.md
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
Is allowPiiLogging the agreed upon name in the feature crew? Am just curious about the PII becoming Pii
There was a problem hiding this comment.
It hasn’t been decided yet. Here’s Vinay saying that this hasn’t been decided yet: #17321 (comment)
| @@ -85,12 +85,14 @@ export function getKnownAuthorities(tenantId: string, authorityHost: string): st | |||
| */ | |||
| export const defaultLoggerCallback: ( | |||
| logger: CredentialLogger, | |||
| allowPiiLogging?: boolean, | |||
| platform?: "Node" | "Browser" | |||
There was a problem hiding this comment.
Having two optional parameters is something we try to avoid. If there are more than 1 optional parameter, we prefer to use the options bag to avoid the case of passing undefined to the first one when we don;t want to set anything there, but do want to pass something to the second parameter.
In this case, do we even need the platform parameter if we can determine that using the isNode property?
There was a problem hiding this comment.
ahh correct! I like how you think :) I’ll clean this up!
| @@ -18,6 +18,7 @@ export interface MsalFlowOptions { | |||
| authorityHost?: string; | |||
| authenticationRecord?: AuthenticationRecord; | |||
| disableAutomaticAuthentication?: boolean; | |||
| allowPiiLogging?: boolean; | |||
There was a problem hiding this comment.
Nit: For ease of readability, I would recommend moving this up to appear right after the logger as the two are related
|
What is the reason for this PR to be in draft mode? Are we waiting for something and this is not ready for merge? |
|
@ramya-rao-a I moved it to draft because the name is not agreed upon. Once the name is agreed upon, I’ll update this and take it out of draft. This is mainly as a double assurance that nobody will merge this until we’ve agreed on the name (though the chances of this happening are extremely low) |
|
Something to mention is that although this PR enables Pii logging, there’s nothing currently using Pii logging. I tried checking through our tests, and no codepaths are making use of Pii logs. Should we try to standardize what gets logged across languages? @g2vinay perhaps later, after this PR. |
|
I just remembered that the latest name we picked on Monday’s meeting was |
|
We’ve decided to close this for now, since currently there’s no use case for these logs on JS. |
This PR makes it so all of the credentials now include
enableUnsafeLoggingon their constructor options. If set to true, Personal Identifiable Information (PII) will be displayed in trace logs.Feedback appreciated!
Fixes #17281