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.
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
Use DataModel::Provider provided command information to handle command validation #35897
base: master
Are you sure you want to change the base?
Use DataModel::Provider provided command information to handle command validation #35897
Changes from 15 commits
17dcef6
3ed30ea
6bd969d
b63c918
0912e9a
6386bf7
4188ae5
af7552f
129ad24
b08d16e
03dcc81
d197a94
f83397f
4ec72e5
3d3e7f6
5ca78df
e03197d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 comment is no longer true. For one thing, there is no "error" here at all anymore.
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.
Should probably be named CheckCommandExistence.
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 know you just copied this, but endpoint ids should generally be decimal, not hex....
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.
Should probably be called CheckCommandAccess? Especially because it checks more than just ACL....
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.
CheckCommandFlags.
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 is that subjectDescriptor field optional at all? That seems like very strange API.
I guess for some read/write bits right now it doesn't exit. But for commands it absolutely always must, and I think just aborting here if it does not is fine.
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 very much not going to work in a world where the ACL check has to come before the existence check (which is the world of the spec).
Can we please write this code so that it won't break when these two checks are reordered in the callsite?
Now how this is supposed to work... the spec does not in fact define it (needs a spec issue, note @tcarmelveilleux) but would think that you should default to Operate privilege for purposes of the ACL check if the command is not known.
Note that the Ember codepath in the
#else
does exactly that: if not known, uses Operate. We should not have behavior differences between the two codepaths.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.
Enough "should not" that we should just abort if it does?
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 do have to ask: why do we need this if we have a subject descriptor member already? What we should have is a getter for the accessing fabric index, which gets it from our subject descriptor, instead of having duplicated members.