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

rmccahill · 2024-06-20T14:29:33Z

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.

…te tests

SaurabhMSFT · 2024-06-21T11:48:19Z

...rc/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt

@@ -310,7 +311,11 @@ 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") {


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

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.

SaurabhMSFT · 2024-06-21T11:49:06Z

...test/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestHandlerTest.kt

@@ -148,6 +151,22 @@ class NativeAuthRequestHandlerTest {
        )
    }

+    @Test
+    fun testSignUpStartWithUnsetCorrelationIdShouldHaveNilHeader() {


testSignUpStartWithUnsetCorrelationIdShouldHaveNilHeader -> 'testSignUpStartWithUnsetCorrelationIdShouldNotHaveHeader'

The title is accurate as it is - the test passes a "UNSET" value, and in this case the header should be nil.

@rmccahill can you elaborate? Why should we be sending an empty header (i.e. the REQUEST_ID header is set, but it doesn't contain a value), rather than not set the header at all?

Actually, what you're saying seems to contradict line 316 of NativeAuthRequestProvider.kt. Or maybe we're mis-communicating :D

The way I see it there's a difference between "no header" vs. "header is set and value is null". Saurabh and I mean the first (and so I would agree that the test name should be "should not have header").

Ah I see what you mean - in my mind 'nil header' refers to the header not being present. But I can see how that's confusing. I've updated the naming on these tests to reflect the suggested name.

...rc/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt

SammyO · 2024-06-24T12:24:07Z

@rmccahill could you remove the conditional setting of the correlation ID in AccountState and see if the multiple tokens tests still pass?

Context:

this issue came up when I was writing the multiple token tests, and added that statement was a quick fix to make the tests pass
why? in the context of tests the threading naming mechanism in DiagnosticContext doesn't work correctly, and so the correlation ID for API calls was set to UNSET by DiagnosticContext. The API returned this, and in subsequent refresh token logic an UUID was attempted to be composed based on the UNSET String. This caused an exception.
with the changes of this PR, this crash shouldn't be happening anymore.

# Conflicts: # changelog.txt

Yuki-YuXin · 2024-07-03T09:50:03Z

The removal and fix of the conditional setting of the correlation ID in AccountState is here AzureAD/microsoft-authentication-library-for-android#2135

# Conflicts: # changelog.txt

# Conflicts: # common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt

…nse(requestCorrelationId = requestCorrelationId),the SDK use the local correlation id rather than the header from the http response. Thus,the filer in the getHeader won't work for this issue.

Yuki-YuXin · 2024-07-18T10:22:05Z

common4j/src/main/com/microsoft/identity/common/java/logging/DiagnosticContext.java

-        if (correlationId == null) {
-            correlationId = UNSET;
+        if (correlationId == null || correlationId.equals(UNSET)) {
+            correlationId = UUID.randomUUID().toString();


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

@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?

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"

Yuki-YuXin · 2024-07-18T10:33:43Z

...rc/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt

@@ -310,8 +311,8 @@ class NativeAuthRequestProvider(private val config: NativeAuthOAuth2Configuratio
    private fun getRequestHeaders(correlationId: String): Map<String, String?> {
        val headers: MutableMap<String, String?> = TreeMap()
        headers[AuthenticationConstants.AAD.CLIENT_REQUEST_ID] = correlationId


Revert it back because we are using the local parameters.getCorrelationId() in the http/SDK response rather than the response header CLIENT_REQUEST_ID, which means even though we filter out the UNSET in the header, it will still exist in the response.

I'm not following this. This method isn't used in responses; it's used to create requests.

Yes, the reason we don't want UNSET to be sent in the http request header is because we don't want the server to response "UNSET" to us in the http response header. However, the SDK didn't use the http response header to construct the SDK response. It used the local correlation passed in or recorded in DiagnosticContext. Since we don't use the correlation id in the http response header then nothing sent in the request will affect the correlation ID of the SDK response

…r.createSignUpChallengeRequest(correlationId = "UNSET")

SammyO · 2024-07-19T08:55:18Z

common4j/src/main/com/microsoft/identity/common/java/logging/DiagnosticContext.java

-        if (correlationId == null) {
-            correlationId = UNSET;
+        if (correlationId == null || correlationId.equals(UNSET)) {
+            correlationId = UUID.randomUUID().toString();


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

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

CommandDispatcher.initializeDiagnosticContext() does the same; generate a UUID.

Yes, that's correct.

SammyO · 2024-07-19T08:57:45Z

...rc/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt

@@ -310,8 +311,8 @@ class NativeAuthRequestProvider(private val config: NativeAuthOAuth2Configuratio
    private fun getRequestHeaders(correlationId: String): Map<String, String?> {
        val headers: MutableMap<String, String?> = TreeMap()
        headers[AuthenticationConstants.AAD.CLIENT_REQUEST_ID] = correlationId
-        headers[AuthenticationConstants.SdkPlatformFields.PRODUCT] = LibraryInfoHelper.getLibraryName()
-        headers[AuthenticationConstants.SdkPlatformFields.VERSION] = LibraryInfoHelper.getLibraryVersion()
+        headers[AuthenticationConstants.SdkPlatformFields.PRODUCT] = DiagnosticContext.INSTANCE.requestContext[AuthenticationConstants.SdkPlatformFields.PRODUCT]


I think this code change is wrong. And the one on the line below.

I have no idea why the changes here. If that's wrong, I will revert it back.

SammyO · 2024-07-19T09:03:27Z

...main/com/microsoft/identity/common/java/nativeauth/providers/interactors/SignInInteractor.kt

@@ -103,7 +103,7 @@ class SignInInteractor(
        )

        val rawApiResponse = nativeAuthResponseHandler.getSignInInitiateResultFromHttpResponse(
-            requestCorrelationId = requestCorrelationId,
+            requestCorrelationId = requestCorrelationId, // Question: should we use the CLIENT_REQUEST_ID header from response here?


I think you're misunderstanding what's happening here (as did I when we talked about it yesterday).

when the request is created (nativeAuthRequestProvider.createSignInInitiateRequest) getRequestHeaders() is used to compose the correlation ID, which in turn takes the correlation ID from the command parameters.

when the response object is created (by converting the raw API response into a local data model), we pass the original correlation ID to the request provider. We do this because the API might not return a correlation ID, in which case we set it manually using requestCorrelationId.

Oh, my bad.
After checking inside getSignInInitiateResultFromHttpResponse

// If the API doesn't return a correlation ID header value, use the correlation ID of the original API request val correlationId: String = response.getHeaderValue(AuthenticationConstants.AAD.CLIENT_REQUEST_ID, 0).let {responseCorrelationId ->

Then we definitely should have the try catch mechanism in the header.

This may explain why SDK will get UNSET even when the header filter is applied,

And the client request id only exists when client request id is valid. The client request id is different from x-ms-request-id

SammyO · 2024-07-19T09:26:29Z

...rc/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt

@@ -310,8 +311,8 @@ class NativeAuthRequestProvider(private val config: NativeAuthOAuth2Configuratio
    private fun getRequestHeaders(correlationId: String): Map<String, String?> {
        val headers: MutableMap<String, String?> = TreeMap()
        headers[AuthenticationConstants.AAD.CLIENT_REQUEST_ID] = correlationId


Going back to the original goal of this ticket: I still think we need a catch-all fallback here that catches any "UNSET" values and ensures we don't send it to the API (because if we do, the API will return "UNSET" back to us, which we don't want). There are 2 ways I can think of:

catch "UNSET" and replace with UUID

catch "UNSET" and not set correlation ID at all. This is what MSAL does. The expectation is that the API should then create a correlation ID and send it back to us.

Currently, I keep the changes to apply the second option: "catch "UNSET" and not set correlation ID at all". The expectation could not be achieved for the reason in the PR summary. I don't think we should make changes to the NativeAuthResponseHandler part about the header in this PR. If the pipeline passes, maybe we can have a discussion later on what would be best.

...test/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestHandlerTest.kt

Yuki-YuXin · 2024-07-19T15:36:47Z

@SammyO After talking to IOS, the UNSET correlation id seems to be only an issue on the Android side.

"In brief what native auth iOS SDK operates around the correlationID:

every initial actions on the public SDK interface (like signIN, getAccessToken, signUp, resetPassword) accept an optional correlationId
this correlation has UUID as type, so this means that when the user specifies a correlation Id, it has a valid format
when the user doesn't not specify a correlationID, the SDK create a new one internally (just a random UUID). This is the same behaviour of MSAL Objective-C
this means that native auth iOS SDK always send a valid UUID string as correlation ID to eSTS."

SammyO · 2024-07-22T08:39:29Z

@SammyO After talking to IOS, the UNSET correlation id seems to be only an issue on the Android side.

"In brief what native auth iOS SDK operates around the correlationID:

every initial actions on the public SDK interface (like signIN, getAccessToken, signUp, resetPassword) accept an optional correlationId

this correlation has UUID as type, so this means that when the user specifies a correlation Id, it has a valid format
when the user doesn't not specify a correlationID, the SDK create a new one internally (just a random UUID). This is the same behaviour of MSAL Objective-C

this means that native auth iOS SDK always send a valid UUID string as correlation ID to eSTS."

@Yuki-YuXin I don't think that's exactly the question we want to ask the iOS team. iOS' intended behaviour that you describe is also the intended behaviour on Android. However, we need to account for situations where that intended design has a bug. The question is: if the SDK (iOS or Android) doesn't send a correlation ID to the API, does the API generate one and send it back through headers? Our assumption is yes, but our findings from Friday state the opposite.

…tProvider.createSignUpChallengeRequest(correlationId = "UNSET")" This reverts commit 4cf05aa.

This reverts commit d0a2618.

This reverts commit 2685650.

This reverts commit abc1279.

Add check for empty correlation ID when setting request headers, upda…

a28a3f7

…te tests

rmccahill requested a review from a team as a code owner June 20, 2024 14:29

Update changelog

046593a

rmccahill requested a review from a team as a code owner June 20, 2024 14:33

SaurabhMSFT reviewed Jun 21, 2024

View reviewed changes

Yuki-YuXin reviewed Jun 21, 2024

View reviewed changes

...rc/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt Outdated Show resolved Hide resolved

Update empty header test naming

9fe242c

Yuki-YuXin approved these changes Jun 27, 2024

View reviewed changes

rmccahill and others added 4 commits June 28, 2024 12:28

Merge branch 'dev' into robert/empty-correlation-id-fixes

428f2ab

# Conflicts: # changelog.txt

Update changelog

f221e24

Merge branch 'dev' into robert/empty-correlation-id-fixes

73f34ff

Add argument to pass MSAL consumer test

2e760ac

Merge branch 'dev' into robert/empty-correlation-id-fixes

c1d5a3f

# Conflicts: # changelog.txt

Yuki-YuXin mentioned this pull request Jul 3, 2024

Remove the unnecessary code line because of fix in common AzureAD/microsoft-authentication-library-for-android#2135

Open

Yuki-YuXin added the Skip-Consumers-Check Only include this if making a breaking change purposefully, and there is an MSAL/ADAL/Broker PR label Jul 3, 2024

Yuki-YuXin added 9 commits July 16, 2024 09:53

Merge branch 'dev' into robert/empty-correlation-id-fixes

b653ea6

# Conflicts: # common4j/src/main/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestProvider.kt

Revert changes to trigger error log

e631037

getThreadCorrelationId

fed453c

getThreadCorrelationId UNSET

c731847

Inside nativeAuthResponseHandler.getSignInInitiateResultFromHttpRespo…

3391582

…nse(requestCorrelationId = requestCorrelationId),the SDK use the local correlation id rather than the header from the http response. Thus,the filer in the getHeader won't work for this issue.

Merge branch 'dev' into robert/empty-correlation-id-fixes

5976371

Modify the test

2685650

Modify the remaining test

d0a2618

Modify the remaining test

abc1279

Yuki-YuXin reviewed Jul 18, 2024

View reviewed changes

Remove tests related with the request header nativeAuthRequestProvide…

4cf05aa

…r.createSignUpChallengeRequest(correlationId = "UNSET")

SammyO reviewed Jul 19, 2024

View reviewed changes

Yuki-YuXin reviewed Jul 19, 2024

View reviewed changes

...test/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestHandlerTest.kt Show resolved Hide resolved

...test/com/microsoft/identity/common/java/nativeauth/providers/NativeAuthRequestHandlerTest.kt Show resolved Hide resolved

Yuki-YuXin added 8 commits July 22, 2024 21:57

Put dev version header into this branch

a9f32fe

Only keep the minor changes

77d3dd3

Merge branch 'dev' into robert/empty-correlation-id-fixes

acc8421

Revert "Remove tests related with the request header nativeAuthReques…

b4a923f

…tProvider.createSignUpChallengeRequest(correlationId = "UNSET")" This reverts commit 4cf05aa.

"UNSET" -> UUID.randomUUID().toString() in getThreadCorrelationId()

c30e22e

Revert "Modify the remaining test"

8181c6f

This reverts commit d0a2618.

Revert "Modify the test"

3c74bb2

This reverts commit 2685650.

Revert "Modify the remaining test"

f0c1be5

This reverts commit abc1279.

SammyO approved these changes Jul 29, 2024

View reviewed changes

Merge branch 'dev' into robert/empty-correlation-id-fixes

af209cc

Yuki-YuXin merged commit c831849 into dev Jul 29, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rmccahill commented Jun 20, 2024 •

edited by Yuki-YuXin

Loading

SaurabhMSFT Jun 21, 2024

rmccahill Jun 24, 2024

SaurabhMSFT Jun 21, 2024

rmccahill Jun 24, 2024

SammyO Jun 24, 2024

SammyO Jun 24, 2024

rmccahill Jun 24, 2024

SammyO commented Jun 24, 2024

Yuki-YuXin commented Jul 3, 2024

Yuki-YuXin Jul 18, 2024

SammyO Jul 19, 2024

Yuki-YuXin Jul 19, 2024

Yuki-YuXin Jul 18, 2024

SammyO Jul 19, 2024

Yuki-YuXin Jul 19, 2024

SammyO Jul 19, 2024

Yuki-YuXin Jul 19, 2024

SammyO Jul 19, 2024

Yuki-YuXin Jul 19, 2024

SammyO Jul 19, 2024

Yuki-YuXin Jul 19, 2024

Yuki-YuXin Jul 19, 2024

Yuki-YuXin Jul 19, 2024

SammyO Jul 19, 2024 •

edited

Loading

Yuki-YuXin Jul 24, 2024

Yuki-YuXin commented Jul 19, 2024 •

edited

Loading

SammyO commented Jul 22, 2024

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

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

Conversation

rmccahill commented Jun 20, 2024 • edited by Yuki-YuXin Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SammyO commented Jun 24, 2024

Yuki-YuXin commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SammyO Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuki-YuXin commented Jul 19, 2024 • edited Loading

SammyO commented Jul 22, 2024

rmccahill commented Jun 20, 2024 •

edited by Yuki-YuXin

Loading

SammyO Jul 19, 2024 •

edited

Loading

Yuki-YuXin commented Jul 19, 2024 •

edited

Loading