Update InitiateLongRunningObo method to retrieve tokens from the cache if request assertion matches.

[pmaytak]

Fixes #4100

Changes proposed in this request
Updates InitiateLongRunningObo method to return valid cached tokens if cached assertion matches the request assertion; otherwise, the assertion is used to retrieve new tokens.
Updates access token cache item .

Testing
Updated tests.

Performance impact
None.

Documentation

• All relevant documentation is updated. - After release.

[bgavrilMS]

Fixes #4100

Changes proposed in this request Updates InitiateLongRunningObo method to return valid cached tokens if cached assertion matches the request assertion; otherwise, the assertion is used to retrieve new tokens. Updates access token cache item .

Testing Updated tests.

Performance impact

Documentation

• All relevant documentation is updated.

Pls ensure that our OBO spec / api review is updated.

[trwalke]

The logic here makes sense overall, just one question about the test.

[pmaytak]

Currently cached/serialized token items (aka legacy tokens) don't have this new assertion hash property. So there's a scenario if app is updated to this new MSAL version that checks the hash now.

An option here is to add additional check - && !string.IsNullOrEmpty(cachedAccessToken.OboAssertionHash).

1. As-is, without the above check, InitiateObo will never match these cached legacy tokens, and will always use the passed-in assertion to get tokens from AAD, even if the cached tokens are unexpired. This actually mimics current behavior (4.51.0+).
2. With the additional check, InitiateObo will return the valid matched legacy access tokens and, if expired, will use legacy refresh token to refresh. This mimics the 4.50.0- behavior but only for legacy tokens. But once the legacy RT is used to refresh, then new tokens will be saved with the assertion hash.

In the scenario when AAD revokes AT and RT, method 2 reintroduces the previous issue of returning revoked tokens, so devs would have to use the StopLongObo method.

AcquireLongObo is not affected since it doesn't have the assertion in the request.

A more general question, do we even want to have an assertion hash match for refresh tokens?

Thoughts @bgavrilMS, @trwalke? Maybe I missed something?

[trwalke]

do we even want to have an assertion hash match for refresh tokens?

The reason we discussed adding this was to prevent the scenario where someone is using initiate long running OBO over and over again from breaking after 4.51.0+. The initial implementation for this PR lead us to the realization that in order to actually do that while maintaining the logic that fixes the revocation issue is to add another check to the refresh OBO logic to check for a matching token hash. So, if we want to prevent breaking customers, we should keep the hash match for refresh tokens.

As-is, without the above check, InitiateObo will never match these cached legacy tokens, and will always use the passed-in assertion to get tokens from AAD, even if the cached tokens are unexpired. This actually mimics current behavior (4.51.0+).

I think this will only mimic the behavior of 4.51.0+ for the first iteration. So, once the user is required to sign in again, the legacy tokens should be overwritten and assertion will be cached with the new tokens. I think this is fine. The other option (#2) will require the developer to add the StopLongRunningApi method to remove the revoked tokens. That is a much more difficult state to recover from than simply asking the user to sign in again.

So, I dont think we need to add the null check. The end user experience is affected, but all they need to do is sign in again to recover and then everything will work fine.

Adding this null check will put the user in a state they cant recover from if the token are revoked without the app developer changing the app or deleting tokens manually.

Lets sync on this tomorrow @pmaytak incase I am missing something.

[pmaytak]

Per discussion, will not implement this at this time.