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.

Sign up for GitHub

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

Jump to bottom

Fix: Cached AccessToken missing realm property in OIDC scenario #6689

Merged
merged 7 commits into from
Jan 29, 2024
Merged

Fix: Cached AccessToken missing realm property in OIDC scenario #6689

merged 7 commits into from
Jan 29, 2024

Conversation

bbush915
Copy link
Contributor

In the case an application is using a separate OIDC-compliant authority and the tid claim is empty, the cached access token does not contain a realm property. One side-effect is that the cached access token will fail the isAccessTokenEntity check, preventing it from being re-used.

@github-actions github-actions bot added the msal-common Related to msal-common package label Nov 11, 2023
tnorling
tnorling requested changes Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Can you please add a test & generate changefiles by running npm run beachball:change from the root of the repo.

@bbush915
Copy link
Contributor Author

@microsoft-github-policy-service agree [company="Dialexa, an IBM Company"]

@bbush915
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Dialexa, an IBM Company"

@bbush915
Copy link
Contributor Author

@tnorling addressed comment and added a test. The beachball command reported no change files are needed.

@bbush915 bbush915 requested a review from tnorling November 14, 2023 23:23
@tnorling
Copy link
Collaborator

@tnorling addressed comment and added a test. The beachball command reported no change files are needed.

Ah I suspect this is because your feature branch is also called dev. Can you try another branch?

@bbush915
Copy link
Contributor Author

@tnorling addressed comment and added a test. The beachball command reported no change files are needed.

Ah I suspect this is because your feature branch is also called dev. Can you try another branch?

Good call! Sorry for the messy commits, but I got the change files in place!

@bbush915
Copy link
Contributor Author

@tnorling Hello! Is there anything I can do to help this land?

@sameerag
Copy link
Member

@bbush915 I can help land this. Looks like some tests are failing.

@bbush915
Copy link
Contributor Author

bbush915 commented Jan 2, 2024

@sameerag I validated that tests are all passing locally. Is there anything I can do to troubleshoot the failing E2E tests? It looks like they are all reporting: Git fetch failed with exit code: 128

@bbush915
Copy link
Contributor Author

bbush915 commented Jan 8, 2024

@tnorling @sameerag following up on the above, I'm not sure why the E2E tests are failing, but can do what is needed to assist. Hoping to get this to land soon, since it is blocking some upgrades.

@bbush915
fix: Ensure access token hasOwnProperty("realm") when using a separat…
95efe39 
…e OIDC-compliant authority.

In the case an application is using a separate OIDC-compliant authority and the tid claim is empty, the cached access token does not contain a realm property. One side-effect is that the cached access token will fail the isAccessTokenEntity check, preventing it from being re-used.
@bbush915
feat: Update constant usage and add test for OIDC scenario.
eb02515
@bbush915
Change files
4523002
@bbush915
fix: Use valid client_info.
83d1b03
@bbush915
fix: Use claimsTenantId
599f219
@BboyAkers
Copy link

BboyAkers commented Jan 11, 2024 •
edited
Loading

Hey cool and awesome former colleagues 🙂! Can this get approved and merged? It's blocking a couple companies and their clients including one I'm helping with 😅🙏🏾. So sorry, for the push and hope I don't come off as rude!

@sameerag
Copy link
Member

Hey cool and awesome former colleagues 🙂! Can this get approved and merged? It's blocking a couple companies and their clients including one I'm helping with 😅🙏🏾. So sorry, for the push and hope I don't come off as rude!

Approved and updated the base branch. Hopefully tests pass now.

@BboyAkers
Copy link

BboyAkers commented Jan 29, 2024 •
edited
Loading

Hey cool and awesome former colleagues 🙂! Can this get approved and merged? It's blocking a couple companies and their clients including one I'm helping with 😅🙏🏾. So sorry, for the push and hope I don't come off as rude!

Approved and updated the base branch. Hopefully tests pass now.

😭 🙏🏾 ❤️
image

@tnorling tnorling merged commit e491e8f into AzureAD:dev Jan 29, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnorling tnorling tnorling approved these changes

@sameerag sameerag sameerag approved these changes

@hectormmg hectormmg Awaiting requested review from hectormmg hectormmg is a code owner

@jo-arroyo jo-arroyo Awaiting requested review from jo-arroyo jo-arroyo is a code owner

@peterzenz peterzenz Awaiting requested review from peterzenz peterzenz is a code owner

@Robbie-Microsoft Robbie-Microsoft Awaiting requested review from Robbie-Microsoft Robbie-Microsoft is a code owner

@konstantin-msft konstantin-msft Awaiting requested review from konstantin-msft konstantin-msft is a code owner

Assignees

@tnorling tnorling

@lalimasharda lalimasharda

Labels
msal-common Related to msal-common package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bbush915 @tnorling @sameerag @BboyAkers @lalimasharda