Skip to content

Commit ede56b8

Browse files
bzbarsky-applebzbarsky-apple
committed
Fix Darwin server cluster checks for wildcard read to check received values.
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.
1 parent f49e684 commit ede56b8

File tree

2 files changed

+172
-56
lines changed

2 files changed

+172
-56
lines changed

‎src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAccessControl.mm

+24-13
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#import <Matter/MTRAccessGrant.h>
2020
#import <Matter/MTRBaseDevice.h> // for MTRClusterPath
21+
#import <Matter/MTRClusterConstants.h> // For MTRClusterIDTypeDescriptorID
2122
#import <Matter/MTRDeviceControllerFactory.h>
2223

2324
#import "MTRDeviceControllerFactory_Internal.h"
@@ -135,36 +136,46 @@ bool GrantSubjectMatchesDescriptor(MTRAccessGrant * grant, const SubjectDescript
135136

136137
} // anonymous namespace
137138

138-
chip::Access::Privilege MatterGetAccessPrivilegeForReadEvent(ClusterId cluster, EventId event)
139+
Privilege MatterGetAccessPrivilegeForReadEvent(ClusterId cluster, EventId event)
139140
{
140141
// We don't support any event bits yet.
141-
return chip::Access::Privilege::kAdminister;
142+
return Privilege::kAdminister;
142143
}
143144

144-
chip::Access::Privilege MatterGetAccessPrivilegeForInvokeCommand(ClusterId cluster, CommandId command)
145+
Privilege MatterGetAccessPrivilegeForInvokeCommand(ClusterId cluster, CommandId command)
145146
{
146147
// For now we only have OTA, which uses Operate.
147-
return chip::Access::Privilege::kOperate;
148+
return Privilege::kOperate;
148149
}
149150

150-
chip::Access::Privilege MatterGetAccessPrivilegeForReadAttribute(ClusterId cluster, AttributeId attribute)
151+
Privilege MatterGetAccessPrivilegeForReadAttribute(ClusterId cluster, AttributeId attribute)
151152
{
153+
// Global attributes always require View privilege to read.
154+
if (IsGlobalAttribute(attribute)) {
155+
return Privilege::kView;
156+
}
157+
158+
// All descriptor attributes just require View privilege to read.
159+
if (cluster == MTRClusterIDTypeDescriptorID) {
160+
return Privilege::kView;
161+
}
162+
152163
NSNumber * _Nullable neededPrivilege = [[MTRDeviceControllerFactory sharedInstance] neededReadPrivilegeForClusterID:@(cluster) attributeID:@(attribute)];
153164
if (neededPrivilege == nil) {
154165
// No privileges declared for this attribute on this cluster. Treat as
155166
// "needs admin privileges", so we fail closed.
156-
return chip::Access::Privilege::kAdminister;
167+
return Privilege::kAdminister;
157168
}
158169

159170
switch (neededPrivilege.unsignedLongLongValue) {
160171
case MTRAccessControlEntryPrivilegeView:
161-
return chip::Access::Privilege::kView;
172+
return Privilege::kView;
162173
case MTRAccessControlEntryPrivilegeOperate:
163-
return chip::Access::Privilege::kOperate;
174+
return Privilege::kOperate;
164175
case MTRAccessControlEntryPrivilegeManage:
165-
return chip::Access::Privilege::kManage;
176+
return Privilege::kManage;
166177
case MTRAccessControlEntryPrivilegeAdminister:
167-
return chip::Access::Privilege::kAdminister;
178+
return Privilege::kAdminister;
168179
case MTRAccessControlEntryPrivilegeProxyView:
169180
// Just treat this as an unknown value; there is no value for this in privilege-storage.
170181
FALLTHROUGH;
@@ -175,13 +186,13 @@ bool GrantSubjectMatchesDescriptor(MTRAccessGrant * grant, const SubjectDescript
175186
// To be safe, treat unknown values as "needs admin privileges". That way the failure case
176187
// disallows access that maybe should be allowed, instead of allowing access that maybe
177188
// should be disallowed.
178-
return chip::Access::Privilege::kAdminister;
189+
return Privilege::kAdminister;
179190
}
180191

181-
chip::Access::Privilege MatterGetAccessPrivilegeForWriteAttribute(ClusterId cluster, AttributeId attribute)
192+
Privilege MatterGetAccessPrivilegeForWriteAttribute(ClusterId cluster, AttributeId attribute)
182193
{
183194
// We don't have any writable attributes yet, but default to Operate.
184-
return chip::Access::Privilege::kOperate;
195+
return Privilege::kOperate;
185196
}
186197

187198
void InitializeServerAccessControl()

‎src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m

+148-43
Original file line numberDiff line numberDiff line change
@@ -1721,15 +1721,6 @@ - (void)testControllerServer
17211721
],
17221722
};
17231723

1724-
#if 0
1725-
__auto_type * listOfStructsValue2 = @{
1726-
MTRTypeKey : MTRArrayValueType,
1727-
MTRValueKey : @[
1728-
@{ MTRDataKey: structValue2, },
1729-
],
1730-
};
1731-
#endif
1732-
17331724
__auto_type responsePathFromRequestPath = ^(MTRAttributeRequestPath * path) {
17341725
return [MTRAttributePath attributePathWithEndpointID:path.endpoint clusterID:path.cluster attributeID:path.attribute];
17351726
};
@@ -2007,32 +1998,60 @@ - (void)testControllerServer
20071998

20081999
// Now do a wildcard read on the endpoint and check that this does the right
20092000
// thing (gets the right things from descriptor, gets both clusters, etc).
2010-
#if 0
2011-
// Unused bits ifdefed out until we doing more testing on the actual values
2012-
// we get back.
20132001
__auto_type globalAttributePath = ^(NSNumber * clusterID, MTRAttributeIDType attributeID) {
20142002
return [MTRAttributePath attributePathWithEndpointID:endpointId1 clusterID:clusterID attributeID:@(attributeID)];
20152003
};
2004+
__auto_type descriptorAttributePath = ^(MTRAttributeIDType attributeID) {
2005+
return [MTRAttributePath attributePathWithEndpointID:endpointId1 clusterID:@(MTRClusterIDTypeDescriptorID) attributeID:@(attributeID)];
2006+
};
20162007
__auto_type unsignedIntValue = ^(NSUInteger value) {
20172008
return @{
2018-
MTRTypeKey: MTRUnsignedIntegerValueType,
2019-
MTRValueKey: @(value),
2009+
MTRTypeKey : MTRUnsignedIntegerValueType,
2010+
MTRValueKey : @(value),
20202011
};
20212012
};
20222013
__auto_type arrayOfUnsignedIntegersValue = ^(NSArray<NSNumber *> * values) {
20232014
__auto_type * mutableArray = [[NSMutableArray alloc] init];
20242015
for (NSNumber * value in values) {
2025-
[mutableArray addObject:@{ MTRDataKey: @{
2026-
MTRTypeKey: MTRUnsignedIntegerValueType,
2027-
MTRValueKey: value,
2028-
}, }];
2016+
[mutableArray addObject:@{
2017+
MTRDataKey : @ {
2018+
MTRTypeKey : MTRUnsignedIntegerValueType,
2019+
MTRValueKey : value,
2020+
},
2021+
}];
20292022
}
20302023
return @{
2031-
MTRTypeKey: MTRArrayValueType,
2032-
MTRValueKey: [mutableArray copy],
2033-
};
2024+
MTRTypeKey : MTRArrayValueType,
2025+
MTRValueKey : [mutableArray copy],
2026+
};
20342027
};
2035-
#endif
2028+
__auto_type endpoint1DeviceTypeValue = @{
2029+
MTRTypeKey : MTRArrayValueType,
2030+
MTRValueKey : @[
2031+
@{
2032+
MTRDataKey : @ {
2033+
MTRTypeKey : MTRStructureValueType,
2034+
MTRValueKey : @[
2035+
@{
2036+
MTRContextTagKey : @(0),
2037+
MTRDataKey : @ {
2038+
MTRTypeKey : MTRUnsignedIntegerValueType,
2039+
MTRValueKey : deviceType1.deviceTypeID,
2040+
},
2041+
},
2042+
@{
2043+
MTRContextTagKey : @(1),
2044+
MTRDataKey : @ {
2045+
MTRTypeKey : MTRUnsignedIntegerValueType,
2046+
MTRValueKey : deviceType1.deviceTypeRevision,
2047+
},
2048+
},
2049+
],
2050+
}
2051+
},
2052+
],
2053+
};
2054+
20362055
XCTestExpectation * wildcardReadExpectation = [self expectationWithDescription:@"Wildcard read of our endpoint"];
20372056
[baseDevice readAttributePaths:@[ [MTRAttributeRequestPath requestPathWithEndpointID:endpointId1 clusterID:nil attributeID:nil] ]
20382057
eventPaths:nil
@@ -2042,32 +2061,118 @@ - (void)testControllerServer
20422061
XCTAssertNil(error);
20432062
XCTAssertNotNil(values);
20442063

2045-
// TODO: Figure out how to test that values is correct that's not
2046-
// too fragile if things get returned in different valid order.
2047-
// For now just check that every path we got has a value, not an
2048-
// error.
20492064
for (NSDictionary<NSString *, id> * value in values) {
20502065
XCTAssertNotNil(value[MTRAttributePathKey]);
20512066
XCTAssertNil(value[MTRErrorKey]);
20522067
XCTAssertNotNil(value[MTRDataKey]);
20532068
}
2054-
#if 0
2055-
XCTAssertEqualObjects(values, @[
2056-
// cluster1
2057-
@{ MTRAttributePathKey: attribute1ResponsePath,
2058-
MTRDataKey: unsignedIntValue2, },
2059-
@{ MTRAttributePathKey: globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeFeatureMapID),
2060-
MTRDataKey: unsignedIntValue(0), },
2061-
@{ MTRAttributePathKey: globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeClusterRevisionID),
2062-
MTRDataKey: clusterRevision1, },
2063-
@{ MTRAttributePathKey: globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID),
2064-
MTRDataKey: arrayOfUnsignedIntegersValue(@[]), },
2065-
@{ MTRAttributePathKey: globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID),
2066-
MTRDataKey: arrayOfUnsignedIntegersValue(@[]), },
2067-
// etc
2068-
2069-
]);
2070-
#endif
2069+
2070+
NSSet<NSDictionary<NSString *, id> *> * receivedValues = [NSSet setWithArray:values];
2071+
NSSet<NSDictionary<NSString *, id> *> * expectedValues = [NSSet setWithArray:@[
2072+
// cluster1
2073+
@ {
2074+
MTRAttributePathKey : attribute1ResponsePath,
2075+
MTRDataKey : unsignedIntValue2,
2076+
},
2077+
// attribute3 requires Operate privileges to read, which we do not have
2078+
// for this cluster, so it will not be present here.
2079+
@ {
2080+
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeFeatureMapID),
2081+
MTRDataKey : unsignedIntValue(0),
2082+
},
2083+
@ {
2084+
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeClusterRevisionID),
2085+
MTRDataKey : unsignedIntValue(clusterRevision1.unsignedIntegerValue),
2086+
},
2087+
@{
2088+
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID),
2089+
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
2090+
},
2091+
@{
2092+
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID),
2093+
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
2094+
},
2095+
@{
2096+
MTRAttributePathKey : globalAttributePath(clusterId1, MTRAttributeIDTypeGlobalAttributeAttributeListID),
2097+
MTRDataKey : arrayOfUnsignedIntegersValue(@[
2098+
attributeId1, @(0xFFF8), @(0xFFF9), @(0xFFFB), attributeId2, @(0xFFFC), @(0xFFFD)
2099+
]),
2100+
},
2101+
2102+
// cluster2
2103+
@ {
2104+
MTRAttributePathKey : attribute2ResponsePath,
2105+
MTRDataKey : listOfStructsValue1,
2106+
},
2107+
@ {
2108+
MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeFeatureMapID),
2109+
MTRDataKey : unsignedIntValue(0),
2110+
},
2111+
@ {
2112+
MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeClusterRevisionID),
2113+
MTRDataKey : unsignedIntValue(clusterRevision2.unsignedIntegerValue),
2114+
},
2115+
@{MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID),
2116+
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
2117+
},
2118+
@{
2119+
MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID),
2120+
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
2121+
},
2122+
@{
2123+
MTRAttributePathKey : globalAttributePath(clusterId2, MTRAttributeIDTypeGlobalAttributeAttributeListID),
2124+
MTRDataKey : arrayOfUnsignedIntegersValue(@[
2125+
@0xFFF8, @(0xFFF9), @(0xFFFB), attributeId2, @(0xFFFC), @(0xFFFD)
2126+
]),
2127+
},
2128+
2129+
// descriptor
2130+
@ {
2131+
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeClusterDescriptorAttributeDeviceTypeListID),
2132+
MTRDataKey : endpoint1DeviceTypeValue,
2133+
},
2134+
@{
2135+
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeClusterDescriptorAttributeServerListID),
2136+
MTRDataKey : arrayOfUnsignedIntegersValue(@[ clusterId1, clusterId2, @(MTRClusterIDTypeDescriptorID) ]),
2137+
},
2138+
@{
2139+
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeClusterDescriptorAttributeClientListID),
2140+
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
2141+
},
2142+
@{
2143+
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeClusterDescriptorAttributePartsListID),
2144+
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
2145+
},
2146+
// No TagList attribute on this descriptor.
2147+
@ {
2148+
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeGlobalAttributeFeatureMapID),
2149+
MTRDataKey : unsignedIntValue(0),
2150+
},
2151+
@ {
2152+
MTRAttributePathKey : descriptorAttributePath(MTRAttributeIDTypeGlobalAttributeClusterRevisionID),
2153+
// Would be nice if we could get the Descriptor cluster revision
2154+
// from somewhere intead of hardcoding it...
2155+
MTRDataKey : unsignedIntValue(2),
2156+
},
2157+
@{
2158+
MTRAttributePathKey : globalAttributePath(@(MTRClusterIDTypeDescriptorID), MTRAttributeIDTypeGlobalAttributeGeneratedCommandListID),
2159+
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
2160+
},
2161+
@{
2162+
MTRAttributePathKey : globalAttributePath(@(MTRClusterIDTypeDescriptorID), MTRAttributeIDTypeGlobalAttributeAcceptedCommandListID),
2163+
MTRDataKey : arrayOfUnsignedIntegersValue(@[]),
2164+
},
2165+
@{
2166+
MTRAttributePathKey : globalAttributePath(@(MTRClusterIDTypeDescriptorID), MTRAttributeIDTypeGlobalAttributeAttributeListID),
2167+
MTRDataKey : arrayOfUnsignedIntegersValue(@[
2168+
@(0), @(1), @(2), @(3), @(0xFFF8), @(0xFFF9), @(0xFFFB), @(0xFFFC), @(0xFFFD)
2169+
]),
2170+
},
2171+
2172+
]];
2173+
2174+
XCTAssertEqualObjects(receivedValues, expectedValues);
2175+
20712176
[wildcardReadExpectation fulfill];
20722177
}];
20732178
[self waitForExpectations:@[ wildcardReadExpectation ] timeout:kTimeoutInSeconds];

0 commit comments

Comments
 (0)