Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TC-ACL-2.4]: Test case result output is failure but the expected result is constraint error #20672

Closed
aswathygrl opened this issue Jul 13, 2022 · 15 comments · Fixed by #20736
Closed
Assignees
Labels
acl Access Control feature cert blocker

Comments

@aswathygrl
Copy link

aswathygrl commented Jul 13, 2022

Test Case ID
TC-ACL-2.4
Describe Issue
Step 29: expected outcome is constraint error but result is failure.
Command used to Verify : ./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 3, "authMode": 1, "subjects": [], "targets":null}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.
Reason: Authmode is PASE RESERVED FOR FUTURE ,.
The subjects list SHALL NOT be empty if the entry’s
AuthMode is PASE.

Step 31: expected outcome is constraint error but result is failure .
Command used to Verify:./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 5, "authMode": 3, "subjects": [], "targets":null}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.
reason: field combination invalid that means Administer privilege with group authmode.

Step 32: expected outcome is constraint error but result is failure.
Command used to Verify:./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 6, "authMode": 2, "subjects": null, "targets":null}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.
Reason: in the privilege field tried with 6,7 ,8,9 values but failure.

Step 33: expected outcome is constraint error but result is failure. In the Authmode field tried the
values 4,5,6,7,8 values but result is failure.
Command used to Verify:./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 3, "authMode": 4, "subjects": [], "targets":null}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.

Step 34: expected outcome is constraint error but result is failure. Invalid subject element is found.
Command used to Verify:./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 3, "authMode": 2, "subjects": [0], "targets":null}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.

Step 35: expected outcome is constraint error but result is failure. Invalid subject element is found.
Command used to Verify:./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 3, "authMode": 2, "subjects": [18446744073709551615], "targets":null}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.

Step 36: expected outcome is constraint error but result is failure.
Command used to Verify:./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 3, "authMode": 2, "subjects": [42949672930000000], "targets": null}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.

Step 37:"expected outcome is constraint error but result is success:
Command used to Verify:./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 3, "authMode": 2, "subjects": [18446744073709486080], "targets":null}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.
tried with privilege 5 and authmode 2 then also getting failure instead of constraint error.
"
Step 38: expected outcome is constraint error but result is failure.
Command used to Verify: ./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 3, "authMode": 2, "subjects": null, "targets":[{ "cluster": null, "endpoint": null, "deviceType": null }]}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.
Reason: Target elements invalid (no field is present)

Step 42: expected outcome is constraint error but result is failure.
Command used to Verify:./chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null}, {"fabricIndex": 1, "privilege": 3, "authMode": 2, "subjects": null, "targets":[{ "cluster": null, "endpoint": 22, "deviceType": 33 }]}]' 1 0
-For the above command , as per spec, SDK should return Constraint error.
Reason: Target element Invalid( both endpoint and deviceTypes are present).
Steps that require review:
Step 29,31,32,33,34,35,36,37,38,42
Specification References:
https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/data_model/ACL-Cluster.adoc#attributes
https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/data_model/ACL-Cluster.adoc#6-error-handling

Test Plan Reference:-
https://github.com/CHIP-Specifications/chip-test-plans/blob/master/src/cluster/AccessControl.adoc#tc-acl-2-4-acl-attribute

@aswathygrl aswathygrl changed the title [TC-ACL-2.4] Test Case need to be revisited. [TC-ACL-2.4]: Test case result output is failure but the expected result is constraint error Jul 13, 2022
@bzbarsky-apple bzbarsky-apple added the acl Access Control feature label Jul 13, 2022
@andy31415
Copy link
Contributor

@aswathygrl is this a manual test only or can I run this via some chip-tool run test that is automated?

@sankarselvam
Copy link

@andy31415 ACL is not yet automated. You may need to run manually.

@andy31415
Copy link
Contributor

Manual commands I ran to reproduce (just the first, for now):

rm /tmp/chip_*
./out/linux-x64-all-clusters/chip-all-clusters-app

# separate terminal
./out/linux-x64-chip-tool/chip-tool pairing onnetwork 1 20202021
./out/linux-x64-chip-tool/chip-tool accesscontrol write acl '[{"fabricIndex": 1, "privilege": 5, "authMode": 2, "subjects": [112233], "targets": null},
{"fabricIndex": 1, "privilege": 3, "authMode": 1, "subjects": [], "targets":null}]' 1 0

TBH I believe this should be automatable ... we have other tests that that check for correct return code (I recently updated userlabel). Will look into having some automation here as well if at all possible.

@andy31415
Copy link
Contributor

@aswathygrl am looking to automate these tests.
For tests 36,37 and 38 the number is too large to be represented in an integer. Where do these tests come from? And is there a reason why we test with so many separate invalid large numbers? the tests seem the same to me, but since the lines seem very similar I may be missing something.

@andy31415
Copy link
Contributor

I see 18446744073709551615.to_s is 0xFFFFFFFFFFFFFFFF (max u64). I believe automation only supports 32-bit values which if very unfortunate :(

@andy31415
Copy link
Contributor

For step 36 I seem to get success in my local tests. I wonder if this is a 32/64 bit issue.

@andy31415
Copy link
Contributor

after my PR, 29 and 31 return constraint_error, but 32 does not, even though unit test is supposed to check for this... need to test more.

@andy31415
Copy link
Contributor

I had not compiled an updated chip tool. Will have to fix my PR a bit more.

@andy31415
Copy link
Contributor

@aswathygrl : Step 32: expected outcome is constraint error but result is failure.

Are you getting failure? I get success and am trying to figure out why. It says Privilege 6 is invalid ... but I also see privileges being a bitfield so privilege 6 is (4 | 2) which is "OPERATE | PROXY_VIEW". why is that test case supposed to fail?

@andy31415
Copy link
Contributor

@cjandhyala @aswathygrl got my automated tests passing, a lot of these now work EXCEPT:

  • step 32 - it passes for me. I am unclear why it should fail and the description does not explain. It says invalid privilege, but I do not see it as invalid. Can you help with referencing the spec explaining more

  • step 35,36,37 work for me (at least 35) ... I am unclear why they should fail/if they should fail. The numbers seem somewhat arbitrary (one is 0xfff...f but unsure what the rest are). Explanation on why these must fail would help.

I think the step 32, 35, 36 and 37 - please split into separate bugs. The root cause of each of the steps seems generally different enough. I have fixed the generic cases that were returning failures and converted the codes, but for the others we need more details and explanation about the tests themselves.

@sankarselvam
Copy link

and am trying to figure out why. It says Privilege 6 is invalid ... but I also see privileges being a bitfield so privilege 6 is (4 | 2) which is "OPERATE | P
for this test we need to get Constraint Error. BUt when we tested we got failure.

@sankarselvam
Copy link

  • for me. I am unclear why it should fail and the description does not explain. It says invalid privilege, but I do not see it as invalid. Can you help with referen

@andy31415 All the step 32 , 35 , 36 , 37 are come under Error handling. Hence we raised in the same issue.
Spec ref is: https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/data_model/ACL-Cluster.adoc#error-handling

Step 32: As the Privilege field is invalid value (not 1-5), we are expecting Constraint Error. But while testing we got Failure.

Step 35, 36 and 37 - Its supposed to get Constraint Error. Not the failure. We raised this ticket to get constraint Error.

Hope this clears your question. Do u still want us to raise a separate bug for Step 35 to 37?

@andy31415
Copy link
Contributor

Please split out in separate issues. I was unable to reproduce the 'failure' return in those cases.

The split out bugs should explain why the test case is expected to fail (what constraint is invalid and why)

@andy31415
Copy link
Contributor

I also had to fix a bunch of tests in TestAccessControlCluster.yaml

So some of these tests were automated, but seemingly wrong (they asked for FAILURE instead of CONSTRAINT_ERROR). We will want to consolidate tests.

@aswathygrl
Copy link
Author

@andy31415 I had raised separate issue tickets for TC-ACL-2.4 ,Test Steps 32,35.36,37 ,mentioned below
#20794
#20796
#20797
#20798

woody-apple pushed a commit to andy31415/connectedhomeip that referenced this issue Jul 16, 2022
woody-apple pushed a commit that referenced this issue Jul 18, 2022
…when the ACL entry is invalid) (#20736)

* Define a test for access control constraints

* Typgo fixes

* Restyle

* Fix some text and formatting

* Zap regen

* Make ACL cluster return constraint error if the ACL entry is not valid

* Add one more test: Step 31

* Zap regen for the new test

* Added the rest of the tests from #20672

* Restyle

* Removed some steps: too large numbers for subjects, cannot be represented

* Zap regen

* Restyle

* Fix build, add more ACL changes, zap regen after adjusting test case to match bug report

* Fix more things to return CONSTRAINT_ERROR

* Convert more invalid argument to constraint errors. This is not ideal and seems like a whack-a-mole bug fixing

* Restyle

* Fix comments in yaml

* Restyle

* Restyle messed things up. Corrected it

* One more comment fix

* Restyle

* Split out IM status code header and cpp into a separate library for layering purposes. Layering still not ideal though.

* Restyle

* Also update TestAccessControlCluster

* Zap regen

* Updated test ACL error codes a bit and zap regen

* Update logic to centrailize error code processing location

* Added unit test for step 35 as well (pass)

* Added even more tests and updated formatting of ACL a bit for readability

* Restyle

* One more test for invalid privilege

* Restyle
github-actions bot pushed a commit that referenced this issue Jul 18, 2022
…when the ACL entry is invalid) (#20736)

* Define a test for access control constraints

* Typgo fixes

* Restyle

* Fix some text and formatting

* Zap regen

* Make ACL cluster return constraint error if the ACL entry is not valid

* Add one more test: Step 31

* Zap regen for the new test

* Added the rest of the tests from #20672

* Restyle

* Removed some steps: too large numbers for subjects, cannot be represented

* Zap regen

* Restyle

* Fix build, add more ACL changes, zap regen after adjusting test case to match bug report

* Fix more things to return CONSTRAINT_ERROR

* Convert more invalid argument to constraint errors. This is not ideal and seems like a whack-a-mole bug fixing

* Restyle

* Fix comments in yaml

* Restyle

* Restyle messed things up. Corrected it

* One more comment fix

* Restyle

* Split out IM status code header and cpp into a separate library for layering purposes. Layering still not ideal though.

* Restyle

* Also update TestAccessControlCluster

* Zap regen

* Updated test ACL error codes a bit and zap regen

* Update logic to centrailize error code processing location

* Added unit test for step 35 as well (pass)

* Added even more tests and updated formatting of ACL a bit for readability

* Restyle

* One more test for invalid privilege

* Restyle
woody-apple added a commit that referenced this issue Jul 18, 2022
…when the ACL entry is invalid) (#20736) (#20890)

* Define a test for access control constraints

* Typgo fixes

* Restyle

* Fix some text and formatting

* Zap regen

* Make ACL cluster return constraint error if the ACL entry is not valid

* Add one more test: Step 31

* Zap regen for the new test

* Added the rest of the tests from #20672

* Restyle

* Removed some steps: too large numbers for subjects, cannot be represented

* Zap regen

* Restyle

* Fix build, add more ACL changes, zap regen after adjusting test case to match bug report

* Fix more things to return CONSTRAINT_ERROR

* Convert more invalid argument to constraint errors. This is not ideal and seems like a whack-a-mole bug fixing

* Restyle

* Fix comments in yaml

* Restyle

* Restyle messed things up. Corrected it

* One more comment fix

* Restyle

* Split out IM status code header and cpp into a separate library for layering purposes. Layering still not ideal though.

* Restyle

* Also update TestAccessControlCluster

* Zap regen

* Updated test ACL error codes a bit and zap regen

* Update logic to centrailize error code processing location

* Added unit test for step 35 as well (pass)

* Added even more tests and updated formatting of ACL a bit for readability

* Restyle

* One more test for invalid privilege

* Restyle

Co-authored-by: Andrei Litvin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acl Access Control feature cert blocker
Projects
None yet
5 participants