search

project-chip / connectedhomeip

Matter (formerly Project CHIP) creates more connections between more objects, simplifying development for manufacturers and increasing compatibility for consumers, guided by the Connectivity Standards Alliance.
https://buildwithmatter.com
Apache License 2.0
7.5k stars 2.01k forks source link

Remaining M-ACL Issues #35165

Open tleacmcsa opened 2 months ago

tleacmcsa commented 2 months ago

These issues need to be addressed in a follow-up PR once #34932 merges:

for these, include #include <app-common/zap-generated/ids/Attributes.h> at the top and use app::Clusters::NetworkCommissioning::Id etc. rather than hard coding.

Missing tests: pase connection with a fabric index set (will happen after addnoc) case connection before commissioning complete has been called.

It would be good to have ARLs that differ only by the fabric, to ensure that the correct fabric ARL is being applied.

"So the way this is currently implemented, the client can get the event before it gets the command response. This is fine, and unavoidable, but the spec should probably have some informative language making this clear to spec readers."

and "I don't see floors for these constants defined in the spec. Or ceilings. There should probably be such definitions." for CHIPConfig.h

Limits from the spec should be checked: constexpr size_t kMaxInstructionStringLength = 512; constexpr size_t kMaxRedirectUrlStringLength = 256;

Tennessee Requested renaming all consts in TestAccessRestrictionProvider like listSelectionDuringCommissioningData to start with 'k'

Cecille pointed out that there are no limit checks in place for restrictions. Also, the Listener list should be switched over to use the new list mechanism instead of the old way it was modeled after.

tleacmcsa commented 2 months ago

@bzbarsky-apple I chatted with @chrisdecenzo about one of your comments in the SDK PR that was moved to be tracked here around limits on ARL counts and their Restrictions. Given that the ARLs are not writable and are set by the device vendor, we dont feel a limit makes sense. Controllers would be the consumers and they shouldnt be size bound either.

bzbarsky-apple commented 2 months ago

@tleacmcsa In general, we do in fact have caps on the size of things clients might need to read, so clients know how much memory to allocate for it.

Some clients are not memory-bound, some are....

tleacmcsa commented 2 months ago

Is this something we should address in the spec and sdk, or just the sdk?

bzbarsky-apple commented 2 months ago

@tleacmcsa Which exact comment are you talking about?

This is why followups should link to the actual comments involved...

tcarmelveilleux commented 2 months ago

Also need to remove AccessRestrictionEntryChanged, and make sure ReviewFabricRestrictions has response command indicated, and update fields in FabricRestrictionReviewUpdate to be optional rather than nullable.