Hookup SubjectDescriptor in CommandHandler #12953
Merged
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
Get the subject descriptor (from the exchange context session handle) in the command handler so an access control check can be performed before executing a command.
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
kpschoedel,
LuDuda,
lzgrablic02,
mrjerryjohns,
msandstedt,
mspang and
pan-apple
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This stale pull request has been automatically closed. Thank you for your contributions. |
|
PR #12953: Size comparison from 65ba5df to aaf1632 Increases above 0.2%:
Increases (19 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
Decreases (8 builds for linux)
Full report (29 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
|
|
PR #12953: Size comparison from 585cdcb to 718acac Increases (16 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
Full report (21 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
|
|
PR #12953: Size comparison from 34e4032 to 4787bb0 Increases (16 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
Full report (21 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
|
|
Is there an override to bypass the permissions check? We can't enable actual ACL checks until our basic flows can actually distribute permissions in a fabric. I would think at a minimum this means we need the commissioner to allow writing an arbitrary AdminSubject and we need CATs supported as subjects. |
|
@msandstedt don't worry, ACL won't be turned on until it's ready to use. |
msandstedt
approved these changes
Jan 12, 2022
|
This PR is just to get the actual subject descriptor from the underlying exchange context, which must always be present. However, we have tests which violate that, by not providing a proper exchange context or mock. This is why CI tests fail on this PR. Now that I got a few other things done, I'll start to look at getting the tests running with this PR. |
Unit tests did not always provide an exchange context for tests of command interaction. This required some special handling for test cases in actual command handling code, and blocked cases (e.g. access control) which require an exchange context. So provide an exchange context even in test cases.
|
OK, I spent the entire day spelunking through TestCommandInteraction.cpp and I think I've managed to add an exchange context for the test cases that did not have it. I removed the special check for an exchange context in test cases in CommandHandler::ProcessInvokeRequest and replaced it with a VerifyOrReturnError. @bzbarsky-apple @yunhanw-google @erjiaqing Tests do seem to pass locally (will see how CI goes) and I was still able to read/write attributes using REPL with all-clusters-app on Linux. |
|
PR #12953: Size comparison from 2d6d1a3 to ebf5202 Increases (26 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
As discussed on Slack, seems fine for now. |
|
Rebase to pull in some fixes for Darwin CI tests. |
|
PR #12953: Size comparison from 827fabd to a10fa63 Increases (26 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
selissia
pushed a commit
to selissia/connectedhomeip
that referenced
this pull request
Jan 28, 2022
Get the subject descriptor (from the exchange context session handle) in the command handler so an access control check can be performed before executing a command. Unit tests did not always provide an exchange context for tests of command interaction. This required some special handling for test cases in actual command handling code, and blocked cases (e.g. access control) which require an exchange context. So provide an exchange context even in test cases.
step0035
pushed a commit
to hank820/connectedhomeip
that referenced
this pull request
Feb 8, 2022
Get the subject descriptor (from the exchange context session handle) in the command handler so an access control check can be performed before executing a command. Unit tests did not always provide an exchange context for tests of command interaction. This required some special handling for test cases in actual command handling code, and blocked cases (e.g. access control) which require an exchange context. So provide an exchange context even in test cases.
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
SubjectDescriptor needs to be hooked up in CommandHandler
so AccessControl check can be performed before executing command.
Change overview
Get the SubjectDescriptor from the ExchangeContext/SessionHandle.
Testing
Testing is currently an issue, they seem to run CommandHandler
without having an ExchangeContext/SessionHandle.