Skip to content

Commit

Permalink
Fix Darwin server cluster checks for wildcard read to check received …
Browse files Browse the repository at this point in the history 
…values. (project-chip#33737)

This caught a bug where the synthesized descriptor clusters were not
spec-compliant in the sense of requiring admin privileges to read their
attributes, and the synthesized global attributes likewise required admin
privileges.  The changes to MTRServerAccessControl fix that.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple authored Jun 5, 2024
1 parent d3cac0c commit d4d9054
Show file tree
Hide file tree
Showing 2 changed files with 182 additions and 58 deletions.
49 changes: 34 additions & 15 deletions src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAccessControl.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#import <Matter/MTRAccessGrant.h>
#import <Matter/MTRBaseDevice.h> // for MTRClusterPath
#import <Matter/MTRClusterConstants.h> // For MTRClusterIDTypeDescriptorID
#import <Matter/MTRDeviceControllerFactory.h>

#import "MTRDeviceControllerFactory_Internal.h"
Expand All @@ -29,6 +30,8 @@
#include <access/SubjectDescriptor.h>
#include <app/InteractionModelEngine.h>
#include <lib/core/CHIPError.h>
#include <lib/core/CHIPVendorIdentifiers.hpp>
#include <lib/core/DataModelTypes.h>
#include <lib/core/Global.h>
#include <lib/core/NodeId.h>

Expand Down Expand Up @@ -135,36 +138,52 @@ bool GrantSubjectMatchesDescriptor(MTRAccessGrant * grant, const SubjectDescript

} // anonymous namespace

chip::Access::Privilege MatterGetAccessPrivilegeForReadEvent(ClusterId cluster, EventId event)
Privilege MatterGetAccessPrivilegeForReadEvent(ClusterId cluster, EventId event)
{
// We don't support any event bits yet.
return chip::Access::Privilege::kAdminister;
return Privilege::kAdminister;
}

chip::Access::Privilege MatterGetAccessPrivilegeForInvokeCommand(ClusterId cluster, CommandId command)
Privilege MatterGetAccessPrivilegeForInvokeCommand(ClusterId cluster, CommandId command)
{
// For now we only have OTA, which uses Operate.
return chip::Access::Privilege::kOperate;
return Privilege::kOperate;
}

chip::Access::Privilege MatterGetAccessPrivilegeForReadAttribute(ClusterId cluster, AttributeId attribute)
Privilege MatterGetAccessPrivilegeForReadAttribute(ClusterId cluster, AttributeId attribute)
{
NSNumber * _Nullable neededPrivilege = [[MTRDeviceControllerFactory sharedInstance] neededReadPrivilegeForClusterID:@(cluster) attributeID:@(attribute)];
if (neededPrivilege == nil) {
// No privileges declared for this attribute on this cluster. Treat as
// "needs admin privileges", so we fail closed.
return chip::Access::Privilege::kAdminister;
// No privileges declared for this attribute on this cluster.

// In some cases, we know based on the spec what the answer is, and our
// API clients my not be able to provide explicit privileges for
// these cases because our API surface does not allow them to create
// custom attribute definitions for them.

// (1) Global attributes always require View privilege to read.
if (IsGlobalAttribute(attribute)) {
return Privilege::kView;
}

// (2) All standard descriptor attributes just require View privilege to read.
if (cluster == MTRClusterIDTypeDescriptorID && ExtractVendorFromMEI(attribute) == VendorId::Common) {
return Privilege::kView;
}

// Treat as "needs admin privileges", so we fail closed.
return Privilege::kAdminister;
}

switch (neededPrivilege.unsignedLongLongValue) {
case MTRAccessControlEntryPrivilegeView:
return chip::Access::Privilege::kView;
return Privilege::kView;
case MTRAccessControlEntryPrivilegeOperate:
return chip::Access::Privilege::kOperate;
return Privilege::kOperate;
case MTRAccessControlEntryPrivilegeManage:
return chip::Access::Privilege::kManage;
return Privilege::kManage;
case MTRAccessControlEntryPrivilegeAdminister:
return chip::Access::Privilege::kAdminister;
return Privilege::kAdminister;
case MTRAccessControlEntryPrivilegeProxyView:
// Just treat this as an unknown value; there is no value for this in privilege-storage.
FALLTHROUGH;
Expand All @@ -175,13 +194,13 @@ bool GrantSubjectMatchesDescriptor(MTRAccessGrant * grant, const SubjectDescript
// To be safe, treat unknown values as "needs admin privileges". That way the failure case
// disallows access that maybe should be allowed, instead of allowing access that maybe
// should be disallowed.
return chip::Access::Privilege::kAdminister;
return Privilege::kAdminister;
}

chip::Access::Privilege MatterGetAccessPrivilegeForWriteAttribute(ClusterId cluster, AttributeId attribute)
Privilege MatterGetAccessPrivilegeForWriteAttribute(ClusterId cluster, AttributeId attribute)
{
// We don't have any writable attributes yet, but default to Operate.
return chip::Access::Privilege::kOperate;
return Privilege::kOperate;
}

void InitializeServerAccessControl()
Expand Down
191 changes: 148 additions & 43 deletions src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1721,15 +1721,6 @@ - (void)testControllerServer
],
};

#if 0
__auto_type * listOfStructsValue2 = @{
MTRTypeKey : MTRArrayValueType,
MTRValueKey : @[
@{ MTRDataKey: structValue2, },
],
};
#endif

__auto_type responsePathFromRequestPath = ^(MTRAttributeRequestPath * path) {
return [MTRAttributePath attributePathWithEndpointID:path.endpoint clusterID:path.cluster attributeID:path.attribute];
};
Expand Down Expand Up @@ -2007,32 +1998,60 @@ - (void)testControllerServer

// Now do a wildcard read on the endpoint and check that this does the right
// thing (gets the right things from descriptor, gets both clusters, etc).
#if 0
// Unused bits ifdefed out until we doing more testing on the actual values
// we get back.
__auto_type globalAttributePath = ^(NSNumber * clusterID, MTRAttributeIDType attributeID) {
return [MTRAttributePath attributePathWithEndpointID:endpointId1 clusterID:clusterID attributeID:@(attributeID)];
};
__auto_type descriptorAttributePath = ^(MTRAttributeIDType attributeID) {
return [MTRAttributePath attributePathWithEndpointID:endpointId1 clusterID:@(MTRClusterIDTypeDescriptorID) attributeID:@(attributeID)];
};
__auto_type unsignedIntValue = ^(NSUInteger value) {
return @{
MTRTypeKey: MTRUnsignedIntegerValueType,
MTRValueKey: @(value),
MTRTypeKey : MTRUnsignedIntegerValueType,
MTRValueKey : @(value),
};
};
__auto_type arrayOfUnsignedIntegersValue = ^(NSArray<NSNumber *> * values) {
__auto_type * mutableArray = [[NSMutableArray alloc] init];
for (NSNumber * value in values) {
[mutableArray addObject:@{ MTRDataKey: @{
MTRTypeKey: MTRUnsignedIntegerValueType,
MTRValueKey: value,
}, }];
[mutableArray addObject:@{
MTRDataKey : @ {
MTRTypeKey : MTRUnsignedIntegerValueType,
MTRValueKey : value,
},
}];
}
return @{
MTRTypeKey: MTRArrayValueType,
MTRValueKey: [mutableArray copy],
};
MTRTypeKey : MTRArrayValueType,
MTRValueKey : [mutableArray copy],
};
};
#endif
__auto_type endpoint1DeviceTypeValue = @{
MTRTypeKey : MTRArrayValueType,
MTRValueKey : @[
@{
MTRDataKey : @ {
MTRTypeKey : MTRStructureValueType,
MTRValueKey : @[
@{
MTRContextTagKey : @(0),
MTRDataKey : @ {
MTRTypeKey : MTRUnsignedIntegerValueType,
MTRValueKey : deviceType1.deviceTypeID,
},
},
@{
MTRContextTagKey : @(1),
MTRDataKey : @ {
MTRTypeKey : MTRUnsignedIntegerValueType,
MTRValueKey : deviceType1.deviceTypeRevision,
},
},
],
}
},
],
};

XCTestExpectation * wildcardReadExpectation = [self expectationWithDescription:@"Wildcard read of our endpoint"];
[baseDevice readAttributePaths:@[ [MTRAttributeRequestPath requestPathWithEndpointID:endpointId1 clusterID:nil attributeID:nil] ]
eventPaths:nil
Expand All @@ -2042,32 +2061,118 @@ - (void)testControllerServer
XCTAssertNil(error);
XCTAssertNotNil(values);

// TODO: Figure out how to test that values is correct that's not
// too fragile if things get returned in different valid order.
// For now just check that every path we got has a value, not an
// error.
for (NSDictionary<NSString *, id> * value in values) {
XCTAssertNotNil(value[MTRAttributePathKey]);
XCTAssertNil(value[MTRErrorKey]);
XCTAssertNotNil(value[MTRDataKey]);
}
#if 0
XCTAssertEqualObjects(values, @[
// cluster1
@{ MTRAttributePathKey: attribute1ResponsePath,
MTRDataKey: unsignedIntValue2, },
@{ MTRAttributePathKey: globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeFeatureMapID),
MTRDataKey: unsignedIntValue(0), },
@{ MTRAttributePathKey: globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeClusterRevisionID),
MTRDataKey: clusterRevision1, },
@{ MTRAttributePathKey: globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID),
MTRDataKey: arrayOfUnsignedIntegersValue(@[]), },
@{ MTRAttributePathKey: globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID),
MTRDataKey: arrayOfUnsignedIntegersValue(@[]), },
// etc

]);
#endif

NSSet<NSDictionary<NSString *, id> *> * receivedValues = [NSSet setWithArray:values];
NSSet<NSDictionary<NSString *, id> *> * expectedValues = [NSSet setWithArray:@[
// cluster1
@ {
MTRAttributePathKey : attribute1ResponsePath,
MTRDataKey : unsignedIntValue2,
},
// attribute3 requires Operate privileges to read, which we do not have
// for this cluster, so it will not be present here.
@ {
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeFeatureMapID),
MTRDataKey : unsignedIntValue(0),
},
@ {
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeClusterRevisionID),
MTRDataKey : unsignedIntValue(clusterRevision1.unsignedIntegerValue),
},
@{
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
},
@{
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
},
@{
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeAttributeListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[
attributeId1, @(0xFFF8), @(0xFFF9), @(0xFFFB), attributeId2, @(0xFFFC), @(0xFFFD)
]),
},

// cluster2
@ {
MTRAttributePathKey : attribute2ResponsePath,
MTRDataKey : listOfStructsValue1,
},
@ {
MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeFeatureMapID),
MTRDataKey : unsignedIntValue(0),
},
@ {
MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeClusterRevisionID),
MTRDataKey : unsignedIntValue(clusterRevision2.unsignedIntegerValue),
},
@{MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
},
@{
MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
},
@{
MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeAttributeListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[
@0xFFF8, @(0xFFF9), @(0xFFFB), attributeId2, @(0xFFFC), @(0xFFFD)
]),
},

// descriptor
@ {
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeClusterDescriptorAttributeDeviceTypeListID),
MTRDataKey : endpoint1DeviceTypeValue,
},
@{
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeClusterDescriptorAttributeServerListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[ clusterId1, clusterId2, @(MTRClusterIDTypeDescriptorID) ]),
},
@{
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeClusterDescriptorAttributeClientListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
},
@{
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeClusterDescriptorAttributePartsListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
},
// No TagList attribute on this descriptor.
@ {
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeGlobalAttributeFeatureMapID),
MTRDataKey : unsignedIntValue(0),
},
@ {
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeGlobalAttributeClusterRevisionID),
// Would be nice if we could get the Descriptor cluster revision
// from somewhere intead of hardcoding it...
MTRDataKey : unsignedIntValue(2),
},
@{
MTRAttributePathKey : globalAttributePath(@(MTRClusterIDTypeDescriptorID), MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
},
@{
MTRAttributePathKey : globalAttributePath(@(MTRClusterIDTypeDescriptorID), MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
},
@{
MTRAttributePathKey : globalAttributePath(@(MTRClusterIDTypeDescriptorID), MTRAttributeIDTypeGlobalAttributeAttributeListID),
MTRDataKey : arrayOfUnsignedIntegersValue(@[
@(0), @(1), @(2), @(3), @(0xFFF8), @(0xFFF9), @(0xFFFB), @(0xFFFC), @(0xFFFD)
]),
},

]];

XCTAssertEqualObjects(receivedValues, expectedValues);

[wildcardReadExpectation fulfill];
}];
[self waitForExpectations:@[ wildcardReadExpectation ] timeout:kTimeoutInSeconds];
Expand Down

0 comments on commit d4d9054

Please sign in to comment.