8000 Move auth error messages out of the bundle by konstantin-msft · Pull Request #7744 · AzureAD/microsoft-authentication-library-for-js · GitHub
[go: up one dir, main page]
Skip to content

Move auth error messages out of the bundle #7744

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.

S 10000 ign 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
Merged
merged 8 commits into from
May 21, 2025
Merged

Move auth error messages out of the bundle #7744

merged 8 commits into from
May 21, 2025

Conversation

konstantin-msft
Copy link
Collaborator
  • Move auth error messages out of the bundle

@github-actions github-actions bot added documentation Related to documentation. msal-node Related to msal-node package msal-browser Related to msal-browser package msal-common Related to msal-common package labels May 12, 2025
@konstantin-msft konstantin-msft marked this pull request as ready for review May 13, 2025 18:29
tnorling
tnorling previously approved these changes May 14, 2025
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.

I'd also like to see some sort of CI validation that a message actually exists in the doc for all error codes. I'm happy to treat that as its own separate work item, however, just calling it out as a requirement to call this work complete.

@konstantin-msft konstantin-msft force-pushed the move_error_message_to_markdown branch from 1e6185a to 1df8a91 Compare May 20, 2025 21:18
@konstantin-msft
Copy link
Collaborator Author

I'd also like to see some sort of CI validation that a message actually exists in the doc for all error codes. I'm happy to treat that as its own separate work item, however, just calling it out as a requirement to call this work complete.

Will address this in a follow-up PR
-->
tnorling
tnorling reviewed May 20, 2025
View reviewed changes
tnorling
tnorling previously approved these changes May 20, 2025
View reviewed changes
@konstantin-msft konstantin-msft enabled auto-merge (rebase) May 20, 2025 22:03
@konstantin-msft konstantin-msft merged commit 71336db into msal-v5 May 21, 2025
8 checks passed
@konstantin-msft konstantin-msft deleted the move_error_message_to_markdown branch May 21, 2025 18:13
jo-arroyo
jo-arroyo reviewed May 21, 2025
View reviewed changes
Copy link
Collaborator
@jo-arroyo jo-arroyo left a comment

Choose a reason for hiding this comment

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

Hi, I know this is merged already, but I noticed some potentially broken links that may need to be addressed in a separate PR, and there are some other suggestions that you can choose to take or not.

docs/errors.md

**[Other](#other)**
This error occurs when MSAL.js surpasses the allotted storage limit when attempting to save token information in the [configured cache storage](./caching.md#cache-storage). See [here](https://developer.mozilla.org/en-US/docs/Web/API/Storage_API/Storage_quotas_and_eviction_criteria#web_storage) for web storage limits.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The link to the caching doc may need to be changed now that this doc has been moved out of msal-browser

docs/errors.md

**Mitigation**:

1. Make sure the configured cache storage has enough capacity to allow MSAL.js to persist token payload. The amount of cache storage required depends on the number of [cached artifacts](./caching.md#cached-artifacts).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here for caching doc

docs/errors.md
**Mitigation**:

1. Make sure the configured cache storage has enough capacity to allow MSAL.js to persist token payload. The amount of cache storage required depends on the number of [cached artifacts](./caching.md#cached-artifacts).
2. Disable [claimsBasedCachingEnabled](./configuration.md#cache-config-options) cache config option. When enabled, it caches access tokens under a key containing the hash of the requested claims. Depending on the MSAL.js API usage, it may result in the vast number of access tokens persisted in the cache storage.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here for configuration doc

docs/errors.md
- Nonce mismatch error.

### `auth_time_not_found`
- Max Age was requested and the ID token is missing the auth_time variable auth_time is an optional claim and is not enabled by default - it must be enabled. See https://aka.ms/msaljs/optional-claims for more information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Max Age was requested and the ID token is missing the auth_time variable auth_time is an optional claim and is not enabled by default - it must be enabled. See https://aka.ms/msaljs/optional-claims for more information.
- Max Age was requested and the ID token is missing the auth_time variable. auth_time is an optional claim and is not enabled by default - it must be enabled. See https://aka.ms/msaljs/optional-claims for more information.

docs/errors.md
- The provided authority does not support logout.

### `key_id_missing`
- A keyId value is missing from the requested bound token's cache record and is required to match the token to it's stored binding key.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- A keyId value is missing from the requested bound token's cache record and is required to match the token to it's stored binding key.
- A keyId value is missing from the requested bound token's cache record and is required to match the token to its stored binding key.

lib/msal-browser/test/interaction_client/PopupClient.spec.ts
@@ -66,7 +65,7 @@ import * as BrowserUtils from "../../src/utils/BrowserUtils.js";
import { FetchClient } from "../../src/network/FetchClient.js";
import { TestTimeUtils } from "msal-test-utils";
import { PopupRequest } from "../../src/request/PopupRequest.js";
import { emptyNavigateUri } from "../../src/error/BrowserAuthErrorCodes.js";
import { getDefaultErrorMessage } from "../../src/error/BrowserAuthError.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be combined with the import on line 59 above?

lib/msal-browser/test/interaction_client/RedirectClient.spec.ts
@@ -89,6 +88,7 @@ import {
TestTimeUtils,
} from "msal-test-utils";
import { BrowserPerformanceClient } from "../../src/telemetry/BrowserPerformanceClient.js";
import { getDefaultErrorMessage } from "../../src/error/BrowserAuthError.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be combined with the import on line 69 above?

lib/msal-browser/test/interaction_client/SilentIframeClient.spec.ts
import { SsoSilentRequest } from "../../src/index.js";
import { getDefaultErrorMessage } from "../../src/error/BrowserAuthError.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with import here

lib/msal-common/test/error/ClientAuthError.spec.ts
createClientAuthError,
} from "../../src/error/ClientAuthError";
import { AuthError } from "../../src/error/AuthError";
import { getDefaultErrorMessage } from "../../src/error/AuthError.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Combine with line above?

lib/msal-common/test/error/JoseHeaderError.spec.ts
createJoseHeaderError,
} from "../../src/error/JoseHeaderError";
import { AuthError } from "../../src/error/AuthError";
import { getDefaultErrorMessage } from "../../src/error/AuthError.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Combine with above line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jo-arroyo jo-arroyo jo-arroyo left review comments

@tnorling tnorling tnorling approved these changes

@lalimasharda lalimasharda lalimasharda approved these changes

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

@hectormmg hectormmg Awaiting requested review from hectormmg hectormmg 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

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

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

Assignees
No one assigned
Labels
documentation Related to documentation. msal-browser Related to msal-browser package msal-common Related to msal-common package msal-node Related to msal-node package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@konstantin-msft @tnorling @sameerag @lalimasharda @jo-arroyo
0