[cli] extend start command to support --connect-only option (#292)

With '--connect-only' option, only establish secure session without attempting to petition to become active commissioner.
openthread · Aug 5, 2024 · c3a7fca · c3a7fca
1 parent 3016ab0
commit c3a7fca
Show file tree

Hide file tree

Showing 10 changed files with 130 additions and 11 deletions.
diff --git a/include/commissioner/commissioner.hpp b/include/commissioner/commissioner.hpp
@@ -54,6 +54,7 @@ namespace commissioner {
 enum class State : uint8_t
 {
     kDisabled = 0,
+    kConnected,
     kPetitioning,
     kActive,
 };

diff --git a/src/app/cli/README.md b/src/app/cli/README.md
@@ -316,10 +316,10 @@ d28443a10126a058aea40366546872656164047818323031392d31322d30315431303a33383a3439
 
 ### Start
 
-To start petitioning as the active Commissioner of a Thread network:
+To start establishing secure session, and petitioning as the active Commissioner of a Thread network by default:
 
 ```shell
-### Petition with Border Agent at [::]:49191
+### Establish secure session and Petition with Border Agent at [::]:49191
 > start :: 49191
 [done]
 >
@@ -331,6 +331,13 @@ To start petitioning as the active Commissioner of a Thread network:
 [done]
 ```
 
+```shell
+### Establish secure session with Border Agent at [192.168.8.2]:49191 without attempt to petition as active commissioner
+> start 192.168.8.2 49191 --connect-only
+[done]
+>
+```
+
 ### Active
 
 Upon success, the Commissioner periodically sends keep alive messages in background. The Commissioner now is in the active state:

diff --git a/src/app/cli/interpreter.cpp b/src/app/cli/interpreter.cpp
@@ -207,7 +207,7 @@ const std::map<std::string, std::string> &Interpreter::mUsageMap = *new std::map
                "config set admincode <9-digits-thread-administrator-passcode>\n"
                "config get pskc\n"
                "config set pskc <pskc-hex-string>"},
-    {"start", "start <border-agent-addr> <border-agent-port>\n"
+    {"start", "start <border-agent-addr> <border-agent-port> [--connect-only]\n"
               "start [ --nwk <network-alias-list | --dom <domain-alias>]"},
     {"stop", "stop\n"
              "stop [ --nwk <network-alias-list | --dom <domain-alias>]"},
@@ -1007,13 +1007,27 @@ Interpreter::Value Interpreter::ProcessStartJob(CommissionerAppPtr &aCommissione
     Error       error;
     uint16_t    port;
     std::string existingCommissionerId;
+    bool        connectOnly = false;
 
     VerifyOrExit(aExpr.size() >= 3, error = ERROR_INVALID_ARGS(SYNTAX_FEW_ARGS));
     SuccessOrExit(error = ParseInteger(port, aExpr[2]));
-    SuccessOrExit(error = aCommissioner->Start(existingCommissionerId, aExpr[1], port));
+
+    {
+        auto it = std::find(mContext.mCommandKeys.begin(), mContext.mCommandKeys.end(), "--connect-only");
+
+        if (it != mContext.mCommandKeys.end())
+        {
+            connectOnly = true;
+            error       = aCommissioner->Connect(aExpr[1], port);
+        }
+        else
+        {
+            error = aCommissioner->Start(existingCommissionerId, aExpr[1], port);
+        }
+    }
 
 exit:
-    if (!existingCommissionerId.empty())
+    if (!connectOnly && !existingCommissionerId.empty())
     {
         error = Error{error.GetCode(), "there is an existing active commissioner: " + existingCommissionerId};
     }

diff --git a/src/app/cli/interpreter_test.cpp b/src/app/cli/interpreter_test.cpp
@@ -949,6 +949,64 @@ TEST_F(InterpreterTestSuite, PC_StartLegacySyntaxSuccess)
     EXPECT_TRUE(value.HasNoError());
 }
 
+TEST_F(InterpreterTestSuite, PC_StartLegacyConnectSyntaxErrorFails)
+{
+    TestContext ctx;
+    InitContext(ctx);
+    ASSERT_EQ(
+        ctx.mRegistry->Add(BorderAgent{"127.0.0.1", 20001, ByteArray{}, "1.1", BorderAgent::State{0, 0, 0, 0, 0},
+                                       "net1", 1, "", "", Timestamp{0, 0, 0}, 0, "", ByteArray{}, "domain1", 0, 0, "",
+                                       0, 0x1F | BorderAgent::kDomainNameBit | BorderAgent::kExtendedPanIdBit}),
+        RegistryStatus::kSuccess);
+    ASSERT_EQ(
+        ctx.mRegistry->Add(BorderAgent{"127.0.0.2", 20001, ByteArray{}, "1.1", BorderAgent::State{0, 0, 0, 0, 0},
+                                       "net2", 2, "", "", Timestamp{0, 0, 0}, 0, "", ByteArray{}, "domain1", 0, 0, "",
+                                       0, 0x1F | BorderAgent::kDomainNameBit | BorderAgent::kExtendedPanIdBit}),
+        RegistryStatus::kSuccess);
+
+    BorderRouter br;
+    br.mNetworkId = NetworkId{0};
+    ASSERT_EQ(ctx.mRegistry->SetCurrentNetwork(br), RegistryStatus::kSuccess);
+
+    EXPECT_CALL(*ctx.mDefaultCommissionerObject, Connect(_, _))
+        .Times(1)
+        .WillRepeatedly(Return(Error{ErrorCode::kAborted, "Test failure"}));
+
+    Interpreter::Expression expr;
+    Interpreter::Value      value;
+    expr  = ctx.mInterpreter.ParseExpression("start 127.0.0.1 20001 --connect-only");
+    value = ctx.mInterpreter.Eval(expr);
+    EXPECT_FALSE(value.HasNoError());
+}
+
+TEST_F(InterpreterTestSuite, PC_StartLegacyConnectSyntaxSuccess)
+{
+    TestContext ctx;
+    InitContext(ctx);
+    ASSERT_EQ(
+        ctx.mRegistry->Add(BorderAgent{"127.0.0.1", 20001, ByteArray{}, "1.1", BorderAgent::State{0, 0, 0, 0, 0},
+                                       "net1", 1, "", "", Timestamp{0, 0, 0}, 0, "", ByteArray{}, "domain1", 0, 0, "",
+                                       0, 0x1F | BorderAgent::kDomainNameBit | BorderAgent::kExtendedPanIdBit}),
+        RegistryStatus::kSuccess);
+    ASSERT_EQ(
+        ctx.mRegistry->Add(BorderAgent{"127.0.0.2", 20001, ByteArray{}, "1.1", BorderAgent::State{0, 0, 0, 0, 0},
+                                       "net2", 2, "", "", Timestamp{0, 0, 0}, 0, "", ByteArray{}, "domain1", 0, 0, "",
+                                       0, 0x1F | BorderAgent::kDomainNameBit | BorderAgent::kExtendedPanIdBit}),
+        RegistryStatus::kSuccess);
+
+    BorderRouter br;
+    br.mNetworkId = 0;
+    ASSERT_EQ(ctx.mRegistry->SetCurrentNetwork(br), RegistryStatus::kSuccess);
+
+    EXPECT_CALL(*ctx.mDefaultCommissionerObject, Connect(_, _)).Times(1).WillRepeatedly(Return(Error{}));
+
+    Interpreter::Expression expr;
+    Interpreter::Value      value;
+    expr  = ctx.mInterpreter.ParseExpression("start 127.0.0.1 20001 --connect-only");
+    value = ctx.mInterpreter.Eval(expr);
+    EXPECT_TRUE(value.HasNoError());
+}
+
 TEST_F(InterpreterTestSuite, PC_StartLegacySyntaxErrorFails)
 {
     TestContext ctx;

diff --git a/src/app/commissioner_app.cpp b/src/app/commissioner_app.cpp
@@ -97,6 +97,20 @@ Error CommissionerApp::Init(const Config &aConfig)
     return error;
 }
 
+Error CommissionerApp::Connect(const std::string &aBorderAgentAddr, uint16_t aBorderAgentPort)
+{
+    Error error;
+
+    SuccessOrExit(error = mCommissioner->Connect(aBorderAgentAddr, aBorderAgentPort));
+
+exit:
+    if (error != ErrorCode::kNone)
+    {
+        Stop();
+    }
+    return error;
+}
+
 Error CommissionerApp::Start(std::string       &aExistingCommissionerId,
                              const std::string &aBorderAgentAddr,
                              uint16_t           aBorderAgentPort)

diff --git a/src/app/commissioner_app.hpp b/src/app/commissioner_app.hpp
@@ -126,6 +126,8 @@ class CommissionerApp : public CommissionerHandler
 
     MOCKABLE void OnDatasetChanged() override;
 
+    MOCKABLE Error Connect(const std::string &aBorderAgentAddr, uint16_t aBorderAgentPort);
+
     MOCKABLE Error Start(std::string       &aExistingCommissionerId,
                          const std::string &aBorderAgentAddr,
                          uint16_t           aBorderAgentPort);

diff --git a/src/app/commissioner_app_dummy.cpp b/src/app/commissioner_app_dummy.cpp
@@ -109,6 +109,13 @@ Error CommissionerApp::Start(std::string       &aExistingCommissionerId,
     return Error{};
 }
 
+Error CommissionerApp::Connect(const std::string &aBorderAgentAddr, uint16_t aBorderAgentPort)
+{
+    UNUSED(aBorderAgentAddr);
+    UNUSED(aBorderAgentPort);
+    return Error{};
+}
+
 void CommissionerApp::Stop()
 {
 }

diff --git a/src/app/commissioner_app_mock.hpp b/src/app/commissioner_app_mock.hpp
@@ -72,6 +72,7 @@ class CommissionerAppMock : public ::ot::commissioner::CommissionerApp
     MOCK_METHOD(void, OnEnergyReport, (const std::string &, const ChannelMask &, const ByteArray &), (override));
     MOCK_METHOD(void, OnDatasetChanged, (), (override));
 
+    MOCK_METHOD(Error, Connect, (const std::string &, uint16_t));
     MOCK_METHOD(Error, Start, (std::string &, const std::string &, uint16_t));
     MOCK_METHOD(void, Stop, ());
     MOCK_METHOD(void, CancelRequests, ());

diff --git a/src/library/commissioner_impl.cpp b/src/library/commissioner_impl.cpp
@@ -258,14 +258,15 @@ void CommissionerImpl::Petition(PetitionHandler aHandler, const std::string &aAd
 {
     Error error;
 
-    auto onConnected = [this, aHandler](Error aError) {
+    auto onConnected = [&, aHandler](Error aError) {
         if (aError != ErrorCode::kNone)
         {
             aHandler(nullptr, aError);
         }
         else
         {
             LOG_DEBUG(LOG_REGION_MESHCOP, "DTLS connection to border agent succeed");
+            this->mState = State::kConnected;
             SendPetition(aHandler);
         }
     };
@@ -308,7 +309,13 @@ void CommissionerImpl::Resign(ErrorHandler aHandler)
 
 void CommissionerImpl::Connect(ErrorHandler aHandler, const std::string &aAddr, uint16_t aPort)
 {
-    auto onConnected = [aHandler](const DtlsSession &, Error aError) { aHandler(aError); };
+    auto onConnected = [&, aHandler](const DtlsSession &, Error aError) {
+        if (aError == ErrorCode::kNone)
+        {
+            this->mState = State::kConnected;
+        }
+        aHandler(aError);
+    };
     mBrClient.Connect(onConnected, aAddr, aPort);
 }
 
@@ -480,7 +487,8 @@ void CommissionerImpl::GetRawActiveDataset(Handler<ByteArray> aHandler, uint16_t
         }
     };
 
-    VerifyOrExit(IsActive(), error = ERROR_INVALID_STATE("the commissioner is not active"));
+    VerifyOrExit(IsActiveOrConnected(),
+                 error = ERROR_INVALID_STATE("the commissioner is not active or secure session is not connected"));
     SuccessOrExit(error = request.SetUriPath(uri::kMgmtActiveGet));
     if (!datasetList.empty())
     {
@@ -586,7 +594,8 @@ void CommissionerImpl::GetPendingDataset(Handler<PendingOperationalDataset> aHan
         }
     };
 
-    VerifyOrExit(IsActive(), error = ERROR_INVALID_STATE("the commissioner is not active"));
+    VerifyOrExit(IsActiveOrConnected(),
+                 error = ERROR_INVALID_STATE("the commissioner is not active or secure session is not connected"));
     SuccessOrExit(error = request.SetUriPath(uri::kMgmtPendingGet));
     if (!datasetList.empty())
     {
@@ -600,7 +609,7 @@ void CommissionerImpl::GetPendingDataset(Handler<PendingOperationalDataset> aHan
     }
 #endif
 
-    mProxyClient.SendRequest(request, onResponse, kLeaderAloc16, kDefaultMmPort);
+    mBrClient.SendRequest(request, onResponse);
 
     LOG_DEBUG(LOG_REGION_MGMT, "sent MGMT_PENDING_GET.req");
 
@@ -1226,7 +1235,8 @@ void CommissionerImpl::SendPetition(PetitionHandler aHandler)
         aHandler(existingCommissionerId.empty() ? nullptr : &existingCommissionerId, error);
     };
 
-    VerifyOrExit(mState == State::kDisabled, error = ERROR_INVALID_STATE("the commissioner is petitioning or active"));
+    VerifyOrExit(mState == State::kConnected,
+                 error = ERROR_INVALID_STATE("the commissioner is not started, petitioning, or already active"));
     SuccessOrExit(error = request.SetUriPath(uri::kPetitioning));
     SuccessOrExit(error = AppendTlv(request, {tlv::Type::kCommissionerId, mConfig.mId}));
 

diff --git a/src/library/commissioner_impl.hpp b/src/library/commissioner_impl.hpp
@@ -251,6 +251,11 @@ class CommissionerImpl : public Commissioner
 
     void HandleJoinerSessionTimer(Timer &aTimer);
 
+    bool IsActiveOrConnected() const
+    {
+        return (mState == State::kActive || mState == State::kConnected);
+    }
+
 private:
     State    mState;
     uint16_t mSessionId; ///< The Commissioner Session ID.