Skip to content

Commit

Permalink
Improve Invalid Action return in CommandHandler (project-chip#21868)
Browse files Browse the repository at this point in the history 
* Improve Invalid Action return in CommandHandler

Improve the error reporting from ProcessGroupCommandDataIB and ProcessCommandDataIB: per spec some of their failures should also be InvalidAction.

* address comments
  • Loading branch information
@yunhanw-google @isiu-apple
yunhanw-google authored and isiu-apple committed Sep 16, 2022
1 parent e438213 commit 77144c7
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 27 deletions.
53 changes: 28 additions & 25 deletions src/app/CommandHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,18 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa
VerifyOrReturnError(TLV::AnonymousTag() == invokeRequestsReader.GetTag(), Status::InvalidAction);
CommandDataIB::Parser commandData;
VerifyOrReturnError(commandData.Init(invokeRequestsReader) == CHIP_NO_ERROR, Status::InvalidAction);

Status status = Status::Success;
if (mExchangeCtx->IsGroupExchangeContext())
{
VerifyOrReturnError(ProcessGroupCommandDataIB(commandData) == CHIP_NO_ERROR, Status::Failure);
status = ProcessGroupCommandDataIB(commandData);
}
else
{
VerifyOrReturnError(ProcessCommandDataIB(commandData) == CHIP_NO_ERROR, Status::Failure);
status = ProcessCommandDataIB(commandData);
}
if (status != Status::Success)
{
return status;
}
}

Expand Down Expand Up @@ -238,7 +242,7 @@ constexpr uint8_t sNoFields[] = {
};
} // anonymous namespace

CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement)
Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement)
{
CHIP_ERROR err = CHIP_NO_ERROR;
CommandPathIB::Parser commandPath;
Expand All @@ -248,16 +252,16 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
// NOTE: errors may occur before the concrete command path is even fully decoded.

err = aCommandElement.GetPath(&commandPath);
SuccessOrExit(err);
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);

err = commandPath.GetClusterId(&concretePath.mClusterId);
SuccessOrExit(err);
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);

err = commandPath.GetCommandId(&concretePath.mCommandId);
SuccessOrExit(err);
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);

err = commandPath.GetEndpointId(&concretePath.mEndpointId);
SuccessOrExit(err);
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);

{
Status commandExists = mpCallback->CommandExists(concretePath);
Expand All @@ -266,7 +270,7 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x",
ChipLogValueMEI(concretePath.mCommandId), ChipLogValueMEI(concretePath.mClusterId),
concretePath.mEndpointId);
return AddStatus(concretePath, commandExists);
return AddStatus(concretePath, commandExists) != CHIP_NO_ERROR ? Status::Failure : Status::Success;
}
}

Expand All @@ -281,18 +285,18 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
{
if (err != CHIP_ERROR_ACCESS_DENIED)
{
return AddStatus(concretePath, Status::Failure);
return AddStatus(concretePath, Status::Failure) != CHIP_NO_ERROR ? Status::Failure : Status::Success;
}
// TODO: when wildcard invokes are supported, handle them to discard rather than fail with status
return AddStatus(concretePath, Status::UnsupportedAccess);
return AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR ? Status::Failure : Status::Success;
}
}

if (CommandNeedsTimedInvoke(concretePath.mClusterId, concretePath.mCommandId) && !IsTimedInvoke())
{
// TODO: when wildcard invokes are supported, discard a
// wildcard-expanded path instead of returning a status.
return AddStatus(concretePath, Status::NeedsTimedInteraction);
return AddStatus(concretePath, Status::NeedsTimedInteraction) != CHIP_NO_ERROR ? Status::Failure : Status::Success;
}

if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId))
Expand All @@ -303,7 +307,7 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
{
// TODO: when wildcard invokes are supported, discard a
// wildcard-expanded path instead of returning a status.
return AddStatus(concretePath, Protocols::InteractionModel::Status::UnsupportedAccess);
return AddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR ? Status::Failure : Status::Success;
}
}

Expand All @@ -328,15 +332,15 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
exit:
if (err != CHIP_NO_ERROR)
{
return AddStatus(concretePath, Status::InvalidCommand);
return AddStatus(concretePath, Status::InvalidCommand) != CHIP_NO_ERROR ? Status::Failure : Status::Success;
}

// We have handled the error status above and put the error status in response, now return success status so we can process
// other commands in the invoke request.
return CHIP_NO_ERROR;
return Status::Success;
}

CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement)
Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement)
{
CHIP_ERROR err = CHIP_NO_ERROR;
CommandPathIB::Parser commandPath;
Expand All @@ -351,13 +355,13 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
Credentials::GroupDataProvider::EndpointIterator * iterator;

err = aCommandElement.GetPath(&commandPath);
SuccessOrExit(err);
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);

err = commandPath.GetClusterId(&clusterId);
SuccessOrExit(err);
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);

err = commandPath.GetCommandId(&commandId);
SuccessOrExit(err);
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);

groupId = mExchangeCtx->GetSessionHandle()->AsIncomingGroupSession()->GetGroupId();
fabric = GetAccessingFabricIndex();
Expand All @@ -373,8 +377,9 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
ChipLogValueMEI(clusterId), ChipLogValueMEI(commandId));
commandDataReader.Init(sNoFields);
err = commandDataReader.Next();
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
}
SuccessOrExit(err);
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::Failure);

// Per spec, we do the "is this a timed command?" check for every path, but
// since all paths that fail it just get silently discarded we can do it
Expand All @@ -384,15 +389,15 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
if (CommandNeedsTimedInvoke(clusterId, commandId))
{
// Group commands are never timed.
ExitNow();
return Status::Success;
}

// No check for `CommandIsFabricScoped` unlike in `ProcessCommandDataIB()` since group commands
// always have an accessing fabric, by definition.

// Find which endpoints can process the command, and dispatch to them.
iterator = groupDataProvider->IterateEndpoints(fabric);
VerifyOrExit(iterator != nullptr, err = CHIP_ERROR_NO_MEMORY);
VerifyOrReturnError(iterator != nullptr, Status::Failure);

while (iterator->Next(mapping))
{
Expand Down Expand Up @@ -444,9 +449,7 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
}
}
iterator->Release();

exit:
return CHIP_NO_ERROR;
return Status::Success;
}

CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus)
Expand Down
4 changes: 2 additions & 2 deletions src/app/CommandHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,13 +350,13 @@ class CommandHandler : public Messaging::ExchangeDelegate
* ProcessCommandDataIB is only called when a unicast invoke command request is received
* It requires the endpointId in its command path to be able to dispatch the command
*/
CHIP_ERROR ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement);
Protocols::InteractionModel::Status ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement);

/**
* ProcessGroupCommandDataIB is only called when a group invoke command request is received
* It doesn't need the endpointId in it's command path since it uses the GroupId in message metadata to find it
*/
CHIP_ERROR ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement);
Protocols::InteractionModel::Status ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement);
CHIP_ERROR SendCommandResponse();
CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus);

Expand Down

0 comments on commit 77144c7

Please sign in to comment.