From ebddcdde129987c6db2bd0607ecb950a386cc69c Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Mon, 4 Apr 2022 12:43:54 -0400 Subject: [PATCH 1/6] Add extension attribute to access control cluster Includes attribute. Does not include events, or fabric removal. Part of issue #10252 --- .../all-clusters-app.matter | 2 +- .../bridge-common/bridge-app.matter | 4 +- .../door-lock-common/door-lock-app.matter | 2 +- .../light-switch-app.matter | 2 +- .../lighting-common/lighting-app.matter | 2 +- examples/lock-app/lock-common/lock-app.matter | 2 +- .../log-source-common/log-source-app.matter | 2 +- .../ota-provider-app.matter | 4 +- .../ota-requestor-app.matter | 2 +- examples/pump-app/pump-common/pump-app.matter | 2 +- .../pump-controller-app.matter | 2 +- .../esp32/main/temperature-measurement.matter | 2 +- .../thermostat-common/thermostat.matter | 2 +- examples/tv-app/tv-common/tv-app.matter | 2 +- .../tv-casting-common/tv-casting-app.matter | 2 +- examples/window-app/common/window-app.matter | 2 +- .../access-control-server.cpp | 95 +++++++++++++++++-- .../chip/access-control-cluster.xml | 4 +- .../data_model/controller-clusters.matter | 2 +- src/lib/support/DefaultStorageKeyAllocator.h | 6 +- .../zap-generated/cluster-objects.cpp | 6 +- 21 files changed, 120 insertions(+), 29 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter index 35e18361877638..5ea1bd89a8a437 100644 --- a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter +++ b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/bridge-app/bridge-common/bridge-app.matter b/examples/bridge-app/bridge-common/bridge-app.matter index cab04724648e52..d34ffdc5b59dde 100644 --- a/examples/bridge-app/bridge-common/bridge-app.matter +++ b/examples/bridge-app/bridge-common/bridge-app.matter @@ -42,7 +42,7 @@ client cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } @@ -104,7 +104,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/door-lock-app/door-lock-common/door-lock-app.matter b/examples/door-lock-app/door-lock-common/door-lock-app.matter index 547b7ae2fe7913..6192710b913d11 100644 --- a/examples/door-lock-app/door-lock-common/door-lock-app.matter +++ b/examples/door-lock-app/door-lock-common/door-lock-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/light-switch-app/light-switch-common/light-switch-app.matter b/examples/light-switch-app/light-switch-common/light-switch-app.matter index b695f7f9577756..29f7f291554502 100644 --- a/examples/light-switch-app/light-switch-common/light-switch-app.matter +++ b/examples/light-switch-app/light-switch-common/light-switch-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/lighting-app/lighting-common/lighting-app.matter b/examples/lighting-app/lighting-common/lighting-app.matter index b81d7297eacdea..fbb26b4dc88dde 100644 --- a/examples/lighting-app/lighting-common/lighting-app.matter +++ b/examples/lighting-app/lighting-common/lighting-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/lock-app/lock-common/lock-app.matter b/examples/lock-app/lock-common/lock-app.matter index 054c4fda4ae105..c61d9b618bdeda 100644 --- a/examples/lock-app/lock-common/lock-app.matter +++ b/examples/lock-app/lock-common/lock-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/log-source-app/log-source-common/log-source-app.matter b/examples/log-source-app/log-source-common/log-source-app.matter index f39b90213f7e03..5c65c053a513f3 100644 --- a/examples/log-source-app/log-source-common/log-source-app.matter +++ b/examples/log-source-app/log-source-common/log-source-app.matter @@ -37,7 +37,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/ota-provider-app/ota-provider-common/ota-provider-app.matter b/examples/ota-provider-app/ota-provider-common/ota-provider-app.matter index 4f3207eb3a79a1..90ed5718260529 100644 --- a/examples/ota-provider-app/ota-provider-common/ota-provider-app.matter +++ b/examples/ota-provider-app/ota-provider-common/ota-provider-app.matter @@ -42,7 +42,7 @@ client cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } @@ -104,7 +104,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/ota-requestor-app/ota-requestor-common/ota-requestor-app.matter b/examples/ota-requestor-app/ota-requestor-common/ota-requestor-app.matter index 107bf6e381472c..fdb5c8665f5f84 100644 --- a/examples/ota-requestor-app/ota-requestor-common/ota-requestor-app.matter +++ b/examples/ota-requestor-app/ota-requestor-common/ota-requestor-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/pump-app/pump-common/pump-app.matter b/examples/pump-app/pump-common/pump-app.matter index db3d9718d74a81..e5a620028c9f84 100644 --- a/examples/pump-app/pump-common/pump-app.matter +++ b/examples/pump-app/pump-common/pump-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/pump-controller-app/pump-controller-common/pump-controller-app.matter b/examples/pump-controller-app/pump-controller-common/pump-controller-app.matter index 430bc8d07b789f..0fbe39a5c5e782 100644 --- a/examples/pump-controller-app/pump-controller-common/pump-controller-app.matter +++ b/examples/pump-controller-app/pump-controller-common/pump-controller-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/temperature-measurement-app/esp32/main/temperature-measurement.matter b/examples/temperature-measurement-app/esp32/main/temperature-measurement.matter index 765ed9c0ea9fb7..1913689175cca9 100644 --- a/examples/temperature-measurement-app/esp32/main/temperature-measurement.matter +++ b/examples/temperature-measurement-app/esp32/main/temperature-measurement.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/thermostat/thermostat-common/thermostat.matter b/examples/thermostat/thermostat-common/thermostat.matter index b67e43bdb72c83..17a2d05b3403a2 100644 --- a/examples/thermostat/thermostat-common/thermostat.matter +++ b/examples/thermostat/thermostat-common/thermostat.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/tv-app/tv-common/tv-app.matter b/examples/tv-app/tv-common/tv-app.matter index 1f241f096c0f22..7c8c46e3a9473a 100644 --- a/examples/tv-app/tv-common/tv-app.matter +++ b/examples/tv-app/tv-common/tv-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/tv-casting-app/tv-casting-common/tv-casting-app.matter b/examples/tv-casting-app/tv-casting-common/tv-casting-app.matter index a4ec6430b37463..ceb5b00a2b160a 100644 --- a/examples/tv-casting-app/tv-casting-common/tv-casting-app.matter +++ b/examples/tv-casting-app/tv-casting-common/tv-casting-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/examples/window-app/common/window-app.matter b/examples/window-app/common/window-app.matter index 0f8fa24967e1a9..e23c65ca32c3bb 100644 --- a/examples/window-app/common/window-app.matter +++ b/examples/window-app/common/window-app.matter @@ -42,7 +42,7 @@ server cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp index 19b17d4406093f..61f639673f59eb 100644 --- a/src/app/clusters/access-control-server/access-control-server.cpp +++ b/src/app/clusters/access-control-server/access-control-server.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -35,6 +36,12 @@ using namespace chip::Access; namespace AccessControlCluster = chip::app::Clusters::AccessControl; +// TODO(#13590): generated code doesn't automatically handle max length so do it manually +constexpr int kExtensionDataMaxLength = 128; + +// Storage version used in keys. +constexpr int kStorageVersion = 1; + namespace { struct Subject @@ -355,7 +362,7 @@ class AccessControlAttribute : public chip::app::AttributeAccessInterface CHIP_ERROR ReadAcl(AttributeValueEncoder & aEncoder); CHIP_ERROR ReadExtension(AttributeValueEncoder & aEncoder); CHIP_ERROR WriteAcl(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder); - CHIP_ERROR WriteExtension(AttributeValueDecoder & aDecoder); + CHIP_ERROR WriteExtension(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder); }; constexpr uint16_t AccessControlAttribute::ClusterRevision; @@ -476,7 +483,28 @@ CHIP_ERROR AccessControlAttribute::ReadAcl(AttributeValueEncoder & aEncoder) CHIP_ERROR AccessControlAttribute::ReadExtension(AttributeValueEncoder & aEncoder) { - return aEncoder.EncodeEmptyList(); + auto & storage = Server::GetInstance().GetPersistentStorage(); + DefaultStorageKeyAllocator key; + + auto & fabrics = Server::GetInstance().GetFabricTable(); + + return aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR { + for (auto it = fabrics.begin(); it != fabrics.end(); ++it) + { + uint8_t buffer[kExtensionDataMaxLength] = { 0 }; + uint16_t size = static_cast(sizeof(buffer)); + auto err = + storage.SyncGetKeyValue(key.AccessControlExtensionEntry(kStorageVersion, it->GetFabricIndex()), buffer, size); + ReturnErrorCodeIf(err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, CHIP_NO_ERROR); + ReturnErrorOnFailure(err); + AccessControlCluster::Structs::ExtensionEntry::Type item = { + .data = ByteSpan(buffer, size), + .fabricIndex = it->GetFabricIndex(), + }; + ReturnErrorOnFailure(encoder.Encode(item)); + } + return CHIP_NO_ERROR; + }); } CHIP_ERROR AccessControlAttribute::Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) @@ -486,7 +514,7 @@ CHIP_ERROR AccessControlAttribute::Write(const ConcreteDataAttributePath & aPath case AccessControlCluster::Attributes::Acl::Id: return WriteAcl(aPath, aDecoder); case AccessControlCluster::Attributes::Extension::Id: - return WriteExtension(aDecoder); + return WriteExtension(aPath, aDecoder); } return CHIP_NO_ERROR; @@ -570,10 +598,65 @@ CHIP_ERROR AccessControlAttribute::WriteAcl(const ConcreteDataAttributePath & aP return CHIP_NO_ERROR; } -CHIP_ERROR AccessControlAttribute::WriteExtension(AttributeValueDecoder & aDecoder) +CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) { - DataModel::DecodableList list; - ReturnErrorOnFailure(aDecoder.Decode(list)); + auto & storage = Server::GetInstance().GetPersistentStorage(); + DefaultStorageKeyAllocator key; + + FabricIndex accessingFabricIndex = aDecoder.AccessingFabricIndex(); + + if (!aPath.IsListItemOperation()) + { + DataModel::DecodableList list; + ReturnErrorOnFailure(aDecoder.Decode(list)); + + size_t count = 0; + ReturnErrorOnFailure(list.ComputeSize(&count)); + + if (count == 0) + { + auto err = storage.SyncDeleteKeyValue(key.AccessControlExtensionEntry(kStorageVersion, accessingFabricIndex)); + ReturnErrorCodeIf(err != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, err); + } + else if (count == 1) + { + auto iterator = list.begin(); + ReturnErrorCodeIf(!iterator.Next(), CHIP_ERROR_MISSING_TLV_ELEMENT); + auto & item = iterator.GetValue(); + // TODO(#13590): generated code doesn't automatically handle max length so do it manually + ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorOnFailure(storage.SyncSetKeyValue(key.AccessControlExtensionEntry(kStorageVersion, accessingFabricIndex), + item.data.data(), static_cast(item.data.size()))); + } + else + { + // Only one item supported per fabric. + return CHIP_ERROR_INVALID_ARGUMENT; + } + } + else if (aPath.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem) + { + { + uint8_t buffer[0]; + uint16_t size = static_cast(sizeof(buffer)); + auto err = + storage.SyncGetKeyValue(key.AccessControlExtensionEntry(kStorageVersion, accessingFabricIndex), buffer, size); + ReturnErrorCodeIf(err != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, err); + } + + AccessControlCluster::Structs::ExtensionEntry::DecodableType item; + ReturnErrorOnFailure(aDecoder.Decode(item)); + ChipLogProgress(DataManagement, "############################ storing item %u", (unsigned) item.data.size()); + // TODO(#13590): generated code doesn't automatically handle max length so do it manually + ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorOnFailure(storage.SyncSetKeyValue(key.AccessControlExtensionEntry(kStorageVersion, accessingFabricIndex), + item.data.data(), static_cast(item.data.size()))); + } + else + { + return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; + } + return CHIP_NO_ERROR; } diff --git a/src/app/zap-templates/zcl/data-model/chip/access-control-cluster.xml b/src/app/zap-templates/zcl/data-model/chip/access-control-cluster.xml index fa7d0eb82254ce..561925936784dc 100644 --- a/src/app/zap-templates/zcl/data-model/chip/access-control-cluster.xml +++ b/src/app/zap-templates/zcl/data-model/chip/access-control-cluster.xml @@ -58,8 +58,8 @@ limitations under the License. - - + + diff --git a/src/controller/data_model/controller-clusters.matter b/src/controller/data_model/controller-clusters.matter index 2f110836cd057d..688c4d0ad69705 100644 --- a/src/controller/data_model/controller-clusters.matter +++ b/src/controller/data_model/controller-clusters.matter @@ -42,7 +42,7 @@ client cluster AccessControl = 31 { } struct ExtensionEntry { - OCTET_STRING<254> data = 1; + OCTET_STRING<128> data = 1; fabric_idx fabricIndex = 254; } diff --git a/src/lib/support/DefaultStorageKeyAllocator.h b/src/lib/support/DefaultStorageKeyAllocator.h index 3bbc29007ac6c2..ccaa5e2c5c0570 100644 --- a/src/lib/support/DefaultStorageKeyAllocator.h +++ b/src/lib/support/DefaultStorageKeyAllocator.h @@ -45,7 +45,11 @@ class DefaultStorageKeyAllocator // FailSafeContext const char * FailSafeContextKey() { return Format("g/fsc"); } - // Access Control List + // Access Control + const char * AccessControlExtensionEntry(size_t version, FabricIndex fabric) + { + return Format("a/%x/1/%x", static_cast(version), static_cast(fabric)); + } const char * AccessControlList() { return Format("acl"); } const char * AccessControlEntry(size_t index) diff --git a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp index 896d137b6ee1bf..d462d03636fb60 100644 --- a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp +++ b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp @@ -4683,9 +4683,13 @@ CHIP_ERROR Type::EncodeForRead(TLV::TLVWriter & writer, TLV::Tag tag, FabricInde CHIP_ERROR Type::DoEncode(TLV::TLVWriter & writer, TLV::Tag tag, const Optional & accessingFabricIndex) const { + bool includeSensitive = !accessingFabricIndex.HasValue() || (accessingFabricIndex.Value() == fabricIndex); TLV::TLVType outer; ReturnErrorOnFailure(writer.StartContainer(tag, TLV::kTLVType_Structure, outer)); - ReturnErrorOnFailure(DataModel::Encode(writer, TLV::ContextTag(to_underlying(Fields::kData)), data)); + if (includeSensitive) + { + ReturnErrorOnFailure(DataModel::Encode(writer, TLV::ContextTag(to_underlying(Fields::kData)), data)); + } if (accessingFabricIndex.HasValue()) { ReturnErrorOnFailure(DataModel::Encode(writer, TLV::ContextTag(to_underlying(Fields::kFabricIndex)), fabricIndex)); From 2cac609b5379b385304d2602e6d0e830fdbffda6 Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Mon, 4 Apr 2022 13:24:58 -0400 Subject: [PATCH 2/6] Remove temp log --- src/app/clusters/access-control-server/access-control-server.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp index 61f639673f59eb..9febc0f18179d4 100644 --- a/src/app/clusters/access-control-server/access-control-server.cpp +++ b/src/app/clusters/access-control-server/access-control-server.cpp @@ -646,7 +646,6 @@ CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePat AccessControlCluster::Structs::ExtensionEntry::DecodableType item; ReturnErrorOnFailure(aDecoder.Decode(item)); - ChipLogProgress(DataManagement, "############################ storing item %u", (unsigned) item.data.size()); // TODO(#13590): generated code doesn't automatically handle max length so do it manually ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_ERROR_INVALID_ARGUMENT); ReturnErrorOnFailure(storage.SyncSetKeyValue(key.AccessControlExtensionEntry(kStorageVersion, accessingFabricIndex), From 81a624e7b257ce2480f2dac0da7b70e4274c3b9a Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Mon, 4 Apr 2022 16:08:46 -0400 Subject: [PATCH 3/6] Add extension event support Also change the keys for storing extensions based on review feedback. --- .../access-control-server.cpp | 117 ++++++++++++------ src/lib/support/DefaultStorageKeyAllocator.h | 5 +- 2 files changed, 81 insertions(+), 41 deletions(-) diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp index 9febc0f18179d4..b99eb238c076b8 100644 --- a/src/app/clusters/access-control-server/access-control-server.cpp +++ b/src/app/clusters/access-control-server/access-control-server.cpp @@ -39,9 +39,6 @@ namespace AccessControlCluster = chip::app::Clusters::AccessControl; // TODO(#13590): generated code doesn't automatically handle max length so do it manually constexpr int kExtensionDataMaxLength = 128; -// Storage version used in keys. -constexpr int kStorageVersion = 1; - namespace { struct Subject @@ -367,13 +364,12 @@ class AccessControlAttribute : public chip::app::AttributeAccessInterface constexpr uint16_t AccessControlAttribute::ClusterRevision; -CHIP_ERROR LogEntryChangedEvent(const AccessControl::Entry & entry, const Access::SubjectDescriptor & subjectDescriptor, - AccessControlCluster::ChangeTypeEnum changeType) +CHIP_ERROR LogAclChangedEvent(const AccessControl::Entry & entry, const Access::SubjectDescriptor & subjectDescriptor, + AccessControlCluster::ChangeTypeEnum changeType) { CHIP_ERROR err; // Record AccessControlEntry event - EventNumber eventNumber; DataModel::Nullable adminNodeID; DataModel::Nullable adminPasscodeID; DataModel::Nullable latestValue; @@ -439,6 +435,7 @@ CHIP_ERROR LogEntryChangedEvent(const AccessControl::Entry & entry, const Access AccessControlCluster::Events::AccessControlEntryChanged::Type event{ adminNodeID, adminPasscodeID, changeType, latestValue, subjectDescriptor.fabricIndex }; + EventNumber eventNumber; err = LogEvent(event, 0, eventNumber); if (CHIP_NO_ERROR != err) { @@ -448,6 +445,34 @@ CHIP_ERROR LogEntryChangedEvent(const AccessControl::Entry & entry, const Access return err; } +CHIP_ERROR LogExtensionChangedEvent(const AccessControlCluster::Structs::ExtensionEntry::Type & item, + const Access::SubjectDescriptor & subjectDescriptor, + AccessControlCluster::ChangeTypeEnum changeType) +{ + AccessControlCluster::Events::AccessControlExtensionChanged::Type event{ .changeType = changeType, + .adminFabricIndex = subjectDescriptor.fabricIndex }; + + if (subjectDescriptor.authMode == Access::AuthMode::kCase) + { + event.adminNodeID.SetNonNull(subjectDescriptor.subject); + } + else if (subjectDescriptor.authMode == Access::AuthMode::kPase) + { + event.adminPasscodeID.SetNonNull(PAKEKeyIdFromNodeId(subjectDescriptor.subject)); + } + + event.latestValue.SetNonNull(item); + + EventNumber eventNumber; + CHIP_ERROR err = LogEvent(event, 0, eventNumber); + if (CHIP_NO_ERROR != err) + { + ChipLogError(DataManagement, "AccessControlCluster: log event failed"); + } + + return err; +} + CHIP_ERROR AccessControlAttribute::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) { switch (aPath.mAttributeId) @@ -489,17 +514,20 @@ CHIP_ERROR AccessControlAttribute::ReadExtension(AttributeValueEncoder & aEncode auto & fabrics = Server::GetInstance().GetFabricTable(); return aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR { - for (auto it = fabrics.begin(); it != fabrics.end(); ++it) + for (auto & fabric : fabrics) { uint8_t buffer[kExtensionDataMaxLength] = { 0 }; uint16_t size = static_cast(sizeof(buffer)); - auto err = - storage.SyncGetKeyValue(key.AccessControlExtensionEntry(kStorageVersion, it->GetFabricIndex()), buffer, size); - ReturnErrorCodeIf(err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, CHIP_NO_ERROR); - ReturnErrorOnFailure(err); + CHIP_ERROR errStorage = storage.SyncGetKeyValue(key.AccessControlExtensionEntry(fabric.GetFabricIndex()), buffer, size); + VerifyOrDie(errStorage != CHIP_ERROR_BUFFER_TOO_SMALL); + if (errStorage == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND) + { + continue; + } + ReturnErrorOnFailure(errStorage); AccessControlCluster::Structs::ExtensionEntry::Type item = { .data = ByteSpan(buffer, size), - .fabricIndex = it->GetFabricIndex(), + .fabricIndex = fabric.GetFabricIndex(), }; ReturnErrorOnFailure(encoder.Encode(item)); } @@ -557,14 +585,14 @@ CHIP_ERROR AccessControlAttribute::WriteAcl(const ConcreteDataAttributePath & aP if (i < oldCount) { ReturnErrorOnFailure(GetAccessControl().UpdateEntry(i, iterator.GetValue().entry, &accessingFabricIndex)); - ReturnErrorOnFailure(LogEntryChangedEvent(iterator.GetValue().entry, aDecoder.GetSubjectDescriptor(), - AccessControlCluster::ChangeTypeEnum::kChanged)); + ReturnErrorOnFailure(LogAclChangedEvent(iterator.GetValue().entry, aDecoder.GetSubjectDescriptor(), + AccessControlCluster::ChangeTypeEnum::kChanged)); } else { ReturnErrorOnFailure(GetAccessControl().CreateEntry(nullptr, iterator.GetValue().entry, &accessingFabricIndex)); - ReturnErrorOnFailure(LogEntryChangedEvent(iterator.GetValue().entry, aDecoder.GetSubjectDescriptor(), - AccessControlCluster::ChangeTypeEnum::kAdded)); + ReturnErrorOnFailure(LogAclChangedEvent(iterator.GetValue().entry, aDecoder.GetSubjectDescriptor(), + AccessControlCluster::ChangeTypeEnum::kAdded)); } ++i; } @@ -577,7 +605,7 @@ CHIP_ERROR AccessControlAttribute::WriteAcl(const ConcreteDataAttributePath & aP --oldCount; ReturnErrorOnFailure(GetAccessControl().ReadEntry(oldCount, entry, &accessingFabricIndex)); ReturnErrorOnFailure( - LogEntryChangedEvent(entry, aDecoder.GetSubjectDescriptor(), AccessControlCluster::ChangeTypeEnum::kRemoved)); + LogAclChangedEvent(entry, aDecoder.GetSubjectDescriptor(), AccessControlCluster::ChangeTypeEnum::kRemoved)); ReturnErrorOnFailure(GetAccessControl().DeleteEntry(oldCount, &accessingFabricIndex)); } } @@ -588,7 +616,7 @@ CHIP_ERROR AccessControlAttribute::WriteAcl(const ConcreteDataAttributePath & aP ReturnErrorOnFailure(GetAccessControl().CreateEntry(nullptr, item.entry, &accessingFabricIndex)); ReturnErrorOnFailure( - LogEntryChangedEvent(item.entry, aDecoder.GetSubjectDescriptor(), AccessControlCluster::ChangeTypeEnum::kAdded)); + LogAclChangedEvent(item.entry, aDecoder.GetSubjectDescriptor(), AccessControlCluster::ChangeTypeEnum::kAdded)); } else { @@ -605,6 +633,12 @@ CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePat FabricIndex accessingFabricIndex = aDecoder.AccessingFabricIndex(); + uint8_t buffer[kExtensionDataMaxLength] = { 0 }; + uint16_t size = static_cast(sizeof(buffer)); + CHIP_ERROR errStorage = storage.SyncGetKeyValue(key.AccessControlExtensionEntry(accessingFabricIndex), buffer, size); + VerifyOrDie(errStorage != CHIP_ERROR_BUFFER_TOO_SMALL); + ReturnErrorCodeIf(errStorage != CHIP_NO_ERROR && errStorage != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, errStorage); + if (!aPath.IsListItemOperation()) { DataModel::DecodableList list; @@ -615,41 +649,50 @@ CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePat if (count == 0) { - auto err = storage.SyncDeleteKeyValue(key.AccessControlExtensionEntry(kStorageVersion, accessingFabricIndex)); - ReturnErrorCodeIf(err != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, err); + ReturnErrorCodeIf(errStorage == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, CHIP_NO_ERROR); + ReturnErrorOnFailure(storage.SyncDeleteKeyValue(key.AccessControlExtensionEntry(accessingFabricIndex))); + AccessControlCluster::Structs::ExtensionEntry::Type item = { + .data = ByteSpan(buffer, size), + .fabricIndex = accessingFabricIndex, + }; + ReturnErrorOnFailure( + LogExtensionChangedEvent(item, aDecoder.GetSubjectDescriptor(), AccessControlCluster::ChangeTypeEnum::kRemoved)); } else if (count == 1) { auto iterator = list.begin(); - ReturnErrorCodeIf(!iterator.Next(), CHIP_ERROR_MISSING_TLV_ELEMENT); + if (!iterator.Next()) + { + ReturnErrorOnFailure(iterator.GetStatus()); + // If counted an item, iterator doesn't return it, iterator has no error, that's bad. + VerifyOrDie(true); + } auto & item = iterator.GetValue(); // TODO(#13590): generated code doesn't automatically handle max length so do it manually - ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorOnFailure(storage.SyncSetKeyValue(key.AccessControlExtensionEntry(kStorageVersion, accessingFabricIndex), - item.data.data(), static_cast(item.data.size()))); + ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_ERROR_INVALID_STRING_LENGTH); + ReturnErrorOnFailure(storage.SyncSetKeyValue(key.AccessControlExtensionEntry(accessingFabricIndex), item.data.data(), + static_cast(item.data.size()))); + ReturnErrorOnFailure(LogExtensionChangedEvent(item, aDecoder.GetSubjectDescriptor(), + errStorage == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND + ? AccessControlCluster::ChangeTypeEnum::kAdded + : AccessControlCluster::ChangeTypeEnum::kChanged)); } else { - // Only one item supported per fabric. - return CHIP_ERROR_INVALID_ARGUMENT; + return CHIP_ERROR_INVALID_LIST_LENGTH; } } else if (aPath.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem) { - { - uint8_t buffer[0]; - uint16_t size = static_cast(sizeof(buffer)); - auto err = - storage.SyncGetKeyValue(key.AccessControlExtensionEntry(kStorageVersion, accessingFabricIndex), buffer, size); - ReturnErrorCodeIf(err != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, err); - } - + ReturnErrorCodeIf(errStorage != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, CHIP_ERROR_INVALID_LIST_LENGTH); AccessControlCluster::Structs::ExtensionEntry::DecodableType item; ReturnErrorOnFailure(aDecoder.Decode(item)); // TODO(#13590): generated code doesn't automatically handle max length so do it manually - ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorOnFailure(storage.SyncSetKeyValue(key.AccessControlExtensionEntry(kStorageVersion, accessingFabricIndex), - item.data.data(), static_cast(item.data.size()))); + ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_ERROR_INVALID_STRING_LENGTH); + ReturnErrorOnFailure(storage.SyncSetKeyValue(key.AccessControlExtensionEntry(accessingFabricIndex), item.data.data(), + static_cast(item.data.size()))); + ReturnErrorOnFailure( + LogExtensionChangedEvent(item, aDecoder.GetSubjectDescriptor(), AccessControlCluster::ChangeTypeEnum::kAdded)); } else { diff --git a/src/lib/support/DefaultStorageKeyAllocator.h b/src/lib/support/DefaultStorageKeyAllocator.h index ccaa5e2c5c0570..cd565d6fcb3dc1 100644 --- a/src/lib/support/DefaultStorageKeyAllocator.h +++ b/src/lib/support/DefaultStorageKeyAllocator.h @@ -46,10 +46,7 @@ class DefaultStorageKeyAllocator const char * FailSafeContextKey() { return Format("g/fsc"); } // Access Control - const char * AccessControlExtensionEntry(size_t version, FabricIndex fabric) - { - return Format("a/%x/1/%x", static_cast(version), static_cast(fabric)); - } + const char * AccessControlExtensionEntry(FabricIndex fabric) { return Format("f/%x/ac/1", fabric); } const char * AccessControlList() { return Format("acl"); } const char * AccessControlEntry(size_t index) From be46ad25fd320696b320bfdbed77076a3b7d9d5c Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Mon, 4 Apr 2022 16:30:54 -0400 Subject: [PATCH 4/6] Relax VerifyOrDie Use CHIP_ERROR_INCORRECT_STATE instead. --- .../access-control-server/access-control-server.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp index b99eb238c076b8..58542baaa089ba 100644 --- a/src/app/clusters/access-control-server/access-control-server.cpp +++ b/src/app/clusters/access-control-server/access-control-server.cpp @@ -519,7 +519,7 @@ CHIP_ERROR AccessControlAttribute::ReadExtension(AttributeValueEncoder & aEncode uint8_t buffer[kExtensionDataMaxLength] = { 0 }; uint16_t size = static_cast(sizeof(buffer)); CHIP_ERROR errStorage = storage.SyncGetKeyValue(key.AccessControlExtensionEntry(fabric.GetFabricIndex()), buffer, size); - VerifyOrDie(errStorage != CHIP_ERROR_BUFFER_TOO_SMALL); + ReturnErrorCodeIf(errStorage == CHIP_ERROR_BUFFER_TOO_SMALL, CHIP_ERROR_INCORRECT_STATE); if (errStorage == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND) { continue; @@ -636,7 +636,7 @@ CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePat uint8_t buffer[kExtensionDataMaxLength] = { 0 }; uint16_t size = static_cast(sizeof(buffer)); CHIP_ERROR errStorage = storage.SyncGetKeyValue(key.AccessControlExtensionEntry(accessingFabricIndex), buffer, size); - VerifyOrDie(errStorage != CHIP_ERROR_BUFFER_TOO_SMALL); + ReturnErrorCodeIf(errStorage == CHIP_ERROR_BUFFER_TOO_SMALL, CHIP_ERROR_INCORRECT_STATE); ReturnErrorCodeIf(errStorage != CHIP_NO_ERROR && errStorage != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, errStorage); if (!aPath.IsListItemOperation()) @@ -665,7 +665,7 @@ CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePat { ReturnErrorOnFailure(iterator.GetStatus()); // If counted an item, iterator doesn't return it, iterator has no error, that's bad. - VerifyOrDie(true); + return CHIP_ERROR_INCORRECT_STATE; } auto & item = iterator.GetValue(); // TODO(#13590): generated code doesn't automatically handle max length so do it manually From 839c13ca3c0508a9ea97466f0b1834d690e077bb Mon Sep 17 00:00:00 2001 From: Marc Lepage <67919234+mlepage-google@users.noreply.github.com> Date: Mon, 4 Apr 2022 16:52:56 -0400 Subject: [PATCH 5/6] Update src/app/clusters/access-control-server/access-control-server.cpp Co-authored-by: Boris Zbarsky --- .../clusters/access-control-server/access-control-server.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp index 58542baaa089ba..05ce2a4cb6a82c 100644 --- a/src/app/clusters/access-control-server/access-control-server.cpp +++ b/src/app/clusters/access-control-server/access-control-server.cpp @@ -467,7 +467,7 @@ CHIP_ERROR LogExtensionChangedEvent(const AccessControlCluster::Structs::Extensi CHIP_ERROR err = LogEvent(event, 0, eventNumber); if (CHIP_NO_ERROR != err) { - ChipLogError(DataManagement, "AccessControlCluster: log event failed"); + ChipLogError(DataManagement, "AccessControlCluster: log event failed %" CHIP_ERROR_FORMAT, err.Format()); } return err; From 72f9d8ce7cfa33e099a3789ada0e0f649eadbfc7 Mon Sep 17 00:00:00 2001 From: Marc Lepage Date: Mon, 4 Apr 2022 17:09:22 -0400 Subject: [PATCH 6/6] Change some error codes For invalid octstr length and invalid list length, instead use constraint error. --- .../access-control-server/access-control-server.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp index 05ce2a4cb6a82c..537f9c9fb2e185 100644 --- a/src/app/clusters/access-control-server/access-control-server.cpp +++ b/src/app/clusters/access-control-server/access-control-server.cpp @@ -439,7 +439,7 @@ CHIP_ERROR LogAclChangedEvent(const AccessControl::Entry & entry, const Access:: err = LogEvent(event, 0, eventNumber); if (CHIP_NO_ERROR != err) { - ChipLogError(DataManagement, "AccessControlCluster: log event failed"); + ChipLogError(DataManagement, "AccessControlCluster: log event failed %" CHIP_ERROR_FORMAT, err.Format()); } return err; @@ -576,7 +576,8 @@ CHIP_ERROR AccessControlAttribute::WriteAcl(const ConcreteDataAttributePath & aP ReturnErrorOnFailure(list.ComputeSize(&newCount)); ReturnErrorOnFailure(GetAccessControl().GetMaxEntryCount(maxCount)); VerifyOrReturnError(allCount >= oldCount, CHIP_ERROR_INTERNAL); - VerifyOrReturnError(static_cast(allCount - oldCount + newCount) <= maxCount, CHIP_ERROR_INVALID_LIST_LENGTH); + VerifyOrReturnError(static_cast(allCount - oldCount + newCount) <= maxCount, + CHIP_IM_GLOBAL_STATUS(ConstraintError)); auto iterator = list.begin(); size_t i = 0; @@ -669,7 +670,7 @@ CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePat } auto & item = iterator.GetValue(); // TODO(#13590): generated code doesn't automatically handle max length so do it manually - ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_ERROR_INVALID_STRING_LENGTH); + ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_IM_GLOBAL_STATUS(ConstraintError)); ReturnErrorOnFailure(storage.SyncSetKeyValue(key.AccessControlExtensionEntry(accessingFabricIndex), item.data.data(), static_cast(item.data.size()))); ReturnErrorOnFailure(LogExtensionChangedEvent(item, aDecoder.GetSubjectDescriptor(), @@ -679,16 +680,16 @@ CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePat } else { - return CHIP_ERROR_INVALID_LIST_LENGTH; + return CHIP_IM_GLOBAL_STATUS(ConstraintError); } } else if (aPath.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem) { - ReturnErrorCodeIf(errStorage != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, CHIP_ERROR_INVALID_LIST_LENGTH); + ReturnErrorCodeIf(errStorage != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, CHIP_IM_GLOBAL_STATUS(ConstraintError)); AccessControlCluster::Structs::ExtensionEntry::DecodableType item; ReturnErrorOnFailure(aDecoder.Decode(item)); // TODO(#13590): generated code doesn't automatically handle max length so do it manually - ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_ERROR_INVALID_STRING_LENGTH); + ReturnErrorCodeIf(item.data.size() > kExtensionDataMaxLength, CHIP_IM_GLOBAL_STATUS(ConstraintError)); ReturnErrorOnFailure(storage.SyncSetKeyValue(key.AccessControlExtensionEntry(accessingFabricIndex), item.data.data(), static_cast(item.data.size()))); ReturnErrorOnFailure(