Fix failsafe extension when we have a device attestation delegate. (#…

…25787) The boolean test in ExtendArmFailSafeForDeviceAttestation was backwards, so there were two failure cases: 1. If we had a delegate but FailSafeExpiryTimeoutSecs() returned NullOptional, we would skip extending the fail-safe (expected), but set failSafeSkipped to _false_, and so not actually make our "continue commissioning" callback. 2. If we had a delegate and FailSafeExpiryTimeoutSecs() returned a too-small value, we would also skip resetting the fail-safe, but end up setting failSafeSkipped to false, and hence again failing to make our "continue commissioning" callback. The fix is to reverse the sense of the "do we need to call back immediately" boolean, to make it a little clearer what the logic flow is here, and then check it appropriately.
project-chip · Oct 19, 2023 · 4f4059b · 4f4059b
1 parent b570ec9
commit 4f4059b
Show file tree

Hide file tree

Showing 10 changed files with 404 additions and 164 deletions.
diff --git a/examples/darwin-framework-tool/commands/pairing/PairingCommandBridge.h b/examples/darwin-framework-tool/commands/pairing/PairingCommandBridge.h
@@ -68,6 +68,12 @@ class PairingCommandBridge : public CHIPCommandBridge
             AddArgument("discriminator", 0, 4096, &mDiscriminator);
             break;
         }
+
+        AddArgument("use-device-attestation-delegate", 0, 1, &mUseDeviceAttestationDelegate,
+                    "If true, use a device attestation delegate that always wants to be notified about attestation results.  "
+                    "Defaults to false.");
+        AddArgument("device-attestation-failsafe-time", 0, UINT16_MAX, &mDeviceAttestationFailsafeTime,
+                    "If set, the time to extend the failsafe to before calling the device attestation delegate");
     }
 
     /////////// CHIPCommandBridge Interface /////////
@@ -89,4 +95,6 @@ class PairingCommandBridge : public CHIPCommandBridge
     uint16_t mDiscriminator;
     uint32_t mSetupPINCode;
     char * mOnboardingPayload;
+    chip::Optional<bool> mUseDeviceAttestationDelegate;
+    chip::Optional<uint16_t> mDeviceAttestationFailsafeTime;
 };
diff --git a/examples/darwin-framework-tool/commands/pairing/PairingCommandBridge.mm b/examples/darwin-framework-tool/commands/pairing/PairingCommandBridge.mm
@@ -28,6 +28,22 @@
 using namespace ::chip;
 using namespace ::chip::Controller;
 
+// A no-op MTRDeviceAttestationDelegate which lets us test (by default, in CI)
+// commissioning flows that have such a delegate.
+@interface NoOpAttestationDelegate : NSObject <MTRDeviceAttestationDelegate>
+@end
+
+@implementation NoOpAttestationDelegate
+- (void)deviceAttestationCompletedForController:(MTRDeviceController *)controller
+                             opaqueDeviceHandle:(void *)opaqueDeviceHandle
+                          attestationDeviceInfo:(MTRDeviceAttestationDeviceInfo *)attestationDeviceInfo
+                                          error:(NSError * _Nullable)error
+{
+    [controller continueCommissioningDevice:opaqueDeviceHandle ignoreAttestationFailure:NO error:nil];
+}
+
+@end
+
 void PairingCommandBridge::SetUpDeviceControllerDelegate()
 {
     dispatch_queue_t callbackQueue = dispatch_queue_create("com.chip.pairing", DISPATCH_QUEUE_SERIAL);
@@ -49,6 +65,13 @@
         break;
     }
 
+    if (mUseDeviceAttestationDelegate.ValueOr(false)) {
+        params.deviceAttestationDelegate = [[NoOpAttestationDelegate alloc] init];
+        if (mDeviceAttestationFailsafeTime.HasValue()) {
+            params.failSafeTimeout = @(mDeviceAttestationFailsafeTime.Value());
+        }
+    }
+
     [deviceControllerDelegate setCommandBridge:this];
     [deviceControllerDelegate setParams:params];
     [deviceControllerDelegate setCommissioner:commissioner];

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -1186,14 +1186,14 @@ void DeviceCommissioner::ExtendArmFailSafeForDeviceAttestation(const Credentials
 
     mAttestationDeviceInfo = Platform::MakeUnique<Credentials::DeviceAttestationVerifier::AttestationDeviceInfo>(info);
 
-    auto expiryLengthSeconds = deviceAttestationDelegate->FailSafeExpiryTimeoutSecs();
-    bool failSafeSkipped     = expiryLengthSeconds.HasValue();
-    if (failSafeSkipped)
+    auto expiryLengthSeconds      = deviceAttestationDelegate->FailSafeExpiryTimeoutSecs();
+    bool waitForFailsafeExtension = expiryLengthSeconds.HasValue();
+    if (waitForFailsafeExtension)
     {
         ChipLogProgress(Controller, "Changing fail-safe timer to %u seconds to handle DA failure", expiryLengthSeconds.Value());
         // Per spec, anything we do with the fail-safe armed must not time out
         // in less than kMinimumCommissioningStepTimeout.
-        failSafeSkipped =
+        waitForFailsafeExtension =
             ExtendArmFailSafe(mDeviceBeingCommissioned, mCommissioningStage, expiryLengthSeconds.Value(),
                               MakeOptional(kMinimumCommissioningStepTimeout), OnArmFailSafeExtendedForDeviceAttestation,
                               OnFailedToExtendedArmFailSafeDeviceAttestation);
@@ -1203,7 +1203,7 @@ void DeviceCommissioner::ExtendArmFailSafeForDeviceAttestation(const Credentials
         ChipLogProgress(Controller, "Proceeding without changing fail-safe timer value as delegate has not set it");
     }
 
-    if (failSafeSkipped)
+    if (!waitForFailsafeExtension)
     {
         // Callee does not use data argument.
         const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType data;

diff --git a/src/darwin/Framework/CHIPTests/MTRBackwardsCompatTests.m b/src/darwin/Framework/CHIPTests/MTRBackwardsCompatTests.m
@@ -20,6 +20,7 @@
 
 #import "MTRErrorTestUtils.h"
 #import "MTRTestKeys.h"
+#import "MTRTestResetCommissioneeHelper.h"
 #import "MTRTestStorage.h"
 
 #import <app/util/af-enums.h>
@@ -1179,59 +1180,7 @@ - (void)test046_MTRThreadOperationalDataset
 #if !MANUAL_INDIVIDUAL_TEST
 - (void)test999_TearDown
 {
-    // Put the device back in the state we found it: open commissioning window, no fabrics commissioned.
-    MTRBaseDevice * device = GetConnectedDevice();
-    dispatch_queue_t queue = dispatch_get_main_queue();
-
-    // Get our current fabric index, for later deletion.
-    XCTestExpectation * readFabricIndexExpectation = [self expectationWithDescription:@"Fabric index read"];
-
-    __block NSNumber * fabricIndex;
-    __auto_type * opCredsCluster = [[MTRBaseClusterOperationalCredentials alloc] initWithDevice:device endpoint:0 queue:queue];
-    [opCredsCluster
-        readAttributeCurrentFabricIndexWithCompletionHandler:^(NSNumber * _Nullable value, NSError * _Nullable readError) {
-            XCTAssertNil(readError);
-            XCTAssertNotNil(value);
-            fabricIndex = value;
-            [readFabricIndexExpectation fulfill];
-        }];
-
-    [self waitForExpectations:@[ readFabricIndexExpectation ] timeout:kTimeoutInSeconds];
-
-    // Open a commissioning window.
-    XCTestExpectation * openCommissioningWindowExpectation = [self expectationWithDescription:@"Commissioning window opened"];
-
-    __auto_type * adminCommissioningCluster = [[MTRBaseClusterAdministratorCommissioning alloc] initWithDevice:device
-                                                                                                      endpoint:0
-                                                                                                         queue:queue];
-    __auto_type * openWindowParams = [[MTRAdministratorCommissioningClusterOpenBasicCommissioningWindowParams alloc] init];
-    openWindowParams.commissioningTimeout = @(900);
-    openWindowParams.timedInvokeTimeoutMs = @(50000);
-    [adminCommissioningCluster openBasicCommissioningWindowWithParams:openWindowParams
-                                                    completionHandler:^(NSError * _Nullable error) {
-                                                        XCTAssertNil(error);
-                                                        [openCommissioningWindowExpectation fulfill];
-                                                    }];
-
-    [self waitForExpectations:@[ openCommissioningWindowExpectation ] timeout:kTimeoutInSeconds];
-
-    // Remove our fabric from the device.
-    XCTestExpectation * removeFabricExpectation = [self expectationWithDescription:@"Fabric removed"];
-
-    __auto_type * removeParams = [[MTROperationalCredentialsClusterRemoveFabricParams alloc] init];
-    removeParams.fabricIndex = fabricIndex;
-
-    [opCredsCluster removeFabricWithParams:removeParams
-                         completionHandler:^(
-                             MTROperationalCredentialsClusterNOCResponseParams * _Nullable data, NSError * _Nullable removeError) {
-                             XCTAssertNil(removeError);
-                             XCTAssertNotNil(data);
-                             XCTAssertEqualObjects(data.statusCode, @(0));
-                             [removeFabricExpectation fulfill];
-                         }];
-
-    [self waitForExpectations:@[ removeFabricExpectation ] timeout:kTimeoutInSeconds];
-
+    ResetCommissionee(GetConnectedDevice(), dispatch_get_main_queue(), self, kTimeoutInSeconds);
     [self shutdownStack];
 }
 #endif

diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m
@@ -26,6 +26,7 @@
 
 #import "MTRErrorTestUtils.h"
 #import "MTRTestKeys.h"
+#import "MTRTestResetCommissioneeHelper.h"
 #import "MTRTestStorage.h"
 
 #import <app/util/af-enums.h>
@@ -1819,59 +1820,7 @@ - (void)test900_SubscribeAllAttributes
 #if !MANUAL_INDIVIDUAL_TEST
 - (void)test999_TearDown
 {
-    // Put the device back in the state we found it: open commissioning window, no fabrics commissioned.
-    MTRBaseDevice * device = GetConnectedDevice();
-    dispatch_queue_t queue = dispatch_get_main_queue();
-
-    // Get our current fabric index, for later deletion.
-    XCTestExpectation * readFabricIndexExpectation = [self expectationWithDescription:@"Fabric index read"];
-
-    __block NSNumber * fabricIndex;
-    __auto_type * opCredsCluster = [[MTRBaseClusterOperationalCredentials alloc] initWithDevice:device endpointID:@(0) queue:queue];
-    [opCredsCluster
-        readAttributeCurrentFabricIndexWithCompletionHandler:^(NSNumber * _Nullable value, NSError * _Nullable readError) {
-            XCTAssertNil(readError);
-            XCTAssertNotNil(value);
-            fabricIndex = value;
-            [readFabricIndexExpectation fulfill];
-        }];
-
-    [self waitForExpectations:@[ readFabricIndexExpectation ] timeout:kTimeoutInSeconds];
-
-    // Open a commissioning window.
-    XCTestExpectation * openCommissioningWindowExpectation = [self expectationWithDescription:@"Commissioning window opened"];
-
-    __auto_type * adminCommissioningCluster = [[MTRBaseClusterAdministratorCommissioning alloc] initWithDevice:device
-                                                                                                    endpointID:@(0)
-                                                                                                         queue:queue];
-    __auto_type * openWindowParams = [[MTRAdministratorCommissioningClusterOpenBasicCommissioningWindowParams alloc] init];
-    openWindowParams.commissioningTimeout = @(900);
-    openWindowParams.timedInvokeTimeoutMs = @(50000);
-    [adminCommissioningCluster openBasicCommissioningWindowWithParams:openWindowParams
-                                                    completionHandler:^(NSError * _Nullable error) {
-                                                        XCTAssertNil(error);
-                                                        [openCommissioningWindowExpectation fulfill];
-                                                    }];
-
-    [self waitForExpectations:@[ openCommissioningWindowExpectation ] timeout:kTimeoutInSeconds];
-
-    // Remove our fabric from the device.
-    XCTestExpectation * removeFabricExpectation = [self expectationWithDescription:@"Fabric removed"];
-
-    __auto_type * removeParams = [[MTROperationalCredentialsClusterRemoveFabricParams alloc] init];
-    removeParams.fabricIndex = fabricIndex;
-
-    [opCredsCluster removeFabricWithParams:removeParams
-                         completionHandler:^(
-                             MTROperationalCredentialsClusterNOCResponseParams * _Nullable data, NSError * _Nullable removeError) {
-                             XCTAssertNil(removeError);
-                             XCTAssertNotNil(data);
-                             XCTAssertEqualObjects(data.statusCode, @(0));
-                             [removeFabricExpectation fulfill];
-                         }];
-
-    [self waitForExpectations:@[ removeFabricExpectation ] timeout:kTimeoutInSeconds];
-
+    ResetCommissionee(GetConnectedDevice(), dispatch_get_main_queue(), self, kTimeoutInSeconds);
     [self shutdownStack];
 }
 #endif