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 <[email protected]>
project-chip · Apr 4, 2022 · 1076496 · 1076496
1 parent 66ea736
commit 1076496
Show file tree

Hide file tree

Showing 9 changed files with 86 additions and 37 deletions.
diff --git 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
@@ -22,6 +22,7 @@
 #include "RequestPath.h"
 #include "SubjectDescriptor.h"
 
+#include "lib/support/CodeUtils.h"
 #include <lib/core/CHIPCore.h>
 
 namespace chip {
@@ -353,30 +354,46 @@ 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.
      */
     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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
 }