-
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
Allow subscriptions if wildcard expansion is empty. #34983
Closed
andy31415
wants to merge
6
commits into
project-chip:master
from
andy31415:allow_subscribe_to_future_expansion
+97
−75
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
3a6010b
Allow subscriptions if wildcard expansion is empty.
andy31415 ae22c25
Update logic a bit and fix up ACL denied tests
andy31415 29d8f4b
Avoid multiple boolean values as method arguments
andy31415 fbfef06
Fix unit test for read that was assuming we get an error on non-exist…
andy31415 8fa109f
restyle
andy31415 6945873
Merge branch 'master' into allow_subscribe_to_future_expansion
andy31415 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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.
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.
If there are valid paths, then we know the client has some level of access. If all paths are empty, we can instead check if the client has access to anything at all (i.e. there is some ACL entry referencing the client). Or we could check for read access to the Descriptor cluster. Arguably wildcard expansion implicitly requires read access to the descriptor cluster anyway.
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.
Is descriptor cluster access sufficient to check? If so we could update to that.
Otherwise generically I can only say "this is a real client, some data could be accessible at some time in the future theoretically" so if that is the case, we should never actually deny this as we do here. But that undoes the original PR that allows auto-rejection.
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.
Thinking about this more, maybe wildcard expansion should actually explicitly check for access to the descriptor cluster anyway? It's a little strange that we're allowing information from the Descriptor cluster to be revealed when access to it hasn't necessarily been granted. On the other hand starting to enforce this now might break existing ACLs out there. But if we're treating information from the Descriptor cluster as special in this way maybe we should make this explicit in the spec, e.g. by having read access to the descriptor cluster be implicitly granted on any endpoints the client can access.
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.
It's been a while since I waded into the spec (would need @tcarmelveilleux or @bzbarsky-apple's input here), but wildcard expansion is a server-side operation, and doesn't require the client have descriptor access I'd think. It's reasonable I'd think for a client to not have privilege to the descriptor cluster, and upon expansion, the server realize this.
A client could have access to just say, OnOff cluster, and request a wildcard, and correctly, be returned just the OnOff cluster on all matching endpoints.