Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Claims don't bypass the cache for OBO and client_credentials #6173

Closed
bgavrilMS opened this issue Jun 29, 2023 · 8 comments · Fixed by #6999
Closed

Claims don't bypass the cache for OBO and client_credentials #6173

bgavrilMS opened this issue Jun 29, 2023 · 8 comments · Fixed by #6999
Assignees
Labels
bug A problem that needs to be fixed for the feature to function as intended. confidential-client Issues regarding ConfidentialClientApplications p1 P1 and P2 are priorities of the bug. P1 bugs should get Fixed/Closed within 4 weeks.

Comments

@bgavrilMS
Copy link
Member

Core Library

MSAL Node (@azure/msal-node)

Core Library Version

1.17.3

Wrapper Library

Not Applicable

Wrapper Library Version

n/a

Public or Confidential Client?

Confidential

Description

  1. Acquire a token from the STS using client_creds or OBO
  2. Make the same request again. Expect the result to come from the cache.
  3. Make a similar request, where you add some claims e.g. claims="foo"

Expected: request should go to the STS
Actual: token is served from the cache

Impact: this breaks the CAE scenario for web api and the upcoming CAE support for S2S

Error Message

see description

Msal Logs

see description

MSAL Configuration

see description

Relevant Code Snippets

see description

Reproduction Steps

see description

Expected Behavior

see description

Identity Provider

Azure AD / MSA

Browsers Affected (Select all that apply)

None (Server)

Regression

No response

Source

Internal (Microsoft)

@bgavrilMS bgavrilMS added question Customer is asking for a clarification, use case or information. bug-unconfirmed A reported bug that needs to be investigated and confirmed labels Jun 29, 2023
@ghost ghost added the Needs: Attention 👋 Awaiting response from the MSAL.js team label Jun 29, 2023
@bgavrilMS bgavrilMS added bug A problem that needs to be fixed for the feature to function as intended. and removed bug-unconfirmed A reported bug that needs to be investigated and confirmed Needs: Attention 👋 Awaiting response from the MSAL.js team labels Jun 29, 2023
@github-actions github-actions bot added confidential-client Issues regarding ConfidentialClientApplications msal-node Related to msal-node package labels Jun 29, 2023
@ghost ghost removed the question Customer is asking for a clarification, use case or information. label Jun 29, 2023
@ghost ghost assigned jo-arroyo Jun 29, 2023
@bgavrilMS bgavrilMS added p2 P1 and P2 are priorities of the bug. P2 bugs should get fixed/closed within 3 months. and removed msal-node Related to msal-node package labels Jun 29, 2023
@bgavrilMS bgavrilMS assigned hectormmg and bgavrilMS and unassigned jo-arroyo Jun 29, 2023
@bgavrilMS
Copy link
Member Author

Assigning to you @hectormmg as discussed over Teams. Thanks for observing this behavior !

@bgavrilMS
Copy link
Member Author

I am not sure this is correct, I think @Robbie-Microsoft you tried it out and the cache is bypassed?

@Robbie-Microsoft
Copy link
Collaborator

I think I tried it out, but I can't remember. I noticed this is from last June and is for msal-node v1. Should I test this in v2? Or only v1?

@bgavrilMS
Copy link
Member Author

Only the current version of MSAL (2.x)

@Robbie-Microsoft
Copy link
Collaborator

I just tried it out for client credentials with

const request: msal.ClientCredentialRequest = { scopes: ["https://graph.microsoft.com/.default"], claims: "foo", };

for the third request, and the token is indeed returned from the cache. Claims DO NOT bypass the cache for client credentials.

@bgavrilMS
Copy link
Member Author

I'm surprised. You closed this #6769 which is a superset. Step 4 there states "token does not come from the cache". What changed?

In any case, this is a P1 that needs to be fixed then.

@bgavrilMS bgavrilMS added p1 P1 and P2 are priorities of the bug. P1 bugs should get Fixed/Closed within 4 weeks. and removed p2 P1 and P2 are priorities of the bug. P2 bugs should get fixed/closed within 3 months. labels Mar 29, 2024
@Robbie-Microsoft
Copy link
Collaborator

I'm not sure what happened, but it looks like that PR is missing steps 4 & 5 for client_credentails, obo, and auth_code. Investigating now.

@Robbie-Microsoft
Copy link
Collaborator

I'm surprised. You closed this #6769 which is a superset. Step 4 there states "token does not come from the cache". What changed?

In any case, this is a P1 that needs to be fixed then.

@bgavrilMS I see my error now. I misinterpreted your description of the unit tests that needed to be written. I have now modified the unit test to properly test that requests with claims skip the cache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A problem that needs to be fixed for the feature to function as intended. confidential-client Issues regarding ConfidentialClientApplications p1 P1 and P2 are priorities of the bug. P1 bugs should get Fixed/Closed within 4 weeks.
Projects
None yet
4 participants