-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Implement ACL attribute read/write #12305
Implement ACL attribute read/write #12305
Conversation
Add empty cluster implementation to all-clusters-app example.
Converted to draft as dependent PRs still need to be merged first. |
Also note that only access-control-server.cpp changed (and its BUILD file), the rest is all what this should be rebased on top of. |
PR #12305: Size comparison from 8a2fd0d to 4561958 Increases above 0.2%:
Increases (2 builds for linux)
Full report (29 builds for efr32, k32w, linux, nrfconnect, qpg, telink)
|
PR #12305: Size comparison from 77258f7 to b8a6bef Increases above 0.2%:
Increases (2 builds for linux)
Full report (29 builds for efr32, k32w, linux, nrfconnect, qpg, telink)
|
For esp32 and mbed, for all-clusters-app.
Code gen isn't working yet for this possibility.
b8a6bef
to
a51525c
Compare
PR #12305: Size comparison from f7f1cc3 to a51525c Increases above 0.2%:
Increases (2 builds for linux)
Decreases (13 builds for efr32, k32w, linux, nrfconnect, qpg, telink)
Full report (29 builds for efr32, k32w, linux, nrfconnect, qpg, telink)
|
All checks passing. This PR will allow read and write of ACL list as a whole. |
fast track: pr open for a long time, created by domain owner |
@mlepage-google Can you please re-gen/fix conflicts? |
@woody-apple yep I was going to merge/regen/restyle/etc. after review/approval. I'll do it shortly when I get a moment between other PRs. |
PR #12305: Size comparison from a1cb341 to 31853e4 Decreases (6 builds for efr32, k32w, qpg, telink)
Full report (11 builds for efr32, k32w, qpg, telink)
|
It was TagBoundEncoder, but now it's ListEncodeHelper. Also had to fix the perfect forwarding in AttributeValueEncoder and ListEncodeHelper and other template functions in the file, so it doesn't try to copy its args.
PR #12305: Size comparison from a1cb341 to 12b0651 Increases above 0.2%:
Increases (11 builds for esp32, k32w, mbed, nrfconnect, p6)
Decreases (14 builds for efr32, k32w, mbed, nrfconnect, p6, telink)
Full report (28 builds for efr32, esp32, k32w, mbed, nrfconnect, p6, telink)
|
These static constexpr member variables were declared but not defined. In C++14 and lower, the definitions are required. Otherwise, undefined reference ensues. C++17 handles this but Matter is still using C++14. Not sure why this was working before...
PR #12305: Size comparison from a1cb341 to e19c1ad Increases (3 builds for k32w, qpg)
Decreases (3 builds for k32w, qpg, telink)
Full report (7 builds for k32w, qpg, telink)
|
PR #12305: Size comparison from 5c70530 to b7e997e Increases above 0.2%:
Increases (22 builds for esp32, k32w, linux, mbed, nrfconnect, p6, qpg)
Decreases (10 builds for efr32, mbed, p6, telink)
Full report (39 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is the 3KB RAM increase about what we expected for the access control bits as they are right now?
@@ -979,12 +979,12 @@ | |||
"mfgCode": null, | |||
"side": "server", | |||
"included": 1, | |||
"storageOption": "External", | |||
"storageOption": "RAM", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? This is going to take up space in the attr store that is completely unused...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the easiest way to get the tests running for now, because the attribute exists. Otherwise I have to go through and ensure all the "not yet implemented" parts have "some kind of implementation" sufficient to keep the tests happy. I made the length 4 just in case any test tried to write more than one, but I could change the length to 1 for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see PR #12690
|
||
struct AccessControlEntryCodec | ||
{ | ||
static CHIP_ERROR Convert(AuthMode from, AccessControlCluster::AuthMode & to) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This to me suggests that Access::AuthMode should be a type alias for AccessControlCluster::AuthMode, so all we need to do here is check that we're in range....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make the code a tiny bit smaller (how much? a few small switch statements would go away). But eventually (when we get beyond features and into optimization) I want to bit encode the auth mode and privilege together. That will optimize for the common runtime check case, whereas managing ACL entries occurs much less often.
return CHIP_NO_ERROR; | ||
} | ||
|
||
static CHIP_ERROR Convert(Privilege from, AccessControlCluster::Privilege & to) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, it feels like the code here could be much smaller/simpler if our types were aligned better.
CHIP_ERROR err = aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR { | ||
while (iterator.Next(codec.entry) == CHIP_NO_ERROR) | ||
{ | ||
encoder.Encode(codec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we ignoring failures here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll add checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see PR #12690
|
||
CHIP_ERROR AccessControlAttribute::ReadExtension(AttributeValueEncoder & aEncoder) | ||
{ | ||
return CHIP_NO_ERROR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we explicitly encode an empty list and not bloat our RAM use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can spin a PR for that, or change the XML length to 1 and reduce the octstr size, until we have a real implementation. Of the two temporary situations, which is preferable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see PR #12690
|
||
CHIP_ERROR AccessControlAttribute::WriteExtension(AttributeValueDecoder & aDecoder) | ||
{ | ||
return CHIP_NO_ERROR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a TODO, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have issues, extensions are #10252. I find if I add TODOs everywhere in the skeleton code, TODObot gets very talkative. The issue should be sufficient? (It's returning no error, because I'd prefer "not implemented", but tests that try to read/write every attribute complain.)
AccessControlAttribute gAttribute; | ||
|
||
AccessControl gAccessControl(Examples::GetAccessControlDelegate()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these should be static or inside an anonymous namespace. Preferably the latter, along with the various struct definitions above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree I'll do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see PR #12690
@mlepage-google See above? |
Problem
Access Control Cluster needs an implementation (issue #10254)
Change overview
Implement read and write of entire ACL attribute. (Not list indexes, not fabric indexing, not parts of entries...)
Includes temporary access control set up (which will be moved later).
Note that access control isn't persisted or hooked up to IM yet.
Testing
Tested by running chip-tool against chip-all-clusters-app to read and write ACL entries and inspect that operation is correct.