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

Add custom required privilege definitions for clusters that need them #14419

Closed
mlepage-google opened this issue Jan 27, 2022 · 6 comments · Fixed by #16327
Closed

Add custom required privilege definitions for clusters that need them #14419

mlepage-google opened this issue Jan 27, 2022 · 6 comments · Fixed by #16327
Assignees
Labels
acl Access Control feature V1.0

Comments

@mlepage-google
Copy link
Contributor

mlepage-google commented Jan 27, 2022

IM operations have default required privileges (e.g. view for read attribute, operate for write attribute) but can override these defaults with custom required privileges (e.g. administer for anything to do with Access Control Cluster).

This functionality was intended to be provided by ZAP (#10246) and Ember (#10247). In the interim, it is being provided by a simple class (#14283).

Regardless, there is still the task (this issue) of populating the data for the custom required privileges. Each cluster that has custom (non-default) required privileges will have to specify them in src/RequiredPrivilege.cpp (interim approach) and cluster XML (desired ZAP/Ember approach).

@mlepage-google mlepage-google added acl Access Control feature V1.0 labels Jan 27, 2022
@mlepage-google mlepage-google changed the title Add custom required privilege definitions Add custom required privilege definitions for clusters that need them Jan 27, 2022
@mlepage-google mlepage-google self-assigned this Jan 31, 2022
mlepage-google added a commit to mlepage-google/connectedhomeip that referenced this issue Mar 16, 2022
ZAPT file iterates over access definitions for app server clusters, for
attributes/commands/events, to generate parallel arrays of custom
privileges for read/write attribute, invoke command, and read event.

These are generated and built per-app, and linked in by the library's
RequiredPrivilege module. Weak implementations provide an empty default
(needed for testing).

Fixes project-chip#14419
mlepage-google added a commit that referenced this issue Mar 23, 2022
New zap template iterates over access definitions for app server
clusters, for attributes/commands/events, to generate parallel arrays of
custom privileges for read/write attribute, invoke command, and read
event.

New privilege storage source files provide an API to access the generated
data, and an implementation using the generated data.

The data is generated and the storage is built per-app.

The library (DM, IM, app common) RequiredPrivilege module now uses the
privilege-storage API to access populated data on a per-app basis. Weak
implementations of the privilege storage API provide a default implementation
lacking generated data, so test artifacts can be built.

Fixes #14419
@mlepage-google mlepage-google reopened this Apr 1, 2022
@mlepage-google
Copy link
Contributor Author

Reopening just so I have an issue to track adding the actual XML declarations for any remaining custom privileges. Normally this would be done by cluster authors, but it's easier overall for me to do them in one big swoop.

mlepage-google added a commit that referenced this issue Apr 1, 2022
This should cover the rest of the clusters in service and device
management, barring Network Commissioning Cluster and Time
Synchronization Cluster, which don't seem to be in use.

The group messaging YAML test attempted to group write an attribute
requiring administer privilege to write. This can never work, because
administer privilege is not permitted via group messaging. Therefore,
changed the test to use another attribute requiring only manage privilege,
and configured the tests to grant manage privilege to group messaging.

Part of #14419
chencheung pushed a commit to chencheung/connectedhomeip that referenced this issue Apr 6, 2022
This should cover the rest of the clusters in service and device
management, barring Network Commissioning Cluster and Time
Synchronization Cluster, which don't seem to be in use.

The group messaging YAML test attempted to group write an attribute
requiring administer privilege to write. This can never work, because
administer privilege is not permitted via group messaging. Therefore,
changed the test to use another attribute requiring only manage privilege,
and configured the tests to grant manage privilege to group messaging.

Part of project-chip#14419
chencheung pushed a commit to chencheung/connectedhomeip that referenced this issue Apr 6, 2022
This should cover the rest of the clusters in service and device
management, barring Network Commissioning Cluster and Time
Synchronization Cluster, which don't seem to be in use.

The group messaging YAML test attempted to group write an attribute
requiring administer privilege to write. This can never work, because
administer privilege is not permitted via group messaging. Therefore,
changed the test to use another attribute requiring only manage privilege,
and configured the tests to grant manage privilege to group messaging.

Part of project-chip#14419
@mlepage-google
Copy link
Contributor Author

I think after #17115 and #17134 PRs it's really only the pump control and configuration cluster that needs updating; that one's waiting for a small spec change regarding the privileges (which currently require manage for read as well as write, but probably will be updated to only require manage for read).

@mlepage-google
Copy link
Contributor Author

@mlepage-google
Copy link
Contributor Author

mlepage-google commented Apr 7, 2022

Probably should also wait for this other pump-configuration-and-control-cluster.xml change to land.
PR #17181

@tlykkeberg-grundfos
Copy link
Contributor

Will be fixed for pump-configuration-and-control-cluster in #17181

@mlepage-google
Copy link
Contributor Author

I do see @tlykkeberg-grundfos is handling the last access definitions for pump in #17181 so I will close this issue now. Thanks!

andrei-menzopol pushed a commit to andrei-menzopol/connectedhomeip that referenced this issue Apr 14, 2022
New zap template iterates over access definitions for app server
clusters, for attributes/commands/events, to generate parallel arrays of
custom privileges for read/write attribute, invoke command, and read
event.

New privilege storage source files provide an API to access the generated
data, and an implementation using the generated data.

The data is generated and the storage is built per-app.

The library (DM, IM, app common) RequiredPrivilege module now uses the
privilege-storage API to access populated data on a per-app basis. Weak
implementations of the privilege storage API provide a default implementation
lacking generated data, so test artifacts can be built.

Fixes project-chip#14419
andrei-menzopol pushed a commit to andrei-menzopol/connectedhomeip that referenced this issue Apr 14, 2022
This should cover the rest of the clusters in service and device
management, barring Network Commissioning Cluster and Time
Synchronization Cluster, which don't seem to be in use.

The group messaging YAML test attempted to group write an attribute
requiring administer privilege to write. This can never work, because
administer privilege is not permitted via group messaging. Therefore,
changed the test to use another attribute requiring only manage privilege,
and configured the tests to grant manage privilege to group messaging.

Part of project-chip#14419
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acl Access Control feature V1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants