Issue 628: Fix inconsistent ownership validation in IoTivity-Lite. #630
Conversation
WalkthroughThe changes introduce a function to clear security-related data when device ownership cannot be established, ensuring consistent ownership validation. Additionally, hardcoded strings in security-related functions are replaced with defined constants for better maintainability and readability. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Device
participant Storage
participant SecurityManager
Device->>SecurityManager: Check Ownership
SecurityManager-->>Device: Ownership not established
SecurityManager->>Storage: Clear pstat, cred, acl, u_ids, ael data
Storage-->>SecurityManager: Data cleared
SecurityManager-->>Device: Security data cleared
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed. ℹ️ To verify your latest change (c27a4a6), label this PR with |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- api/oc_main.c (3 hunks)
Additional comments not posted (1)
api/oc_main.c (1)
303-308: Verify the implementation ofoc_sec_get_doxmand theownedproperty.Ensure that the new conditional logic in
main_load_resourcescorrectly handles the device's ownership status without unintended side effects.Verification successful
The verification process has provided the following insights:
- The
oc_sec_get_doxmfunction definition was not found, indicating it might be defined differently or in a different pattern than expected.- The
ownedproperty is widely used across the codebase, including in thesecurity/oc_doxm.cfile, which seems to handle the ownership status of devices.- The
main_set_resource_defaultsfunction was found, confirming its existence and usage.Given these findings, we need to ensure that the
oc_sec_get_doxmfunction is correctly implemented and that theownedproperty is appropriately used within this function. Additionally, we should verify that the conditional logic inmain_load_resourcescorrectly handles the device's ownership status without unintended side effects.To proceed, let's refine our search for the
oc_sec_get_doxmfunction and ensure its correct implementation.
The refined search has provided the following insights:
- The
oc_sec_get_doxmfunction is defined insecurity/oc_doxm.c.- The
ownedproperty is used within theoc_sec_get_doxmfunction and other parts of the codebase.Given these findings, we can confirm that the
oc_sec_get_doxmfunction and theownedproperty are correctly implemented and used. The conditional logic inmain_load_resourcesthat checks theownedstatus and callsmain_set_resource_defaultsif the device is not owned appears to be correctly handling the device's ownership status.Therefore, the review comment regarding the conditional logic in
main_load_resourcesis verified to be correct.
oc_sec_get_doxmfunction is defined insecurity/oc_doxm.c.- The
ownedproperty is used appropriately within the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of `oc_sec_get_doxm` and the `owned` property. # Test: Search for the function and property usage. Expect: Correct implementation and usage patterns. ast-grep --lang c --pattern $'oc_sec_get_doxm($_) { $$$ owned $$$ }'Length of output: 75
Script:
#!/bin/bash # Description: Verify the implementation and usage of `oc_sec_get_doxm` and the `owned` property. # Step 1: Search for the definition of the `oc_sec_get_doxm` function. ast-grep --lang c --pattern 'oc_sec_doxm_t *oc_sec_get_doxm($_) { $$$ }' # Step 2: Search for the usage of the `owned` property within the codebase. rg --word-regexp 'owned' # Step 3: Verify the usage of `main_set_resource_defaults` function. ast-grep --lang c --pattern 'void main_set_resource_defaults($_) { $$$ }'Length of output: 22400
Script:
#!/bin/bash # Description: Refine the search for the `oc_sec_get_doxm` function definition and verify its implementation. # Step 1: Search for the definition of the `oc_sec_get_doxm` function using a broader pattern. rg --word-regexp 'oc_sec_get_doxm' # Step 2: Verify the usage of the `owned` property within the `oc_sec_get_doxm` function. ast-grep --lang c --pattern 'oc_sec_doxm_t *oc_sec_get_doxm($_) { $$$ owned $$$ }'Length of output: 2156
|
I have read the CLA Document and I hereby sign the CLA. |
|
recheck |
|
recheck |
sp-milos
force-pushed
the
sp-milos/issue-628
branch
from
c27a4a6
to
d8b1a7e
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- api/oc_main.c (4 hunks)
- security/oc_acl_internal.h (1 hunks)
- security/oc_ael_internal.h (1 hunks)
- security/oc_cred_internal.h (1 hunks)
- security/oc_pstat_internal.h (1 hunks)
- security/oc_store.c (15 hunks)
- security/oc_u_ids_internal.h (1 hunks)
Files skipped from review due to trivial changes (5)
- security/oc_acl_internal.h
- security/oc_ael_internal.h
- security/oc_cred_internal.h
- security/oc_pstat_internal.h
- security/oc_u_ids_internal.h
Additional comments not posted (8)
api/oc_main.c (2)
276-298: The implementation and documentation ofmain_sec_clear_resourcesare clear and align well with the PR's objectives.
312-318: The usage ofmain_sec_clear_resourceswithin the ownership check is appropriate and well-placed.security/oc_store.c (6)
95-100: Replacing hardcoded strings with constants likeOCF_SEC_PSTAT_STORE_NAMEenhances maintainability and reduces error potential.
114-114: Using constants for storage names inoc_sec_dump_pstatis a good practice.
234-240: The consistent use of constants for storage names inoc_sec_load_credis commendable.
Line range hint
262-279: Good use of constants for storage names inoc_sec_dump_cred, enhancing code maintainability.
291-297: The consistent use of constants for storage names inoc_sec_load_aclis commendable.
Line range hint
318-335: Good use of constants for storage names inoc_sec_dump_acl, enhancing code maintainability.
sp-milos
force-pushed
the
sp-milos/issue-628
branch
from
d8b1a7e
to
4d25f75
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- api/oc_main.c (4 hunks)
- security/oc_acl_internal.h (1 hunks)
- security/oc_ael_internal.h (1 hunks)
- security/oc_cred_internal.h (1 hunks)
- security/oc_pstat_internal.h (1 hunks)
- security/oc_store.c (15 hunks)
- security/oc_u_ids_internal.h (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- api/oc_main.c
- security/oc_acl_internal.h
- security/oc_ael_internal.h
- security/oc_cred_internal.h
- security/oc_pstat_internal.h
- security/oc_store.c
- security/oc_u_ids_internal.h
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
~format |
|
A new ownership check was added to
oc_main.c. If the device is not owned based upon that check, all of its existing resources are reset before attempting to load them again.Fixes #628