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

Remove the unnecessary code line because of fix in common #2135

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

Remove the unnecessary code line because of fix in common #2135

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
87e051c
Comment the code to see the test
Yuki-YuXin Jul 1, 2024
44e0f07
Update the pr for test. Revert it back later.
Yuki-YuXin Jul 1, 2024
5d8f15c
Decomment because of the test failed
Yuki-YuXin Jul 1, 2024
304ba3b
Modify CommandParametersAdapter.java to move the temporary solution i…
Yuki-YuXin Jul 3, 2024
2780cc6
Merge branch 'dev' into yuki/company-with-robert-PR-test
SammyO Jul 15, 2024
e6614e3
Merge branch 'dev' into yuki/company-with-robert-PR-test
Yuki-YuXin Jul 16, 2024
a4ae188
Merge remote-tracking branch 'origin/yuki/company-with-robert-PR-test…
Yuki-YuXin Jul 16, 2024
11605bb
Revert changes to trigger error log
Yuki-YuXin Jul 16, 2024
b07663c
update submodule reference
Yuki-YuXin Jul 17, 2024
862c126
Use empty string "" to instead "UNSET" because inside Dispatcher.init…
Yuki-YuXin Jul 17, 2024
f87a447
Update the comment
Yuki-YuXin Jul 18, 2024
f799c82
update submodule reference
Yuki-YuXin Jul 18, 2024
73c241c
Merge branch 'dev' into yuki/company-with-robert-PR-test
Yuki-YuXin Jul 19, 2024
2a024b5
Revert back to DiagnosticContext.INSTANCE.threadCorrelationId because…
Yuki-YuXin Jul 22, 2024
c10ec23
update submodule reference
Yuki-YuXin Jul 22, 2024
0ce6946
Use "UNSET"
Yuki-YuXin Jul 22, 2024
74d3658
Merge branch 'dev' into yuki/company-with-robert-PR-test
Yuki-YuXin Jul 22, 2024
1262d2c
Tidy up imports
Yuki-YuXin Jul 24, 2024
b392bda
Revert ref branch
Yuki-YuXin Jul 24, 2024
0b1cd98
ref: dev
Yuki-YuXin Jul 24, 2024
2050eb2
Add try catch block according to AuthenticationResult
Yuki-YuXin Jul 24, 2024
319bd43
Add try catch block according to AuthenticationResult
Yuki-YuXin Jul 24, 2024
c6f864a
Address comment
Yuki-YuXin Jul 29, 2024
856c013
Update submodule reference
Yuki-YuXin Jul 29, 2024
d2f0162
Merge branch 'dev' into yuki/company-with-robert-PR-test
Yuki-YuXin Jul 29, 2024
43be7bf
Merge branch 'dev' into yuki/company-with-robert-PR-test
Yuki-YuXin Jul 31, 2024
f056483
Add changelog entry
Yuki-YuXin Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion azure-pipelines/pull-request-validation/pr-msal.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ resources:
- repository: common
type: github
name: AzureAD/microsoft-authentication-library-common-for-android
ref: dev
ref: robert/empty-correlation-id-fixes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert it back before the merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot change the targe branch in the DevOps portal thus change the command here.

endpoint: ANDROID_GITHUB

pool:
Expand Down
2 changes: 1 addition & 1 deletion common
Open in desktop
Submodule common updated 61 files
+4 −3 ...Utilities/src/main/com/microsoft/identity/labapi/utilities/authentication/KeyVaultAuthenticationClient.java
+96 −0 ...in/com/microsoft/identity/labapi/utilities/authentication/KeyVaultCertificateBasedAuthenticationClient.java
+19 −8 ...piUtilities/src/main/com/microsoft/identity/labapi/utilities/authentication/LabApiAuthenticationClient.java
+1 −0 LabApiUtilities/src/main/com/microsoft/identity/labapi/utilities/authentication/msal4j/Msal4jAuthClient.java
+34 −59 LabApiUtilities/src/main/com/microsoft/identity/labapi/utilities/client/LabClient.java
+5 −0 LabApiUtilities/src/main/com/microsoft/identity/labapi/utilities/constants/LabConstants.java
+2 −2 LabApiUtilities/src/test/com/microsoft/identity/labapi/utilities/client/LabClientTest.java
+2 −1 ...lities/src/test/com/microsoft/identity/labapi/utilties/authentication/common/CertificateCredentialTest.java
+2 −1 ...ies/src/test/com/microsoft/identity/labapi/utilties/authentication/common/MicrosoftClientAssertionTest.java
+1 −1 azure-pipelines/pull-request-validation/build-consumers.yml
+1 −1 azure-pipelines/pull-request-validation/common.yml
+1 −1 azure-pipelines/pull-request-validation/common4j.yml
+1 −1 azure-pipelines/templates/steps/automation-cert.yml
+4 −4 azure-pipelines/templates/steps/continuous-delivery/assemble-publish-projversion.yml
+3 −1 changelog.txt
+5 −2 common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java
+5 −1 common/src/main/java/com/microsoft/identity/common/internal/broker/BrokerRequest.java
+0 −1 common/src/main/java/com/microsoft/identity/common/internal/broker/ipc/BrokerOperationBundle.java
+37 −1 common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/AuthorizationActivityFactory.java
+21 −0 common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java
+3 −1 common/src/main/java/com/microsoft/identity/common/internal/request/MsalBrokerRequestAdapter.java
+27 −3 ...n/src/main/java/com/microsoft/identity/common/internal/ui/webview/EmbeddedWebViewAuthorizationStrategy.java
+82 −2 common/src/test/java/com/microsoft/identity/common/internal/request/MsalBrokerRequestAdapterTests.java
+5 −0 common4j/src/main/com/microsoft/identity/common/java/AuthenticationConstants.java
+2 −3 ...rc/main/com/microsoft/identity/common/java/commands/parameters/BrokerInteractiveTokenCommandParameters.java
+5 −0 ...on4j/src/main/com/microsoft/identity/common/java/commands/parameters/InteractiveTokenCommandParameters.java
+0 −5 common4j/src/main/com/microsoft/identity/common/java/exception/ClientException.java
+0 −3 common4j/src/main/com/microsoft/identity/common/java/exception/ErrorStrings.java
+4 −2 common4j/src/main/com/microsoft/identity/common/java/logging/DiagnosticContext.java
+81 −0 common4j/src/main/com/microsoft/identity/common/java/logging/LibraryInfoHelper.java
+2 −2 common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt
+1 −1 common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/interactors/SignInInteractor.kt
+2 −1 common4j/src/main/com/microsoft/identity/common/java/opentelemetry/SpanName.java
+5 −0 common4j/src/main/com/microsoft/identity/common/java/platform/Device.java
+5 −5 ...rc/main/com/microsoft/identity/common/java/providers/microsoft/microsoftsts/MicrosoftStsOAuth2Strategy.java
+0 −4 common4j/src/main/com/microsoft/identity/common/java/providers/oauth2/AuthorizationRequest.java
+15 −4 common4j/src/main/com/microsoft/identity/common/java/providers/oauth2/OAuth2Strategy.java
+96 −0 common4j/src/main/com/microsoft/identity/common/java/util/ClientExtraSku.java
+66 −0 common4j/src/test/com/microsoft/identity/common/java/logging/LibraryInfoHelperTest.java
+255 −16 common4j/src/test/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestHandlerTest.kt
+48 −0 common4j/src/test/com/microsoft/identity/common/java/util/ClientExtraSkuTest.java
+12 −2 keyvault/src/main/java/com/microsoft/identity/internal/test/keyvault/ApiClient.java
+14 −2 keyvault/src/main/java/com/microsoft/identity/internal/test/keyvault/Configuration.java
+0 −1 labapi/src/main/java/com/microsoft/identity/internal/test/labapi/ApiClient.java
+0 −10 labapi/src/main/java/com/microsoft/identity/internal/test/labapi/Configuration.java
+14 −24 labapi/src/main/java/com/microsoft/identity/internal/test/labapi/api/CreateTempUserApi.java
+13 −24 labapi/src/main/java/com/microsoft/identity/internal/test/labapi/api/DeleteDeviceApi.java
+29 −36 labapi/src/main/java/com/microsoft/identity/internal/test/labapi/api/DisablePolicyApi.java
+29 −36 labapi/src/main/java/com/microsoft/identity/internal/test/labapi/api/EnablePolicyApi.java
+30 −38 labapi/src/main/java/com/microsoft/identity/internal/test/labapi/api/ResetApi.java
+0 −55 testutils/build.gradle
+2 −2 testutils/src/main/java/com/microsoft/identity/internal/testutils/labutils/ConfidentialClientHelper.java
+26 −4 testutils/src/main/java/com/microsoft/identity/internal/testutils/labutils/KeyVaultAuthHelper.java
+54 −0 testutils/src/main/java/com/microsoft/identity/internal/testutils/labutils/KeyVaultFetchHelper.java
+1 −13 testutils/src/main/java/com/microsoft/identity/internal/testutils/labutils/LabDeviceHelper.java
+7 −21 testutils/src/main/java/com/microsoft/identity/internal/testutils/labutils/LabResetHelper.java
+4 −21 testutils/src/main/java/com/microsoft/identity/internal/testutils/labutils/LabUserHelper.java
+6 −19 testutils/src/main/java/com/microsoft/identity/internal/testutils/labutils/PolicyHelper.java
+37 −0 testutils/src/main/java/com/microsoft/identity/internal/testutils/nativeauth/ConfigType.kt
+0 −96 testutils/src/main/java/com/microsoft/identity/internal/testutils/nativeauth/NativeAuthCredentialHelper.kt
+48 −0 ...utils/src/main/java/com/microsoft/identity/internal/testutils/nativeauth/api/models/NativeAuthTestConfig.kt
8 changes: 5 additions & 3 deletions msal/src/main/java/com/microsoft/identity/client/internal/CommandParametersAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.UUID;


/**
Expand Down Expand Up @@ -360,7 +362,7 @@ public static SignUpStartCommandParameters createSignUpStartCommandParameters(
.challengeType(configuration.getChallengeTypes())
.userAttributes(userAttributes)
// Start of the flow, so there is no correlation ID to use from a previous API response.
// Set it to a default value.
// If default value is empty, generate a new UUID as correlationId.
.correlationId(DiagnosticContext.INSTANCE.getThreadCorrelationId())
.build();
}
Expand Down Expand Up @@ -563,7 +565,7 @@ public static SignInStartCommandParameters createSignInStartCommandParameters(
.challengeType(configuration.getChallengeTypes())
.scopes(scopes)
// Start of the flow, so there is no correlation ID to use from a previous API response.
// Set it to a default value.
// If default value is empty, generate a new UUID as correlationId.
.correlationId(DiagnosticContext.INSTANCE.getThreadCorrelationId())
.build();

Expand Down Expand Up @@ -792,7 +794,7 @@ public static ResetPasswordStartCommandParameters createResetPasswordStartComman
.challengeType(configuration.getChallengeTypes())
.clientId(configuration.getClientId())
// Start of the flow, so there is no correlation ID to use from a previous API response.
// Set it to a default value.
// If default value is empty, generate a new UUID as correlationId.
.correlationId(DiagnosticContext.INSTANCE.getThreadCorrelationId())
.build();

Expand Down
10 changes: 5 additions & 5 deletions msal/src/main/java/com/microsoft/identity/nativeauth/NativeAuthPublicClientApplication.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ class NativeAuthPublicClientApplication(
GetAccountResult.AccountFound(
resultValue = AccountState.createFromAccountResult(
account = account,
correlationId = DiagnosticContext.INSTANCE.threadCorrelationId,
correlationId = "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you check what MSAL does here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what was your thinking behind changing this from the thread ID to an empty string?

Copy link
Contributor Author

@Yuki-YuXin Yuki-YuXin Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This path generate AccountState from the SDK cache (authClient.getCurrentAccount())

The default value of DiagnosticContext.INSTANCE.threadCorrelationId is UNSET. And currently, SDK won't transform “UNSET" into UUID.

If use DiagnosticContext.INSTANCE.threadCorrelationId,

  1. authClient.getCurrentAccount() before signIn. correlation id would be "UNSET"
  2. signIn. correlation id should be UUID
  3. authClient.getCurrentAccount() after signIn. When getting correlation id again then becomes UUID as the sign in flow.

I am not sure if the difference between the two correlation ids before and after is what we want

config = nativeAuthConfig
)
)
Expand All @@ -274,7 +274,7 @@ class NativeAuthPublicClientApplication(
errorType = ErrorTypes.CLIENT_EXCEPTION,
errorMessage = "MSAL client exception occurred in getCurrentAccount.",
exception = e,
correlationId = DiagnosticContext.INSTANCE.threadCorrelationId
correlationId = ""
)
}
}
Expand Down Expand Up @@ -341,7 +341,7 @@ class NativeAuthPublicClientApplication(
return@withContext SignInError(
errorType = ErrorTypes.INVALID_USERNAME,
errorMessage = "Empty or blank username",
correlationId = "UNSET"
correlationId = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not set it at all, meaning the value will be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correlationId parameter is mandatory. Setting it to “” would be much easier than dealing with if null.

)
}

Expand Down Expand Up @@ -582,7 +582,7 @@ class NativeAuthPublicClientApplication(
return@withContext SignUpError(
errorType = ErrorTypes.INVALID_USERNAME,
errorMessage = "Empty or blank username",
correlationId = "UNSET"
correlationId = ""
)
}

Expand Down Expand Up @@ -810,7 +810,7 @@ class NativeAuthPublicClientApplication(
return@withContext ResetPasswordError(
errorType = ErrorTypes.INVALID_USERNAME,
errorMessage = "Empty or blank username",
correlationId = "UNSET"
correlationId = ""
)
}

Expand Down
5 changes: 2 additions & 3 deletions msal/src/main/java/com/microsoft/identity/nativeauth/statemachine/states/AccountState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class AccountState private constructor(

constructor (parcel: Parcel) : this (
account = parcel.serializable<IAccount>() as IAccount,
correlationId = parcel.readString() ?: "UNSET",
correlationId = parcel.readString() ?: "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't UUID.fromString("") also produce an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. How about generating random UUID here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep the original "UNSET" string.

config = parcel.serializable<NativeAuthPublicClientApplicationConfiguration>() as NativeAuthPublicClientApplicationConfiguration
)

Expand Down Expand Up @@ -309,12 +309,11 @@ class AccountState private constructor(
correlationId = correlationId
)

val privateCorrelationId = if (correlationId == "UNSET") { UUID.randomUUID().toString() } else { correlationId }

val acquireTokenSilentParameters = AcquireTokenSilentParameters.Builder()
.forAccount(currentAccount)
.fromAuthority(currentAccount.authority)
.withCorrelationId(UUID.fromString(privateCorrelationId))
.withCorrelationId(UUID.fromString(correlationId))
.forceRefresh(forceRefresh)
.withScopes(scopes)
.build()
Expand Down
2 changes: 2 additions & 0 deletions ...st/java/com/microsoft/identity/client/e2e/tests/network/nativeauth/GetAccessTokenTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ import com.microsoft.identity.client.e2e.shadows.ShadowBaseController
import com.microsoft.identity.client.e2e.utils.assertState
import com.microsoft.identity.internal.testutils.nativeauth.NativeAuthCredentialHelper
import com.microsoft.identity.nativeauth.statemachine.errors.GetAccessTokenError
import com.microsoft.identity.nativeauth.statemachine.errors.GetAccountError
import com.microsoft.identity.nativeauth.statemachine.errors.SignInError
import com.microsoft.identity.nativeauth.statemachine.results.GetAccessTokenResult
import com.microsoft.identity.nativeauth.statemachine.results.GetAccountResult
import com.microsoft.identity.nativeauth.statemachine.results.SignInResult
import kotlinx.coroutines.test.runTest
import org.junit.Assert
Expand Down