Enforce param values, Add mDiscriminator, Update docs

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

Author: samadDotDev

examples/fabric-admin/commands/pairing/OpenCommissioningWindowCommand.h

@@ -48,7 +48,9 @@ class OpenCommissioningWindowCommand : public CHIPCommand
                     &mIteration, "Number of PBKDF iterations to use to derive the verifier.  Ignored if 'option' is 0.");
         AddArgument("discriminator", 0, 4096, &mDiscriminator, "Discriminator to use for advertising.  Ignored if 'option' is 0.");
         AddArgument("timeout", 0, UINT16_MAX, &mTimeout, "Time, in seconds, before this command is considered to have timed out.");
-        AddArgument("salt", &mSalt, "Salt payload encoded in hexadecimal. Random salt will be generated if absent");
+        AddArgument("salt", &mSalt,
+                    "Salt payload encoded in hexadecimal. Random salt will be generated if absent. "
+                    "This needs to be present if verifier is provided, corresponding to salt used for generating verifier");
         AddArgument("verifier", &mVerifier,
                     "PAKE Passcode verifier encoded in hexadecimal format. Will be generated from random setup pin and other "
                     "params if absent");

src/controller/CHIPDeviceController.h

@@ -167,16 +167,6 @@ struct CommissionerInitParams : public ControllerInitParams
     Credentials::DeviceAttestationVerifier * deviceAttestationVerifier = nullptr;
 };
 
-// Interface class for DeviceController methods that need to be mocked
-class IDeviceController
-{
-public:
-    virtual ~IDeviceController()                                                                 = default;
-    virtual CHIP_ERROR GetConnectedDevice(NodeId peerNodeId, chip::Callback::Callback<OnDeviceConnected> * onConnection,
-                                          chip::Callback::Callback<OnDeviceConnectionFailure> * onFailure,
-                                          TransportPayloadCapability transportPayloadCapability) = 0;
-};
-
 /**
  * @brief
  *   Controller applications can use this class to communicate with already paired CHIP devices. The
@@ -185,7 +175,7 @@ class IDeviceController
  *   and device pairing information for individual devices). Alternatively, this class can retrieve the
  *   relevant information when the application tries to communicate with the device
  */
-class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController, IDeviceController
+class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController
 {
 public:
     DeviceController();
@@ -253,10 +243,10 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController, IDe
      * An error return from this function means that neither callback has been
      * called yet, and neither callback will be called in the future.
      */
-    CHIP_ERROR
+    virtual CHIP_ERROR
     GetConnectedDevice(NodeId peerNodeId, Callback::Callback<OnDeviceConnected> * onConnection,
-                       chip::Callback::Callback<OnDeviceConnectionFailure> * onFailure,
-                       TransportPayloadCapability transportPayloadCapability = TransportPayloadCapability::kMRPPayload) override
+                       Callback::Callback<OnDeviceConnectionFailure> * onFailure,
+                       TransportPayloadCapability transportPayloadCapability = TransportPayloadCapability::kMRPPayload)
     {
         VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
         mSystemState->CASESessionMgr()->FindOrEstablishSession(ScopedNodeId(peerNodeId, GetFabricIndex()), onConnection, onFailure,

src/controller/CommissioningWindowOpener.cpp

@@ -98,7 +98,8 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(const Commissionin
     }
 
     mSetupPayload.version = 0;
-    mSetupPayload.discriminator.SetLongValue(params.GetDiscriminator());
+    mDiscriminator.SetLongValue(params.GetDiscriminator());
+    mSetupPayload.discriminator = mDiscriminator;
     mSetupPayload.rendezvousInformation.SetValue(RendezvousInformationFlag::kOnNetwork);
 
     if (params.HasSalt())
@@ -156,10 +157,7 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(const Commissionin
     mCommissioningWindowTimeout          = params.GetTimeout();
     mPBKDFIterations                     = params.GetIteration();
     mCommissioningWindowOption           = CommissioningWindowOption::kTokenWithProvidedPIN;
-    mSetupPayload                        = SetupPayload();
-    mSetupPayload.version                = 0;
-    mSetupPayload.discriminator.SetLongValue(params.GetDiscriminator());
-    mSetupPayload.rendezvousInformation.SetValue(RendezvousInformationFlag::kOnNetwork);
+    mDiscriminator.SetLongValue(params.GetDiscriminator());
 
     mNextStep = Step::kOpenCommissioningWindow;
 
@@ -184,7 +182,7 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowInternal(Messaging:
         AdministratorCommissioning::Commands::OpenCommissioningWindow::Type request;
         request.commissioningTimeout = mCommissioningWindowTimeout.count();
         request.PAKEPasscodeVerifier = serializedVerifierSpan;
-        request.discriminator        = mSetupPayload.discriminator.GetLongValue();
+        request.discriminator        = mDiscriminator.GetLongValue();
         request.iterations           = mPBKDFIterations;
         request.salt                 = mPBKDFSalt;
 

src/controller/CommissioningWindowOpener.h

@@ -164,6 +164,7 @@ class CommissioningWindowOpener
     Callback::Callback<OnOpenCommissioningWindowWithVerifier> * mCommissioningWindowVerifierCallback = nullptr;
     Callback::Callback<OnOpenBasicCommissioningWindow> * mBasicCommissioningWindowCallback           = nullptr;
     SetupPayload mSetupPayload;
+    SetupDiscriminator mDiscriminator{};
     NodeId mNodeId                                       = kUndefinedNodeId;
     System::Clock::Seconds16 mCommissioningWindowTimeout = System::Clock::kZero;
     CommissioningWindowOption mCommissioningWindowOption = CommissioningWindowOption::kOriginalSetupCode;

src/controller/CommissioningWindowParams.h

@@ -40,7 +40,11 @@ class CommissioningWindowCommonParams
 public:
     CommissioningWindowCommonParams() = default;
 
-    NodeId GetNodeId() const { return mNodeId; }
+    NodeId GetNodeId() const
+    {
+        VerifyOrDie(mNodeId != kUndefinedNodeId);
+        return mNodeId;
+    }
     // The node identifier of device
     Derived & SetNodeId(NodeId nodeId)
     {
@@ -67,18 +71,19 @@ class CommissioningWindowCommonParams
     }
 
     // The long discriminator for the DNS-SD advertisement.
-    uint16_t GetDiscriminator() const { return mDiscriminator; }
+    uint16_t GetDiscriminator() const { return mDiscriminator.Value(); }
+    bool HasDiscriminator() const { return mDiscriminator.HasValue(); }
     Derived & SetDiscriminator(uint16_t discriminator)
     {
-        mDiscriminator = discriminator;
+        mDiscriminator = MakeOptional(discriminator);
         return static_cast<Derived &>(*this);
     }
 
 private:
-    NodeId mNodeId                    = 0;
-    System::Clock::Seconds16 mTimeout = System::Clock::Seconds16(300);
-    uint32_t mIteration               = 1000;
-    uint16_t mDiscriminator           = 0;
+    NodeId mNodeId                    = kUndefinedNodeId;
+    System::Clock::Seconds16 mTimeout = System::Clock::Seconds16(300); // Defaulting
+    uint32_t mIteration               = 1000;                          // Defaulting
+    Optional<uint16_t> mDiscriminator = NullOptional; // Using optional type to avoid picking a sentinnel in valid range
 };
 
 class CommissioningWindowPasscodeParams : public CommissioningWindowCommonParams<CommissioningWindowPasscodeParams>
@@ -87,9 +92,8 @@ class CommissioningWindowPasscodeParams : public CommissioningWindowCommonParams
     CommissioningWindowPasscodeParams() = default;
 
     bool HasSetupPIN() const { return mSetupPIN.HasValue(); }
-    // Get the value of salt if present.
-    // Returns 0 if absent, make sure to check HasSetupPIN() if a valid value is required.
-    uint32_t GetSetupPIN() const { return mSetupPIN.ValueOr(0); }
+    // Get the value of setup PIN (Passcode) if present, crashes otherwise.
+    uint32_t GetSetupPIN() const { return mSetupPIN.Value(); }
     // The setup PIN (Passcode) to use. A random one will be generated if not provided.
     CommissioningWindowPasscodeParams & SetSetupPIN(uint32_t setupPIN) { return SetSetupPIN(MakeOptional(setupPIN)); }
     // The setup PIN (Passcode) to use. A random one will be generated if NullOptional is used.
@@ -148,21 +152,21 @@ class CommissioningWindowVerifierParams : public CommissioningWindowCommonParams
 public:
     CommissioningWindowVerifierParams() = default;
 
-    ByteSpan GetVerifier() const { return mVerifier; }
+    ByteSpan GetVerifier() const { return mVerifier.Value(); }
     // The PAKE passcode verifier generated with enclosed iterations, salt and not-enclosed passcode.
     CommissioningWindowVerifierParams & SetVerifier(ByteSpan verifier)
     {
-        mVerifier = verifier;
+        mVerifier = MakeOptional(verifier);
         return *this;
     }
 
-    ByteSpan GetSalt() const { return mSalt; }
+    ByteSpan GetSalt() const { return mSalt.Value(); }
     // The salt that was used to generate the verifier.
     // It must be at least kSpake2p_Min_PBKDF_Salt_Length bytes.
     // Note: This is REQUIRED when verifier is used
     CommissioningWindowVerifierParams & SetSalt(ByteSpan salt)
     {
-        mSalt = salt;
+        mSalt = MakeOptional(salt);
         return *this;
     }
 
@@ -176,8 +180,8 @@ class CommissioningWindowVerifierParams : public CommissioningWindowCommonParams
     }
 
 private:
-    ByteSpan mVerifier;
-    ByteSpan mSalt;
+    Optional<ByteSpan> mSalt;
+    Optional<ByteSpan> mVerifier;
     Callback::Callback<OnOpenCommissioningWindowWithVerifier> * mCallback = nullptr;
 };
 

src/controller/tests/TestCommissioningWindowOpener.cpp

@@ -82,7 +82,7 @@ TEST_F(TestCommissioningWindowOpener, OpenCommissioningWindowVerifier_Success)
     EXPECT_EQ(err, CHIP_NO_ERROR);
 }
 
-TEST_F(TestCommissioningWindowOpener, OpenCommissioningWindowVerifier_Failure_NoSalt)
+TEST_F(TestCommissioningWindowOpener, OpenCommissioningWindowVerifier_Failure_InvalidSalt)
 {
     Callback::Callback<Controller::OnOpenCommissioningWindowWithVerifier> callback(OCWVerifierCallback, this);
 
@@ -91,12 +91,13 @@ TEST_F(TestCommissioningWindowOpener, OpenCommissioningWindowVerifier_Failure_No
                                                         .SetTimeout(300)
                                                         .SetIteration(sTestSpake2p01_IterationCount)
                                                         .SetDiscriminator(3840)
+                                                        .SetSalt(ByteSpan())
                                                         .SetVerifier(ByteSpan(sTestSpake2p01_SerializedVerifier))
                                                         .SetCallback(&callback));
     EXPECT_EQ(err, CHIP_ERROR_INVALID_ARGUMENT);
 }
 
-TEST_F(TestCommissioningWindowOpener, OpenCommissioningWindowVerifier_Failure_NoVerifier)
+TEST_F(TestCommissioningWindowOpener, OpenCommissioningWindowVerifier_Failure_InvalidVerifier)
 {
     Callback::Callback<Controller::OnOpenCommissioningWindowWithVerifier> callback(OCWVerifierCallback, this);
 
@@ -106,6 +107,7 @@ TEST_F(TestCommissioningWindowOpener, OpenCommissioningWindowVerifier_Failure_No
                                                         .SetIteration(sTestSpake2p01_IterationCount)
                                                         .SetDiscriminator(3840)
                                                         .SetSalt(ByteSpan(sTestSpake2p01_Salt))
+                                                        .SetVerifier(ByteSpan())
                                                         .SetCallback(&callback));
     EXPECT_EQ(err, CHIP_ERROR_INVALID_ARGUMENT);
 }