-
Notifications
You must be signed in to change notification settings - Fork 120
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
Native auth safe logging test #2092
Merged
Merged
Changes from 36 commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
7b69ea5
Safe logging test
Yuki-YuXin 93d0805
minor fix
Yuki-YuXin 66679a6
Use regex rules to filter the logs
Yuki-YuXin 3e545b2
Modify rules and pass tests
Yuki-YuXin aad4fc5
Update tag to use regex
Yuki-YuXin c8072e4
Modify regex rules and use any() in tag log.
Yuki-YuXin d3df120
spy(ILoggerCallback) + assertFalse(regex.matches)
Yuki-YuXin 21d4910
variable order
Yuki-YuXin 1fead78
assertFalse(spyLoggerCallback.failCalled) in every test to fail test.
Yuki-YuXin 0f67b09
remove blank line
Yuki-YuXin 761a77f
Java version sample
Yuki-YuXin c6a08a6
Merge branch 'dev' into yuki/logging-check
Yuki-YuXin 1d542e6
Revert "Java version sample"
Yuki-YuXin 9939418
Fix for password regex rule
Yuki-YuXin 248a338
Narrow down the regex scopes and pass tests.
Yuki-YuXin 4c2ba7c
First version for CountDownLatch but the callback takes too much time…
Yuki-YuXin 07498f3
Fix error which caused by wrong order.
Yuki-YuXin 07bd03b
Use Parameterized for allowPII=true/false but has issue with latch
Yuki-YuXin e91bb0d
Fix the latch bug issue
Yuki-YuXin dd407cd
Fix bug on log callback part to check both message and containsPII item
Yuki-YuXin 71be953
Clean the code logic in the callback part
Yuki-YuXin 3f8e788
Remove withContext block over latch.await
Yuki-YuXin 0c513ff
assertTrue(containsPII) => fail()
Yuki-YuXin a7d6908
Merge branch 'dev' into yuki/logging-check
Yuki-YuXin 7404a79
Use verify instead of fail
Yuki-YuXin eedb6b1
Add containPii check
Yuki-YuXin b6a5c76
Rename variables
Yuki-YuXin c061727
Update Java version
Yuki-YuXin 1a7d1bf
Minor in Kotlin version: fix any()->anyBoolean()
Yuki-YuXin c539cfd
Create a component for the logger safe checking
Yuki-YuXin 7750639
JWT token regex rule
Yuki-YuXin 6816d2d
Remove redundant blank line
Yuki-YuXin 0a05fff
Remove redundant remove logger in Java version
Yuki-YuXin 3c2dc77
Add black line for the new class
Yuki-YuXin f12c022
Use the updated secret
Yuki-YuXin 7c97e54
Use the updated secret
Yuki-YuXin 9bcd81c
Revert "Use the updated secret"
Yuki-YuXin bd552ed
Revert "Use the updated secret"
Yuki-YuXin e03b9da
Merge branch 'dev' into yuki/logging-check
Yuki-YuXin File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
89 changes: 89 additions & 0 deletions
89
msal/src/test/java/com/microsoft/identity/nativeauth/utils/LoggerCheckHelper.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| package com.microsoft.identity.nativeauth.utils | ||
|
|
||
| import com.microsoft.identity.client.ILoggerCallback | ||
| import com.microsoft.identity.client.Logger | ||
| import org.mockito.ArgumentMatcher | ||
| import org.mockito.ArgumentMatchers | ||
| import org.mockito.kotlin.any | ||
| import org.mockito.kotlin.argThat | ||
| import org.mockito.kotlin.eq | ||
| import org.mockito.kotlin.never | ||
| import org.mockito.kotlin.verify | ||
|
|
||
| class LoggerCheckHelper(private val externalLogger: ILoggerCallback, private val allowPII: Boolean) { | ||
|
|
||
| private val sensitivePIIMessages = listOf( | ||
| """(?<![\[\(])["]password["][:=]?(?![\]\)\}])""", // '"password":' '"password"=' exclude 'password' '"challengeType":["password"]' '"challenge_type":"password"}' | ||
| """(?<![\s\?\(])(code)[:=]""", // 'code:' 'code=' exclude 'codeLength' 'error?code' | ||
| """(?<![\(])continuationToken[:=]""", | ||
| """(?<![\(])attributes[:=]""", | ||
| """(?i)\b(accessToken|access_token)[:=]""", // access_token, accessToken | ||
| """(?i)\b(refreshToken|refresh_token)[:=]""", | ||
| """(?i)\b(idToken|id_token)[:=]""", | ||
| """(?i)\b(continuation_token)[:=]""", | ||
| """^[A-Za-z0-9_-]+\\.[A-Za-z0-9_-]+\\.[A-Za-z0-9_-]+$""" // JWT token | ||
| ) | ||
| private val permittedPIIMessages = listOf( | ||
| """(?<![\(])username[:=]""", | ||
| """(?i)\b(challengeTargetLabel|challenge_target_label)[:=]""" | ||
| ) | ||
|
|
||
| init { | ||
| setupLogger() | ||
| } | ||
|
|
||
| private fun setupLogger() { | ||
| Logger.getInstance().setLogLevel(Logger.LogLevel.INFO) | ||
| Logger.getInstance().setEnablePII(allowPII) | ||
| Logger.getInstance().setExternalLogger(externalLogger) | ||
| } | ||
|
|
||
| private fun clearLogger() { | ||
| Logger.getInstance().removeExternalLogger() | ||
| } | ||
|
|
||
| fun checkSafeLogging() { | ||
| var allowList = listOf<String>() | ||
| val disableList: List<String> | ||
|
|
||
| if (allowPII) { | ||
| allowList = permittedPIIMessages | ||
| disableList = sensitivePIIMessages | ||
| } else { | ||
| disableList = sensitivePIIMessages + permittedPIIMessages | ||
| } | ||
|
|
||
| allowList.forEach { regex -> | ||
| verifyLogCouldContain(regex) | ||
| } | ||
| disableList.forEach { regex -> | ||
| verifyLogDoesNotContain(regex) | ||
| } | ||
|
|
||
| clearLogger() | ||
| } | ||
|
|
||
| private fun verifyLogDoesNotContain(regex: String) { | ||
| verify(externalLogger, never()).log( | ||
| any(), | ||
| any(), | ||
| argThat(RegexMatcher(regex)), | ||
| ArgumentMatchers.anyBoolean() | ||
| ) | ||
| } | ||
|
|
||
| private fun verifyLogCouldContain(regex: String) { | ||
| verify(externalLogger, never()).log( | ||
| any(), | ||
| any(), | ||
| argThat(RegexMatcher(regex)), // allowList items are logged but the containsPII should be true. | ||
| eq(false) | ||
| ) | ||
| } | ||
|
|
||
| class RegexMatcher(private val regex: String) : ArgumentMatcher<String> { | ||
| override fun matches(argument: String?): Boolean { | ||
| return regex.toRegex().containsMatchIn(argument ?: "") | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Are you sure about this change?
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.
There is opening PR (and has not merged) from MSAL to update the expired secret. I include their changes to see if that's the reason for the failed pipeline verification. I personally think we need to wait that PR to be merged in order to merge our work.
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.
Agree. Let's not introduce changes with this PR that don't belong here. Let's mark this PR as "draft" in the meantime.