Skip to content

Commit

Permalink
Fix and refactor ReadSingleClusterData (project-chip#13468)
Browse files Browse the repository at this point in the history
* Fix and refactor ReadSingleClusterData

PR project-chip#12660 seems to have refactored ReadSingleClusterData in such a way
that the access control check may be skipped. (Possibly due to merge?)

Fix this by refactoring the function to a sensible flow of checks.

Progress towards project-chip#10239

* Apply review comment.

Co-authored-by: Boris Zbarsky <[email protected]>
  • Loading branch information
2 people authored and selissia committed Jan 28, 2022
1 parent de4b724 commit c1a0418
Showing 1 changed file with 31 additions and 29 deletions.
60 changes: 31 additions & 29 deletions src/app/util/ember-compatibility-functions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -369,35 +369,32 @@ CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, c
" (expanded=%d)",
ChipLogValueMEI(aPath.mClusterId), aPath.mEndpointId, ChipLogValueMEI(aPath.mAttributeId), aPath.mExpanded);

// Check attribute existence. This includes attributes with registered metadata, but also specially handled
// mandatory global attributes (which just check for cluster on endpoint).

EmberAfCluster * attributeCluster = nullptr;
EmberAfAttributeMetadata * attributeMetadata = nullptr;

if (aPath.mAttributeId == Clusters::Globals::Attributes::AttributeList::Id)
{
// This is not in our attribute metadata, so we just check for this
// endpoint+cluster existing.
EmberAfCluster * cluster = emberAfFindCluster(aPath.mEndpointId, aPath.mClusterId, CLUSTER_MASK_SERVER);
if (cluster)
{
AttributeListReader reader(cluster);
bool ignored; // Our reader always tries to encode
return ReadViaAccessInterface(aSubjectDescriptor.fabricIndex, aPath, aAttributeReports, apEncoderState, &reader,
&ignored);
}

// else to save codesize just fall through and do the metadata search
// (which we know will fail and error out);
attributeCluster = emberAfFindCluster(aPath.mEndpointId, aPath.mClusterId, CLUSTER_MASK_SERVER);
}
else
{
attributeMetadata =
emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId, CLUSTER_MASK_SERVER, 0);
}

EmberAfAttributeMetadata * attributeMetadata =
emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId, CLUSTER_MASK_SERVER, 0);

if (attributeMetadata == nullptr)
if (attributeCluster == nullptr && attributeMetadata == nullptr)
{
AttributeReportIB::Builder & attributeReport = aAttributeReports.CreateAttributeReport();
ReturnErrorOnFailure(aAttributeReports.GetError());

// This path is not actually supported.
return SendFailureStatus(aPath, attributeReport, Protocols::InteractionModel::Status::UnsupportedAttribute, nullptr);
}

// Check access control. A failed check will disallow the operation, and may or may not generate an attribute report
// depending on whether the path was expanded.

{
Access::RequestPath requestPath{ .cluster = aPath.mClusterId, .endpoint = aPath.mEndpointId };
Access::Privilege requestPrivilege = Access::Privilege::kView; // TODO: get actual request privilege
Expand All @@ -419,27 +416,32 @@ CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, c
}
}

// Value encoder will encode the whole AttributeReport, including the path, value and the version.
// The AttributeValueEncoder may encode more than one AttributeReportIB for the list chunking feature.
if (auto * attrOverride = findAttributeAccessOverride(aPath.mEndpointId, aPath.mClusterId))
{
bool triedEncode;
ReturnErrorOnFailure(ReadViaAccessInterface(aSubjectDescriptor.fabricIndex, aPath, aAttributeReports, apEncoderState,
attrOverride, &triedEncode));
// Read attribute using attribute override, if appropriate. This includes registered overrides, but also
// specially handled mandatory global attributes (which use unregistered overrides).

if (triedEncode)
{
// Special handling for mandatory global attributes: these are always for attribute list, using a special
// reader (which can be lightweight constructed even from nullptr).
AttributeListReader reader(attributeCluster);
AttributeAccessInterface * attributeOverride =
(attributeCluster != nullptr) ? &reader : findAttributeAccessOverride(aPath.mEndpointId, aPath.mClusterId);
if (attributeOverride)
{
return CHIP_NO_ERROR;
bool triedEncode;
ReturnErrorOnFailure(ReadViaAccessInterface(aSubjectDescriptor.fabricIndex, aPath, aAttributeReports, apEncoderState,
attributeOverride, &triedEncode));
ReturnErrorCodeIf(triedEncode, CHIP_NO_ERROR);
}
}

// Read attribute using Ember, if it doesn't have an override.

AttributeReportIB::Builder & attributeReport = aAttributeReports.CreateAttributeReport();
ReturnErrorOnFailure(aAttributeReports.GetError());

TLV::TLVWriter backup;
attributeReport.Checkpoint(backup);

// We have verified that the attribute exists.
AttributeDataIB::Builder & attributeDataIBBuilder = attributeReport.CreateAttributeData();
ReturnErrorOnFailure(attributeDataIBBuilder.GetError());

Expand Down

0 comments on commit c1a0418

Please sign in to comment.