Update Xpc silent controler error handling as fallback #1516

kaisong1990 · 2025-04-29T23:41:10Z

Proposed changes

In this PR, a new protocol is added to the base MSIDRequestControlling.h. This new protocol is used to decide whether the current fallback request controller should continue acquiring token based on the passed in error from the main controller.

Updated XPC silent controller when work as a SsoExt fallback controller, only below 4 error code will trigger a fallback:
case ASAuthorizationErrorNotHandled:
case ASAuthorizationErrorUnknown:
case ASAuthorizationErrorFailed:
case MSIDErrorSSOExtensionUnexpectedError:
Update the original error handling logic that either returns the main token request controller error or the valid token result from the fallback controller. This also aligns with OA

Type of change

Risk

High – Errors could cause MAJOR regression of many scenarios. (Example: new large features or high level infrastructure changes)
Medium – Errors could cause regression of 1 or more scenarios. (Example: somewhat complex bug fixes, small new features)
Small – No issues are expected. (Example: Very small bug fixes, string changes, or configuration settings changes)

Additional information

azure-pipelines · 2025-04-29T23:42:10Z

IdentityCore/src/controllers/MSIDLocalInteractiveController.m

        self.currentRequest = nil;

        completionBlock(result, error);
    }];


This pull request does not update changelog.txt.

Please consider if this change would be noticeable to a partner or user and either update changelog.txt or resolve this conversation.

antrix1989 · 2025-05-07T23:38:32Z

IdentityCore/src/controllers/MSIDSilentController.m

            else
            {
-                completionBlock(fallResult, fallError);
+                completionBlock(result, error);


why do we call fallback if we already have result?

In this case, the result is nil, the valid part is error only. I can send nil specifically

antrix1989 · 2025-05-07T23:45:12Z

IdentityCore/src/controllers/MSIDRequestControlling.h

 @protocol MSIDRequestControlling <NSObject>

 - (void)acquireToken:(nonnull MSIDRequestCompletionBlock)completionBlock;
+- (void)shouldSkipAcquireTokenBasedOn:(nonnull NSError *)error;


can we add this flag directly to "acquireToken" method?

Nit: Can we rename it to "skipAcquire..." instead of "should..."?"should" sounds like a dynamic condition to me, while here we know for sure that we want to skip.

+1, methods with 'should' prefix usually indicate return type as BOOL but this does not seem to be the case here.

ameyapat · 2025-05-08T21:31:10Z

IdentityCore/src/controllers/broker/mac/MSIDXpcSilentTokenRequestController.m


+- (void)shouldSkipAcquireTokenBasedOn:(NSError *)error
+{
+    self.shouldSkipAcquireToken = YES;


What if error is nil? This method will log incorrect info which might confuse in case of issue debugging

ameyapat · 2025-05-08T21:31:50Z

IdentityCore/src/controllers/broker/mac/MSIDXpcSilentTokenRequestController.m

+    if (![error.domain isEqualToString:ASAuthorizationErrorDomain] && ![error.domain isEqualToString:MSIDErrorDomain]) {
+    }


What is this supposed to do? Did you miss something?

ameyapat · 2025-05-08T21:34:09Z

IdentityCore/src/controllers/broker/mac/MSIDXpcSilentTokenRequestController.m

+        case ASAuthorizationErrorUnknown:
+        case ASAuthorizationErrorFailed:
+        case MSIDErrorSSOExtensionUnexpectedError:
+            self.shouldSkipAcquireToken = NO;


Can the controller be reused for multiple requests? If it is, controller's shouldSkipAcquireToken might get set to NO in 1st request and remain NO for second request

ameyapat · 2025-05-08T21:37:11Z

IdentityCore/src/controllers/broker/mac/MSIDXpcSilentTokenRequestController.m

+// shouldSkipXpcRequest is used when the fallback is not needed for non ASAuthorizationErrorDomain or MSIDErrorSSOExtensionUnexpectedError
+@property (nonatomic) BOOL shouldSkipAcquireToken;


The comment says shouldSkipXpcRequest but property is named shouldSkipAcquireToken. Can you rename property to shouldSkipCallingXpc? shouldSkipAcquireToken is confusing

Since this boolean would be initialized to NO by default, I would suggest naming property to 'shouldUseXpcToAcquireToken' instead of 'shouldSkip..'

ameyapat · 2025-05-08T21:48:42Z

IdentityCore/src/controllers/broker/mac/MSIDXpcSilentTokenRequestController.m

+    }
+    else
+    {
+        // self.fallbackController cannot be nil here as it has been validated from caller


Is this comment inaccurate? If we know fallbackController cannot be nil here, why do a nil check and log in the code? Please remove the comment as it isn't accurate according to the code following it

Copilot

Pull Request Overview

This PR updates the error handling flow for the XPC silent controller to enable a fallback mechanism when the primary token acquisition fails with certain error codes. Key changes include:

Adding a new protocol method for error-based fallback control.
Modifying the XPC silent token request controller to conditionally delegate to a fallback controller.
Updating integration tests to reflect the adjusted error handling logic.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
IdentityCore/tests/integration/MSIDSilentControllerIntegrationTests.m	Updated expected error comparisons and added new tests for XPC and SSO fallback scenarios.
IdentityCore/src/controllers/broker/mac/MSIDXpcSilentTokenRequestController.m	Modified logging and control flow in acquireToken, and introduced logic to determine when to skip the XPC request.
IdentityCore/src/controllers/broker/ios/MSIDBrokerInteractiveController.m	Added an unused implementation for shouldSkipAcquireTokenBasedOn:.
IdentityCore/src/controllers/MSIDSilentController.m	Updated fallback logic to determine which error or result to return, and added a call to the fallback controller’s skip check.
IdentityCore/src/controllers/MSIDRequestControlling.h	Added the new protocol method shouldSkipAcquireTokenBasedOn:.
IdentityCore/src/controllers/MSIDLocalInteractiveController.m	Implemented an empty shouldSkipAcquireTokenBasedOn: as it is not used in this controller.

Copilot · 2025-05-19T20:02:19Z

IdentityCore/src/controllers/broker/mac/MSIDXpcSilentTokenRequestController.m

+    self.shouldSkipAcquireToken = YES;
+    MSID_LOG_WITH_CTX(MSIDLogLevelInfo, self.requestParameters, @"Looking if we should fallback to Xpc controller, error: %ld error domain: %@.", (long)error.code, error.domain);
+    // If it is MSIDErrorDomain and Sso Extension returns unexpected error, we should fall back to local controler and unblock user
+    if (![error.domain isEqualToString:ASAuthorizationErrorDomain] && ![error.domain isEqualToString:MSIDErrorDomain]) {


The empty if-block checking the error domain may be unclear to future maintainers. Consider adding a comment or explicitly setting 'shouldSkipAcquireToken' to clarify the intended behavior when the error domain is not ASAuthorizationErrorDomain or MSIDErrorDomain.

Suggested change

if (![error.domain isEqualToString:ASAuthorizationErrorDomain] && ![error.domain isEqualToString:MSIDErrorDomain]) {

if (![error.domain isEqualToString:ASAuthorizationErrorDomain] && ![error.domain isEqualToString:MSIDErrorDomain]) {

// No action is required for error domains other than ASAuthorizationErrorDomain or MSIDErrorDomain.

// This block is intentionally left empty.

Copilot · 2025-05-19T20:02:19Z

IdentityCore/src/controllers/MSIDSilentController.m

-            if (!fallResult && (fallError.code == MSIDErrorSSOExtensionUnexpectedError))
+            // Only return fallback when there is a valid result, otherwise, return error from main controller
+            // (!result && !error) is a special case when using local silent controller and to skip local refreh token (broker first flow). In this case, the main controller is the 1st fallback controller, and should return the error
+            if (fallResult || (!result && !error))


[nitpick] The compound condition determining when to use the fallback result versus the main controller's error could benefit from additional inline documentation. A brief explanation of why the condition prioritizes a valid fallback result or the absence of both result and error would improve maintainability.

kai added 2 commits April 29, 2025 12:20

Add shouldFallBack to requestController protocol

4e1f4bc

Add error handling for XPC fallback logic

dd9e701

kaisong1990 marked this pull request as ready for review April 29, 2025 23:41

kaisong1990 requested a review from a team as a code owner April 29, 2025 23:41

kaisong1990 requested review from ameyapat and antrix1989 April 29, 2025 23:41

kaisong1990 added the XPC label Apr 29, 2025

azure-pipelines bot reviewed Apr 29, 2025

View reviewed changes

Merge branch 'kasong/3236668-xpc-error-handling' into dev

a6f8ac9

antrix1989 reviewed May 7, 2025

View reviewed changes

ameyapat reviewed May 8, 2025

View reviewed changes

kaisong1990 requested a review from Copilot May 19, 2025 20:01

Copilot AI reviewed May 19, 2025

View reviewed changes

		if (![error.domain isEqualToString:ASAuthorizationErrorDomain] && ![error.domain isEqualToString:MSIDErrorDomain]) {
		}

		// shouldSkipXpcRequest is used when the fallback is not needed for non ASAuthorizationErrorDomain or MSIDErrorSSOExtensionUnexpectedError
		@property (nonatomic) BOOL shouldSkipAcquireToken;

Update Xpc silent controler error handling as fallback #1516

Are you sure you want to change the base?

Update Xpc silent controler error handling as fallback #1516

Uh oh!

Conversation

kaisong1990 commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Type of change

Risk

Additional information

Uh oh!

azure-pipelines bot Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kaisong1990 commented Apr 29, 2025 •

edited

Loading