Using a Variant for the NextStep logic in HandleSigma1, to either send a Sigma message or a status report

Author: Alami-Amine

src/lib/support/CodeUtils.h

@@ -156,6 +156,34 @@
         }                                                                                                                          \
     } while (false)
 
+/**
+ *  @def ReturnErrorVariantOnFailure(expr)
+ *
+ *  @brief
+ *    This is for use when the caller function returns a Variant type. It returns a CHIP_ERROR variant with the corresponding error
+ *    code if the expression returns an error. For a CHIP_ERROR expression, this means any value other
+ *    than CHIP_NO_ERROR. For an integer expression, this means non-zero.
+ *
+ *  Example usage:
+ *
+ *  @code
+ *    ReturnErrorVariantOnFailure(NextStep, ParseSigma1(tlvReader, parsedSigma1));
+ *  @endcode
+ *
+ *  @param[in]  variantType   The Variant type that the calling function returns.
+ *  @param[in]  expr          An expression to be tested.
+
+ */
+#define ReturnErrorVariantOnFailure(variantType, expr)                                                                             \
+    do                                                                                                                             \
+    {                                                                                                                              \
+        auto __err = (expr);                                                                                                       \
+        if (!::chip::ChipError::IsSuccess(__err))                                                                                  \
+        {                                                                                                                          \
+            return variantType::Create<CHIP_ERROR>(__err);                                                                         \
+        }                                                                                                                          \
+    } while (false)
+
 /**
  *  @def ReturnLogErrorOnFailure(expr)
  *

src/protocols/secure_channel/CASESession.cpp

@@ -946,12 +946,12 @@ CHIP_ERROR CASESession::HandleSigma1_and_SendSigma2(System::PacketBufferHandle &
     MATTER_TRACE_SCOPE("HandleSigma1_and_SendSigma2", "CASESession");
 
     CHIP_ERROR err = CHIP_NO_ERROR;
-    Step nextStep  = Step::kNone;
 
     // Parse and Validate Received Sigma1, and decide next step
-    SuccessOrExit(err = HandleSigma1(std::move(msg), nextStep));
+    NextStep nextStep = HandleSigma1(std::move(msg));
+    VerifyOrExit(nextStep.Is<Step>(), err = nextStep.Get<CHIP_ERROR>());
 
-    switch (nextStep)
+    switch (nextStep.Get<Step>())
     {
     case Step::kSendSigma2: {
 
@@ -982,15 +982,11 @@ CHIP_ERROR CASESession::HandleSigma1_and_SendSigma2(System::PacketBufferHandle &
         mDelegate->OnSessionEstablishmentStarted();
         break;
     }
-        // TODO should I keep this?
-    case Step::kSendStatusReport:
     default:
-        ExitNow();
         break;
     }
 
 exit:
-
     if (err == CHIP_ERROR_KEY_NOT_FOUND)
     {
         SendStatusReport(mExchangeCtxt, kProtocolCodeNoSharedRoot);
@@ -1082,23 +1078,23 @@ CHIP_ERROR CASESession::TryResumeSession(SessionResumptionStorage::ConstResumpti
 
     return CHIP_NO_ERROR;
 }
-CHIP_ERROR CASESession::HandleSigma1(System::PacketBufferHandle && msg, Step & nextStep)
+CASESession::NextStep CASESession::HandleSigma1(System::PacketBufferHandle && msg)
 {
     MATTER_TRACE_SCOPE("HandleSigma1", "CASESession");
     ChipLogProgress(SecureChannel, "Received Sigma1 msg");
     MATTER_TRACE_COUNTER("Sigma1");
 
-    VerifyOrReturnError(mFabricsTable != nullptr, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(mFabricsTable != nullptr, NextStep::Create<CHIP_ERROR>(CHIP_ERROR_INCORRECT_STATE));
 
-    ReturnErrorOnFailure(mCommissioningHash.AddData(ByteSpan{ msg->Start(), msg->DataLength() }));
+    ReturnErrorVariantOnFailure(NextStep, mCommissioningHash.AddData(ByteSpan{ msg->Start(), msg->DataLength() }));
 
     System::PacketBufferTLVReader tlvReader;
     tlvReader.Init(std::move(msg));
 
     // Struct that will serve as output in ParseSigma1
     ParsedSigma1 parsedSigma1;
 
-    ReturnErrorOnFailure(ParseSigma1(tlvReader, parsedSigma1));
+    ReturnErrorVariantOnFailure(NextStep, ParseSigma1(tlvReader, parsedSigma1));
 
     ChipLogDetail(SecureChannel, "Peer assigned session key ID %d", parsedSigma1.initiatorSessionId);
     SetPeerSessionId(parsedSigma1.initiatorSessionId);
@@ -1119,12 +1115,9 @@ CHIP_ERROR CASESession::HandleSigma1(System::PacketBufferHandle && msg, Step & n
         std::copy(parsedSigma1.initiatorRandom.begin(), parsedSigma1.initiatorRandom.end(), mInitiatorRandom);
         std::copy(parsedSigma1.resumptionId.begin(), parsedSigma1.resumptionId.end(), mResumeResumptionId.begin());
 
-        // Next Step is to send Sigma2Resume message to the initiator
-        nextStep = Step::kSendSigma2Resume;
-
         //  Early returning here, since the next Step is known to be Sigma2Resume, and no further processing is needed for the
         //  Sigma1 message
-        return CHIP_NO_ERROR;
+        return NextStep::Create<Step>(Step::kSendSigma2Resume);
     }
 
     //  ParseSigma1 ensures that:
@@ -1143,21 +1136,21 @@ CHIP_ERROR CASESession::HandleSigma1(System::PacketBufferHandle && msg, Step & n
         // Side-effect of FindLocalNodeFromDestinationId success was that mFabricIndex/mLocalNodeId are now
         // set to the local fabric and associated NodeId that was targeted by the initiator.
 
-        nextStep = Step::kSendSigma2;
+        return NextStep::Create<Step>(Step::kSendSigma2);
     }
     else
     {
         ChipLogError(SecureChannel, "CASE failed to match destination ID with local fabrics");
         ChipLogByteSpan(SecureChannel, parsedSigma1.destinationId);
 
-        // FindLocalNodeFromDestinationId returns CHIP_ERROR_KEY_NOT_FOUND if validation of DestinationID fails, which will trigger
-        // status Report with ProtocolCode = NoSharedTrustRoots.
-        nextStep = Step::kSendStatusReport;
+        // FindLocalNodeFromDestinationId returns CHIP_ERROR_KEY_NOT_FOUND if Sigma1's DestinationId does not match any
+        // candidateDestinationId, this will trigger a status Report with ProtocolCode = NoSharedTrustRoots.
 
-        return err;
+        // Returning a CHIP_ERROR variant that will trigger a corresponding Status Report.
+        return NextStep::Create<CHIP_ERROR>(err);
     }
 
-    return CHIP_NO_ERROR;
+    return NextStep::Create<CHIP_ERROR>(CHIP_NO_ERROR);
 }
 
 CHIP_ERROR CASESession::PrepareSigma2Resume(EncodeSigma2ResumeInputs & outSigma2ResData)

src/protocols/secure_channel/CASESession.h

@@ -197,8 +197,11 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
         kNone,
         kSendSigma2,
         kSendSigma2Resume,
-        kSendStatusReport
     };
+    // Making NextStep a Variant allows HandleSigma() to return either a Step value (indicating
+    // the next Sigma step to send) or a CHIP_ERROR (indicating a failure that will trigger
+    // a Status Report).
+    using NextStep = Variant<Step, CHIP_ERROR>;
     // This struct  only serves as a base struct for EncodedSigma1Inputs and ParsedSigma1
     struct Sigma1Param
     {
@@ -328,7 +331,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
 
     CHIP_ERROR SendSigma1();
     CHIP_ERROR HandleSigma1_and_SendSigma2(System::PacketBufferHandle && msg);
-    CHIP_ERROR HandleSigma1(System::PacketBufferHandle && ms, Step & nextStep);
+    NextStep HandleSigma1(System::PacketBufferHandle && msg);
     CHIP_ERROR TryResumeSession(SessionResumptionStorage::ConstResumptionIdView resumptionId, ByteSpan resume1MIC,
                                 ByteSpan initiatorRandom);