[SigninSilent only] CommonCore interface for XPC silent request. #1482

kaisong1990 · 2025-02-22T00:09:49Z

Proposed changes

Added necessary files to start up and connect to the XPC service (these files will be reused for other XPC requests, e.g., interactive).
The entire XPC flow is guarded by an enum and is disabled by default. The XPC mode can be enabled through the Mac MSAL Test app.
Added end-to-end support for the Sign-in silent XPC flow. This requires an additional Broker XPC service to run for tests.
Refactored/extracted common code and added MSIDSSORemoteSilentTokenRequest, which can be used for both silent XPC and silent SsoExtension requests.
Refactored the logic in MSIDRequestControllerFactory to better handle the choice of which controller to use.

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-02-22T00:10:48Z

IdentityCore/IdentityCore.xcodeproj/project.pbxproj

@@ -499,6 +499,15 @@
 		23FB5C452255A11D002BF1EB /* MSIDClaimsRequest.m in Sources */ = {isa = PBXBuildFile; fileRef = 23FB5C24225517AA002BF1EB /* MSIDClaimsRequest.m */; };


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.

IdentityCore/src/controllers/MSIDRequestControllerFactory.m

…troller factory with XPC support

IdentityCore/src/MSIDConstants.h

ameyapat · 2025-03-12T23:28:20Z

IdentityCore/src/controllers/MSIDRequestControllerFactory.m

+#import "MSIDXpcSilentTokenRequestController.h"
+#endif
+
+static NSString *newXpcController = @"new_xpc_controller";


nit: Should this be static const NSString *newXpcController?

Good catch, this is not used , removed

IdentityCore/src/controllers/MSIDRequestControllerFactory.m

ameyapat · 2025-03-12T23:39:58Z

IdentityCore/src/controllers/MSIDRequestControllerFactory.m

+{
+    if (parameters.msidXpcMode != MSIDXpcModeDisable)
+    {
+        return [self v2SilentControllerForParameters:parameters


IMO, I would avoid using v1, v2 etc in the naming of things. As time goes by what the versions mean relies of anecdotal knowledge and gets lost. Also, this implies that there will be new versions in the future like v3, v4 etc.

I would suggest renaming v2SilentControllerForParameters: as xpcEnabledSilentControllerForParameters or add a new argument like isXpcEnabled to the original method.

Updated the method name. The ending goal is to remove the v1 and to use v2. For now I changed the name to reflect the purpose

ameyapat · 2025-03-12T23:45:09Z

IdentityCore/src/controllers/MSIDRequestControllerFactory.m

+                                                               error:(NSError *__autoreleasing*)error
+{
+    // Nested auth protocol - Reverse client id & redirect uri
+    if ([parameters isNestedAuthProtocol])


I think duplicate code from 'v1' method here can be refactored

I did not do the partial refactoring because I am planning to move to new v2 completely. I will do refactoring for duplicate code if the methods needs to be co-exist all the time.

ameyapat · 2025-03-12T23:51:25Z

IdentityCore/src/controllers/MSIDRequestControllerFactory.m

+                                                                                   tokenRequestProvider:tokenRequestProvider
+                                                                          fallbackInteractiveController:fallbackController
+                                                                                                  error:error];
+            if (parameters.msidXpcMode == MSIDXpcModeFull || parameters.msidXpcMode == MSIDXpcModeOverride)


What about parameters.msidXpcMode == MSIDXpcModeBackup? In such a case, fallbackInteractiveController will be fallbackController

Correct
in the condition parameters.xpcMode == MSIDXpcModeFull || parameters.xpcMode == MSIDXpcModeOverride
the xpcController is set to nil

So in MSIDXpcModeBackup condition, the xpcController is not set to nil, and will be used as a fallbackInteractiveController when creating MSIDSSOExtensionSilentTokenRequestController (L188)

Sorry the fallbackInteractiveController is really misleading as this is silent request. I will modify the name in a different PR as it is from old code and XPC unrelated.

ameyapat · 2025-03-13T00:11:30Z

IdentityCore/src/controllers/MSIDRequestControllerFactory.m

+                                                                               tokenRequestProvider:tokenRequestProvider
+                                                                      fallbackInteractiveController:fallbackController
+                                                                                              error:error];
+    if (!silentController) return nil;


Can we log error if silentController is nil?

I put the level as warning as this is unexpected

ameyapat · 2025-03-13T00:12:21Z

IdentityCore/src/controllers/MSIDRequestControllerFactory.m

+                                                                                                           error:error];
+        }
+    }
+


Can we log when fallbackController is nil?

ameyapat · 2025-03-13T00:24:26Z

IdentityCore/src/controllers/broker/MSIDXpcSilentTokenRequestController.m

+
+ (BOOL)canPerformRequest
+{
+    if (@available(macOS 13, *)) {


nit: Does return @available(macOS 13, *); work here?

Yes, I saw patterns and tested myself that when running on macOS 15, this doe return true as expected

ameyapat · 2025-03-13T00:25:30Z

IdentityCore/src/parameters/MSIDRequestParameters.h

@@ -108,6 +108,9 @@
 #pragma mark - SSO context
 @property (nonatomic) MSIDExternalSSOContext *ssoContext;

+#pragma mark - Xpc Mode
+@property (nonatomic) MSIDXpcMode msidXpcMode;


nit: I don't think msid prefix is required in the name of the property here

ameyapat · 2025-03-13T01:49:24Z

IdentityCore/src/requests/broker/MSIDSSOXpcSilentTokenRequest.m

+    NSBundle *mainBundle = [NSBundle mainBundle];
+
+    // Retrieve the bundle identifier
+    NSString *bundleIdentifier = [mainBundle bundleIdentifier];


Curious to know if there are 2 apps with the same bundle id on the device, which app will the xpc redirect to?

Good question.

The bundle_id passed here is only for XPC service to identify if the self-claimed app the same as the app bundle from signed cert.

The redirect/reply is not controlled by this, but through connection object. Thinking of ESTS, if there are 2 applications with same bundle calling into ests, ests knows how to reply each app as the apps have different connections/channels to ests

ameyapat · 2025-03-13T01:53:15Z

IdentityCore/tests/MSIDRequestControllerFactoryTests.m

+    parameters.allowUsingLocalCachedRtWhenSsoExtFailed = YES;
+
+    NSError *error;
+    SEL selectorForMSIDSSOExtensionSilentTokenRequestController = NSSelectorFromString(@"canPerformRequest");


I'm not sure if we had decided to not use swizzling in tests or not. I think the way was to create a mock test controller or override this method in test target

I believe the protocol/mockl should be the way for future. For this existing test class, I was following the swizzling pattern from upper tests.

IdentityCore/src/controllers/MSIDRequestControllerFactory.m

IdentityCore/src/requests/broker/MSIDSSOExtensionSilentTokenRequest.m

IdentityCore/src/requests/broker/MSIDSSOXpcSilentTokenRequest.m

IdentityCore/src/requests/sdk/adal/MSIDLegacyTokenRequestProvider.m

IdentityCore/src/util/MSIDXpcSingleSignOnProvider.m

Kai Song and others added 10 commits October 4, 2024 16:00

initial work to add silent xpc token request and controller

524bc55

Remove pre-check on canPerform

67398ab

Add requestType for telemetry usage in broker

f1f6c96

Merge branch 'dev' into kasong/2914053-common-interface

b830a97

Merge branch 'dev' into kasong/2914053-common-interface

32c71df

Add can perform request

23334a8

Merge branch 'dev' into kasong/2914053-common-interface

dc19668

initial commit to add XPC interface for silent token request

7024e55

Remove crypto part for response

7721a7c

add UTs to check fallback controlers' order

019d5d3

kaisong1990 requested a review from a team as a code owner February 22, 2025 00:09

kaisong1990 requested review from ameyapat and jasoncoolmax February 22, 2025 00:09

azure-pipelines bot reviewed Feb 22, 2025

View reviewed changes

Kai Song added 2 commits February 26, 2025 09:09

Add enum to enabl/disable XPC service

9bd5556

add enum to support xpc mode

5603852

kaisong1990 force-pushed the kasong/2914053-common-interface branch from 4fc4cb4 to 5603852 Compare February 26, 2025 19:32

Kai Song added 2 commits February 26, 2025 11:39

Merge branch 'dev' into kasong/2914053-common-interface

195252b

Remove ssoPerf telemetry

4861e4b

kaisong1990 added the XPC label Feb 26, 2025

kaisong1990 changed the title ~~[WPI] CommonCore interface for XPC, silent token request only~~ [SigninSilent only] CommonCore interface for XPC silent request. Feb 26, 2025

kaisong1990 requested review from oldalton and antrix1989 February 26, 2025 19:45

kaisong1990 commented Feb 26, 2025

View reviewed changes

IdentityCore/src/controllers/MSIDRequestControllerFactory.m Outdated Show resolved Hide resolved

Kai Song added 3 commits February 26, 2025 12:12

Make sure status code is used in log

7312a98

Merge branch 'dev' into kasong/2914053-common-interface

4d799e4

Add param control to use v2 XPC controller factory or V1 original con…

1b68c57

…troller factory with XPC support

ameyapat reviewed Mar 12, 2025

View reviewed changes

IdentityCore/src/MSIDConstants.h Show resolved Hide resolved

ameyapat reviewed Mar 12, 2025

View reviewed changes

IdentityCore/src/controllers/MSIDRequestControllerFactory.m Outdated Show resolved Hide resolved

ameyapat reviewed Mar 12, 2025

View reviewed changes

ameyapat reviewed Mar 13, 2025

View reviewed changes

ameyapat requested changes Mar 13, 2025

View reviewed changes

Kai Song added 2 commits March 13, 2025 13:26

Address CR

453e551

remove msid prefix

447af73

kaisong1990 requested a review from ameyapat March 13, 2025 20:47

Fix a typo

1191653