-
Notifications
You must be signed in to change notification settings - Fork 101
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
[Metadata] Review device profile edit function #453
Comments
@tonyespy Hi, great thanks for your advice, that's really helpful for me, but can you tell me where can I find out any corresponding documents indicates that existing resources or commands are not be allowed modifying? so our team can update it in time. |
@badboy-huaqiao I'm not sure if these restrictions are documented anywhere which is why I tagged @iain-anderson and @lenny-intel in my original description. |
@tonyespy thanks, did you run into any errors when you executed updating operation? I will create a backlog about this and update it after I finish the secure work. |
@tonyespy @cloudxxx8 @jpwhitemn @bnevis-i @lenny-intel , Hi all, sorry for the late reply, so now, do we have a general consensus? I will update it if there is a document tell me what properties of one device profile can be allowed to edit and what's not allowed. |
It's really up to the REST API to enforce integrity constraints on updating the device profile. I think it's a waste of time to do it in the GUI as that option provides no real protection. |
@bnevis-i I agree with you, the parameters validation of the API is indeed necessary on server side, server-side validation is more secure, the purpose of client-side validation is simply to reduce the number of request when parameters is invalid, reduce the pressure on the server-side and for a better user experience. both are ok to me, but I need a document that tells me what properties are not allowed to be edited if we think GUI should do that. |
Huaqiao - the following document outlines which changes can occur to the device profile: https://docs.edgexfoundry.org/2.2/design/adr/core/0021-Device-Profile-Changes/. Having said that, the metadata API should enforce any and all updates to the profile. So, the UI should call the appropriate API and allow the operation to succeed or fail per what the API enforces. If we find that the API is not in alignment with this document, then we should work with the core working group team to get it corrected. In this specific case, we should test to see if an element of the profile YAML can be changed via the UI that should not be allowed. Specifically, set the new StrictDeviceProfileChanges configuration on and then try to update a device profile by YAML with the following changes when the profile is already associated to a device: These should fail and the update of the device profile should be rejected when using the UI to change the profile. Oppositely, these changes are some that should be allowed always (regardless of what the profile is associated to): |
@jpwhitemn great thanks for your effort works, the below is my testing result:
|
@jpwhitemn I just saw that metatdata had been updated,here are the latest test results: change
|
@badboy-huaqiao |
@cloudxxx8 I tested it using the device profile of virtual-device |
we haven't completed the implementation, so it will be blocked |
@cloudxxx8 ok, looking forward to your team updates |
@lenny-intel @iain-anderson
The latest version (see details below) of the UI allows a user to edit a device profile's raw YAML. We should review this feature, as there's been an active discussion in both the devices and core working groups about whether or not device profiles can be edited. The current thinking has been that modifying a profile's metadata (description, labels, ...; but not name) is OK, and that it's also OK to add new device resources or commands, but that modifying existing resources or commands should not be allowed.
I think it would make sense for the UI to enforce these constraints as opposed to allowing raw edits to a profile's YAML.
Version:
I ran the UI using the edgex-ui snap from edge channel. Please note that the version of this snap incorrectly shows 1.0 vs. 2.0.1-dev.18). I also testing using the ireland version of the edgexfoundry snap:
The text was updated successfully, but these errors were encountered: