-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Authority mismatch instrumentation #8212
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
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 enhances authority mismatch handling by improving environment value validation and adding telemetry instrumentation for diagnostic purposes. The changes ensure that both empty strings and undefined values are treated as invalid environments, and provide better visibility into authority mismatch scenarios through performance telemetry.
Key changes:
- Updated validation in
AccountEntityandResponseHandlerto explicitly reject empty string environment values - Added telemetry fields (
discoveredAuthorityandaccountEnvironment) to track authority mismatch details - Implemented comprehensive test coverage for environment validation error scenarios
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/msal-common/test/cache/entities/AccountEntity.spec.ts |
Adds comprehensive test suite for error scenarios when environment values are empty, undefined, or null |
lib/msal-common/src/telemetry/performance/PerformanceEvent.ts |
Adds optional telemetry fields for discovered authority and account environment values |
lib/msal-common/src/response/ResponseHandler.ts |
Updates validation to explicitly check for empty string environment values |
lib/msal-common/src/cache/entities/AccountEntity.ts |
Updates validation to explicitly check for empty string environment values |
lib/msal-browser/src/interaction_client/BaseInteractionClient.ts |
Adds telemetry instrumentation to log normalized authority values when mismatch occurs |
| this.performanceClient.addFields( | ||
| { | ||
| discoveredAuthority: normalizeValue(discoveredAuthority.canonicalAuthority), | ||
| accountEnvironment: normalizeValue(account.environment), | ||
| }, | ||
| this.correlationId | ||
| ); |
Copilot
AI
Dec 28, 2025
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.
The new telemetry instrumentation in BaseInteractionClient should have test coverage. The existing test file BaseInteractionClient.spec.ts has a test for the authority mismatch scenario (line 243), but it doesn't verify that the telemetry fields are correctly added when the mismatch occurs.
Consider adding assertions to verify that performanceClient.addFields is called with the correct discoveredAuthority and accountEnvironment values, including verification of the normalization behavior for empty strings and undefined values.
lib/msal-browser/src/interaction_client/BaseInteractionClient.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
change/@azure-msal-common-adda0fb4-dea9-4116-b965-68b15040c909.json
Outdated
Show resolved
Hide resolved
change/@azure-msal-browser-8d6fcd39-1f3b-4b4d-81e6-b729b83cdd7e.json
Outdated
Show resolved
Hide resolved
….json Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…e.json Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
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
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
| it("throws invalidCacheEnvironment when authority.getPreferredCache() returns empty string and no environment provided", () => { | ||
| jest.spyOn(Authority.prototype, "getPreferredCache").mockReturnValue(""); | ||
|
|
||
| const homeAccountId = AccountEntity.generateHomeAccountId( | ||
| TEST_DATA_CLIENT_INFO.TEST_RAW_CLIENT_INFO_GUIDS, | ||
| AuthorityType.Default, | ||
| logger, | ||
| cryptoInterface, | ||
| idTokenClaims | ||
| ); | ||
|
|
||
| expect(() => { | ||
| AccountEntity.createAccount( | ||
| { | ||
| homeAccountId, | ||
| idTokenClaims: idTokenClaims, | ||
| }, | ||
| authority | ||
| ); | ||
| }).toThrow("invalid_cache_environment"); | ||
| }); |
Copilot
AI
Dec 30, 2025
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.
Consider adding a test case for when authority.getPreferredCache() returns a whitespace-only string (e.g., " ") to match the validation logic in AccountEntity.ts which checks env.trim() === "". This would ensure comprehensive coverage of the new validation behavior.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request enhances authority/environment validation and telemetry in the MSAL libraries, improving error handling and diagnostics around authority mismatches and cache environment issues. It introduces stricter checks for empty or undefined environment values, ensures telemetry is recorded with normalized values, and adds comprehensive test coverage for these scenarios.
Authority/environment validation and error handling:
AccountEntityandResponseHandlerto throw aninvalid_cache_environmenterror if the environment is empty or consists only of whitespace, not just when undefined. This prevents invalid or ambiguous environment values from being used. [1] [2]AccountEntity.spec.tsto cover error scenarios when the environment is empty, undefined, or null, and to verify correct behavior when a valid environment is provided.Telemetry and instrumentation improvements:
BaseInteractionClientto record telemetry fields fordiscoveredAuthorityandaccountEnvironment, normalizing undefined or empty values for better diagnostics.PerformanceEventtype to include the new telemetry fieldsdiscoveredAuthorityandaccountEnvironment.BaseInteractionClient.spec.tsto verify that telemetry is correctly recorded with normalized values for various account environment edge cases (undefined, empty string, whitespace).Changelog updates:
@azure/msal-commonand@azure/msal-browserdocumenting the authority mismatch instrumentation work. [1] [2]Test maintenance:
AccountEntity.spec.tsby restoring all mocks after each test, preventing test interference.