Address feedback around returning errors and adding comments

project-chip · Aug 18, 2022 · 672ca85 · 672ca85
1 parent a33bbc1
commit 672ca85
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 22 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRNOCChainIssuer.h b/src/darwin/Framework/CHIP/MTRNOCChainIssuer.h
@@ -43,9 +43,9 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (void)onNOCChainGenerationNeeded:(CSRInfo *)csrInfo
                    attestationInfo:(AttestationInfo *)attestationInfo
-      onNOCChainGenerationComplete:(NSNumber * (^)(NSData * operationalCertificate, NSData * intermediateCertificate,
-                                       NSData * rootCertificate, NSData * ipk,
-                                       NSNumber * adminSubject))onNOCChainGenerationComplete;
+      onNOCChainGenerationComplete:(void (^)(NSData * operationalCertificate, NSData * intermediateCertificate,
+                                       NSData * rootCertificate, NSData * ipk, NSNumber * adminSubject,
+                                       NSError * __autoreleasing * error))onNOCChainGenerationComplete;
 
 @end
 

diff --git a/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.h b/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.h
@@ -126,11 +126,11 @@ class MTROperationalCredentialsDelegate : public chip::Controller::OperationalCr
      * Caller must pass a non-nil value for the rootCertificate, intermediateCertificate, operationalCertificate
      * If ipk and adminSubject are non nil, then they will be used in the AddNOC command sent to the commissionee. If they are not
      * populated, then the values provided in the MTRDeviceController initialization will be used.
-     *
-     * @return error code (0 is no error)
      */
-    NSNumber * onNOCChainGenerationComplete(MTROperationalCredentialsDelegate * thisDelegate, NSData * operationalCertificate,
-        NSData * intermediateCertificate, NSData * rootCertificate, NSData * _Nullable ipk, NSNumber * _Nullable adminSubject);
+    void onNOCChainGenerationComplete(NSData * operationalCertificate, NSData * intermediateCertificate, NSData * rootCertificate,
+        NSData * _Nullable ipk, NSNumber * _Nullable adminSubject, NSError * __autoreleasing * error);
+
+    void setNSError(CHIP_ERROR err, NSError * __autoreleasing * outError);
 
     CHIP_ERROR CallbackGenerateNOCChain(const chip::ByteSpan & csrElements, const chip::ByteSpan & csrNonce,
         const chip::ByteSpan & attestationSignature, const chip::ByteSpan & attestationChallenge, const chip::ByteSpan & DAC,

diff --git a/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm b/src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm
@@ -184,6 +184,8 @@
     });
     VerifyOrReturnError(commissioningParameters.HasValue(), CHIP_ERROR_INCORRECT_STATE);
 
+    // Attestation Elements, nonce and signature will have a value in Commissioning Params as the CSR needs a signature or else we
+    // cannot trust it
     ReturnErrorOnFailure(
         chip::Credentials::DeconstructAttestationElements(commissioningParameters.Value().GetAttestationElements().Value(),
             certificationDeclarationSpan, attestationNonceSpan, timestampDeconstructed, firmwareInfoSpan, vendorReserved));
@@ -201,38 +203,50 @@
     dispatch_sync(mNocChainIssuerQueue, ^{
         [mNocChainIssuer onNOCChainGenerationNeeded:csrInfo
                                     attestationInfo:attestationInfo
-                       onNOCChainGenerationComplete:^NSNumber *(NSData * operationalCertificate, NSData * intermediateCertificate,
-                           NSData * rootCertificate, NSData * ipk, NSNumber * adminSubject) {
-                           return onNOCChainGenerationComplete(
-                               this, operationalCertificate, intermediateCertificate, rootCertificate, ipk, adminSubject);
+                       onNOCChainGenerationComplete:^void(NSData * operationalCertificate, NSData * intermediateCertificate,
+                           NSData * rootCertificate, NSData * ipk, NSNumber * adminSubject, NSError * __autoreleasing * error) {
+                           onNOCChainGenerationComplete(
+                               operationalCertificate, intermediateCertificate, rootCertificate, ipk, adminSubject, error);
                        }];
     });
 
     return CHIP_NO_ERROR;
 }
 
-NSNumber * MTROperationalCredentialsDelegate::onNOCChainGenerationComplete(MTROperationalCredentialsDelegate * thisDelegate,
-    NSData * operationalCertificate, NSData * intermediateCertificate, NSData * rootCertificate, NSData * _Nullable ipk,
-    NSNumber * _Nullable adminSubject)
+void MTROperationalCredentialsDelegate::setNSError(CHIP_ERROR err, NSError * __autoreleasing * outError)
 {
-    VerifyOrReturnValue(operationalCertificate != nil, [NSNumber numberWithUnsignedInt:CHIP_ERROR_INVALID_ARGUMENT.AsInteger()]);
-    VerifyOrReturnValue(intermediateCertificate != nil, [NSNumber numberWithUnsignedInt:CHIP_ERROR_INVALID_ARGUMENT.AsInteger()]);
-    VerifyOrReturnValue(rootCertificate != nil, [NSNumber numberWithUnsignedInt:CHIP_ERROR_INVALID_ARGUMENT.AsInteger()]);
+    if (outError) {
+        *outError = [MTRError errorForCHIPErrorCode:err];
+    }
+}
+
+void MTROperationalCredentialsDelegate::onNOCChainGenerationComplete(NSData * operationalCertificate,
+    NSData * intermediateCertificate, NSData * rootCertificate, NSData * _Nullable ipk, NSNumber * _Nullable adminSubject,
+    NSError * __autoreleasing * error)
+{
+    if (operationalCertificate == nil || intermediateCertificate == nil || rootCertificate == nil) {
+        setNSError(CHIP_ERROR_INVALID_ARGUMENT, error);
+        return;
+    }
 
     // use ipk and adminSubject from CommissioningParameters if not passed in
     __block chip::Optional<chip::Controller::CommissioningParameters> commissioningParameters;
     dispatch_sync(mChipWorkQueue, ^{
         commissioningParameters = mCppCommissioner->GetCommissioningParameters();
     });
-    VerifyOrReturnValue(
-        commissioningParameters.HasValue(), [NSNumber numberWithUnsignedInt:CHIP_ERROR_INCORRECT_STATE.AsInteger()]);
+    if (!commissioningParameters.HasValue()) {
+        setNSError(CHIP_ERROR_INCORRECT_STATE, error);
+        return;
+    }
 
     chip::Optional<chip::Crypto::AesCcm128KeySpan> ipkOptional;
     uint8_t ipkValue[chip::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES];
     chip::Crypto::AesCcm128KeySpan ipkTempSpan(ipkValue);
     if (ipk != nil) {
-        VerifyOrReturnValue(
-            [ipk length] == sizeof(ipkValue), [NSNumber numberWithUnsignedInt:CHIP_ERROR_INCORRECT_STATE.AsInteger()]);
+        if ([ipk length] != sizeof(ipkValue)) {
+            setNSError(CHIP_ERROR_INCORRECT_STATE, error);
+            return;
+        }
         memcpy(&ipkValue[0], [ipk bytes], [ipk length]);
         ipkOptional.SetValue(ipkTempSpan);
     } else if (commissioningParameters.Value().GetIpk().HasValue()) {
@@ -251,8 +265,8 @@
 
     if (err != CHIP_NO_ERROR) {
         MTR_LOG_ERROR("Failed to SetNocChain for the device: %" CHIP_ERROR_FORMAT, err.Format());
+        setNSError(CHIP_ERROR_INCORRECT_STATE, error);
     }
-    return [NSNumber numberWithUnsignedInt:err.AsInteger()];
 }
 
 CHIP_ERROR MTROperationalCredentialsDelegate::LocalGenerateNOCChain(const chip::ByteSpan & csrElements,