For Mtls, throw exception when service config region is set to null.

[ksaaf]

This PR fixes throwing exception when service config region is set to null for Mtls.

Fixes #5181

Changes proposed in this request

• Replaced string.IsNullOrEmpty with null in AcquireTokenForClientParameterBuilder.cs for Azure region check when the authority is AAD.

Testing

• Updates existing unit tests to cover another case for this case when service config region is set to null and fixes existing tests.

Performance impact

N/A

Documentation

• All relevant documentation is updated.

[gladjohn]

not sure if this is the correct fix. The issue is not in MTLS code path but in the the Azure Region API code path. After your fix trying running the test MsalForceRegionIsSet_WithRegionIsSet_WithRegionWins with " "

[gladjohn]

may not fix all region cases

[ksaaf]

not sure if this is the correct fix. The issue is not in MTLS code path in the the Azure Region API code path. After your fix trying running the test MsalForceRegionIsSet_WithRegionIsSet_WithRegionWins with " "

@gladjohn - I ran the tests as original and with your suggestion under debugger putting a breakpoint on the line I fixed. It didn't even hit the breakpoint.

Do you have a different test in mind? Can you run tests against this PR branch and check? Thanks.

[ksaaf]

may not fix all region cases

I am not sure if we I am following you. The fix is simple and adds test cases for string.empty scenario as well as for the new whitespace. And the test passes in all three cases. What am I missing here?

Also, do you have a change in mind as you requested a change here?

[gladjohn]

I ran the tests as original and with your suggestion under debugger putting a breakpoint on the line I fixed. It didn't even hit the breakpoint.

And what is the outcome of the test?

[gladjohn]

do you have a change in mind as you requested a change here?

The correct fix should happen here

[bgavrilMS]

I believe the test MtlsPopWithoutRegionAsync is misleading. It should not set an env variable. Regional opt-in is done via API.

[ksaaf]

MsalForceRegionIsSet_WithRegionIsSet_WithRegionWins

It passed.

[ksaaf]

I believe the test MtlsPopWithoutRegionAsync is misleading. It should not set an env variable. Regional opt-in is done via API.

Correct, see my response here as well.

[ksaaf]

do you have a change in mind as you requested a change here?

The correct fix should happen here

This is a different issue. Please see my response here.

[gladjohn]

do you have a change in mind as you requested a change here?

The correct fix should happen here

This is a different issue. Please see my response here.

@ksaaf the actual issue is not in the AcquireTokenForClient API or flows, The issue you are describing stems from WithAzureRegion API. I am happy to discuss this with you and go over the actual problem.

As @bgavrilMS, mentioned there could be other bugs that are present in the Region flow that has been exposed by this test. So this needs a little more investigation.

[ksaaf]

do you have a change in mind as you requested a change here?

The correct fix should happen here

This is a different issue. Please see my response here.

@ksaaf the actual issue is not in the AcquireTokenForClient API or flows, The issue you are describing stems from WithAzureRegion API. I am happy to discuss this with you and go over the actual problem.

As @bgavrilMS, mentioned there could be other bugs that are present in the Region flow that has been exposed by this test. So this needs a little more investigation.

Yes, let's discuss; happy to learn the context as I am new to the codebase.

[gladjohn]

looks good.

two approvals granted