Skip to content

Commit

Permalink
Add check for unset correlation ID when configuring request headers (#…
Browse files Browse the repository at this point in the history
…2435)

If a correlation ID hasn't been configured for a request, the 'UNSET'
value will be sent to the server, and the server will then return that
'UNSET' value from the response. To prevent this, the correlation ID
should only be appended if it is a valid value.

Additionally, tests have been added to ensure that an 'UNSET' value
isn't added to the request headers, and valid correlation IDs are being
added to the request headers on a valid request.

Also, there are a few tweaks made to `NativeAuthRequestHandlerTest` -
there are some tests where a request is configured and created, but
there are no assertions made on those requests. I've added a few checks
to tests that were missing validations, and some other small tweaks.

Update: 
“If a correlation ID hasn't been configured for a request, the 'UNSET'
value will be sent to the server, and the server will then return that
'UNSET' value from the response.” -> After experiment, this assumption
is wrong. The header used on the SDK side is client-request-id not
ms-client-request-id, these two values are different if they both
attached in the response header. The interesting thing is that, if
client sends invalid correlation id like "UNSET", the server would not
return the client-request-id to the client. On the contrary, if the
correlation id is valid and client sent it in the client-request-id
header, the server will return it as the same value in the
client-request-id header. SDK made the assumption that if no
client-request-id header is sent to the server from client, the server
will generate one for the client seems untrue. The correlation id is
dependant on client-request-id x-ms-request-id is the request specific,
it targets the Service Request Id not the CorrelationId, while
x-ms-request-id is returned from the server in all times.

---------

Co-authored-by: yuxin <[email protected]>
  • Loading branch information
rmccahill and Yuki-YuXin authored Jul 29, 2024
1 parent 44d9252 commit c831849
Show file tree
Hide file tree
Showing 4 changed files with 265 additions and 20 deletions.
1 change: 1 addition & 0 deletions changelog.txt
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
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();
}
return correlationId;
}
Expand Down
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") {
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

0 comments on commit c831849

Please sign in to comment.