From f2bcc15a7d08812413cf053ec526ac36e7cd22e1 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Thu, 10 Mar 2022 18:37:47 -0500 Subject: [PATCH] Allow runtime init of some `Server` members (2/2) (#16070) * Allow runtime init of some `Server` members (2/2) - This PR is on the path towards having Server::Server no longer statically initialize its members with storage, and instead relying on Server::Init(). This will simplify organization of unit tests and also the convergence of Controller/Server storage to address issue #16028 Issue #16028 This PR separates init of AccessControl so that the delegate need not be passed to the Constructor, which allows simpler splitting of storage init and decoupling from Server::Server() static inializer list. This is a structural, non-functional change, touching only AccessControl. Testing - Ran cert tests, still pass - Ran unit tests, still pass * Restyled by clang-format * Address review comments * Restyled by clang-format * Address review comments to remove mIsInitialized * Restyled by clang-format Co-authored-by: Restyled.io --- src/access/AccessControl.cpp | 32 ++++++++--- src/access/AccessControl.h | 57 ++++++++++++++----- .../examples/ExampleAccessControlDelegate.cpp | 4 +- .../examples/ExampleAccessControlDelegate.h | 4 +- .../PermissiveAccessControlDelegate.cpp | 4 +- .../PermissiveAccessControlDelegate.h | 2 +- src/access/tests/TestAccessControl.cpp | 5 +- src/app/server/Server.cpp | 9 ++- src/app/tests/AppTestContext.cpp | 6 +- 9 files changed, 86 insertions(+), 37 deletions(-) diff --git a/src/access/AccessControl.cpp b/src/access/AccessControl.cpp index b193b0fb0716be..44a880965b74ee 100644 --- a/src/access/AccessControl.cpp +++ b/src/access/AccessControl.cpp @@ -161,37 +161,53 @@ namespace Access { AccessControl::Entry::Delegate AccessControl::Entry::mDefaultDelegate; AccessControl::EntryIterator::Delegate AccessControl::EntryIterator::mDefaultDelegate; -AccessControl::Delegate AccessControl::mDefaultDelegate; -CHIP_ERROR AccessControl::Init() +CHIP_ERROR AccessControl::Init(AccessControl::Delegate * delegate) { + VerifyOrReturnError(!IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + ChipLogProgress(DataManagement, "AccessControl: initializing"); - return mDelegate.Init(); + + // delegate can never be null. This was already checked + CHIP_ERROR retval = delegate->Init(); + if (retval == CHIP_NO_ERROR) + { + mDelegate = delegate; + } + + return retval; } CHIP_ERROR AccessControl::Finish() { + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); ChipLogProgress(DataManagement, "AccessControl: finishing"); - return mDelegate.Finish(); + CHIP_ERROR retval = mDelegate->Finish(); + mDelegate = nullptr; + return retval; } CHIP_ERROR AccessControl::Check(const SubjectDescriptor & subjectDescriptor, const RequestPath & requestPath, Privilege requestPrivilege) { + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + #if CHIP_PROGRESS_LOGGING { - char buf[6 * kCharsPerCatForLogging]; + constexpr size_t kMaxCatsToLog = 6; + char catLogBuf[kMaxCatsToLog * kCharsPerCatForLogging]; ChipLogProgress(DataManagement, "AccessControl: checking f=%u a=%c s=0x" ChipLogFormatX64 " t=%s c=" ChipLogFormatMEI " e=%" PRIu16 " p=%c", subjectDescriptor.fabricIndex, GetAuthModeStringForLogging(subjectDescriptor.authMode), ChipLogValueX64(subjectDescriptor.subject), - GetCatStringForLogging(buf, sizeof(buf), subjectDescriptor.cats), ChipLogValueMEI(requestPath.cluster), - requestPath.endpoint, GetPrivilegeStringForLogging(requestPrivilege)); + GetCatStringForLogging(catLogBuf, sizeof(catLogBuf), subjectDescriptor.cats), + ChipLogValueMEI(requestPath.cluster), requestPath.endpoint, GetPrivilegeStringForLogging(requestPrivilege)); } #endif // TODO(#13867): this will go away - if (mDelegate.TemporaryCheckOverride()) + if (mDelegate->TemporaryCheckOverride()) { ChipLogProgress(DataManagement, "AccessControl: temporary check override (this will go away)"); return CHIP_NO_ERROR; diff --git a/src/access/AccessControl.h b/src/access/AccessControl.h index d6f7254fa52d0b..ce31345bb0256c 100644 --- a/src/access/AccessControl.h +++ b/src/access/AccessControl.h @@ -22,6 +22,7 @@ #include "RequestPath.h" #include "SubjectDescriptor.h" +#include "lib/support/CodeUtils.h" #include namespace chip { @@ -353,19 +354,27 @@ class AccessControl AccessControl() = default; - AccessControl(Delegate & delegate) : mDelegate(delegate) {} - AccessControl(const AccessControl &) = delete; AccessControl & operator=(const AccessControl &) = delete; - ~AccessControl() { mDelegate.Release(); } + ~AccessControl() + { + // Never-initialized AccessControl instances will not have the delegate set. + if (IsInitialized()) + { + mDelegate->Release(); + } + } /** * Initialize the access control module. Must be called before first use. * - * @retval various errors, probably fatal. + * @param delegate - The delegate to use for acces control + * + * @return CHIP_NO_ERROR on success, CHIP_ERROR_INCORRECT_STATE if called more than once, + * CHIP_ERROR_INVALID_ARGUMENT if delegate is null, or other fatal error. */ - CHIP_ERROR Init(); + CHIP_ERROR Init(AccessControl::Delegate * delegate); /** * Deinitialize the access control module. Must be called when finished. @@ -373,10 +382,18 @@ class AccessControl CHIP_ERROR Finish(); // Capabilities - CHIP_ERROR GetMaxEntryCount(size_t & value) const { return mDelegate.GetMaxEntryCount(value); } + CHIP_ERROR GetMaxEntryCount(size_t & value) const + { + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + return mDelegate->GetMaxEntryCount(value); + } // Actualities - CHIP_ERROR GetEntryCount(size_t & value) const { return mDelegate.GetEntryCount(value); } + CHIP_ERROR GetEntryCount(size_t & value) const + { + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + return mDelegate->GetEntryCount(value); + } /** * Prepares an entry. @@ -385,7 +402,11 @@ class AccessControl * * @param [in] entry Entry to prepare. */ - CHIP_ERROR PrepareEntry(Entry & entry) { return mDelegate.PrepareEntry(entry); } + CHIP_ERROR PrepareEntry(Entry & entry) + { + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + return mDelegate->PrepareEntry(entry); + } /** * Creates an entry in the access control list. @@ -397,7 +418,8 @@ class AccessControl CHIP_ERROR CreateEntry(size_t * index, const Entry & entry, FabricIndex * fabricIndex = nullptr) { ReturnErrorCodeIf(!IsValid(entry), CHIP_ERROR_INVALID_ARGUMENT); - return mDelegate.CreateEntry(index, entry, fabricIndex); + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + return mDelegate->CreateEntry(index, entry, fabricIndex); } /** @@ -409,7 +431,8 @@ class AccessControl */ CHIP_ERROR ReadEntry(size_t index, Entry & entry, const FabricIndex * fabricIndex = nullptr) const { - return mDelegate.ReadEntry(index, entry, fabricIndex); + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + return mDelegate->ReadEntry(index, entry, fabricIndex); } /** @@ -422,7 +445,8 @@ class AccessControl CHIP_ERROR UpdateEntry(size_t index, const Entry & entry, const FabricIndex * fabricIndex = nullptr) { ReturnErrorCodeIf(!IsValid(entry), CHIP_ERROR_INVALID_ARGUMENT); - return mDelegate.UpdateEntry(index, entry, fabricIndex); + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + return mDelegate->UpdateEntry(index, entry, fabricIndex); } /** @@ -433,7 +457,8 @@ class AccessControl */ CHIP_ERROR DeleteEntry(size_t index, const FabricIndex * fabricIndex = nullptr) { - return mDelegate.DeleteEntry(index, fabricIndex); + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + return mDelegate->DeleteEntry(index, fabricIndex); } /** @@ -444,7 +469,8 @@ class AccessControl */ CHIP_ERROR Entries(EntryIterator & iterator, const FabricIndex * fabricIndex = nullptr) const { - return mDelegate.Entries(iterator, fabricIndex); + VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + return mDelegate->Entries(iterator, fabricIndex); } /** @@ -458,10 +484,11 @@ class AccessControl CHIP_ERROR Check(const SubjectDescriptor & subjectDescriptor, const RequestPath & requestPath, Privilege requestPrivilege); private: + bool IsInitialized() const { return (mDelegate != nullptr); } + bool IsValid(const Entry & entry); - static Delegate mDefaultDelegate; - Delegate & mDelegate = mDefaultDelegate; + Delegate * mDelegate = nullptr; }; /** diff --git a/src/access/examples/ExampleAccessControlDelegate.cpp b/src/access/examples/ExampleAccessControlDelegate.cpp index 53a83edbea48ad..95c659a493c155 100644 --- a/src/access/examples/ExampleAccessControlDelegate.cpp +++ b/src/access/examples/ExampleAccessControlDelegate.cpp @@ -1310,11 +1310,11 @@ namespace chip { namespace Access { namespace Examples { -AccessControl::Delegate & GetAccessControlDelegate(PersistentStorageDelegate * storageDelegate) +AccessControl::Delegate * GetAccessControlDelegate(PersistentStorageDelegate * storageDelegate) { static AccessControlDelegate accessControlDelegate; accessControlDelegate.SetStorageDelegate(storageDelegate); - return accessControlDelegate; + return &accessControlDelegate; } } // namespace Examples diff --git a/src/access/examples/ExampleAccessControlDelegate.h b/src/access/examples/ExampleAccessControlDelegate.h index a6d095905e027c..add53e3114e5d8 100644 --- a/src/access/examples/ExampleAccessControlDelegate.h +++ b/src/access/examples/ExampleAccessControlDelegate.h @@ -30,9 +30,9 @@ namespace Examples { * not manage lifecycle considerations. * * @param storageDelegate Storage instance to access persisted ACL data. - * @return a reference to the AccessControl::Delegate singleton. + * @return a pointer to the AccessControl::Delegate singleton. */ -AccessControl::Delegate & GetAccessControlDelegate(PersistentStorageDelegate * storageDelegate); +AccessControl::Delegate * GetAccessControlDelegate(PersistentStorageDelegate * storageDelegate); } // namespace Examples } // namespace Access diff --git a/src/access/examples/PermissiveAccessControlDelegate.cpp b/src/access/examples/PermissiveAccessControlDelegate.cpp index 3ce22908e2ec9f..ddee48a9f48e48 100644 --- a/src/access/examples/PermissiveAccessControlDelegate.cpp +++ b/src/access/examples/PermissiveAccessControlDelegate.cpp @@ -69,10 +69,10 @@ namespace chip { namespace Access { namespace Examples { -AccessControl::Delegate & GetPermissiveAccessControlDelegate() +AccessControl::Delegate * GetPermissiveAccessControlDelegate() { static AccessControlDelegate accessControlDelegate; - return accessControlDelegate; + return &accessControlDelegate; } } // namespace Examples diff --git a/src/access/examples/PermissiveAccessControlDelegate.h b/src/access/examples/PermissiveAccessControlDelegate.h index bea5f24e616d07..0734e38ff61fe7 100644 --- a/src/access/examples/PermissiveAccessControlDelegate.h +++ b/src/access/examples/PermissiveAccessControlDelegate.h @@ -22,7 +22,7 @@ namespace chip { namespace Access { namespace Examples { -AccessControl::Delegate & GetPermissiveAccessControlDelegate(); +AccessControl::Delegate * GetPermissiveAccessControlDelegate(); } // namespace Examples } // namespace Access diff --git a/src/access/tests/TestAccessControl.cpp b/src/access/tests/TestAccessControl.cpp index 4b180b91f8e86f..da1000aa41902c 100644 --- a/src/access/tests/TestAccessControl.cpp +++ b/src/access/tests/TestAccessControl.cpp @@ -33,7 +33,7 @@ using Entry = AccessControl::Entry; using EntryIterator = AccessControl::EntryIterator; using Target = Entry::Target; -AccessControl accessControl(Examples::GetAccessControlDelegate(nullptr)); +AccessControl accessControl; constexpr ClusterId kOnOffCluster = 0x0000'0006; constexpr ClusterId kLevelControlCluster = 0x0000'0008; @@ -2133,8 +2133,9 @@ void TestUpdateEntry(nlTestSuite * inSuite, void * inContext) int Setup(void * inContext) { + AccessControl::Delegate * delegate = Examples::GetAccessControlDelegate(nullptr); SetAccessControl(accessControl); - GetAccessControl().Init(); + VerifyOrDie(GetAccessControl().Init(delegate) == CHIP_NO_ERROR); return SUCCESS; } diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index c1d5da11a95653..0c755679559874 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -107,12 +107,14 @@ Server::Server() : .devicePool = &mDevicePool, .dnsResolver = nullptr, }), - mGroupsProvider(mDeviceStorage), mAccessControl(Access::Examples::GetAccessControlDelegate(&mDeviceStorage)) + mGroupsProvider(mDeviceStorage) {} CHIP_ERROR Server::Init(AppDelegate * delegate, uint16_t secureServicePort, uint16_t unsecureServicePort, Inet::InterfaceId interfaceId) { + Access::AccessControl::Delegate * accessDelegate = nullptr; + mSecuredServicePort = secureServicePort; mUnsecuredServicePort = unsecureServicePort; mInterfaceId = interfaceId; @@ -145,7 +147,10 @@ CHIP_ERROR Server::Init(AppDelegate * delegate, uint16_t secureServicePort, uint SetGroupDataProvider(&mGroupsProvider); // Access control must be initialized after mDeviceStorage. - err = mAccessControl.Init(); + accessDelegate = Access::Examples::GetAccessControlDelegate(&mDeviceStorage); + VerifyOrExit(accessDelegate != nullptr, ChipLogError(AppServer, "Invalid access delegate found.")); + + err = mAccessControl.Init(accessDelegate); SuccessOrExit(err); Access::SetAccessControl(mAccessControl); diff --git a/src/app/tests/AppTestContext.cpp b/src/app/tests/AppTestContext.cpp index 817122c02f51ea..8fed737f995ff5 100644 --- a/src/app/tests/AppTestContext.cpp +++ b/src/app/tests/AppTestContext.cpp @@ -24,7 +24,7 @@ namespace { -chip::Access::AccessControl permissiveAccessControl(chip::Access::Examples::GetPermissiveAccessControlDelegate()); +chip::Access::AccessControl gPermissiveAccessControl; } // namespace @@ -36,8 +36,8 @@ CHIP_ERROR AppContext::Init() ReturnErrorOnFailure(Super::Init()); ReturnErrorOnFailure(chip::app::InteractionModelEngine::GetInstance()->Init(&GetExchangeManager())); - Access::SetAccessControl(permissiveAccessControl); - ReturnErrorOnFailure(Access::GetAccessControl().Init()); + Access::SetAccessControl(gPermissiveAccessControl); + ReturnErrorOnFailure(Access::GetAccessControl().Init(chip::Access::Examples::GetPermissiveAccessControlDelegate())); return CHIP_NO_ERROR; }