Adjust SSP Viewer for FedRAMP template #140

Bronstrom · 2021-07-27T19:24:54Z

Adjustments have been made in the visual display of the FedRAMP SSP template, importing a profile, and displaying statements.

Visually, columns in System Characteristics and System Implementation have been adjusted in placement and size to lessen word wrap and fill more of the negative space. While making visual adjustments, the placement of other elements on screen were maintained.

After resolving the import-profile xml link in OSCALSspResolver, there was a need to rework how statements are fetched for the implementation controls to display correctly. This FedRAMP SSP template provided the set-parameters in the first/overarching statement, rather than each individual statement for the controls. For example, the set-parameters are listed purely in "ac-1_smt" versus being listed in something like "ac-1_smt.a" or "ac-1_smt.b.1" in NIST's SSPs. OSCALControl now tries to grab these statements (which are null if provided like NIST's statements), then passes them to underlying components. With this added feature, the SSP Viewer has no problem handling controls from both NIST and FedRAMP.

Resolves Minor Adjustments for FedRAMP SSP Templates #133

Testing

Use the FedRAMP SSP template when running the SSP Viewer.

To fill negative space and limit word wrap, stylistic adjustments have been made to OSCALSystemCharacteristics and OSCALSystemImplementation.

The FedRAMP SSP template provides a different format to how statement set-parameters are provide than seen in NIST's SSPs. Instead of having set-parameters provided on each statement of a control, they are provided by only the initial statement of a control in this SSP template. Control and underlying components have been reworked to fit this description.

... fetch

src/components/OSCALControl.js

src/components/OSCALControlProse.js

rgauss · 2021-07-28T12:16:15Z

src/components/OSCALControl.js

@@ -61,6 +61,21 @@ export default function OSCALControl(props) {
    );
  }

+  // When FedRAMP, retrieve the set-parameters from the initial implementation request statement
+  let firstImplReqStatement = null;


Why are we just pulling the first of each here?

In this sample it contains the set-parameters only in the first (initial) implemented requirement statement

Right, but we don't have guaranteed ordering here.

What I think we're really looking for are set-parameters at the 'top-level' of the control, so within ac-1_smt, which won't necessarily be the first statement in the array.

Oh okay, that makes more sense. Maybe instead here, we should find the 'top-level' of the control with an ends with "_smt" check?

Something like that, we may already be doing something similar elsewhere?

I've added an endsWith check when searching through the array of implReqStatements for now. I haven't found a place where we're doing something similar but will continue looking.

src/components/OSCALSystemCharacteristics.js

src/components/OSCALSystemImplementation.js

src/components/OSCALControl.js

rgauss · 2021-07-28T16:49:08Z

src/components/OSCALControl.js

+      }
+    });
+  }
+  // NOTE: The top-level by-component is the first element in the by-components array


I'm not sure this is an accurate statement.

Each component in the by-components array represents how each component address the control.

I believe OSCALContorlImplementationImplReq sends in a componentId that we should check for?

I was a little hesitant to include that part as well. Grabbing the first array is definitely not the best practice here as this doesn't seem to be a consistent pattern. Also, thanks for clarifying on this. Your right about OSCALControlImplementationImplReq passing componentId as a prop. I can make a by-component id match check as well - I'm not seeing it elsewhere.

src/components/OSCALSystemImplementation.js

To help with readability and reusability of code, the implemented requirements set-parameters fetch has been made into a seperate method in OSCALControl.

Bronstrom · 2021-07-28T20:32:13Z

I chose to make the set-parameter fetch into a separate function in OSCALControl. I was wondering if we would even want to split this up further or place it elsewhere - like in ./oscal-utils? At this point, the implReqStatements grab and by-component id checks aren't used outside the scope of OSCALControl, but could potentially be referenced by components in the future?

tuckerzp

Looking good so far! This does not have to prevent an approval. I just had a few potential things to look into.

tuckerzp · 2021-07-30T18:53:11Z

src/components/OSCALControl.js

+ * @param {Object} implReqStatements
+ * @param {String} componentId


Would a brief description of these two parameters be useful here?

tuckerzp · 2021-07-30T19:22:21Z

src/components/OSCALControl.js

+  if (implReqStatements) {
+    // NOTE: The top level statement ends with "_smt"
+    topLevelImplReqStatement = implReqStatements.find((statement) =>
+      statement["statement-id"].endsWith("_smt")
+    );
+  }
+  // NOTE: The top-level by-component has a matching uuid to the componentId
+  let topLevelByComp = null;
+  if (topLevelImplReqStatement?.["by-components"]) {
+    topLevelByComp = topLevelImplReqStatement["by-components"].find(
+      (byComp) => byComp["component-uuid"] === componentId
+    );
+  }
+  return topLevelByComp?.["set-parameters"] || null;


I could be wrong here, but is it possible to make this into just a pipe of find().find() or something along those lines? It seems like there could be a cleaner way of doing this.

hreineck

Looks good to me. Viewer looks as specified locally.
I would like to see @tuckerzp 's comments implemented, but other than that looks great.

rgauss · 2021-08-02T20:33:01Z

src/components/OSCALControl.js

+ * @param {String} componentId
+ * @returns An array of set-parameters
+ */
+function getImplReqSetParameters(implReqStatements, componentId) {


Should this entire function be pushed down to OSCALControlProse.js since that code is already responsible for finding the parameters and has the componentId?

I will push the function to OSCALControlProse.js

Clean up code in getImplReqSetParameters by creating a find.find pipe and removing the extra lines no longer needed. Add brief description to the parameters of getImplReqSetParameters to improve understanding of their purpose.

Move getImplReqSetParameters from OSCALControl to OSCALControlProse, as OSCALControlProse already finds the parameters and has the componentId.

rgauss · 2021-08-03T16:46:40Z

src/components/OSCALControl.js

@@ -61,6 +62,12 @@ export default function OSCALControl(props) {
    );
  }

+  // Retrieve implemented requirements set-parameters
+  const setParams = getImplReqSetParameters(


Sorry, I meant the function and the execution of that function could be moved to OSCALControlProse.

There shouldn't be any need to evaluate here and pass the results as implReqSetParameters all the way down to OSCALControlProse where it's actually used, I think we should be able to just call from within OSCALControlProse.

Move getImplReqSetParameters to OSCALControlProse file, as the result is only used in that file. Modified OSCALControl and OSCALControlPart to no longer use implReqSetParameters, as it is not used in those files. This improve readability and understanding of the code and where results of functions are used.

Move getImplReqSetParameters to top of file, as it was previously being used before it was defined.

kylelaker

Some thoughts but not really required.

kylelaker · 2021-08-04T13:34:24Z

src/components/OSCALControlProse.js

+  // Get the top-level implemented requirement statement
+  const topLevelByComp = implReqStatements
+    ?.find((statement) => statement["statement-id"].endsWith("_smt"))
+    ?.["by-components"]?.find(


Suggested change

?.["by-components"]?.find(

?.["by-components"]

?.find(

Would the linter allow this? Makes the chain a little more "obvious"

I've tried locally but whenever I run npm run lint, the suggested change is fixed to what it originally looked like so I don't think so.

kylelaker · 2021-08-04T13:45:53Z

src/components/OSCALControlProse.js

+  // Locate set-parameters when found in the by-component
+  if (statementByComponent?.["set-parameters"]) {
+    foundParameterSetting = statementByComponent["set-parameters"].find(
+      (parameterSetting) => parameterSetting["param-id"] === parameterId


If we're using this twice (once per branch), we should move it to it's own function within getParameterValue.

// This assumes that `parameterId` from `getParameterValue` is in scope function parameterHasGivenId(parameterSetting) { return parameterSetting["param-id"] === parameterId; }

And then you can use it as:

if (statementByComponent?.["set-parameters"]) { foundParameterSetting = statementByComponent["set-parameters"].find(parameterHasGivenId); } else if (implReqSetParameters) { foundParameterSetting = implReqSetParameters.find(parameterHasGivenId); } else { return null; }

But I'd also suggest that we've got quite a bit of repetition there still (between the if and the actual clause). And we may want to consider also doing something like:

const setParameters = () => statementByComponents?.["set-parameters"] || implReqSetParameters; const foundParameterSetting = setParameters()?.find(parameterHasGivenId);

And since the next check is if (!foundParameterSetting?.values) { return null; } we shouldn't need to modify any code. But we could do a if (!foundParameterSetting) { return null; } if we really wanted to.

This has the benefits of not only reducing repetition but also keeping more things const. It's also one less null that we really need to worry about.

I made all recommended changes except the change in the conditional because we do foundParameterSetting.values.toString() right after the check and if foundParameterSetting.values is null, we would get an error.

kylelaker · 2021-08-04T13:46:58Z

src/components/OSCALControlProse.js

+  const topLevelByComp = implReqStatements
+    ?.find((statement) => statement["statement-id"].endsWith("_smt"))
+    ?.["by-components"]?.find(
+      (byComp) => byComp["component-uuid"] === componentId
+    );
+  return topLevelByComp?.["set-parameters"] || null;


Suggested change

const topLevelByComp = implReqStatements

?.find((statement) => statement["statement-id"].endsWith("_smt"))

?.["by-components"]?.find(

(byComp) => byComp["component-uuid"] === componentId

);

return topLevelByComp?.["set-parameters"] || null;

return implReqStatements

?.find((statement) => statement["statement-id"].endsWith("_smt"))

?.["by-components"]?.find(

(byComp) => byComp["component-uuid"] === componentId

)

?.["set-parameters"] || null;

Any reason to keep the temporary value variable there?

Clean up code in OSCALControlProse to remove duplication, make code easier to read and understand, and keep more variables const.

src/components/OSCALControlProse.js

Clean up OSCALControlProse file to remove a function definition and convert it to an expression, as the function is only used once and is no longer needed.

kkennedy26

Adjustments look good

Bronstrom added 3 commits July 27, 2021 09:38

Adjust SSP visual placement to FedRAMP SSP

8395085

To fill negative space and limit word wrap, stylistic adjustments have been made to OSCALSystemCharacteristics and OSCALSystemImplementation.

Resolve XML profile in FedRAMP SSP template

8381097

Bronstrom requested review from rgauss, kylelaker, folksgl, tuckerzp, mikeisen1, hreineck, zclarkEDC, kkennedy26 and snorouzzadeh July 27, 2021 19:24

tuckerzp assigned Bronstrom Jul 27, 2021

Improve readability in OSCALControl statement ...

65a4080

... fetch

rgauss suggested changes Jul 28, 2021

View reviewed changes

Bronstrom added 3 commits July 28, 2021 09:51

Address style and comment requests

f666aea

Add endsWith check in OSCALControl statement fetch

ed58796

Adjust grid sizes in OSCALSystemCharacteristics

721d959

rgauss suggested changes Jul 28, 2021

View reviewed changes

Bronstrom added 2 commits July 28, 2021 14:51

Minor style adjustments

63d75e3

Improve statement fetch & add component id check

f88c5b9

Bronstrom force-pushed the EGRC-474 branch from d725834 to f88c5b9 Compare July 28, 2021 19:05

Make set-parameters fetch a function

48d62f0

To help with readability and reusability of code, the implemented requirements set-parameters fetch has been made into a seperate method in OSCALControl.

tuckerzp reviewed Jul 30, 2021

View reviewed changes

hreineck approved these changes Jul 30, 2021

View reviewed changes

rgauss suggested changes Aug 2, 2021

View reviewed changes

mikeisen1 marked this pull request as draft August 3, 2021 13:11

mikeisen1 added 2 commits August 3, 2021 11:56

Clean up code and enhance documentation

cb74ce5

Clean up code in getImplReqSetParameters by creating a find.find pipe and removing the extra lines no longer needed. Add brief description to the parameters of getImplReqSetParameters to improve understanding of their purpose.

Move set-parameters fetch to a different file

9d5dbbd

Move getImplReqSetParameters from OSCALControl to OSCALControlProse, as OSCALControlProse already finds the parameters and has the componentId.

mikeisen1 marked this pull request as ready for review August 3, 2021 16:29

mikeisen1 self-assigned this Aug 3, 2021

rgauss suggested changes Aug 3, 2021

View reviewed changes

mikeisen1 added 2 commits August 3, 2021 14:12

Move getImplReqSetParameters to top of file

bfa0b43

Move getImplReqSetParameters to top of file, as it was previously being used before it was defined.

kylelaker reviewed Aug 4, 2021

View reviewed changes

Clean up and de-duplicate code

414d9e6

Clean up code in OSCALControlProse to remove duplication, make code easier to read and understand, and keep more variables const.

kylelaker suggested changes Aug 4, 2021

View reviewed changes

src/components/OSCALControlProse.js Outdated Show resolved Hide resolved

Clean up code

83c22af

Clean up OSCALControlProse file to remove a function definition and convert it to an expression, as the function is only used once and is no longer needed.

mikeisen1 requested review from kylelaker, rgauss and hreineck August 5, 2021 20:38

rgauss approved these changes Aug 5, 2021

View reviewed changes

kkennedy26 approved these changes Aug 6, 2021

View reviewed changes

rgauss merged commit dcf0502 into develop Aug 9, 2021

rgauss deleted the EGRC-474 branch August 9, 2021 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust SSP Viewer for FedRAMP template #140

Adjust SSP Viewer for FedRAMP template #140

Bronstrom commented Jul 27, 2021

rgauss Jul 28, 2021

Bronstrom Jul 28, 2021 •

edited

rgauss Jul 28, 2021

Bronstrom Jul 28, 2021

rgauss Jul 28, 2021

Bronstrom Jul 28, 2021

rgauss Jul 28, 2021

Bronstrom Jul 28, 2021

Bronstrom commented Jul 28, 2021

tuckerzp left a comment

tuckerzp Jul 30, 2021

tuckerzp Jul 30, 2021 •

edited

hreineck left a comment

rgauss Aug 2, 2021 •

edited

mikeisen1 Aug 3, 2021

rgauss Aug 3, 2021

kylelaker left a comment

kylelaker Aug 4, 2021

mikeisen1 Aug 4, 2021

kylelaker Aug 4, 2021

mikeisen1 Aug 4, 2021

kylelaker Aug 4, 2021

kkennedy26 left a comment

		* @param {Object} implReqStatements
		* @param {String} componentId

Adjust SSP Viewer for FedRAMP template #140

Adjust SSP Viewer for FedRAMP template #140

Conversation

Bronstrom commented Jul 27, 2021

Testing

Choose a reason for hiding this comment

Bronstrom Jul 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bronstrom commented Jul 28, 2021

tuckerzp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuckerzp Jul 30, 2021 • edited

Choose a reason for hiding this comment

hreineck left a comment

Choose a reason for hiding this comment

rgauss Aug 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylelaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkennedy26 left a comment

Choose a reason for hiding this comment

Bronstrom Jul 28, 2021 •

edited

tuckerzp Jul 30, 2021 •

edited

rgauss Aug 2, 2021 •

edited