-
Notifications
You must be signed in to change notification settings - Fork 2.7k
loadExternalTokens bug fixes #8242
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
Conversation
|
@copilot Please generate changefiles for this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes critical bugs in token caching and authority handling, particularly addressing environment mismatches when using authority aliases (e.g., login.microsoftonline.com vs login.windows.net). The changes ensure tokens are always cached under the correct preferred cache environment and properly handle refresh token expiration.
Changes:
- Authority resolution now uses
AuthorityFactory.createDiscoveredInstanceto determine the preferred cache environment before caching tokens - Refresh token expiration is now calculated from
refresh_token_expires_inand stored with the credential, with telemetry support via newextRtExpiresOnSecondsfield - AccountEntity conversion ensures home tenant profiles are always present to prevent missing tenant profile issues
- Account lookup in ResponseHandler uses
getAllAccountsto find accounts across authority aliases
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/msal-browser/src/cache/TokenCache.ts |
Uses AuthorityFactory.createDiscoveredInstance for authority resolution and preferred cache environment; calculates refresh token expiration from response |
lib/msal-browser/src/cache/BrowserCacheManager.ts |
Removes optional chaining on result.account properties (now guaranteed non-null) |
lib/msal-common/src/response/ResponseHandler.ts |
Changes account lookup from getAccount to getAllAccounts to find accounts across authority aliases |
lib/msal-common/src/cache/entities/AccountEntity.ts |
Ensures home tenant profile exists in both getAccountInfo and createFromAccountInfo methods |
lib/msal-common/src/telemetry/performance/PerformanceEvent.ts |
Adds extRtExpiresOnSeconds telemetry field |
lib/msal-browser/test/cache/TokenCache.spec.ts |
Adds tests for refresh token expiration, preferred cache environment, and removes obsolete test |
lib/msal-common/test/cache/entities/AccountEntity.spec.ts |
Updates test to verify home tenant profile creation |
lib/msal-common/test/response/ResponseHandler.spec.ts |
Adds tests for account lookup with authority aliases |
lib/msal-browser/test/interaction_client/SilentRefreshClient.spec.ts |
Adds mock for getAllAccounts |
lib/msal-browser/test/custom_auth/test_resources/TestConstants.ts |
Fixes test account IDs to match CLIENT_INFO |
lib/msal-common/apiReview/msal-common.api.md |
Updates API surface with new telemetry field and TSDoc line numbers |
Comments suppressed due to low confidence (1)
lib/msal-common/test/response/ResponseHandler.spec.ts:1
- The test manually creates a tenant profile array but doesn't verify that
AccountEntity.getAccountInfocorrectly creates this profile when it's missing. Consider adding a test case where the account entity has no tenant profiles to ensure the new logic ingetAccountInfois tested.
import { ServerAuthorizationTokenResponse } from "../../src/response/ServerAuthorizationTokenResponse.js";
Co-authored-by: Copilot <[email protected]>
Co-authored-by: tnorling <[email protected]>
- [x] Generate beachball changefiles for @azure/msal-browser and @azure/msal-common - [x] Set change type to "patch" for both packages (bug fixes) - [x] Update change messages to include PR #8242 - [x] Validate changefiles with beachball:check - [x] Convert HTML anchor tags to markdown-style links <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: tnorling <[email protected]> Co-authored-by: Thomas Norling <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
This pull request introduces several important improvements and bug fixes to token caching and authority handling in the browser library. The main changes include more robust handling of authority discovery and preferred cache environments, correct calculation and caching of refresh token expiration, and enhanced test coverage for these scenarios. Additionally, the AccountEntity conversion now ensures tenant profiles are always present, and telemetry fields have been updated to capture new metrics.
Authority and Token Caching Improvements:
TokenCachenow usesAuthorityFactory.createDiscoveredInstanceto resolve the authority and its preferred cache environment, ensuring tokens are always cached under the correct environment. This fixes issues with mismatched environments when using authorities likelogin.microsoftonline.com, which should uselogin.windows.netfor caching. (lib/msal-browser/src/cache/TokenCache.ts, [1] [2]lib/msal-browser/src/cache/BrowserCacheManager.ts, [1] [2]Refresh Token Expiration Handling:
expiresOn) based onrefresh_token_expires_infrom the token response. This value is now passed to the credential and recorded in telemetry. (lib/msal-browser/src/cache/TokenCache.ts, [1] [2];lib/msal-common/apiReview/msal-common.api.md, [3]Account Entity Conversion Robustness:
AccountEntity.getAccountInfomethod now ensures that at least the home tenant profile is present when converting toAccountInfo, preventing missing tenant profile issues. (lib/msal-common/src/cache/entities/AccountEntity.ts, [1] [2]Test Coverage and Reliability Enhancements:
lib/msal-browser/test/cache/TokenCache.spec.ts, [1] [2] [3];lib/msal-browser/test/custom_auth/test_resources/TestConstants.ts, [4]lib/msal-browser/test/interaction_client/SilentRefreshClient.spec.ts, lib/msal-browser/test/interaction_client/SilentRefreshClient.spec.tsR235-R238)Telemetry and Miscellaneous:
extRtExpiresOnSecondsto record external refresh token expiration in performance events. (lib/msal-common/apiReview/msal-common.api.md, lib/msal-common/apiReview/msal-common.api.mdR3517)lib/msal-common/apiReview/msal-common.api.md, lib/msal-common/apiReview/msal-common.api.mdL4740-R4743)These changes collectively improve the reliability, correctness, and observability of token caching and authority resolution in the browser library.