From 8d7bcf34547bc399d4a861967df740219623f1e1 Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Mon, 13 Dec 2021 13:48:36 -0500 Subject: [PATCH 1/4] Hookup SubjectDescriptor in CommandHandler Get the subject descriptor (from the exchange context session handle) in the command handler so an access control check can be performed before executing a command. --- src/app/CommandHandler.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 0f37b30a4c33b9..dace440cbe1e70 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -254,7 +254,12 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand VerifyOrExit(mpCallback->CommandExists(concretePath), err = CHIP_ERROR_INVALID_PROFILE_ID); { - Access::SubjectDescriptor subjectDescriptor; // TODO: get actual subject descriptor + if (mpExchangeCtx == nullptr || mpExchangeCtx->HasSessionHandle() == false) + { + // No exchange context or session handle means no subject descriptor so cannot check access control + return AddStatus(concretePath, Protocols::InteractionModel::Status::Failure); + } + Access::SubjectDescriptor subjectDescriptor = mpExchangeCtx->GetSessionHandle().GetSubjectDescriptor(); Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId }; Access::Privilege requestPrivilege = Access::Privilege::kOperate; // TODO: get actual request privilege err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); From 69a47044eb8c890a6ee3bd71edfda19633336657 Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Mon, 13 Dec 2021 14:09:24 -0500 Subject: [PATCH 2/4] Non-functional syntax change --- src/app/CommandHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index dace440cbe1e70..b4724884acf26b 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -254,7 +254,7 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand VerifyOrExit(mpCallback->CommandExists(concretePath), err = CHIP_ERROR_INVALID_PROFILE_ID); { - if (mpExchangeCtx == nullptr || mpExchangeCtx->HasSessionHandle() == false) + if (mpExchangeCtx == nullptr || !mpExchangeCtx->HasSessionHandle()) { // No exchange context or session handle means no subject descriptor so cannot check access control return AddStatus(concretePath, Protocols::InteractionModel::Status::Failure); From 718acac5132a9d06e8eb605faa4c3793475b85a2 Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Fri, 7 Jan 2022 15:12:21 -0500 Subject: [PATCH 3/4] Fix merge conflict --- src/app/CommandHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 91cb644361794e..a4a64c8079f73e 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -259,7 +259,7 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand // No exchange context or session handle means no subject descriptor so cannot check access control return AddStatus(concretePath, Protocols::InteractionModel::Status::Failure); } - Access::SubjectDescriptor subjectDescriptor = mpExchangeCtx->GetSessionHandle().GetSubjectDescriptor(); + Access::SubjectDescriptor subjectDescriptor = mpExchangeCtx->GetSessionHandle()->GetSubjectDescriptor(); Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId }; Access::Privilege requestPrivilege = Access::Privilege::kOperate; // TODO: get actual request privilege err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); From ebf52027c832f077381acd6d50a0838b66e17d62 Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Thu, 13 Jan 2022 16:14:56 -0500 Subject: [PATCH 4/4] Improve unit tests for command interaction Unit tests did not always provide an exchange context for tests of command interaction. This required some special handling for test cases in actual command handling code, and blocked cases (e.g. access control) which require an exchange context. So provide an exchange context even in test cases. --- src/app/CommandHandler.cpp | 12 ++++-------- src/app/tests/TestCommandInteraction.cpp | 23 ++++++++++++----------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index ef3c19db5ee12c..4a2b77aeb983cd 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -98,6 +98,8 @@ CHIP_ERROR CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && pa ReturnErrorOnFailure(invokeRequestMessage.GetTimedRequest(&mTimedRequest)); ReturnErrorOnFailure(invokeRequestMessage.GetInvokeRequests(&invokeRequests)); + VerifyOrReturnError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE); + if (mTimedRequest != isTimedInvoke) { // The message thinks it should be part of a timed interaction but it's @@ -105,9 +107,7 @@ CHIP_ERROR CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && pa err = StatusResponse::Send(Protocols::InteractionModel::Status::UnsupportedAccess, mpExchangeCtx, /* aExpectResponse = */ false); - // Some unit tests call this function in an abnormal state when we don't - // even have an exchange. - if (err != CHIP_NO_ERROR && mpExchangeCtx) + if (err != CHIP_NO_ERROR) { // We have to manually close the exchange, because we called // WillSendMessage already. @@ -252,13 +252,9 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand SuccessOrExit(err); VerifyOrExit(mpCallback->CommandExists(concretePath), err = CHIP_ERROR_INVALID_PROFILE_ID); + VerifyOrExit(mpExchangeCtx != nullptr && mpExchangeCtx->HasSessionHandle(), err = CHIP_ERROR_INCORRECT_STATE); { - if (mpExchangeCtx == nullptr || !mpExchangeCtx->HasSessionHandle()) - { - // No exchange context or session handle means no subject descriptor so cannot check access control - return AddStatus(concretePath, Protocols::InteractionModel::Status::Failure); - } Access::SubjectDescriptor subjectDescriptor = mpExchangeCtx->GetSessionHandle()->GetSubjectDescriptor(); Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId }; Access::Privilege requestPrivilege = Access::Privilege::kOperate; // TODO: get actual request privilege diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 2af13c597ce36e..67fcd195fe2c8b 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -542,12 +542,18 @@ void TestCommandInteraction::TestCommandHandlerWithSendSimpleStatusCode(nlTestSu void TestCommandInteraction::TestCommandHandlerWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext) { - CHIP_ERROR err = CHIP_NO_ERROR; + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; app::CommandHandler commandHandler(&mockCommandHandlerDelegate); System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); + TestExchangeDelegate delegate; + commandHandler.mpExchangeCtx = ctx.NewExchangeToAlice(&delegate); + GenerateInvokeRequest(apSuite, apContext, commandDatabuf, true /*aNeedCommandData*/, /* aIsTimedRequest = */ false); err = commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), false); + + ChipLogDetail(DataManagement, "###################################### %s", err.AsString()); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); } @@ -570,6 +576,7 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistComman void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext) { + TestContext & ctx = *static_cast(apContext); bool allBooleans[] = { true, false }; for (auto messageIsTimed : allBooleans) { @@ -579,19 +586,13 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(n app::CommandHandler commandHandler(&mockCommandHandlerDelegate); System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); - chip::isCommandDispatched = false; + TestExchangeDelegate delegate; + commandHandler.mpExchangeCtx = ctx.NewExchangeToAlice(&delegate); + chip::isCommandDispatched = false; GenerateInvokeRequest(apSuite, apContext, commandDatabuf, false /*aNeedCommandData*/, messageIsTimed); - err = commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), transactionIsTimed); - if (messageIsTimed == transactionIsTimed) - { - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && chip::isCommandDispatched); - } - else - { - NL_TEST_ASSERT(apSuite, err != CHIP_NO_ERROR && !chip::isCommandDispatched); - } + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && chip::isCommandDispatched == (messageIsTimed == transactionIsTimed)); } } }