Managed ACL: Add AccessRestrictionList support #34932
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
|
Review changes with SemanticDiff. Analyzed 1 of 40 files. File Information
|
|
PR #34932: Size comparison from e994cbf to 5501b22 Increases above 0.2%:
Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tleacmcsa
changed the title
|
PR #34932: Size comparison from ddf44dc to 5e96425 Increases above 0.2%:
Full report (10 builds for cc32xx, nrfconnect, qpg, stm32, tizen)
|
|
PR #34932: Size comparison from ddf44dc to 668fefe Increases above 0.2%:
Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #34932: Size comparison from 62139bf to 9ed7dcc Increases above 0.2%:
Full report (7 builds for cc32xx, qpg, stm32, tizen)
|
|
PR #34932: Size comparison from 62139bf to c16e6e2 Increases above 0.2%:
Full report (5 builds for cc32xx, stm32, tizen)
|
|
PR #34932: Size comparison from 62139bf to 3bba771 Increases above 0.2%:
Full report (46 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
|
PR #34932: Size comparison from 62139bf to 9766e30 Increases above 0.2%:
Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
chrisdecenzo
approved these changes
Aug 13, 2024
|
PR #34932: Size comparison from 128c37a to febc716 Increases above 0.2%:
Full report (5 builds for cc32xx, stm32, tizen)
|
|
PR #34932: Size comparison from 128c37a to 0739363 Increases above 0.2%:
Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tcarmelveilleux
approved these changes
Aug 26, 2024
|
PR #34932: Size comparison from 7ddba36 to cbcd55a Increases above 0.2%:
Full report (77 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
anush-apple
dismissed
bzbarsky-apple’s stale review
Clearing since outstanding issues have been cleared. Boris is on PTO and team is trying to hit deadline to merge today. If anything is missed, can follow up in a subsequent PT.
bzbarsky-apple
approved these changes
Aug 27, 2024
auto-merge was automatically disabled
August 27, 2024 14:24
Head branch was pushed to by a user without write access
|
PR #34932: Size comparison from d185778 to 31a7ddd Increases above 0.2%:
Full report (41 builds for bl602, bl702, bl702l, cyw30739, linux, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
|
PR #34932: Size comparison from d185778 to 141c555 Increases above 0.2%:
Full report (12 builds for nrfconnect, nxp, qpg, stm32, tizen)
|
src/app/clusters/access-control-server/access-control-server.cpp
Outdated
Show resolved
Hide resolved
|
PR #34932: Size comparison from d185778 to 3d976bf Increases above 0.2%:
Full report (77 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
PeterC1965
pushed a commit
to PeterC1965/connectedhomeip
that referenced
this pull request
Aug 28, 2024
* Add AccessRestrictionList support * Update src/access/AccessConfig.h Co-authored-by: C Freeman <cecille@google.com> * Reworked data manipulators and other cleanup * Fixed encode/decode so reading CommissioningARL and Arl attributes work * Reworked ARL storage Previously ARL related data was persisted in KVS. This has been removed and now the responsibility for managing/maintaining the related data (CommissioningARL and ARL attributes) is up to the app to set on AccessRestrictionProvider class. * Review fixes cleanup ArlEncoder interface. return error to client if arl review request fails return token to client in FabricRestrictionReviewUpdate * Fixed GetEntries vector pointer arg * Updated core restriction logic/integration * Restyled by clang-format * fixed include check for renamed AccessRestrictionProvider.h file * M-ACL updates - refactored AccessControl::Check into CheckACL and CheckARL - added placeholders for the upcoming CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL - extracted ARL exception processing to standalone class for better testing * Add plumbing for subject descriptor IsCommissioning field - Make session manager update that state on a message-per-message basis - Add tests Missing test: MRP test against a not-yet-committed fabric over CASE showing that IsCommissioning is true. * Fix crash * Use new IsCommissioning in ARL check * Updates for review comments * restyled * Review updates - fixed return type for some command failures - enhanced unit tests * restyled * Updated ARL tests per review comments * work around nuttx and jsoncpp contention * Review comments and nuttx build failure fix attempt * review updates --------- Co-authored-by: C Freeman <cecille@google.com> Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: tennessee.carmelveilleux@gmail.com <tennessee.carmelveilleux@gmail.com>
shgutte
pushed a commit
to shgutte/connectedhomeip
that referenced
this pull request
Sep 10, 2024
* Add AccessRestrictionList support * Update src/access/AccessConfig.h Co-authored-by: C Freeman <cecille@google.com> * Reworked data manipulators and other cleanup * Fixed encode/decode so reading CommissioningARL and Arl attributes work * Reworked ARL storage Previously ARL related data was persisted in KVS. This has been removed and now the responsibility for managing/maintaining the related data (CommissioningARL and ARL attributes) is up to the app to set on AccessRestrictionProvider class. * Review fixes cleanup ArlEncoder interface. return error to client if arl review request fails return token to client in FabricRestrictionReviewUpdate * Fixed GetEntries vector pointer arg * Updated core restriction logic/integration * Restyled by clang-format * fixed include check for renamed AccessRestrictionProvider.h file * M-ACL updates - refactored AccessControl::Check into CheckACL and CheckARL - added placeholders for the upcoming CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL - extracted ARL exception processing to standalone class for better testing * Add plumbing for subject descriptor IsCommissioning field - Make session manager update that state on a message-per-message basis - Add tests Missing test: MRP test against a not-yet-committed fabric over CASE showing that IsCommissioning is true. * Fix crash * Use new IsCommissioning in ARL check * Updates for review comments * restyled * Review updates - fixed return type for some command failures - enhanced unit tests * restyled * Updated ARL tests per review comments * work around nuttx and jsoncpp contention * Review comments and nuttx build failure fix attempt * review updates --------- Co-authored-by: C Freeman <cecille@google.com> Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: tennessee.carmelveilleux@gmail.com <tennessee.carmelveilleux@gmail.com>
andy31415
reviewed
Nov 25, 2024
Comment on lines
+198
to
+203
| if (IsCASESession() && mIsCaseCommissioningSession) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
This should be:
Suggested change
| if (IsCASESession() && mIsCaseCommissioningSession) | |
| { | |
| return true; | |
| } | |
| return false; | |
| return IsCASESession() && mIsCaseCommissioningSession; |
andy31415
reviewed
Nov 25, 2024
| CHIP_ERROR Stage() const; | ||
|
|
||
| Entry mEntry; | ||
| mutable StagingEntry mStagingEntry; |
There was a problem hiding this comment.
Please remove mutable usage as a followup
andy31415
reviewed
Nov 25, 2024
Comment on lines
+48
to
+50
| ReturnErrorOnFailure(chip::app::LogEvent(event, kRootEndpointId, eventNumber)); | ||
|
|
||
| return CHIP_NO_ERROR; |
There was a problem hiding this comment.
Suggested change
| ReturnErrorOnFailure(chip::app::LogEvent(event, kRootEndpointId, eventNumber)); | |
| return CHIP_NO_ERROR; | |
| return chip::app::LogEvent(event, kRootEndpointId, eventNumber); |
andy31415
reviewed
Nov 25, 2024
Comment on lines
+547
to
+550
| return result; | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Suggested change
| return result; | |
| } | |
| return result; | |
| } | |
| return result; |
andy31415
reviewed
Nov 25, 2024
| { | ||
| return CHIP_NO_ERROR; | ||
| } | ||
| else |
There was a problem hiding this comment.
ideally we should not have else after return (decreases nesting)
andy31415
reviewed
Nov 25, 2024
Comment on lines
+110
to
+111
| ReturnErrorOnFailure(mStagingEntry.Encode(writer, tag)); | ||
| return CHIP_NO_ERROR; |
There was a problem hiding this comment.
Throughout this should look like:
Suggested change
| ReturnErrorOnFailure(mStagingEntry.Encode(writer, tag)); | |
| return CHIP_NO_ERROR; | |
| return mStagingEntry.Encode(writer, tag); |
andy31415
reviewed
Nov 25, 2024
| mStagingEntry.cluster = mEntry.clusterId; | ||
| ReturnErrorOnFailure(StageEntryRestrictions(mEntry.restrictions, mStagingRestrictions, | ||
| sizeof(mStagingRestrictions) / sizeof(mStagingRestrictions[0]))); | ||
| mStagingEntry.restrictions = Span<StagingRestriction>(mStagingRestrictions, mEntry.restrictions.size()); |
There was a problem hiding this comment.
you use vector anyway. Are these frequently encoded? We can avoid mutable by letting the ToTLV encode things to a vector and then use a span into that vector and not require members of fixed sizes.
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.
Add Access Restriction support