Implementing ILoggable in all relevant native auth classes #2081
Conversation
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
rmccahill
approved these changes
Apr 19, 2024
Yuki-YuXin
approved these changes
Apr 19, 2024
SammyO
added a commit
to AzureAD/microsoft-authentication-library-common-for-android
that referenced
this pull request
Apr 25, 2024
The default implementation of `toString()` of Kotlin's data classes uses all fields to compose the String. This unintentionally includes fields that may not be safe for logging. Summary of solution: - Introduce a new `Logging` interface. Recommendation is that all native auth classes implement this interface. - `Logging` contains 2 methods: - `fun toUnsanitizedString(): String` - This method produces a String that may contain PII (PII = Personally identifiable information). The value of `containsPii()` will indicate whether the value actually contains PII. - `fun containsPii(): Boolean` - This method indicates whether the implementing class contains data fields that are considered PII (Personally identifiable information). If this method returns true, then the value of `toSafeString(true)` will return a String that contains PII. - Extend `Logger` with a new methods `*withObject()` & parameter, e.g.: `public static void infoWithObject(final String tag, final String message, final Logging object)`. This logs safe object contents, based on the value of Logger's `allowPii` and the object's `containsPii()` value. - If Logger's `allowPii` is false, then `object.toString()` is logged, with `containsPii` set to `false`. - If Logger's `allowPii` is true, then `object.toUnsanitised()` is logged, with `containsPii` set to the object's `containsPii()`. - Every class should implement `toString()` as well, in case an object is accidentally turned into a String without using `toSafeString()`. E.g. "SDK will return result $result" (which is short-hand for `result.toString()`). - `LoggerTest` has been updated with 4 additional test cases. Note 1: it would have been more efficient to have `Logging` define `override fun toString()`, meaning every class would be required to override `toString()`. However, this is not possible in Kotlin (see [link](https://stackoverflow.com/questions/53771394/override-and-implement-fn-from-class-in-interface)). Note 2: the issue that this PR addresses applies to `data` classes. However, this PR also includes some updates to regular classes, and thereby partially addressing https://identitydivision.visualstudio.com/Engineering/_workitems/edit/2811937 Accompanying MSAL PR: AzureAD/microsoft-authentication-library-for-android#2081
iamgusain
pushed a commit
that referenced
this pull request
Apr 25, 2024
The default implementation of `toString()` of Kotlin's data classes uses all fields to compose the String. This unintentionally includes fields that may not be safe for logging. Solution outline, see MSAL common PR: AzureAD/microsoft-authentication-library-common-for-android#2384
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The default implementation of
toString()of Kotlin's data classes uses all fields to compose the String. This unintentionally includes fields that may not be safe for logging.Solution outline, see MSAL common PR: AzureAD/microsoft-authentication-library-common-for-android#2384