AzureAD / microsoft-authentication-library-for-java

Microsoft Authentication Library (MSAL) for Java http://aka.ms/aadv2
MIT License
285 stars 142 forks source link

MSI CAE Claims handling - claims merging issue #757

Closed gladjohn closed 1 month ago

gladjohn commented 10 months ago

Library version used

not applicable

Java version

latest

Scenario

Other - please specify

Is this a new or an existing app?

None

Issue description and reproduction steps

We discovered an bug in MSAL .NET on how we merge the claims and capabilities json in CAE scenarios

Incoming claims :

{
  "access_token": {
    "nbf": {
      "essential": true,
      "value": "1701477303"
    }
  }
}

And the merged claims and capab should be like this,

{
  "access_token": {
    "xms_cc": {
      "values": ["cp1", "cp2"]
    },
    "nbf": {
      "essential": true,
      "value": "1701477303"
    }
  }
}

In MSAL .NET, we build claims and capab with access_token and xms_cc, but with the new incoming claim, we fail to do a proper merge, instead just return the incoming claim without the capab.

All MSAL's need to check if this is being properly handled. This issue started happening in MSAL .NET when we moved to start using system.text.json and wrote our own merge logic.

Please refer to MSAL .NET PR for the fix

Proposed unit test

Have 3 unit test as follows:

  1. Create a confidential client application and set client capabilities "cp1" and "cp2"
  2. Acquire a token for a { client_credentails, obo, auth_code } (hence 3 tests)

Assert that the request has the "claims" set to

 "access_token": {
    "xms_cc": {
      "values": ["cp1", "cp2"]
    }
  1. Repeat step 2 to get a token from the cache (for web site, use AcquireTokenClient, becuase AcquireTokenByAuthCode does not pull elements from the cache)

Assert: token comes from cache

  1. Now simulate CAE by requesting a token with claims:
    {
    "access_token": {
    "nbf": {
      "essential": true,
      "value": "1701477303"
    }
    }
    }

Assert:

{
  "access_token": {
    "xms_cc": {
      "values": ["cp1", "cp2"]
    },
    "nbf": {
      "essential": true,
      "value": "1701477303"
    }
  }
}
  1. Repeat step 4
bgavrilMS commented 10 months ago

@Avery-Dunn - this isn't a bug per se, it's more of a question. Can you just get a unit test out to prove that claims + CP are handled correctly for the test case above? The fact that the claims mention access_token is important.

Avery-Dunn commented 1 month ago

Adjusted unit tests in https://github.com/AzureAD/microsoft-authentication-library-for-java/pull/811 to confirm this behavior