From d4d905414b47ea2da52c8719948d351331560d51 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 5 Jun 2024 18:02:10 -0400 Subject: [PATCH] Fix Darwin server cluster checks for wildcard read to check received values. (#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. --- .../ServerEndpoint/MTRServerAccessControl.mm | 49 +++-- .../CHIPTests/MTRPerControllerStorageTests.m | 191 ++++++++++++++---- 2 files changed, 182 insertions(+), 58 deletions(-) diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAccessControl.mm b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAccessControl.mm index 28dacef27c2a78..e98bdc0b9e61a4 100644 --- a/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAccessControl.mm +++ b/src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAccessControl.mm @@ -18,6 +18,7 @@ #import #import // for MTRClusterPath +#import // For MTRClusterIDTypeDescriptorID #import #import "MTRDeviceControllerFactory_Internal.h" @@ -29,6 +30,8 @@ #include #include #include +#include +#include #include #include @@ -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; @@ -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() diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index fc233ffaebf93d..7b821d8648d949 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -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]; }; @@ -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 * 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 @@ -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 * 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 *> * receivedValues = [NSSet setWithArray:values]; + NSSet *> * 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];