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

Extend msal-browser TokenCache loadExternalTokens to load refresh tokens #5233

Merged

Conversation

lvalentine
Copy link
Contributor

@lvalentine lvalentine commented Sep 21, 2022

This change extends the TokenCache.loadExternalTokens method to support loading refresh tokens. It also makes a few small changes in TokenCache to make it easier to use this functionality from an application:

  • TokenCache.loadExternalTokens now returns a AuthenticationResult for the response that was loaded. This allows the calling code to easily get access to the newly loaded account without having to know anything about how msal creates the account id from the Request/Response that was loaded (ie. from IPublicClientApplication.getAccountByHomeId).
  • Changes the way in which TokenCache.loadIdToken generates the homeAccountId from the request to match the same method used in NativeInteractionClient, using AccountEntity.generateHomeAccountId. This change was made so that the tokens that are initially side loaded via the TokenCache.loadExternalTokens method can be found in the cache and refreshed when acquiring access tokens via a supported path like IPublicClientApplication.acquireTokenSilent.
  • Updates to TokenCache tests to account for and cover these changes.

@ghost ghost assigned jasonnutter and hectormmg Sep 21, 2022
@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package labels Sep 21, 2022
@lvalentine lvalentine force-pushed the user/louisv/external_load_refresh_tokens branch from 06e27c7 to 879f28f Compare September 21, 2022 18:32
@jasonnutter
Copy link
Contributor

@lvalentine Please run npm run beachball:change from the root of the repo, complete the prompts, and push up the generated change files, thanks! (this can be marked as a minor bump for msal-browser)

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2022

Codecov Report

Merging #5233 (3993ca8) into dev (b354804) will decrease coverage by 0.01%.
The diff coverage is 87.17%.

Flag Coverage Δ *Carryforward flag
msal-angular 96.50% <ø> (ø)
msal-browser 86.89% <86.84%> (-0.06%) ⬇️
msal-common 85.49% <100.00%> (+<0.01%) ⬆️
msal-core 82.84% <ø> (ø) Carriedforward from fb22ccb
msal-node 83.09% <ø> (ø)
msal-node-extensions 76.03% <ø> (ø)
msal-react 94.50% <ø> (ø)
node-token-validation 88.88% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
lib/msal-browser/src/cache/TokenCache.ts 87.34% <86.84%> (-7.31%) ⬇️
lib/msal-common/src/index.ts 100.00% <100.00%> (ø)
lib/msal-browser/src/encode/Base64Decode.ts 94.73% <0.00%> (+5.26%) ⬆️

lvalentine and others added 2 commits September 26, 2022 11:36
…1.json

Co-authored-by: Jason Nutter <jasonnutter@outlook.com>
….json

Co-authored-by: Jason Nutter <jasonnutter@outlook.com>
@jasonnutter
Copy link
Contributor

One more comment on style, otherwise looks great, thanks for addressing feedback!

lib/msal-browser/src/cache/TokenCache.ts Outdated Show resolved Hide resolved
lib/msal-browser/src/cache/TokenCache.ts Outdated Show resolved Hide resolved
@lvalentine lvalentine requested review from hectormmg and jasonnutter and removed request for jo-arroyo and hectormmg September 28, 2022 00:35
Copy link
Member

@hectormmg hectormmg left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a final optional nit. Thanks for the contribution!

Copy link
Contributor

@jasonnutter jasonnutter 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 addressing feedback!

Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

Approving

@jasonnutter jasonnutter merged commit b929529 into AzureAD:dev Oct 10, 2022
@ghost
Copy link

ghost commented Oct 10, 2022

🎉@azure/msal-common@v7.6.0 has been released which incorporates this pull request.:tada:

We recommend upgrading to the latest version of @azure/msal-browser or @azure/msal-node to take advantage of this change.

Handy links:

@ghost
Copy link

ghost commented Oct 10, 2022

🎉@azure/msal-browser@v2.30.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package msal-common Related to msal-common package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants