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.

Sign up for GitHub

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

Jump to bottom

Allow overriding log level and PII setting with session storage key-values #6704

Merged
merged 16 commits into from
Dec 12, 2023
Merged

Allow overriding log level and PII setting with session storage key-values #6704

merged 16 commits into from
Dec 12, 2023

Conversation

konstantin-msft
Copy link
Collaborator

@konstantin-msft konstantin-msft commented Nov 17, 2023 •
edited
Loading

  • Allow overriding log level and PII setting with session storage key-values to troubleshoot errors in non-dev environments.

@github-actions github-actions bot added documentation Related to documentation. msal-browser Related to msal-browser package labels Nov 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2023 •
edited
Loading

Codecov Report

Merging #6704 (ded6160) into dev (81d34b4) will decrease coverage by 4.28%.
Report is 399 commits behind head on dev.
The diff coverage is n/a.

Additional details and impacted files
Flag Coverage Δ
msal-angular 96.73% <ø> (+0.22%) ⬆️
msal-browser 78.96% <ø> (-7.51%) ⬇️
msal-common ?
msal-core ?
msal-node ?
msal-node-extensions ?
msal-react 94.24% <ø> (-0.45%) ⬇️
node-token-validation ?
Files Coverage Δ
lib/msal-angular/src/constants.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.broadcast.service.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.guard.ts 90.78% <ø> (+0.64%) ⬆️
lib/msal-angular/src/msal.interceptor.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.module.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.navigation.client.ts 93.33% <ø> (+0.47%) ⬆️
lib/msal-angular/src/msal.redirect.component.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.service.ts 100.00% <ø> (ø)
lib/msal-angular/src/packageMetadata.ts 100.00% <ø> (ø)
...b/msal-browser/src/app/IPublicClientApplication.ts 41.17% <ø> (-2.58%) ⬇️
... and 48 more

... and 187 files with indirect coverage changes

@konstantin-msft konstantin-msft changed the title Allow overriding log level and PII setting with local storage key-values Allow overriding log level and PII setting with session storage key-values Nov 17, 2023
@konstantin-msft konstantin-msft marked this pull request as ready for review November 17, 2023 21:48
tnorling
tnorling approved these changes Dec 5, 2023
View reviewed changes
Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

Looks good - recommend adding a couple tests as well

lib/msal-browser/docs/logging.md Outdated

Add `msal.browser.log.pii` key to `Session Storage`, set it's value to `true` or `false` and refresh the page.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor suggestion - 0 or 1 may be easier/less error prone than using string values

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would prefer to stick to true/false to be consistent with the pii setting in the MSAL config.

lib/msal-browser/src/operatingcontext/BaseOperatingContext.ts Outdated Show resolved Hide resolved
peterzenz
peterzenz reviewed Dec 5, 2023
View reviewed changes
Copy link
Contributor

@peterzenz peterzenz left a comment

Choose a reason for hiding this comment

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

Tests?

lib/msal-browser/docs/logging.md Outdated
### Navigate to session storage

1. Open developer tools by pressing F12
2. Navigate to `Application` tab
Copy link
Contributor

Choose a reason for hiding this comment

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

These instructions vary per browser. For instance, Firefox is Developer Tools -> Storage Tab... The Application tab isn't needed here. Can you update these to be called out per browser.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a very good call, thank you. Updated docs.

lib/msal-browser/docs/logging.md Outdated

Add `msal.browser.log.pii` key to `Session Storage`, set it's value to `true` or `false` and refresh the page.
Copy link
Contributor

Choose a reason for hiding this comment

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

We need more steps here. This should be something we can refer folks to for log collection and troubleshooting. Something along the lines of:
Once you have enabled Verbose logging and (optionally) enabled PII logging, refresh the page and retry the sign-in operation. The captured logs can be found *here*. Please review them to ensure there is no sensitive data included in them before sharing them with the team requesting them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated docs, thank you for the suggestion.

@konstantin-msft
Copy link
Collaborator Author

Tests?

Added tests

@konstantin-msft konstantin-msft merged commit 8c1c63d into dev Dec 12, 2023
32 checks passed
@konstantin-msft konstantin-msft deleted the enable_local_logging branch December 12, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnorling tnorling tnorling approved these changes

@hectormmg hectormmg hectormmg approved these changes

@sameerag sameerag Awaiting requested review from sameerag sameerag is a code owner

@jo-arroyo jo-arroyo Awaiting requested review from jo-arroyo jo-arroyo is a code owner

@peterzenz peterzenz Awaiting requested review from peterzenz peterzenz is a code owner

Assignees
No one assigned
Labels
documentation Related to documentation. msal-browser Related to msal-browser package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@konstantin-msft @codecov-commenter @hectormmg @tnorling @peterzenz