Permit OSCAL Object URL as URL Parameter #143

mikeisen1 · 2021-07-28T15:50:04Z

Adjustments were made to allow for someone to pass in a url parameter in the browser window for each of the OSCAL viewers, overriding the corresponding default url. This allows for any data contained in a valid url to be loaded into an OSCAL viewer, lending greater flexibility to what can be displayed.

To input an external url and display its data, add the following to the end of the browser url:

?url=<external-url>.

Listed below are example files which can be used as external URLs:

Note that the component viewer example does not load properly due to an issue with one of the sources in the data. However, there were no other examples, so just seeing whether the url in the OSCAL Component Viewer url box changed will have to do for now. Additionally, the above are just examples, as there are other urls for each of the Catalog, SSP, and Profile viewers that can be used.

This will resolve #134

tuckerzp

Looking good so far! I had a few comments.

I also wonder if we need to test for both cases when a url is provided vs when it is not? Could we primarily test that things load properly when we load a url and just have a few tests to make sure things work otherwise?

tuckerzp · 2021-07-28T16:57:09Z

src/test-data/SystemData.js

@@ -1,4 +1,8 @@
-import { metadataTestData, responsibleRolesTestData } from "./OtherData";


In develop, the change from OtherData -> CommonData was done already I believe.

If it was already done, I think I pulled from the develop branch prior to the change being made, so that's why you see the renaming in my files.

Oh maybe do a git pull rebase to be sure?

Even a git pull origin develop that does a merge commit should help resolve that.

src/components/OSCALLoader.js

src/components/OSCALProfile.test.js

src/components/OSCALLoader.js

src/test-data/ComponentsData.js

src/components/OSCALLoader.js

kylelaker · 2021-07-28T18:01:52Z

src/test-data/Urls.js

+export const defaultOSCALProfileUrl =
+  "https://raw.githubusercontent.com/usnistgov/oscal-content/master/nist.gov/SP800-53/rev4/json/NIST_SP-800-53_rev4_MODERATE-baseline_profile.json";


Is this the same as parentUrlTestData? Will it always be?

Yes, this is the same as parentUrlTestData

It seems we do not use parentUrlTestData anywhere. Can we just get rid of it?

That depends on how we handle the default viewer urls

Hey @mikeisen1 please try to let the thread authors resolve conversations. I think I still need an explanation for exactly what we plan to use this for, why the value is the same, and why it's a different variable. Are we expecting to eventually use different URLs? If the answer is "it depends" then I think we should talk about the assumptions and design and make sure we understand what it depends on and what we've chosen here. I am missing an understanding of that justification and it seems that @tuckerzp may be as well.

I am not sure why the value is the same and they're in different variables. Both parentUrlTestData and defaultOSCALProfileUrl have been in the codebase since before I have been working on the project.

My "depends" reply was in response to Zach Tucker's reply. When I moved the default urls back to the OSCALLoader file, I kept their original scope (none of them had export). So, I would only delete parentUrlTestData if I moved defaultOSCALProfileUrl back to the OSCALLoader file and did not export it. As for whether they will both always be the same, I think they would only be different if we want the urls used in the stories and tests to be different from the default viewer urls.

Both parentUrlTestData and defaultOSCALProfileUrl have been in the codebase since before I have been working on the project.

So that is an issue that I have had to deal with a few times. A lot of changes were made and thought out long before we started on project. Using Git Blame and checking the history for why something was done and when is super helpful to get an idea of why something may or may not be important.

From looking purely at the code you have added, parentUrlTestData is not used anywhere as of now. defaultOSCALProfileUrl seems to have replaced all those places where it was used. Going off of conversations above, defaultOSCALProfileUrl is not really needed in Urls.js, so parentUrlTestData could be needed again.

With that said, I am curious why they are the same value and in different variables. Maybe look into why they might be similar?

rgauss

At a high level, the majority of these test changes are really testing whether a different source URL loads correctly, not necessarily related to whether or not that source URL was provided as a URL parameter when making the request for the viewer in a browser, which we probably should not be testing with component tests.

While those additional tests may be valuable, we might want to consider whether we want to duplicate all tests with different expectations, or restructure the existing tests to follow a pattern of "for this URL, here are the expected values for the different elements", keeping in mind that some sources may not be expected to render certain elements at all.

That close tie of the source URL and expected rendering might also lend itself to the source URL being 'close to the expectations', i.e. in the test file, rather than in a separate URL file.

See a few other line-specific comments.

src/components/OSCALLoader.js

src/components/OSCALCatalogGroup.test.js

tuckerzp · 2021-07-28T18:54:59Z

Might be more readable, maybe not, interested in what others think.

When looking at the code I thought the same thing.

While those additional tests may be valuable, we might want to consider whether we want to duplicate all tests with different expectations, or restructure the existing tests to follow a pattern of "for this URL, here are the expected values for the different elements", keeping in mind that some sources may not be expected to render certain elements at all.

I also wonder if it is a good idea to test for specific things to be found when loading external components that, as we have seen before, change pretty frequently.

kylelaker

As @tuckerzp mentioned, there are a few changes here that I am wondering if they're a result of the merge conflict and the diff is off. Can you update with the develop branch and resolve the merge conflicts then push again?

example/package-lock.json

src/components/OSCALLoader.js

kylelaker · 2021-07-28T21:12:39Z

src/test-data/SystemData.js

@@ -1,4 +1,8 @@
-import { metadataTestData, responsibleRolesTestData } from "./OtherData";


Even a git pull origin develop that does a merge commit should help resolve that.

package.json

src/common-tests/UrlParameter.test.js

tuckerzp · 2021-07-30T18:41:16Z

@rgauss @kylelaker Just to keep history clean, would this be a good place to squash and force push the Resolve Linter Errors commits? Either way is fine, I was mostly just curious what everyone thought in this case.

tuckerzp

Has line ending issue been addressed?

tuckerzp · 2021-07-30T18:58:27Z

example/public/robots.txt

-# https://www.robotstxt.org/robotstxt.html
-User-agent: *
-Disallow: /
+# https://www.robotstxt.org/robotstxt.html
+User-agent: *
+Disallow: /


For example

Line ending issue was addressed at least on my end.

If it is not line ending issue, why is this change occurring? It seems like these files should not need to be deleted and then added in an identical way.

tuckerzp · 2021-07-30T19:33:47Z

src/components/OSCALLoader.js

+export function getRequestedUrl() {
+  // Returns url parameter provided by the browser url, if
+  //  it exists. If the url parameter exists, we want to
+  //  override the default viewer url.
+  return new URLSearchParams(window.location.search).get("url");
+}
+


Now that this is being exported, is JSDoc needed here?

Add functionality that allows users to specify a URL parameter in the browser, for each of the OSCAL viewers. Specifying a URL parameter will override the provided default URLs for the corresponding viewer.

Rename urlToLoad function to loadDefaultOrExternalUrl to make it easier to understand its purpose. Additionally, added comment explaining what the function does as well as its purpose.

Move default components urls back to OSCALLoader so they stay separated from the test and storybook data. Rename and modify urlToLoad as well as its description to improve overall code readability.

Create the UrlParameter test file to test the getRequestedUrl function in the OSCALLoader, allowing us to determine whether the method of obtaining the url parameter works as expected.

Add JSDoc documentation to getRequestedUrl function, as it is now part of the public API and needs to be documented.

kkennedy26

Looks good

kylelaker

Thanks for your work on this! Looking really good!

mikeisen1 requested review from zclarkEDC, kkennedy26, Bronstrom, hreineck, kylelaker, rgauss, snorouzzadeh and tuckerzp July 28, 2021 16:15

mikeisen1 self-assigned this Jul 28, 2021

tuckerzp suggested changes Jul 28, 2021

View reviewed changes

kylelaker suggested changes Jul 28, 2021

View reviewed changes

rgauss suggested changes Jul 28, 2021

View reviewed changes

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

src/components/OSCALCatalogGroup.test.js Outdated Show resolved Hide resolved

kylelaker suggested changes Jul 28, 2021

View reviewed changes

tuckerzp suggested changes Jul 29, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/common-tests/UrlParameter.test.js Outdated Show resolved Hide resolved

tuckerzp suggested changes Jul 30, 2021

View reviewed changes

tuckerzp reviewed Jul 30, 2021

View reviewed changes

kylelaker marked this pull request as draft July 30, 2021 20:48

mikeisen1 marked this pull request as ready for review August 2, 2021 15:20

mikeisen1 marked this pull request as draft August 2, 2021 15:21

mikeisen1 added 4 commits August 2, 2021 16:58

Allow URL parameter to override default URL

6f63b37

Add functionality that allows users to specify a URL parameter in the browser, for each of the OSCAL viewers. Specifying a URL parameter will override the provided default URLs for the corresponding viewer.

Improve code readability and documentation

cb45d15

Rename urlToLoad function to loadDefaultOrExternalUrl to make it easier to understand its purpose. Additionally, added comment explaining what the function does as well as its purpose.

Clean up data and component code

a1a134e

Move default components urls back to OSCALLoader so they stay separated from the test and storybook data. Rename and modify urlToLoad as well as its description to improve overall code readability.

Add tests for obtaining url parameter

f751ae1

Create the UrlParameter test file to test the getRequestedUrl function in the OSCALLoader, allowing us to determine whether the method of obtaining the url parameter works as expected.

mikeisen1 force-pushed the EGRC-473 branch 2 times, most recently from c140a27 to a37af31 Compare August 2, 2021 21:07

Add Documentation For getRequestedUrl

76a3ee6

Add JSDoc documentation to getRequestedUrl function, as it is now part of the public API and needs to be documented.

mikeisen1 force-pushed the EGRC-473 branch from 55587a1 to 76a3ee6 Compare August 2, 2021 21:35

mikeisen1 marked this pull request as ready for review August 2, 2021 21:51

rgauss approved these changes Aug 3, 2021

View reviewed changes

kkennedy26 approved these changes Aug 3, 2021

View reviewed changes

kylelaker approved these changes Aug 3, 2021

View reviewed changes

hreineck merged commit a713499 into develop Aug 3, 2021

rgauss deleted the EGRC-473 branch August 25, 2021 13:52

kylelaker mentioned this pull request Aug 8, 2022

url query string parameter is broken #544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit OSCAL Object URL as URL Parameter #143

Permit OSCAL Object URL as URL Parameter #143

mikeisen1 commented Jul 28, 2021

tuckerzp left a comment

tuckerzp Jul 28, 2021

mikeisen1 Jul 28, 2021

tuckerzp Jul 28, 2021

kylelaker Jul 28, 2021

kylelaker Jul 28, 2021

mikeisen1 Jul 28, 2021

tuckerzp Jul 28, 2021

mikeisen1 Jul 28, 2021

kylelaker Jul 28, 2021

mikeisen1 Jul 28, 2021

tuckerzp Jul 28, 2021 •

edited

rgauss left a comment

tuckerzp commented Jul 28, 2021

kylelaker left a comment

kylelaker Jul 28, 2021

tuckerzp commented Jul 30, 2021 •

edited

tuckerzp left a comment

tuckerzp Jul 30, 2021

mikeisen1 Jul 30, 2021

tuckerzp Jul 30, 2021

tuckerzp Jul 30, 2021

kkennedy26 left a comment

kylelaker left a comment

		@@ -1,4 +1,8 @@
		import { metadataTestData, responsibleRolesTestData } from "./OtherData";

		export const defaultOSCALProfileUrl =
		"https://raw.githubusercontent.com/usnistgov/oscal-content/master/nist.gov/SP800-53/rev4/json/NIST_SP-800-53_rev4_MODERATE-baseline_profile.json";

Permit OSCAL Object URL as URL Parameter #143

Permit OSCAL Object URL as URL Parameter #143

Conversation

mikeisen1 commented Jul 28, 2021

tuckerzp 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

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

tuckerzp Jul 28, 2021 • edited

Choose a reason for hiding this comment

rgauss left a comment

Choose a reason for hiding this comment

tuckerzp commented Jul 28, 2021

kylelaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuckerzp commented Jul 30, 2021 • edited

tuckerzp 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

kkennedy26 left a comment

Choose a reason for hiding this comment

kylelaker left a comment

Choose a reason for hiding this comment

tuckerzp Jul 28, 2021 •

edited

tuckerzp commented Jul 30, 2021 •

edited