-
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
Prevent Subscription to attributes with no access privilege and absence #22349
Merged
yunhanw-google
merged 3 commits into
project-chip:master
from
yunhanw-google:feature/prevent-sub-with-no-acl
Sep 8, 2022
Merged
Prevent Subscription to attributes with no access privilege and absence #22349
yunhanw-google
merged 3 commits into
project-chip:master
from
yunhanw-google:feature/prevent-sub-with-no-acl
Sep 8, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94e27ba
to
9b52fb9
Compare
ef05563
to
fecf743
Compare
bzbarsky-apple
approved these changes
Sep 7, 2022
yufengwangca
approved these changes
Sep 7, 2022
22a999d
to
de3b7ce
Compare
PR #22349: Size comparison from 383c416 to de3b7ce Increases above 0.2%:
Increases (44 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, psoc6, qpg, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (44 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, psoc6, qpg, telink)
|
0fa33e6
to
b8b83d6
Compare
PR #22349: Size comparison from 383c416 to b8b83d6 Increases above 0.2%:
Increases (44 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, psoc6, qpg, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (44 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, psoc6, qpg, telink)
|
6b15d05
to
61a2830
Compare
PR #22349: Size comparison from b4505ac to 61a2830 Increases above 0.2%:
Increases (31 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, psoc6, qpg, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (31 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, psoc6, qpg, telink)
|
76c8548
to
ba0d804
Compare
PR #22349: Size comparison from e1966f5 to ba0d804 Increases above 0.2%:
Increases (19 builds for bl602, cc13x2_26x2, k32w, linux, mbed, qpg, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (19 builds for bl602, cc13x2_26x2, k32w, linux, mbed, qpg, telink)
|
ba0d804
to
e88696b
Compare
PR #22349: Size comparison from e1966f5 to e88696b Increases above 0.2%:
Increases (35 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (35 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
1.0 accepted: this is fixing a 1.0 release blocker |
msandstedt
approved these changes
Sep 8, 2022
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Reads and Subscribes from nodes with no access still get serviced to completion (which for subs mean that a subscription is established). This ends up stealing precious resources from nodes that do have access.
Fixes: issue #18485
Change overview
Prevents reads/subs from moving past the processing stage to actual allocation of resources. A StatusResponse is returned immediately.
Caveat: This doesn't address lack of access for event paths. That will come separately. currently we don't have ways to expand wildcard event path and check if path exists, suggest to figure out this in post1.0. At this moment, subscription prevention with no access privilege and absence is the best effort we can go
Testing
add unit test to validate empty attribute paths and event paths in subscribe request
add unit test to validate non-exist concrete attribute path and event paths in subscribe request
add unit test to validate acl-denied attribute path and event path in subscribe request