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.

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;
 };

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];

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;

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

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