[minor][engg]: Adding matching error code in MSAL for MSIDErrorServerInvalidRequestResetPasswordRequired #2867
Conversation
![azure-pipelines[bot]](https://avatars.githubusercontent.com/in/9426?s=60&v=4){kind=small}
| @@ -139,7 +139,8 @@ + (void)initialize | |||
| @(MSIDErrorServerInvalidState) : @(MSALInternalErrorInvalidState), | |||
There was a problem hiding this comment.
This pull request does not update CHANGELOG.md.
Please consider if this change would be noticeable to a partner or user and either update CHANGELOG.md or resolve this conversation.
There was a problem hiding this comment.
Pull request overview
Adds an MSAL-facing error code intended to mirror/represent MSIDErrorServerInvalidRequestResetPasswordRequired, and wires it through the MSID→MSAL error conversion layer so apps can detect this condition.
Changes:
- Introduces
MSALErrorServerInvalidRequestResetPasswordRequiredwith value-50142in the public error header. - Adds an error-code mapping in
MSALErrorConverterfromMSIDErrorServerInvalidRequestResetPasswordRequiredto the new MSAL error code.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
MSAL/src/public/MSALError.h |
Adds a new error constant intended for consumption by MSAL callers. |
MSAL/src/MSALErrorConverter.m |
Maps the MSID error code to the new MSAL error code during conversion/classification. |
| Error is thrown when PSSO user registration attempted with no biometrics configured and sekey biometric policy is configured | ||
| */ | ||
| MSALErrorPSSOBiometricsNotEnrolled = -42744, | ||
|
|
There was a problem hiding this comment.
There’s an extra blank line (and potentially trailing whitespace) added just before the new enum value. Please remove any trailing whitespace and keep spacing consistent with the surrounding enum declarations.
| @(MSIDErrorServerUnhandledResponse) : @(MSALInternalErrorUnhandledResponse), | ||
| @(MSIDErrorUnexpectedHttpResponse) : @(MSALInternalErrorUnexpectedHttpResponse) | ||
| @(MSIDErrorUnexpectedHttpResponse) : @(MSALInternalErrorUnexpectedHttpResponse), | ||
| @(MSIDErrorServerInvalidRequestResetPasswordRequired) : @(MSALErrorServerInvalidRequestResetPasswordRequired) | ||
| } |
There was a problem hiding this comment.
As written, this new mapping will typically not be observable by callers: msalErrorFromMsidError: uses classifyErrors:YES, and errorWithDomain:... reclassifies any mapped code not in s_recoverableErrorCode to MSALErrorInternal (storing the original mapped code under MSALInternalErrorCodeKey). If the intent is to expose this as a new handleable MSALError code, add it to s_recoverableErrorCode so it isn’t collapsed to MSALErrorInternal.
| @(MSIDErrorUnexpectedHttpResponse) : @(MSALInternalErrorUnexpectedHttpResponse), | ||
| @(MSIDErrorServerInvalidRequestResetPasswordRequired) : @(MSALErrorServerInvalidRequestResetPasswordRequired) |
There was a problem hiding this comment.
MSIDErrorServerInvalidRequestResetPasswordRequired is referenced here, but it doesn’t appear to be defined anywhere in this repo snapshot (searching under the repo only finds this usage). If this constant was added in IdentityCore, this PR likely also needs to update the IdentityCore submodule pointer (or otherwise include the header/version that defines it) to avoid a compile failure.
| @(MSIDErrorUnexpectedHttpResponse) : @(MSALInternalErrorUnexpectedHttpResponse), | ||
| @(MSIDErrorServerInvalidRequestResetPasswordRequired) : @(MSALErrorServerInvalidRequestResetPasswordRequired) | ||
| } |
There was a problem hiding this comment.
Please add a unit test to cover this new error mapping, asserting that an MSIDErrorDomain error with code MSIDErrorServerInvalidRequestResetPasswordRequired converts to MSALErrorServerInvalidRequestResetPasswordRequired (and stays that code when classifyErrors:YES, if that’s the intended behavior). The existing MSALErrorConverterTests suite has coverage for other mapped server errors but nothing that would fail if this specific mapping gets reclassified to MSALErrorInternal.
| MSALErrorPSSOBiometricsNotEnrolled = -42744, | ||
|
|
||
| MSALErrorServerInvalidRequestResetPasswordRequired = -50142, | ||
| }; |
There was a problem hiding this comment.
MSALErrorServerInvalidRequestResetPasswordRequired is being added inside the MSALInternalError enum, but its name/value pattern (-50xxx) matches the public MSALError enum (e.g., MSALErrorServerDeclinedScopes, MSALErrorUserCanceled). This makes it easy for SDK consumers to miss it (it won’t appear as part of MSALError) and muddles the public vs internal error contract. Consider moving this constant (with a brief doc comment) into the MSALError enum section near the other server errors, and keep MSALInternalError limited to internal/debug-only codes.
1 participant
PR Title Format
Required Format:
[Keyword1] [Keyword2]: Descriptionmajor,minor, orpatch(case-insensitive)feature,bugfix,engg, ortests(case-insensitive)Examples:
[MAJOR] [Feature]: new API[minor] [bugfix]: fix crash[PATCH] [tests]: add coverageProposed changes
Describe what this PR is trying to do.
Type of change
Risk
Additional information