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

[DRAFT] Managed ACL as a list of restrictions #34485

Closed
wants to merge 6 commits into from

Conversation

tleacmcsa
Copy link
Contributor

@tleacmcsa tleacmcsa commented Jul 24, 2024

This PR adds support for the HRAP "Managed ACL" feature which works through a new Access Restriction List.

It is a large PR and will be broken up into sensible chunks. The following PRs have parts of this large PR for better review:

These changes should mirror the spec PR: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/10010

The primary files (non-generated) related to this change are:

src/access/AccessControl.cpp
src/access/AccessControl.h
src/access/AccessRestriction.cpp
src/access/AccessRestriction.h
src/access/BUILD.gn
src/access/RequestPath.h
src/access/tests/TestAccessControl.cpp
src/app/CommandHandlerImpl.cpp
src/app/EventManagement.cpp
src/app/InteractionModelEngine.cpp
src/app/clusters/access-control-server/access-control-server.cpp
src/app/reporting/Engine.cpp
src/app/server/ArlStorage.cpp
src/app/server/ArlStorage.h
src/app/server/BUILD.gn
src/app/server/DefaultArlStorage.cpp
src/app/server/DefaultArlStorage.h
src/app/server/Server.cpp
src/app/server/Server.h
src/app/util/ember-compatibility-functions.cpp
src/lib/core/CHIPConfig.h
src/lib/support/DefaultStorageKeyAllocator.h

This draft should generally be "code complete" but has limited testing beyond manual testing with chip-tool and network-manager-app.

This draft mostly mirrors the existing ACL code, with the main exception of a lack of delegates since the logic is not expected to change. The storage mechanism may need the delegate design used by ACL, which can be discussed in comments and implemented later if needed.

Used alchemy 0.6.0 on spec sha b9fcda51251d165a58b0e7b0620ec17203cc72e1
Discarded changes unrelated to ARL.
ZAP was not happy with missing descriptions for the events.
Updated network-manager-app.zap to include the new ARL features, then
generated.  Added empty implementation of
emberAfAccessControlClusterReviewFabricRestrictionsCallback to main.cpp
@CLAassistant
Copy link

CLAassistant commented Jul 24, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be split up into several PRs, to make this sanely reviewable:

  1. Changes to the core ACL APIs and the required callsite changes to propagate more information along with the checks (e.g. AccessControl.h changes, but not including the AccessRestriction bits, the various src/app changes to event/command/reporting code, etc).
  2. Changes to the XML and the resulting codegen.
  3. Possibly changes to add the core ARL infrastructure (AccessRestriction) and its hookups to the ACL bits, if it's posssible to do this separately from item 4, which I expect it is.
  4. Changes to ACL cluster code to implement the new functionality.
  5. Changes to example apps.

Item 1 can happen in parallel with item 2. Item 3 probably depends on item 1 but not item 2. Item 4 depends on items 2 and 3, and item 5 depends on item 4. But there is some parallelism here, both in terms of review and in terms of iteration and getting things in, which will be valuable.

Automatically add ARL entry that blocks access to the wifi network passphrase request command
once the network-manager-app is commissioned.

If the network-manager-app receives the ReviewFabricRestrictions command, all ReviewFabricRestrictions
are removed.
Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way to review 167K added / 138K removed lines.
This needs to start with splitting at least codegen changes and then see about the rest. Even after that probably needs to be split into something incrementa.

@tleacmcsa
Copy link
Contributor Author

@andy31415 I agree and have started the split of the work in this draft into smaller PRs. There are two ready now as listed in the summary above, but the remaining changes would have dependencies on these PRs, and github is not good at that as far as I'm aware (I'm mostly a gerrit guy). I'm open to suggestions on how to represent the remaining work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants