-
Notifications
You must be signed in to change notification settings - Fork 59
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
HTML Reports based on filter data #5595
Conversation
Signed-off-by: George M Dias <gdias@mitre.org>
This pull request has a conflict. Could you fix it @georgedias? |
Signed-off-by: George M Dias <gdias@mitre.org>
Signed-off-by: George M Dias <gdias@mitre.org>
Signed-off-by: George M Dias <gdias@mitre.org>
… htmlFilteredReports
Signed-off-by: Emily Rodriguez <ecrodriguez@mitre.org>
This reverts commit ebb6630.
…different cause Signed-off-by: Emily Rodriguez <ecrodriguez@mitre.org>
…ack that need not be changed Signed-off-by: Emily Rodriguez <ecrodriguez@mitre.org>
@@ -29,12 +29,14 @@ type InputData = { | |||
data: ContextualizedEvaluation | string; | |||
fileName: string; | |||
fileID: string; | |||
filteredControls: string[]; |
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 make this optional? i don't think we expose filtered stuff in the saf cli so it'd be annoying to have to modify it to accept this new parameter.
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 have no problems making this optional, but I don't think this is used by the SAF CLI! Well at least, I was not under that impression.
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.
work has stalled but the PR exists
libs/hdf-converters/src/converters-from-hdf/html/reverse-html-mapper.ts
Outdated
Show resolved
Hide resolved
* ".contains" it returns ann results where the file.evaluations | ||
* returns the filtered controls. | ||
*/ | ||
let filteredControls: string[] = []; |
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 way that this is implemented will not satisfy the issue where it was stated that we should be able to handle any of the filters that the user specifies. this will only filter on pass/fail/whatever.
const controls = FilteredDataModule.controls({ |
use this example for how to get all the filter-passing controls. it should also mean that we don't need basically all of what're currently lines 122-123 and 150-164 since it'll just be a simple assignment directly onto 'filteredControls' (which can also then be made const).
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.
Absolutely unequivocally disagree that the implementations does not satisfy the requirements. It works on all possible filtered permutations, did you even tried, apparently not if you're making this statement.
"the way that this is implemented will not satisfy the issue where it was stated that we should be able to handle any of the filters that the user specifies. this will only filter on pass/fail/whatever."
Regarding the second opinion, the code that is being referenced (from the ExportCaat.vue) will not work. If you take a look at the reverse-html-mapper.ts it uses the file.data, which is the file.evaluation being set in ExportHTMLModule.vue and you are comparing to this code in the ExportCaat,vue const data = file?.evaluation ?? '';
now how does that filters the proper evaluations? it doesn't. The code on both are accomplishing different tasks.
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.
Here I've filtered it down to a single control using the nist filters. The html export does not only include that single control. Consequently, it does not meet the requirement of the original ticket according to my reading of it. Like I said elsewhere, we should get @em-c-rod or @ejaronne's opinions on which of our interpretations is correct.
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.
w/r to your second paragraph, I'm only talking about extracting out the set of controls that pass the filtering. You can use the store to get that set I'm hoping.
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 may be a use case, but the ask for this is just to be able to show only the failed controls (aka filter on status). If this becomes an additional ask, we can make another 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.
I do think it is relevant to consider severity filters here. Looking at severity filters as well was not explicitly the ask, but seems like it is paired in the same use case.
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 be fair, the UC was to show report based on filtering data aka, Passed, Failure, etc. not filtered on NIST controls, there is, we’re just filtering on STIG control status.
For future implementation for the HTML Export process I proposed the format provided in issue #5596
"I want to export reports of only the failures in a set of results, for example, or whatever I have filtered in the results view." I read this as supporting any/all filtering, but I could be wrong. @em-c-rod can you please provide input? You saying that you want to match the behavior of the other exports is unfortunately not usable input since there is unfortunately a wide variety of behaviors that occur. |
I'm not gonna make it a blocker, but it would be nice if we could actually do the filtering process for an execjson within the store so that we don't need to pass around a set of controls at all. This'll make both the mappers easier to implement due to them not needing to understand any filtering concepts at all and heimdall integrations with these mappers easier to write and overall less duplicative due to just needing to call the one store function instead of each mapper needing to implement filtering in its own special way. |
Adding methods or computed properties into the store - from the docs - seems to be the intended implemntation in the Vuex world. https://v3-migration.vuejs.org/breaking-changes/filters.html Given that we will eventually move to https://pinia.vuejs.org/core-concepts/getters.html moving the processing to the backend / middleware seems a good way to be able to reuse it with other parts of the GUI. |
Signed-off-by: George M Dias <gdias@mitre.org>
Signed-off-by: George M Dias <gdias@mitre.org>
Signed-off-by: George M Dias <gdias@mitre.org>
… htmlFilteredReports
Signed-off-by: George M Dias <gdias@mitre.org>
Signed-off-by: George M Dias <gdias@mitre.org>
Quality Gate passedIssues Measures |
Fixes issue #5541