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.

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

Clean up native auth msal logging #2071

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from
Open

Clean up native auth msal logging #2071

wants to merge 19 commits into from

Conversation

Yuki-YuXin
Copy link
Contributor

@Yuki-YuXin Yuki-YuXin commented Apr 8, 2024

Goal:

  • Review our log statements and reduce to only those that are relevant and useful. Especially the logMethodCall should be much reduced. Not every method call is relevant to know about when debugging.
  • Double check that the existing logs use string interpolation correctly to avoid leaking sensitive information in the logs.

Changes summary:

  • Apply the rule "only logger unknown error" to NativeAuthPublicClientApplication and states. Exception: Sign in has password CodeRequired since it's unusual for the case.
  • Add arguments under method LogSession.logMethodCall tp distinguish callback and coroutine.

Company PR:

AzureAD/microsoft-authentication-library-common-for-android#2396

@Yuki-YuXin Yuki-YuXin added the No-Changelog This change does not update the changelog. label Apr 8, 2024
@github-actions github-actions bot added the msal label Apr 8, 2024
@Yuki-YuXin Yuki-YuXin marked this pull request as ready for review April 8, 2024 10:49
@Yuki-YuXin Yuki-YuXin requested a review from a team as a code owner April 8, 2024 10:49
@@ -372,11 +373,6 @@ class NativeAuthPublicClientApplication(
}
}
is SignInCommandResult.CodeRequired -> {
Logger.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

We have this log here on purpose, please leave it in (notice how it's a warning and not an info). This is to deal with the scenario where a developer calls signIn(username, password) with an account that's an email + OTP signup. The password is silently ignored; this log tries to warn developers of that. Perhaps not the best solution, but best to leave it in for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. The reason is strong here.

@@ -410,11 +395,6 @@ class ResetPasswordPasswordRequiredState internal constructor(
}

is ResetPasswordCommandResult.UserNotFound -> {
Logger.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

re. https://github.com/AzureAD/microsoft-authentication-library-common-for-android/pull/2374/files#r1555927999 - if we add in some form of CommandResult logging, then I agree we can remove this log here. But I would like to have a way of seeing what the CommandResult was that the internal SDK business logic produced, so that we for example can determine that it was an "unexpected result" in the SDK interface and why we're returning an Error to the developer. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CommandResult has been put back in that PR.

@Yuki-YuXin Yuki-YuXin requested a review from a team as a code owner April 26, 2024 17:16
Yuki-YuXin and others added 8 commits April 26, 2024 18:26
This reverts commit c448b88.
# Conflicts:
#	msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplication.kt
#	msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/ResetPasswordStates.kt
#	msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/SignUpStates.kt
# Conflicts:
#	msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplication.kt
#	msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt
#	msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/SignInStates.kt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal No-Changelog This change does not update the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants