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 more logs in getDeviceInfo flow due to an IcM #1439
Conversation
@@ -1459,7 +1459,7 @@ - (void)signoutWithAccount:(nonnull MSALAccount *)account | |||
- (void)getDeviceInformationWithParameters:(MSALParameters *)parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request does not update CHANGELOG.md.
Please consider if this change would be noticeable to a partner or user and either update CHANGELOG.md or resolve this conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments to double check
@@ -53,12 +53,14 @@ - (void)deviceInfoWithRequestParameters:(MSIDRequestParameters *)requestParamete | |||
{ | |||
[msalDeviceInfo addRegisteredDeviceMetadataInformation:deviceRegMetaDataInfo]; | |||
} | |||
|
|||
|
|||
MSID_LOG_WITH_CTX_PII(MSIDLogLevelInfo, nil, @"GetDeviceInfo: Completing filling device info: %@, error: %@", msalDeviceInfo, error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could msalDeviceInfo have potential PII? e.g. UPN
[ssoExtensionRequest executeRequestWithCompletion:^(MSIDDeviceInfo * _Nullable deviceInfo, NSError * _Nullable error) | ||
{ | ||
MSID_LOG_WITH_CTX(MSIDLogLevelInfo, requestParameters, @"GetDeviceInfo: Receiving results from Sso Extension with device info: %@, error: %@", deviceInfo, error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could deviceInfo contain PII?
@@ -81,31 +84,37 @@ - (void)deviceInfoWithRequestParameters:(MSIDRequestParameters *)requestParamete | |||
if (@available(iOS 13.0, macOS 10.15, *)) | |||
{ | |||
// We are here means canCallSSOExtension is TRUE | |||
MSID_LOG_WITH_CTX(MSIDLogLevelInfo, requestParameters, @"GetDeviceInfo: Creating Sso Extension request"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this potentially too verbose for info level?
NSError *error = MSIDCreateError(MSIDErrorDomain, MSIDErrorInternal, @"Trying to start a get accounts request while one is already executing", nil, nil, nil, nil, nil, YES); | ||
completionBlock(nil, error); | ||
return; | ||
} | ||
|
||
MSID_LOG_WITH_CTX(MSIDLogLevelInfo, requestParameters, @"GetDeviceInfo: Start querying Sso Extension request with request params"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be too verbose for info level logs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated per comment.
deviceInfo contains extraDict which may contain trackable information, so make it as PII
@@ -1471,7 +1471,7 @@ - (void)getDeviceInformationWithParameters:(MSALParameters *)parameters | |||
} | |||
else | |||
{ | |||
MSID_LOG_WITH_CTX_PII(MSIDLogLevelInfo, nil, @"Retrieved device info %@", deviceInformation); | |||
MSID_LOG_WITH_CTX_PII(MSIDLogLevelInfo, nil, @"Retrieved device info %@", MSID_PII_LOG_MASKABLE(deviceInformation)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, if the deviceInfo already hased, is the output PII or regular info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is if the value is array, it will only show the length of array, if error, will because error domain + error code. For the rest, will show as null/nonull
So if it is hashed as a string, it will only show as nonull/null
Let me know if that is correct
* Allow to pass eqp to the /token endpoint. * Update changelog. * .. * updated changelog * Update common core * Updating common-core for fix in embedded wv * Updating changelog * Add PR number in changelog * Add additional flag to know when to wipe cache for all accounts. * Add button to wipe all accounts in MSAL Test App * Update submodule * Update submodule * Update submodule * Merge master in dev (#1432) * Updating MSAL framework checksum & url for 1.1.25 [skip ci] * Merge release 1.1.26 in master (#1429) * Allow to pass eqp to the /token endpoint. * Update changelog. * .. * updated changelog * Update common core * Updating common-core for fix in embedded wv * Updating changelog * Add PR number in changelog * Update changelog. Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Jason Zeng <zeyong@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> * Updating MSAL framework checksum & url for 1.1.26 [skip ci] Co-authored-by: Yong Zeng <zeyong@microsoft.com> Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> * Multiple WPJ PkeyAuth support * Add a list of additional locations for partner caches to be wiped. Add unit test. * Update changelog * Update comments for wipeCacheForAllAccounts property. * Add SBOM task (#1425) * Add SBOM task. * Update sbom task. * Changes to expose Mdm Id via Device Info sso ext request * Addressed review comments * Multitenant PkeyAuth support * Merge dev in common core * Updated changelog * Fixed changelog * Fixed library version in Info.plist files * Revert breaking API change to unbreak MSAL CPP * Update code to remove token cache from additional locations, allow to continue in case one fails but keep track of the list of failed locations. * Fix build error, code will only run on MacOS * Updated to return mdmId only if not nil or not empty * Add more logs in getDeviceInfo flow due to an IcM (#1439) * Add more logs in getDeviceInfo flow due to an IcM * Address Cr * Read the keychain data in order to trigger the prompt asking for login password, user HAS TO click 'Always Allow' to then be able to delete it. * Update submodule * Return failing additional partner locations as userInfo in case of error. Add test that check wiping for additional partner locations. * Continue trying to remove remaining partner locations cache if there is a read error. * Update MSAL.podspec The project does not seem to be a preliminary preview any longer. * Update common core to v2 pkeyauth changes * Addressed additional comments * Addressed comments * Added more test cases * common core updated * updated msal to point to latest common core after common core merge * update versions * Kaisong1990/resolve merge release conflicts (#1442) * Updating MSAL framework checksum & url for 1.1.25 [skip ci] * Merge release 1.1.26 in master (#1429) * Allow to pass eqp to the /token endpoint. * Update changelog. * .. * updated changelog * Update common core * Updating common-core for fix in embedded wv * Updating changelog * Add PR number in changelog * Update changelog. Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Jason Zeng <zeyong@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> * Updating MSAL framework checksum & url for 1.1.26 [skip ci] * point to release 1.7.5 * add a space to trigger pipeline Co-authored-by: Yong Zeng <zeyong@microsoft.com> Co-authored-by: Sergei Demchenko <sedemche@microsoft.com> Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> * Kaisong1990/update automation device version (#1445) * Update the simulator target * fix for Umbrella header for module 'MSAL' does not include header 'MSALWipeCacheForAllAccountsConfig.h' in automation * Add fix to disable to disable 3 tests due to Labe pending certificate, also apply fix on one test * Update CommonCore submodule to align with its latest master hash (#1450) Co-authored-by: Sergey Demchenko <sedemche@microsoft.com> Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Jason Zeng <zeyong@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> Co-authored-by: Juan Arias Roldan <jariasroldan@microsoft.com> Co-authored-by: Olga Dalton <olgadalton@olgas-work-mac.lan> Co-authored-by: Veena Soman <veenasoman@microsoft.com> Co-authored-by: Olga Dalton <olgadalton@Olgas-Work-Mac.local> Co-authored-by: Wolfgang Lutz <WLBORg@gmx.de> Co-authored-by: Olga Dalton <oldalton@microsoft.com> Co-authored-by: Juan Arias <juan-arias@users.noreply.github.com>
* Updating MSAL framework checksum & url for 1.1.25 [skip ci] * Merge release 1.1.26 in master (#1429) * Allow to pass eqp to the /token endpoint. * Update changelog. * .. * updated changelog * Update common core * Updating common-core for fix in embedded wv * Updating changelog * Add PR number in changelog * Update changelog. Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Jason Zeng <zeyong@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> * Updating MSAL framework checksum & url for 1.1.26 [skip ci] * Release/1.2.0 (#1444) * Allow to pass eqp to the /token endpoint. * Update changelog. * .. * updated changelog * Update common core * Updating common-core for fix in embedded wv * Updating changelog * Add PR number in changelog * Add additional flag to know when to wipe cache for all accounts. * Add button to wipe all accounts in MSAL Test App * Update submodule * Update submodule * Update submodule * Merge master in dev (#1432) * Updating MSAL framework checksum & url for 1.1.25 [skip ci] * Merge release 1.1.26 in master (#1429) * Allow to pass eqp to the /token endpoint. * Update changelog. * .. * updated changelog * Update common core * Updating common-core for fix in embedded wv * Updating changelog * Add PR number in changelog * Update changelog. Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Jason Zeng <zeyong@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> * Updating MSAL framework checksum & url for 1.1.26 [skip ci] Co-authored-by: Yong Zeng <zeyong@microsoft.com> Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> * Multiple WPJ PkeyAuth support * Add a list of additional locations for partner caches to be wiped. Add unit test. * Update changelog * Update comments for wipeCacheForAllAccounts property. * Add SBOM task (#1425) * Add SBOM task. * Update sbom task. * Changes to expose Mdm Id via Device Info sso ext request * Addressed review comments * Multitenant PkeyAuth support * Merge dev in common core * Updated changelog * Fixed changelog * Fixed library version in Info.plist files * Revert breaking API change to unbreak MSAL CPP * Update code to remove token cache from additional locations, allow to continue in case one fails but keep track of the list of failed locations. * Fix build error, code will only run on MacOS * Updated to return mdmId only if not nil or not empty * Add more logs in getDeviceInfo flow due to an IcM (#1439) * Add more logs in getDeviceInfo flow due to an IcM * Address Cr * Read the keychain data in order to trigger the prompt asking for login password, user HAS TO click 'Always Allow' to then be able to delete it. * Update submodule * Return failing additional partner locations as userInfo in case of error. Add test that check wiping for additional partner locations. * Continue trying to remove remaining partner locations cache if there is a read error. * Update MSAL.podspec The project does not seem to be a preliminary preview any longer. * Update common core to v2 pkeyauth changes * Addressed additional comments * Addressed comments * Added more test cases * common core updated * updated msal to point to latest common core after common core merge * update versions * Kaisong1990/resolve merge release conflicts (#1442) * Updating MSAL framework checksum & url for 1.1.25 [skip ci] * Merge release 1.1.26 in master (#1429) * Allow to pass eqp to the /token endpoint. * Update changelog. * .. * updated changelog * Update common core * Updating common-core for fix in embedded wv * Updating changelog * Add PR number in changelog * Update changelog. Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Jason Zeng <zeyong@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> * Updating MSAL framework checksum & url for 1.1.26 [skip ci] * point to release 1.7.5 * add a space to trigger pipeline Co-authored-by: Yong Zeng <zeyong@microsoft.com> Co-authored-by: Sergei Demchenko <sedemche@microsoft.com> Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> * Kaisong1990/update automation device version (#1445) * Update the simulator target * fix for Umbrella header for module 'MSAL' does not include header 'MSALWipeCacheForAllAccountsConfig.h' in automation * Add fix to disable to disable 3 tests due to Labe pending certificate, also apply fix on one test * Update CommonCore submodule to align with its latest master hash (#1450) Co-authored-by: Sergey Demchenko <sedemche@microsoft.com> Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Jason Zeng <zeyong@microsoft.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> Co-authored-by: Juan Arias Roldan <jariasroldan@microsoft.com> Co-authored-by: Olga Dalton <olgadalton@olgas-work-mac.lan> Co-authored-by: Veena Soman <veenasoman@microsoft.com> Co-authored-by: Olga Dalton <olgadalton@Olgas-Work-Mac.local> Co-authored-by: Wolfgang Lutz <WLBORg@gmx.de> Co-authored-by: Olga Dalton <oldalton@microsoft.com> Co-authored-by: Juan Arias <juan-arias@users.noreply.github.com> * Use dotnet 2.1.0 in release pipeline for ESRP * Adding workaround of installing corenet app 2.1.x for ESRP task * Updating MSAL framework checksum & url for 1.2.0 [skip ci] * resolve merge conflicts Co-authored-by: Yong Zeng <zeyong@microsoft.com> Co-authored-by: Sergei Demchenko <sedemche@microsoft.com> Co-authored-by: petlee <petlee@microsoft.com> Co-authored-by: Peter Lee <30302999+NerevarineRule@users.noreply.github.com> Co-authored-by: Ameya Patil <amepatil@microsoft.com> Co-authored-by: Juan Arias Roldan <jariasroldan@microsoft.com> Co-authored-by: Olga Dalton <olgadalton@olgas-work-mac.lan> Co-authored-by: Veena Soman <veenasoman@microsoft.com> Co-authored-by: Olga Dalton <olgadalton@Olgas-Work-Mac.local> Co-authored-by: Wolfgang Lutz <WLBORg@gmx.de> Co-authored-by: Olga Dalton <oldalton@microsoft.com> Co-authored-by: Juan Arias <juan-arias@users.noreply.github.com> Co-authored-by: Ameya Patil <ameyapat@buffalo.edu>
Proposed changes
There is a customer issue where getDeviceInfo is hanging, but no local repro yet, based on the logs, the flow stopped responding somewhere, but logs only show the staring of the whole flow. Adding more logs for debugging.
Type of change
Risk
Additional information