Skip to content
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

Open
tonyespy opened this issue Oct 9, 2021 · 13 comments
Open

[Metadata] Review device profile edit function #453

tonyespy opened this issue Oct 9, 2021 · 13 comments
Labels
2-medium priority denoting issues with cross-cutting project impact

Comments

@tonyespy
Copy link
Member

tonyespy commented Oct 9, 2021

@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:

$ snap list | grep edgex
edgex-ui           1.0                 23     latest/edge      canonical*  -
edgexfoundry       2.0.0-2             3196   2.0/stable       canonical*  -
@badboy-huaqiao
Copy link
Member

@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.

@tonyespy
Copy link
Member Author

@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.

@badboy-huaqiao
Copy link
Member

@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.

@badboy-huaqiao badboy-huaqiao added 2-medium priority denoting issues with cross-cutting project impact jakarta labels Oct 14, 2021
@badboy-huaqiao
Copy link
Member

@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.

@bnevis-i
Copy link
Collaborator

@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.

@badboy-huaqiao
Copy link
Member

badboy-huaqiao commented Mar 22, 2022

@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.

@jpwhitemn
Copy link
Member

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:
Change the name of a device resource
Change the value type of a device resource

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):
adding new device resources to a device profile
changing the description of a device profile.

@badboy-huaqiao
Copy link
Member

@jpwhitemn great thanks for your effort works, the below is my testing result:

  • I didn't find StrictDeviceProfileChanges metadata configuration
  • Change the name of a device resource is not allowed
  • Change the value type of a device resource is allowed
  • adding new device resources to a device profile is allowed
  • changing the description of a device profile is allowed

@badboy-huaqiao
Copy link
Member

badboy-huaqiao commented Apr 11, 2022

@jpwhitemn I just saw that metatdata had been updated,here are the latest test results:

change StrictDeviceProfileChanges to true

  • Change the name of a device resource is not allowed
  • Change the value type of a device resource is allowed
  • adding new device resources to a device profile is allowed
  • changing the description of a device profile is allowed

@cloudxxx8
Copy link
Member

@badboy-huaqiao
Change the value type of a device resource is not allowed, unless there is no Device associated to it and StrictDeviceProfileChanges = false
Change the name of a device resource is always not allowed

@badboy-huaqiao
Copy link
Member

@cloudxxx8 StrictDeviceProfileChanges = true and one device associated to it, but I stiil get:
Change the value type of a device resource is allowed

I tested it using the device profile of virtual-device

@cloudxxx8
Copy link
Member

we haven't completed the implementation, so it will be blocked

@badboy-huaqiao
Copy link
Member

@cloudxxx8 ok, looking forward to your team updates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2-medium priority denoting issues with cross-cutting project impact
Projects
None yet
Development

No branches or pull requests

5 participants