Skip to content

Commit d4d9054

Browse files
bzbarsky-applebzbarsky-apple
authored
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.
1 parent d3cac0c commit d4d9054

File tree

2 files changed

+182
-58
lines changed

2 files changed

+182
-58
lines changed

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

+34-15
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"
@@ -29,6 +30,8 @@
2930
#include <access/SubjectDescriptor.h>
3031
#include <app/InteractionModelEngine.h>
3132
#include <lib/core/CHIPError.h>
33+
#include <lib/core/CHIPVendorIdentifiers.hpp>
34+
#include <lib/core/DataModelTypes.h>
3235
#include <lib/core/Global.h>
3336
#include <lib/core/NodeId.h>
3437

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

136139
} // anonymous namespace
137140

138-
chip::Access::Privilege MatterGetAccessPrivilegeForReadEvent(ClusterId cluster, EventId event)
141+
Privilege MatterGetAccessPrivilegeForReadEvent(ClusterId cluster, EventId event)
139142
{
140143
// We don't support any event bits yet.
141-
return chip::Access::Privilege::kAdminister;
144+
return Privilege::kAdminister;
142145
}
143146

144-
chip::Access::Privilege MatterGetAccessPrivilegeForInvokeCommand(ClusterId cluster, CommandId command)
147+
Privilege MatterGetAccessPrivilegeForInvokeCommand(ClusterId cluster, CommandId command)
145148
{
146149
// For now we only have OTA, which uses Operate.
147-
return chip::Access::Privilege::kOperate;
150+
return Privilege::kOperate;
148151
}
149152

150-
chip::Access::Privilege MatterGetAccessPrivilegeForReadAttribute(ClusterId cluster, AttributeId attribute)
153+
Privilege MatterGetAccessPrivilegeForReadAttribute(ClusterId cluster, AttributeId attribute)
151154
{
152155
NSNumber * _Nullable neededPrivilege = [[MTRDeviceControllerFactory sharedInstance] neededReadPrivilegeForClusterID:@(cluster) attributeID:@(attribute)];
153156
if (neededPrivilege == nil) {
154-
// No privileges declared for this attribute on this cluster. Treat as
155-
// "needs admin privileges", so we fail closed.
156-
return chip::Access::Privilege::kAdminister;
157+
// No privileges declared for this attribute on this cluster.
158+
159+
// In some cases, we know based on the spec what the answer is, and our
160+
// API clients my not be able to provide explicit privileges for
161+
// these cases because our API surface does not allow them to create
162+
// custom attribute definitions for them.
163+
164+
// (1) Global attributes always require View privilege to read.
165+
if (IsGlobalAttribute(attribute)) {
166+
return Privilege::kView;
167+
}
168+
169+
// (2) All standard descriptor attributes just require View privilege to read.
170+
if (cluster == MTRClusterIDTypeDescriptorID && ExtractVendorFromMEI(attribute) == VendorId::Common) {
171+
return Privilege::kView;
172+
}
173+
174+
// Treat as "needs admin privileges", so we fail closed.
175+
return Privilege::kAdminister;
157176
}
158177

159178
switch (neededPrivilege.unsignedLongLongValue) {
160179
case MTRAccessControlEntryPrivilegeView:
161-
return chip::Access::Privilege::kView;
180+
return Privilege::kView;
162181
case MTRAccessControlEntryPrivilegeOperate:
163-
return chip::Access::Privilege::kOperate;
182+
return Privilege::kOperate;
164183
case MTRAccessControlEntryPrivilegeManage:
165-
return chip::Access::Privilege::kManage;
184+
return Privilege::kManage;
166185
case MTRAccessControlEntryPrivilegeAdminister:
167-
return chip::Access::Privilege::kAdminister;
186+
return Privilege::kAdminister;
168187
case MTRAccessControlEntryPrivilegeProxyView:
169188
// Just treat this as an unknown value; there is no value for this in privilege-storage.
170189
FALLTHROUGH;
@@ -175,13 +194,13 @@ bool GrantSubjectMatchesDescriptor(MTRAccessGrant * grant, const SubjectDescript
175194
// To be safe, treat unknown values as "needs admin privileges". That way the failure case
176195
// disallows access that maybe should be allowed, instead of allowing access that maybe
177196
// should be disallowed.
178-
return chip::Access::Privilege::kAdminister;
197+
return Privilege::kAdminister;
179198
}
180199

181-
chip::Access::Privilege MatterGetAccessPrivilegeForWriteAttribute(ClusterId cluster, AttributeId attribute)
200+
Privilege MatterGetAccessPrivilegeForWriteAttribute(ClusterId cluster, AttributeId attribute)
182201
{
183202
// We don't have any writable attributes yet, but default to Operate.
184-
return chip::Access::Privilege::kOperate;
203+
return Privilege::kOperate;
185204
}
186205

187206
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)