-
Notifications
You must be signed in to change notification settings - Fork 11
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
Edit SSP Details (UI) #200
Conversation
Add icons for cases of modifying and not modifying the metadata version property. Display editable TextField and the save and close icons when in the edit state.
Create HTTP PATCH request to call a backend service which will update an OSCAL document with the appropriate metadata changes.
Rename and modify variables as well as adjust function paramters to make code more readable and concise.
Modify the Typography component variant prop for the editable TextFields to reflect the type of information displayed.
Currently there was no update to the storybook or tests. I wanted to create the draft PR so comments could at least start coming in as needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is off to a really good start! Great work on getting the initial PATCH
call and mutability in here. I tried to catch as much as I could; I understand this is a draft so no worries if you were already tracking a few of these.
Biggest things are:
- the safer handling of query string params
- more secure way to determine the server-side URL
src/components/OSCALMetadata.js
Outdated
const startOfUrl = window.location.href.indexOf("?") + 5; | ||
const url = window.location.href.substring(startOfUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const startOfUrl = window.location.href.indexOf("?") + 5; | |
const url = window.location.href.substring(startOfUrl); | |
const urlParams = URLSearchParams(window.location.search); | |
const url = urlParams.get("url"); |
https://caniuse.com/urlsearchparams. I don't think we've been targeting IE 11 but I'm not sure if the other limitations are too narrow here. In any case, we should have window.location.search
available (https://caniuse.com/mdn-api_location_search)
src/components/OSCALMetadata.js
Outdated
metadata: { | ||
modifiedField: newValue, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so this is the biggest question I have about the server-side implementation; will this set the entire metadata
object to just "key": "value"
or will it keep the whole object with the single change made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, since we're dealing with an object "literal" here, the key is interpreted more like a string, so you'd be posting: "modifiedField": newValue
. Instead, to make the key more dynamic, you wrap it in []
. Not sure if I'm explaining this in a way that makes sense.
metadata: { | |
modifiedField: newValue, | |
}, | |
metadata: { | |
[modifiedField]: newValue, | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, partial updates are expected. From the back-end story:
As a developer building apps against the oscal-rest-service I want to be able to update only specific elements of an existing SSP via REST requests so that I can incrementally edit the SSP without transmitting the entire SSP on every call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that the request body needs to maintain the entire JSON model even if only one field is being updated. If this is for an SSP, metadata
needs to be within a system-security-plan
structure.
See the back-end tests for an example.
src/components/OSCALMetadata.js
Outdated
/* Since the browser url will be of the form | ||
* http://localhost:8080/system-security-plans?url=http://localhost:8080/oscal/v1/ssps/{uuid} | ||
* and we just want the URL defined in the url parameter of the browser url, we must find the | ||
* location of the ? and increment that value by 5 to get the start of the desired url. | ||
*/ | ||
const startOfUrl = window.location.href.indexOf("?") + 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be too fragile if we ever support other query parameters. If we can't use the strategy proposed in the other comment, we need to write (or find) a utility that does this based of grabbing the query string, splitting on &
, then by =
, and then handling multiple values correctly, and dumping it in a map-like object to query for a key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're also going to want to reconsider this on a march towards a production-quality solution. This would make it a little too easy to convince the app to talk to a random server and to display those values back to the user. Imagine I send a user to http://localhost:8080/system-security-plans?url=https://mymaliciousserver.com/
where that always returns some sort of awful malicious output. Of course, the server has to be stored/configured on the client side somewhere (usually we'd inject it during an npm run build
or something). The URL query string is too easy for a malicious actor to convince a user to click and therefore send data to (consider how much harder it is for a user to open up dev tools and change a global variable). Additionally, this isn't really the sort of thing a library should concern itself with; the library should be accepting the server URL as a parameter (or configuration value) and the application should determine how we know what that value is.
If we stick with this direction for getting the URL, because of the security implications, I'd prefer to see us open an issue to track the security fix before merging this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this will drastically change with EasyDynamics/easygrc#6, so we shouldn't spend too much time on it in this PR.
src/components/OSCALMetadata.js
Outdated
/* If we reach this point, then the OSCAL file has successfully been modified | ||
* and we want those changes to be visible to the user. | ||
*/ | ||
modifiableMetadata["last-modified"][1]( | ||
formatDate(result["last-modified"]) | ||
); | ||
modifiableMetadata[modifiedField].value[1](newValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have missed this in the API spec, but I do want to highlight that this relies on the assumption that the only modified values returned are the (single) field we sent and last-modified
; and that the field we send isn't a date. That may be fine for now.
What we probably want to do is move the logic for how to display/format/represent as far "down" as we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response of the PATCH request is the entire object. The incoming changes are merged with the existing, the object is persisted, re-fetched from persistence, then returned in the response.
@mikeisen1, the back-end is also responsible for updating the last-modified
field.
Things will be a bit more complicated here because the top-level component, i.e. OSCALSsp
should re-render the entire SSP given in the PATCH response, but this metadata component is currently handling the request.
src/components/OSCALMetadata.js
Outdated
const edit = modifiableMetadata[modifiedField].edit[0]; | ||
const setEdit = modifiableMetadata[modifiedField].edit[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do:
const [edit, setEdit] = modifiableMetadata[modifiedField].edit;
I am actually asking if this ES6 feature is something we support; not just blindly suggesting we do it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're only targeting a few metadata fields in this particular issue/PR, we want this to form the foundation for edits throughout the app and the current approach seems like it will lead to a lot of duplication of code as we expand the editing capabilities.
As mentioned in a few of the individual comments, we should consider moving some of this functionality into smaller, lower-level components.
We may also want to consider an approach where child components (like OSCALMetadata
) signal the top-level component to handle the PATCH
request, since the entire top-level component should be re-rendered after a successful update. That could take the form of passing something like an onSave
function down to child components, which may also need some form of JSON path so that the top-level component can properly construct the partial update request.
src/components/OSCALMetadata.js
Outdated
); | ||
} | ||
|
||
function displayEditIcons(modifiedField, modifiableMetadata) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it should be its own lower-level component that is only responsible for telling some other component to enter into editing mode.
Could we also do this for the editable text field? |
I like this approach because in using it, we can resolve the potential security issues with how we generate the PATCH request url. |
src/components/OSCALMetadata.js
Outdated
|
||
export default function OSCALControlGuidance(props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason this is named OSCALControlGuidance
and not OSCALMetadata
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a leftover from a copy/paste to me.
I would think so. We may need some form of variants for different sizes, etc. |
Add two new React subcomponents to be used currently and in future editing features. This reduces what would have been duplicated code in the future.
Create function that will be passed down to lower level components, which will provide it the necessary arguments to handle a PATCH request.
Write tests for the OSCALEditableTextField and OSCALModificationIcons components to verify they both function as expected. Add supporting test data and icon ids for those tests.
Add storybook documentation for the newly created components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note and context on linter failures
component: OSCALEditableTextField, | ||
}; | ||
|
||
const Template = (args) => <OSCALEditableTextField {...args} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #209, the linter was updated. Functions now have to be declared using the function foo() { ... }
format, per the lint rules.
Update function declarations to conform to the style of the updated linter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall making great progress here.
I may be getting confused with some of the variable and function naming.
Left several specific comments.
|
||
export default function OSCALEditableTextField(props) { | ||
return props.modifiableData.edit[0] ? ( | ||
<Typography variant={props.modifiableData.typographyVariant}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should typographyVariant
be a 'root' prop or under a textElement
prop rather than within modifiableData
?
"data-testid": `textField-${getElementLabel(props.editedField)}`, | ||
}} | ||
inputRef={props.modifiableData.ref} | ||
size={props.textFieldSize} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should textFieldSize
and textFieldVariant
be included in a textElement
root prop mentioned as a possibility above?
src/components/OSCALMetadata.js
Outdated
* updated, we want to update the last modified field. | ||
*/ | ||
const modifiableMetadata = { | ||
"last-modified": useState(formatDate(props.metadata["last-modified"])), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UI should not be able to try to change last-modified
. It will be updated by the backend service as part of the PATCH REST call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I overlooked that this needed to be updated. It was just leftover from the draft version of this PR when I was not sure how the response would look like.
src/components/OSCALMetadata.js
Outdated
const modifiableMetadata = { | ||
"last-modified": useState(formatDate(props.metadata["last-modified"])), | ||
version: { | ||
ref: useRef("Version TextField Reference"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these ref
instances required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are required to provide access to the TextField
values when we click on the save icon.
src/components/OSCALMetadata.js
Outdated
"last-modified": useState(formatDate(props.metadata["last-modified"])), | ||
version: { | ||
ref: useRef("Version TextField Reference"), | ||
edit: useState(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be something more descriptive like isInEditState
?
src/components/OSCALSsp.js
Outdated
* @param editedField path to the field that is being updated | ||
* @param newValue updated value for the edited field | ||
*/ | ||
function onSave(data, update, editedField, newValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this functionality can probably be more easily moved into OSCALLoader
after #226.
size={props.textFieldSize} | ||
variant={props.textFieldVariant} | ||
> | ||
{props.modifiableData.value} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be easier to conceptualize the code if the value were separate from the editable field definitions, i.e calling the component from OSCALMetadata
might look something like:
<OSCALEditableTextField
value={props.metadata.title}
fieldDefinition={editableFieldDefinitions.title}
/>
We also want to the text input to be populated with the current value when we enter edit mode, so the above approach may help there.
src/components/OSCALMetadata.js
Outdated
/* A JSON formatted variable so only one variable needs to be passed around between | ||
* functions that deals with the modification of metadata fields. | ||
* | ||
* For each top-level field, we have a value sub-field which is an array of two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing. We may need to revisit this approach after addressing other changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to get rid of this approach, as there are now very few fields in the object.
props.modifiableData.edit[1](!props.modifiableData.edit[0]); | ||
}} | ||
> | ||
<CloseIcon fontSize={props.iconFontSize} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should consider CancelIcon
here?
@mikeisen1 FYSA, #226 has been merged; I'm not sure what impact that has on this PR. |
Add additional props to components in the test so the components can be properly rendered.
…-library into edit-ssp-details-ui
Move the onSave function to OSCALLoader to avoid future code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor adjustments, looking good.
src/components/OSCALCatalog.js
Outdated
<OSCALMetadata metadata={props.catalog.metadata} /> | ||
<OSCALMetadata | ||
metadata={props.catalog.metadata} | ||
editedField={["metadata"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we necessarily need these when calling OSCALMetadata
. OSCALMetadata
should know that it's editedField
is metadata
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to specify metadata
, but we do need to specify catalog
or one of the other top-level fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that being sent in what's currently called patchData
?
src/components/OSCALLoader.js
Outdated
@@ -49,6 +49,52 @@ const oscalObjectTypes = { | |||
}, | |||
}; | |||
|
|||
function updateData(data, editedField, newValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a better name here be something like populatePartialPatchData
?
src/components/OSCALLoader.js
Outdated
* @param editedField path to the field that is being updated | ||
* @param newValue updated value for the edited field | ||
*/ | ||
function onSaveComplete( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the function that performs the save request, not what happens when save is complete, and we may want to distinguish from creation saves we'll introduce in the future vs updates.
Given those considerations, perhaps a better name here would be something like updateOscalData
if following the precedence of similar methods in the loader, or performUpdate
, or even start a new pattern of restUpdate
or restPatch
.
src/components/OSCALLoader.js
Outdated
/** | ||
* | ||
* @param data data that will be passed into the body of the PATCH request, doesn't initially contain the updates | ||
* @param update function that will update a state, forcing a re-rendering if the PATCH request is successful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called after a successful update so perhaps a better name might be onUpdateComplete
.
Also, very minor detail, but technically this function (currently named onSaveComplete
) doesn't know or care what will happen when it calls the function currently named update
as indicated in the param documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think something like onSuccessfulPatch
or updateOnSuccessfulPatch
as this is called only when the PATCH request is successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onSuccessfulPatch
sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the documentation param name?
src/components/OSCALLoader.js
Outdated
|
||
/** | ||
* | ||
* @param data data that will be passed into the body of the PATCH request, doesn't initially contain the updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be something like partialPatchData
?
src/components/OSCALMetadata.js
Outdated
|
||
if (props.isEditable) { | ||
patchData = props.patchData; | ||
patchData[props.editedField[0]].metadata = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need this approach, i.e. we don't necessarily have to update version
if the user was only updating title
.
src/components/OSCALMetadata.js
Outdated
<Typography variant="h6">{props.metadata.title}</Typography> | ||
<Grid container direction="row" alignItems="center"> | ||
<Grid item> | ||
<OSCALEditableTextField |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we increase the size of the input field here?
src/components/OSCALLoader.js
Outdated
* | ||
* @param data data that will be passed into the body of the PATCH request, doesn't initially contain the updates | ||
* @param update function that will update a state, forcing a re-rendering if the PATCH request is successful | ||
* @param editedField path to the field that is being updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a bit more descriptive, maybe something like editedFieldJsonPath
or fieldJsonPath
throughout the changes?
); | ||
} | ||
|
||
export default function OSCALModificationIcons(props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid confusion with control modifications in the OSCAL model, to keep similar to the test field added, and to allow for any future implementation changes (perhaps just buttons instead of icons for example) should we name this something like OSCALEditableFieldActions
?
@mikeisen1, as mentioned in standup, we'll also want to make sure editing is only enabled when the viewer is in REST mode. |
Update variable and function names to be more in line with their use.
src/components/OSCALSsp.js
Outdated
<OSCALMetadata | ||
metadata={ssp.metadata} | ||
isEditable={props.isEditable} | ||
editedField={["system-security-plan"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were able to drop editedField
since we have it in patchData
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can and that should be dropped.
return editedField.toString().replace(/,/g, "-"); | ||
} | ||
|
||
function getIconButtons(props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for this being a separate function rather than in OSCALEditableFieldActions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it because the getIconsButtons
also returns based on a condition (using a ternary operator), which would mean there's a nested ternary operator return statement if we included everything in one function. This causes a linter error as seen in the recent PR run failure (which has a few other minor linting issues that need to be fixed).
src/components/OSCALLoader.js
Outdated
/** | ||
* | ||
* @param data data that will be passed into the body of the PATCH request, doesn't initially contain the updates | ||
* @param update function that will update a state, forcing a re-rendering if the PATCH request is successful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the documentation param name?
}; | ||
|
||
function Template(args) { | ||
return <OSCALEditableFieldActions {...args} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to rename OSCALModificationIcons.stories.js
as well.
Update how editedField props are generated to move away from adding values to an undefined object.
src/components/OSCALLoader.js
Outdated
<OSCALCatalog | ||
catalog={oscalData[oscalObjectType.jsonRootName]} | ||
parentUrl={oscalUrl} | ||
onError={onError} | ||
onResolutionComplete={onResolutionComplete} | ||
restPatch={(data, update, editedField, newValue) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want these type of function props to be named according to the behavior they address, not the underlying implementation.
Perhaps onFieldSave=...
.
src/components/OSCALMetadata.js
Outdated
const classes = useStyles(); | ||
const versionIsInEditState = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems odd to track the state of editing for each field at this level, and this could become unmanageable as we add more editing capabilities.
Can OSCALEditableFieldActions
be imported by OSCALEditableTextField
so the state can be tracked in OSCALEditableTextField
?
That should also make the code more readable without having to define the text field and actions for each field.
Import the edit icons and track the current editing state and TextField reference in OSCALEditableTextField to make code more manageable when more editing capabilities are added in the future.
Remove JSON object containing component props and create a default props for OSCALEditableTextField to make code easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sticking through this @mikeisen1!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks great! Thanks for all the work on this; I know it's been a long effort! Great to see the restPatch
function reused quite a bit (and of course to actually have editing functionality in here!!). I think perhaps we're overusing the ternary operator in a few of these; we might end up being better served by doing really if
s or breaking things out into functions as we continue to add additional editing functionality across the application. That will likely help in testing parts of of the code base as complexity grows later.
Again, I really appreciate your commitment to getting this feature added!
Closes EasyDynamics/easygrc#8
Provide the user interface for editing the metadata title and version for an OSCAL Ssp. This also sends an HTTP PATCH request to a backend service and handles the response. This allows someone to, with the backend functionality provided, modify an SSP using the OSCAL Viewer. If the SSP is successfully updated, then the desired changes are reflected on the viewer. If the SSP is not successfully updated, the desired changes are not reflected on the viewer.
For the HTTP request, it is assumed for now that the window url is of the form
http://localhost:8080/system-security-plans?url=http://localhost:8080/oscal/v1/ssps/{uuid}
, as when the Viewer is in REST mode, we will not have the reload form and when we select a file to view, we do so by using the url parameter.If the PATCH fails, an alert will be sent to the user that the update could not be made.
Changes were mostly made to the metadata component file, so not much needs to be done to display the UI for modifying metadata of component definitions, catalogs, and profiles.