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

Add check for unset correlation ID when configuring request headers #2435

Merged
merged 27 commits into from
Jul 29, 2024
Merged

Add check for unset correlation ID when configuring request headers #2435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a28a3f7
Add check for empty correlation ID when setting request headers, upda…
rmccahill Jun 20, 2024
046593a
Update changelog
rmccahill Jun 20, 2024
9fe242c
Update empty header test naming
rmccahill Jun 24, 2024
428f2ab
Merge branch 'dev' into robert/empty-correlation-id-fixes
rmccahill Jun 28, 2024
f221e24
Update changelog
rmccahill Jun 28, 2024
73f34ff
Merge branch 'dev' into robert/empty-correlation-id-fixes
Yuki-YuXin Jul 1, 2024
2e760ac
Add argument to pass MSAL consumer test
Yuki-YuXin Jul 1, 2024
c1d5a3f
Merge branch 'dev' into robert/empty-correlation-id-fixes
Yuki-YuXin Jul 3, 2024
b653ea6
Merge branch 'dev' into robert/empty-correlation-id-fixes
Yuki-YuXin Jul 16, 2024
e631037
Revert changes to trigger error log
Yuki-YuXin Jul 16, 2024
fed453c
getThreadCorrelationId
Yuki-YuXin Jul 17, 2024
c731847
getThreadCorrelationId UNSET
Yuki-YuXin Jul 17, 2024
3391582
Inside nativeAuthResponseHandler.getSignInInitiateResultFromHttpRespo…
Yuki-YuXin Jul 17, 2024
5976371
Merge branch 'dev' into robert/empty-correlation-id-fixes
Yuki-YuXin Jul 18, 2024
2685650
Modify the test
Yuki-YuXin Jul 18, 2024
d0a2618
Modify the remaining test
Yuki-YuXin Jul 18, 2024
abc1279
Modify the remaining test
Yuki-YuXin Jul 18, 2024
4cf05aa
Remove tests related with the request header nativeAuthRequestProvide…
Yuki-YuXin Jul 18, 2024
a9f32fe
Put dev version header into this branch
Yuki-YuXin Jul 22, 2024
77d3dd3
Only keep the minor changes
Yuki-YuXin Jul 22, 2024
acc8421
Merge branch 'dev' into robert/empty-correlation-id-fixes
Yuki-YuXin Jul 22, 2024
b4a923f
Revert "Remove tests related with the request header nativeAuthReques…
Yuki-YuXin Jul 24, 2024
c30e22e
"UNSET" -> UUID.randomUUID().toString() in getThreadCorrelationId()
Yuki-YuXin Jul 24, 2024
8181c6f
Revert "Modify the remaining test"
Yuki-YuXin Jul 24, 2024
3c74bb2
Revert "Modify the test"
Yuki-YuXin Jul 24, 2024
f0c1be5
Revert "Modify the remaining test"
Yuki-YuXin Jul 24, 2024
af209cc
Merge branch 'dev' into robert/empty-correlation-id-fixes
Yuki-YuXin Jul 29, 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
1 change: 1 addition & 0 deletions changelog.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ Version 17.6.1
- [PATCH] Return API error and errorDescription in case of unexpected response (#2431)
- [MINOR] Support certificate with password (#2405)
- [MINOR] Classifying TimeoutException as timed_out (#2441)
- [PATCH] Add check for unset correlation ID when sending Native Auth requests (#2435)

Version 17.5.0
---------
Expand Down
6 changes: 4 additions & 2 deletions common4j/src/main/com/microsoft/identity/common/java/logging/DiagnosticContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
// THE SOFTWARE.
package com.microsoft.identity.common.java.logging;

import java.util.UUID;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

public enum DiagnosticContext {
Expand Down Expand Up @@ -76,8 +78,8 @@ public IRequestContext getRequestContext() {
public String getThreadCorrelationId() {
IRequestContext context = getRequestContext();
String correlationId = context.get(DiagnosticContext.CORRELATION_ID);
if (correlationId == null) {
correlationId = UNSET;
if (correlationId == null || correlationId.equals("UNSET")) {
correlationId = UUID.randomUUID().toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also use correlationId = ""; and let CommandDispatcher.initializeDiagnosticContext
final String correlationId = StringUtil.isNullOrEmpty(requestCorrelationId) ? UUID.randomUUID().toString() : requestCorrelationId;

Copy link
Contributor

Choose a reason for hiding this comment

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

@Yuki-YuXin if what you mention is also a solution, then why did you add this code change here? to rephrase: I agree that what CommandDispatcher.initializeDiagnosticContext does should be sufficient. So why is your code change needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the current code snippet will set the correlationId to UNSET. If the correlationId passed into CommandDispatcher.initializeDiagnosticContext is UNSET, then after correlationId = StringUtil.isNullOrEmpty(requestCorrelationId) ? UUID.randomUUID().toString() : requestCorrelationId, the correlationId would keep as "UNSET"

Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right in saying:
it's okay to create a UUID here, because:

  1. we don't need to use/set the thread ID, because that's not what's being used in the request headers (i.e. when setting the request headers, we use map key DiagnosticContext.CORRELATION_ID, not DiagnosticContext.THREAD_ID
  2. CommandDispatcher.initializeDiagnosticContext() does the same; generate a UUID.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's correct.

}
return correlationId;
}
Expand Down
7 changes: 5 additions & 2 deletions ...main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ package com.microsoft.identity.common.java.nativeauth.providers

import com.microsoft.identity.common.java.AuthenticationConstants
import com.microsoft.identity.common.java.eststelemetry.EstsTelemetry
import com.microsoft.identity.common.java.logging.LibraryInfoHelper
import com.microsoft.identity.common.java.nativeauth.commands.parameters.ResetPasswordStartCommandParameters
import com.microsoft.identity.common.java.nativeauth.commands.parameters.ResetPasswordSubmitCodeCommandParameters
import com.microsoft.identity.common.java.nativeauth.commands.parameters.ResetPasswordSubmitNewPasswordCommandParameters
Expand All @@ -34,7 +35,6 @@ import com.microsoft.identity.common.java.nativeauth.commands.parameters.SignUpS
import com.microsoft.identity.common.java.nativeauth.commands.parameters.SignUpSubmitCodeCommandParameters
import com.microsoft.identity.common.java.nativeauth.commands.parameters.SignUpSubmitPasswordCommandParameters
import com.microsoft.identity.common.java.nativeauth.commands.parameters.SignUpSubmitUserAttributesCommandParameters
import com.microsoft.identity.common.java.logging.LibraryInfoHelper
import com.microsoft.identity.common.java.nativeauth.commands.parameters.SignInStartCommandParameters
import com.microsoft.identity.common.java.net.HttpConstants
import com.microsoft.identity.common.java.nativeauth.providers.requests.resetpassword.ResetPasswordChallengeRequest
Expand All @@ -49,6 +49,7 @@ import com.microsoft.identity.common.java.nativeauth.providers.requests.signup.S
import com.microsoft.identity.common.java.nativeauth.providers.requests.signup.SignUpContinueRequest
import com.microsoft.identity.common.java.nativeauth.providers.requests.signup.SignUpStartRequest
import com.microsoft.identity.common.java.platform.Device
import com.microsoft.identity.common.java.util.StringUtil
import java.util.TreeMap

/**
Expand Down Expand Up @@ -309,7 +310,9 @@ class NativeAuthRequestProvider(private val config: NativeAuthOAuth2Configuratio
//region helpers
private fun getRequestHeaders(correlationId: String): Map<String, String?> {
val headers: MutableMap<String, String?> = TreeMap()
headers[AuthenticationConstants.AAD.CLIENT_REQUEST_ID] = correlationId
if (correlationId != "UNSET") {
Copy link
Contributor

Choose a reason for hiding this comment

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

The string 'UNSET' should be used from a constant file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having a look at the rest of the codebase and "UNSET" seems to be used in place instead of being used as a constant value. I'd rather stick with the convention here, given that it's a straightforward name and something that doesn't have a specific meaning or is used outside of the context of the SDK.

headers[AuthenticationConstants.AAD.CLIENT_REQUEST_ID] = correlationId
}
headers[AuthenticationConstants.SdkPlatformFields.PRODUCT] = LibraryInfoHelper.getLibraryName()
headers[AuthenticationConstants.SdkPlatformFields.VERSION] = LibraryInfoHelper.getLibraryVersion()
headers.putAll(Device.getPlatformIdParameters())
Expand Down
Loading
Loading