Groupcast Access Control: EXPERIMENTAL #42066

rcasallas-silabs · 2025-11-20T17:52:52Z

Summary

Adds a wrapper to the AccessControl, so virtual ACL entries may be automatically added by the Groupcast cluster, without affecting the actual ACL entries managed by the Access Control cluster.

Related issues

Testing

Readability checklist

The checklist below will help the reviewer finish PR review in time and keep the
code readable:

PR title is
descriptive
Apply the
“When in Rome…”
rule (coding style)
PR size is short
Try to avoid "squashing" and "force-update" in commit history
CI time didn't increase

See: Pull Request Guidelines

Copilot

Pull Request Overview

This PR introduces an experimental groupcast access control feature by implementing a wrapper around the existing AccessControl system. The wrapper allows virtual ACL entries to be automatically managed by the Groupcast cluster without modifying the ACL entries handled by the Access Control cluster.

Key changes:

Adds a new PersistentArray template class for managing persistent storage of array data
Implements GroupcastAccessControlDelegate to manage groupcast-specific ACL entries
Introduces ExtendedAccessControlDelegate to combine primary and secondary access control delegates

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
src/lib/support/PersistentArray.h	New template class for persistent array storage with serialization support
src/lib/support/DefaultStorageKeyAllocator.h	Adds storage key allocation for groupcast access control data
src/app/server/Server.h	Integrates extended access control delegate into server initialization
src/app/server/Server.cpp	Declares static instance of ExtendedAccessControlDelegate
src/access/examples/GroupcastAccessControlDelegate.h	Header defining interface for groupcast access control
src/access/examples/GroupcastAccessControlDelegate.cpp	Implementation of groupcast-specific access control delegate
src/access/examples/ExtendedAccessControlDelegate.h	Header for delegate that combines primary and secondary access controls
src/access/examples/ExtendedAccessControlDelegate.cpp	Implementation of the extended access control wrapper
src/access/ExtendedAccessControlDelegate.h	Duplicate header file for extended access control delegate
src/access/ExtendedAccessControlDelegate.cpp	Duplicate implementation file for extended access control delegate
src/access/BUILD.gn	Updates build configuration to include new delegate files

Comments suppressed due to low confidence (1)

src/lib/support/PersistentArray.h:1

The equality operator should return true when fabric indices are equal, not when they are different. Change != to ==.

/*

Copilot · 2025-11-20T17:54:46Z

src/access/examples/GroupcastAccessControlDelegate.cpp

+
+    virtual CHIP_ERROR Next(Entry & entry) override
+    {
+        if(mIndex < sList.Count()) {


Missing space after if keyword. Should be if (mIndex < sList.Count()).

Suggested change

if(mIndex < sList.Count()) {

if (mIndex < sList.Count()) {

Copilot · 2025-11-20T17:54:47Z

src/access/examples/GroupcastAccessControlDelegate.cpp

+        // FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME
+        FabricIndex fabric_index = 1;
+        // FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME FIXME


Hardcoded fabric index value needs to be replaced with proper fabric index handling. This appears in multiple methods (GetEntryCount, CreateEntry, ReadEntry, UpdateEntry, DeleteEntry).

Copilot · 2025-11-20T17:54:47Z

src/access/examples/GroupcastAccessControlDelegate.cpp

+
+AccessControl::Delegate * GetAccessControlDelegate(PersistentStorageDelegate *storage) {
+    static AccessControlDelegate sDelegate;
+    if(storage) {


Missing space after if keyword. Should be if (storage).

Suggested change

if(storage) {

if (storage) {

Copilot · 2025-11-20T17:54:47Z

src/access/examples/ExtendedAccessControlDelegate.cpp

+    if(mFirst) {
+        mFirst->Release();
+    }
+    if(mSecond) {


Missing space after if keyword. Should be if (mFirst) and if (mSecond).

Copilot · 2025-11-20T17:54:48Z

src/access/examples/ExtendedAccessControlDelegate.cpp

+    if(CHIP_NO_ERROR == mError) {
+        mError = mFirst->Next(entry);
+    }
+    if(mSecond && (CHIP_ERROR_SENTINEL == mError)) {


Missing space after if keyword. Should be if (CHIP_NO_ERROR == mError) and if (mSecond && (CHIP_ERROR_SENTINEL == mError)).

Copilot · 2025-11-20T17:54:48Z

src/access/examples/ExtendedAccessControlDelegate.cpp

+// Capabilities
+CHIP_ERROR ExtendedAccessControlDelegate::GetMaxEntriesPerFabric(size_t & value) const
+{
+    ChipLogDetail(DeviceLayer, "~~~ ExtendedAccessControlDelegate::GetMaxEntriesPerFabric: #%p/%p\n", mPrimary, mSecondary);


Debug logging statements should be removed or converted to proper debug macros before merging experimental code.

Suggested change

ChipLogDetail(DeviceLayer, "~~~ ExtendedAccessControlDelegate::GetMaxEntriesPerFabric: #%p/%p\n", mPrimary, mSecondary);

Copilot · 2025-11-20T17:54:48Z

src/access/examples/ExtendedAccessControlDelegate.cpp

+// Iteration
+CHIP_ERROR ExtendedAccessControlDelegate::Entries(AccessControl::EntryIterator & iterator, const FabricIndex * fabricIndex) const {
+    VerifyOrReturnError(nullptr != mPrimary || nullptr != mSecondary, CHIP_ERROR_INCORRECT_STATE);
+    ChipLogDetail(DeviceLayer, "~~~ ExtendedAccessControlDelegate::Entries: #%p/%p\n", mPrimary, mSecondary);


Debug logging statements should be removed or converted to proper debug macros before merging experimental code.

Suggested change

ChipLogDetail(DeviceLayer, "~~~ ExtendedAccessControlDelegate::Entries: #%p/%p\n", mPrimary, mSecondary);

Copilot · 2025-11-20T17:54:49Z

src/access/examples/GroupcastAccessControlDelegate.cpp

+        entry.subject_count = 0;
+        memset(entry.subjects, 0x00, sizeof(entry.subjects));
+        entry.target_count = 0;
+        for(size_t i=0; i < kMaxTargets; i++) {


Missing spaces around operators. Should be for (size_t i = 0; i < kMaxTargets; i++).

Suggested change

for(size_t i=0; i < kMaxTargets; i++) {

for (size_t i = 0; i < kMaxTargets; i++) {

Copilot · 2025-11-20T17:54:49Z

src/access/BUILD.gn

+    "examples/ExtendedAccessControlDelegate.cpp",
+    "examples/ExtendedAccessControlDelegate.h",
+    "examples/GroupcastAccessControlDelegate.cpp",
+    "examples/GroupcastAccessControlDelegate.h",


Files are duplicated in both src/access/examples/ and src/access/ directories. These duplicate files (ExtendedAccessControlDelegate.h/cpp) should be consolidated to a single location.

Copilot · 2025-11-20T17:54:49Z

src/lib/support/PersistentArray.h

+        ReturnErrorOnFailure(this->Load());
+        for (size_t i = this->mCount; (this->mCount > 0) && (i > 0); --i)
+        {
+            return Remove(i - 1);


The loop will only execute once due to the unconditional return statement inside. The return should be changed to ReturnErrorOnFailure(Remove(i - 1)); to continue removing all entries.

Suggested change

return Remove(i - 1);

ReturnErrorOnFailure(Remove(i - 1));

github-actions · 2025-11-20T18:12:54Z

PR #42066: Size comparison from ee2ab5e to acd89dd

Increases above 0.2%:

platform	target	config	section	`ee2ab5e`	`acd89dd`	change	% change
realtek	light-switch-app	rtl8777g	FLASH	709256	715368	6112	0.9
			RAM	107148	111052	3904	3.6
	lighting-app	rtl8777g	FLASH	757960	764080	6120	0.8
			RAM	127296	131192	3896	3.1
stm32	light	STM32WB5MM-DK	FLASH	470592	475124	4532	1.0
			RAM	141352	145244	3892	2.8

Full report (3 builds for realtek, stm32)

platform	target	config	section	`ee2ab5e`	`acd89dd`	change	% change
realtek	light-switch-app	rtl8777g	FLASH	709256	715368	6112	0.9
			RAM	107148	111052	3904	3.6
	lighting-app	rtl8777g	FLASH	757960	764080	6120	0.8
			RAM	127296	131192	3896	3.1
stm32	light	STM32WB5MM-DK	FLASH	470592	475124	4532	1.0
			RAM	141352	145244	3892	2.8

tcarmelveilleux · 2025-11-21T12:38:09Z

src/access/ExtendedAccessControlDelegate.cpp

+}
+
+
+void ExtendedAccessControlDelegate::Release() {}


Extended should be renamed to Auxiliary everywhere

tcarmelveilleux · 2025-11-21T12:39:37Z

src/access/ExtendedAccessControlDelegate.cpp

+    AccessControl::EntryIterator first;
+    ReturnErrorOnFailure(mPrimary->Entries(first, fabricIndex)); // TODO: OPTIONAL fabricIndex
+    AccessControl::EntryIterator::Delegate *it1 = &first.GetDelegate();
+    // Secondary
+    AccessControl::EntryIterator second;
+    ReturnErrorOnFailure(mSecondary->Entries(second, fabricIndex)); 
+    AccessControl::EntryIterator::Delegate *it2 = &second.GetDelegate();


What difference between primary and secondary?

tcarmelveilleux · 2025-11-21T12:41:17Z

src/lib/support/DefaultStorageKeyAllocator.h

    }

+    // Groupcast
+    static StorageKeyName GroupcastAccess(chip::FabricIndex fabric) { return StorageKeyName::Formatted("f/%x/ga", fabric); }


What is stored there?

Groupcast Access Control.

acd89dd

Copilot AI review requested due to automatic review settings November 20, 2025 17:52

github-actions bot added lib app support labels Nov 20, 2025

Copilot AI reviewed Nov 20, 2025

View reviewed changes

tcarmelveilleux reviewed Nov 21, 2025

View reviewed changes

	for(size_t i=0; i < kMaxTargets; i++) {
	for (size_t i = 0; i < kMaxTargets; i++) {

		}


		void ExtendedAccessControlDelegate::Release() {}

Groupcast Access Control: EXPERIMENTAL #42066

Are you sure you want to change the base?

Groupcast Access Control: EXPERIMENTAL #42066

Uh oh!

Conversation

rcasallas-silabs commented Nov 20, 2025

Summary

Related issues

Testing

Readability checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 20, 2025

Uh oh!

tcarmelveilleux Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

tcarmelveilleux Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

tcarmelveilleux Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants