Closed SammyO closed 6 months ago
My preferred approach would have been the design suggested by Giannis in Teams chat where the toString method is implemented only once in the abstract Logging class. As multiple inheritance is not supported, we had to rule out that option. So, we are left with a situation where we have to override toString method in all classes. In this scenario I would prefer the toString method to be the "safe" option by default and create another method toUnsafeString() that would return a string containing PII values. This would mean the caller of the classes by default get the benefit of "safety". If anyone wants a string with PII then they must call the toUnsafeString() method.
The interface would look like
interface Logging { fun toUnsafeString(): String fun containsPii(): Boolean } data class UnknownError( override val error: String?, override val errorDescription: String?, override val details: List<Map<String, String>>? = null, override val correlationId: String, override val errorCodes: List<Int>? = null, val exception: Exception? = null ): Error(error, errorDescription, details, correlationId, errorCodes), INativeAuthCommandResult, SignInStartCommandResult, SignInWithContinuationTokenCommandResult, SignInSubmitCodeCommandResult, SignInResendCodeCommandResult, SignInSubmitPasswordCommandResult, SignUpStartCommandResult, SignUpSubmitUserAttributesCommandResult, SignUpSubmitCodeCommandResult, SignUpResendCodeCommandResult, SignUpSubmitPasswordCommandResult, ResetPasswordStartCommandResult, ResetPasswordSubmitCodeCommandResult, ResetPasswordResendCodeCommandResult, ResetPasswordSubmitNewPasswordCommandResult { override fun toUnsafeString(): String { return "UnknownError(correlationId=$correlationId, error=$error, errorDescription=$errorDescription), details=$details, errorCodes=$errorCodes" } override fun containsPii(): Boolean = true override fun toString(): String { return "UnknownError(correlationId=$correlationId)" } }
This design has the same complexity but is more readable as the developer does not have to understand the meaning of the parameter
@SaurabhMSFT I see your point. I don't like this exact approach though, because:
toUnsafeString()
doesn't apply to all classes; not all classes have unsafe parameters. It depends on the value of containsPii()
. The term unsafe string
gives a false impression that the string is always unsafe, which it isn't.toSafeString()
returns a string that given the context is considered safe (either by removing PII, or by including it if the developer wants to add it, in which case it's also safe).can you think of a way to address the above? For example, by renaming mayContainPii
and setting a default value?
My preferred approach would have been the design suggested by Giannis in Teams chat where the toString method is implemented only once in the abstract Logging class. As multiple inheritance is not supported, we had to rule out that option. So, we are left with a situation where we have to override toString method in all classes. In this scenario I would prefer the toString method to be the "safe" option by default and create another method toUnsafeString() that would return a string containing PII values. This would mean the caller of the classes by default get the benefit of "safety". If anyone wants a string with PII then they must call the toUnsafeString() method. The interface would look like
interface Logging { fun toUnsafeString(): String fun containsPii(): Boolean } data class UnknownError( override val error: String?, override val errorDescription: String?, override val details: List<Map<String, String>>? = null, override val correlationId: String, override val errorCodes: List<Int>? = null, val exception: Exception? = null ): Error(error, errorDescription, details, correlationId, errorCodes), INativeAuthCommandResult, SignInStartCommandResult, SignInWithContinuationTokenCommandResult, SignInSubmitCodeCommandResult, SignInResendCodeCommandResult, SignInSubmitPasswordCommandResult, SignUpStartCommandResult, SignUpSubmitUserAttributesCommandResult, SignUpSubmitCodeCommandResult, SignUpResendCodeCommandResult, SignUpSubmitPasswordCommandResult, ResetPasswordStartCommandResult, ResetPasswordSubmitCodeCommandResult, ResetPasswordResendCodeCommandResult, ResetPasswordSubmitNewPasswordCommandResult { override fun toUnsafeString(): String { return "UnknownError(correlationId=$correlationId, error=$error, errorDescription=$errorDescription), details=$details, errorCodes=$errorCodes" } override fun containsPii(): Boolean = true override fun toString(): String { return "UnknownError(correlationId=$correlationId)" } }
This design has the same complexity but is more readable as the developer does not have to understand the meaning of the parameter
@SaurabhMSFT I see your point. I don't like this exact approach though, because:
toUnsafeString()
doesn't apply to all classes; not all classes have unsafe parameters. It depends on the value ofcontainsPii()
. The termunsafe string
gives a false impression that the string is always unsafe, which it isn't.- the current
toSafeString()
returns a string that given the context is considered safe (either by removing PII, or by including it if the developer wants to add it, in which case it's also safe).can you think of a way to address the above? For example, by renaming
mayContainPii
and setting a default value?
We can call the method `toUnsanitizedString()'. That would should the intent that the output of this method may contain PII.
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:
ILoggable
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 ofcontainsPii()
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 oftoSafeString(true)
will return a String that contains PII.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'sallowPii
and the object'scontainsPii()
value.allowPii
is false, thenobject.toString()
is logged, withcontainsPii
set tofalse
.allowPii
is true, thenobject.toUnsanitised()
is logged, withcontainsPii
set to the object'scontainsPii()
.toString()
as well, in case an object is accidentally turned into a String without usingtoSafeString()
. E.g. "SDK will return result $result" (which is short-hand forresult.toString()
).LoggerTest
has been updated with 4 additional test cases.Note 1: it would have been more efficient to have
Logging
defineoverride fun toString()
, meaning every class would be required to overridetoString()
. However, this is not possible in Kotlin (see link).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/2811937Accompanying MSAL PR: https://github.com/AzureAD/microsoft-authentication-library-for-android/pull/2081