Feedback from Android commissioning fixes

src/controller/CHIPDeviceController.h

@@ -561,6 +561,15 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
      */
     CHIP_ERROR NetworkCredentialsReady();
 
+    /**
+     * @brief
+     *  This function returns the current CommissioningStage for this commissioner.
+     */
+    CommissioningStage GetCommissioningStage()
+    {
+        return mCommissioningStage;
+    }
+
 #if CONFIG_NETWORK_LAYER_BLE
 #if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
     /**

src/controller/java/AndroidOperationalCredentialsIssuer.cpp

@@ -205,7 +205,6 @@ CHIP_ERROR AndroidOperationalCredentialsIssuer::CallbackGenerateNOCChain(const B
 
     P256PublicKey pubkey;
     ReturnErrorOnFailure(VerifyCertificateSigningRequest(csr.data(), csr.size(), pubkey));
-    // TODO: verify signed by DAC creds?
     ChipLogProgress(chipTool, "VerifyCertificateSigningRequest");
 
     jobject csrInfo;

src/controller/java/CHIPDeviceController-JNI.cpp

@@ -524,7 +524,9 @@ JNI_METHOD(void, setUseJavaCallbackForNOCRequest)
 
     if (useCallback)
     {
-        // if we are assigning a callback, then make the device commissioner delegate verification to the cloud
+        // if we are assigning a callback, then make the device commissioner delegate verification to the
+        // PartialDACVerifier so that DAC chain and CD validation can be performed by custom code
+        // triggered by ChipDeviceController.NOCChainIssuer.onNOCChainGenerationNeeded().
         wrapper->Controller()->SetDeviceAttestationVerifier(wrapper->GetPartialDACVerifier());
     }
     else
@@ -554,6 +556,17 @@ JNI_METHOD(void, updateCommissioningNetworkCredentials)
         ChipLogError(Controller, "UpdateCommissioningParameters failed. Err = %" CHIP_ERROR_FORMAT, err.Format());
         JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err);
     }
+
+    // Only invoke NetworkCredentialsReady when called in response to NetworkScan result
+    if (wrapper->Controller()->GetCommissioningStage() == CommissioningStage::kNeedsNetworkCreds)
+    {
+        err = wrapper->Controller()->NetworkCredentialsReady();
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(Controller, "NetworkCredentialsReady failed. Err = %" CHIP_ERROR_FORMAT, err.Format());
+            JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err);
+        }
+    }
 }
 
 JNI_METHOD(jbyteArray, convertX509CertToMatterCert)

src/controller/java/src/chip/devicecontroller/ChipDeviceController.java

@@ -63,10 +63,18 @@ public void setScanNetworksListener(ScanNetworksListener listener) {
   }
 
   /**
-   * Sets this DeviceController to use the given issuer for issuing operational certs. By default,
-   * the DeviceController uses an internal, OperationalCredentialsDelegate (see
-   * AndroidOperationalCredentialsIssuer)
+   * Sets this DeviceController to use the given issuer for issuing operational certs and 
+   * verifying the DAC. By default, the DeviceController uses an internal, OperationalCredentialsDelegate 
+   * (see AndroidOperationalCredentialsIssuer).
    *
+   * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be
+   * called when the NOC CSR needs to be signed and DAC verified. This allows for custom credentials 
+   * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform 
+   * DAC verification and the CSR signing.
+   * 
+   * <p>When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used
+   * rather than the DefaultDACVerifier.
+   * 
    * @param issuer
    */
   public void setNOCChainIssuer(NOCChainIssuer issuer) {
@@ -692,8 +700,12 @@ protected void finalize() throws Throwable {
   public interface NOCChainIssuer {
     /**
      * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be
-     * called when the NOC CSR needs to be signed. This allows for custom credentials issuer
-     * implementations, for example, when a proprietary cloud API will perform the CSR signing.
+     * called when the NOC CSR needs to be signed and DAC verified. This allows for custom credentials 
+     * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform 
+     * DAC verification and the CSR signing.
+     * 
+     * <p>When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used
+     * rather than the DefaultDACVerifier.
      *
      * <p>The commissioning workflow will stop upon the onNOCChainGenerationNeeded callback and
      * resume once onNOCChainGeneration is called.
@@ -716,6 +728,10 @@ public interface NOCChainIssuer {
    * <p>Set the AttemptNetworkScanWiFi or AttemptNetworkScanThread to configure the enable/disable
    * WiFi or Thread network scan during commissioning in the the default CommissioningDelegate used
    * by the ChipDeviceCommissioner.
+   * 
+   * When the callbacks onScanNetworksFailure or onScanNetworksSuccess are invoked, the commissioning
+   * flow has reached the kNeedsNetworkCreds and will wait to advance until this device controller's
+   * updateCommissioningNetworkCredentials method is called with the desired network credentials set.
    */
   public interface ScanNetworksListener {
     /** Notifies when scan networks call fails. */

src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp

@@ -36,6 +36,10 @@ namespace Credentials {
 // As per specifications section 11.22.5.1. Constant RESP_MAX
 constexpr size_t kMaxResponseLength = 900;
 
+/**
+ * The implementation should track DefaultDACVerifier::VerifyAttestationInformation but with the checks
+ * disabled that are outlined at the top of DacOnlyPartialAttestationVerifier.h.
+ */
 void PartialDACVerifier::VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info,
                                                       Callback::Callback<OnAttestationInformationVerification> * onCompletion)
 {
@@ -144,7 +148,7 @@ void PartialDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer
     }
 
 exit:
-    onCompletion->mCall(onCompletion->mContext, attestationError); // TODO: is this check getting done?
+    onCompletion->mCall(onCompletion->mContext, attestationError);
 }
 
 } // namespace Credentials

src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h

@@ -21,11 +21,37 @@
 namespace chip {
 namespace Credentials {
 
+/**
+ * @brief
+ *   This class is based upon the DefaultDACVerifier but has all checks removed which require
+ * local availability of public keys that are not available from the commissionee, such as the
+ * PAA public key and the CSA keys used to sign the Certification Declaration (CD).
+ *
+ *   This class should only be used in conjunction with an OperationalCredentialsDelegate
+ * which performs the removed checks. For example, an OperationalCredentialsDelegate implementation
+ * might send the DAC chain and signed CD to custom code which obtains these keys from the DCL.
+ *
+ * Specifically, the following list of checks have been removed:
+ * (1) Make sure the PAA is valid and approved by CSA.
+ * (2) vid-scoped PAA check: if the PAA is vid scoped, then its vid must match the DAC vid.
+ * (3) cert chain check: verify PAI is signed by PAA, and DAC is signed by PAI.
+ * (4) PAA subject key id extraction: the PAA subject key must match the PAA key referenced in the PAI.
+ * (5) CD signature check: make sure a valid CSA CD key is used to sign the CD.
+ *
+ * Any other checks performed by the DefaultDACVerifier should be performed here too. Changes
+ * made to DefaultDACVerifier::VerifyAttestationInformation should be made to
+ * PartialDACVerifier::VerifyAttestationInformation.
+ */
 class PartialDACVerifier : public DefaultDACVerifier
 {
 public:
     PartialDACVerifier() {}
 
+    /**
+     * @brief
+     * The implementation should track DefaultDACVerifier::VerifyAttestationInformation but with the checks
+     * disabled that are outlined at the top of DacOnlyPartialAttestationVerifier.h.
+     */
     void VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info,
                                       Callback::Callback<OnAttestationInformationVerification> * onCompletion) override;