From c3a7fca3125bd880223e03212844245f0d8baaa9 Mon Sep 17 00:00:00 2001 From: Rongli Sun Date: Tue, 6 Aug 2024 06:15:53 +0800 Subject: [PATCH] [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. --- include/commissioner/commissioner.hpp | 1 + src/app/cli/README.md | 11 ++++- src/app/cli/interpreter.cpp | 20 +++++++-- src/app/cli/interpreter_test.cpp | 58 +++++++++++++++++++++++++++ src/app/commissioner_app.cpp | 14 +++++++ src/app/commissioner_app.hpp | 2 + src/app/commissioner_app_dummy.cpp | 7 ++++ src/app/commissioner_app_mock.hpp | 1 + src/library/commissioner_impl.cpp | 22 +++++++--- src/library/commissioner_impl.hpp | 5 +++ 10 files changed, 130 insertions(+), 11 deletions(-) diff --git a/include/commissioner/commissioner.hpp b/include/commissioner/commissioner.hpp index 2a128619..41f87110 100644 --- 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 index e7170bdf..f843ba4e 100644 --- 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 index e0549f81..aa002a2e 100644 --- a/src/app/cli/interpreter.cpp +++ b/src/app/cli/interpreter.cpp @@ -207,7 +207,7 @@ const std::map &Interpreter::mUsageMap = *new std::map "config set admincode <9-digits-thread-administrator-passcode>\n" "config get pskc\n" "config set pskc "}, - {"start", "start \n" + {"start", "start [--connect-only]\n" "start [ --nwk ]"}, {"stop", "stop\n" "stop [ --nwk ]"}, @@ -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 index 39602721..e0e3db38 100644 --- 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 index fbd612e3..00731757 100644 --- 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 index fe08d5df..1b39d089 100644 --- 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 index 248ab00a..07c2d3f8 100644 --- 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 index 5b31862f..2a7303b2 100644 --- 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 index 94404161..baa32cb1 100644 --- a/src/library/commissioner_impl.cpp +++ b/src/library/commissioner_impl.cpp @@ -258,7 +258,7 @@ 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); @@ -266,6 +266,7 @@ void CommissionerImpl::Petition(PetitionHandler aHandler, const std::string &aAd 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 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 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 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 index 752095c3..be58fe8a 100644 --- 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.