[Test Failed] TestAccessControlCluster.yaml is expecting wrong error on "too many subjects/targets"

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.

Apache License 2.0

7.38k stars 1.98k forks source link

Test issue(s)

I know that the test cases that are not listed in "certification are chip internal test cases, but I assume they are still executed.

If they are then there is an issue in my opinion vs Specification.

The test steps about "too many subjects/targets" (e.g. https://github.com/project-chip/connectedhomeip/blob/master/src/app/tests/suites/TestAccessControlCluster.yaml#L375) are expecting that FAILURE is returned as error code.

The specification requires:

An attempt to create an entry with more subjects than the node can support SHALL result in a RESOURCE_EXHAUSTED error and the entry SHALL NOT be created.

and

An attempt to create an entry with more targets than the node can support SHALL result in a RESOURCE_EXHAUSTED error and the entry SHALL NOT be created.

In my understanding this "more targets then the node supports" should be read exactly about the determination of the maSubjectsPerAccessControlEntry and maxTargetsPerAccessControlEntry properties, right?

Ingo

Platform

No response

Anything else?

No response

Yes, this is clearly buggy in the cluster impl and worse yet in the general access control APIs. It does:

            if (i < oldCount)
            {
                ReturnErrorOnFailure(GetAccessControl().UpdateEntry(&aDecoder.GetSubjectDescriptor(), accessingFabricIndex, i,
                                                                    iterator.GetValue().GetEntry()));
            }
            else
            {
                ReturnErrorOnFailure(GetAccessControl().CreateEntry(&aDecoder.GetSubjectDescriptor(), accessingFabricIndex, nullptr,
                                                                    iterator.GetValue().GetEntry()));
            }

where CreateEntry and UpdateEntry have logic like so:

    ReturnErrorCodeIf(!IsValid(entry), CHIP_ERROR_INVALID_ARGUMENT);

IsValid() checks various stuff that should lead to CONSTRAINT_ERROR bits or whatnot, not ERROR, but the thing it's returning will get mapped to Error.

Then CreateEntry calls into the access control delegate, which will return whatever it returns. It seems to be doing things like:

ReturnErrorCodeIf(subjectCount > EntryStorage::kMaxSubjects, CHIP_ERROR_BUFFER_TOO_SMALL);

in ExampleAccessControlDelegate, so could conceivably be changed to return the right IM error, but the API is entirely undocumented so other implementations of the delegate may well keep doing the wrong thing.

So we need to:

Really improve the tests here.
Make sure there are cert tests for this (there clearly are not right now).
Fix the implementation and/or APIs to follow the spec.

project-chip / connectedhomeip