Improve OVAL test info #126

Honny1 · 2023-01-20T10:30:45Z

This PR adds:

OVAL state to OVAL test info
check and check_existence attributes to OVAL test info

evgenyz · 2023-02-07T20:32:01Z

There is a conflict. Can you please rebase the PR?

jan-cerny

General feedback: it would be better to put some hints to the PR description for the reviewer regarding what the reviewer should focus on.

jan-cerny · 2023-02-13T12:34:30Z

openscap_report/scap_results_parser/parsers/oval_object_parser.py

+    def _collected_object_is_not_none_and_contain_var_ref(element, collected_object):
+        return collected_object is not None and 'var_ref' in element.attrib
+
+    def _get_ref_var(self, element, xml_collected_object):


You can make the code in this method nicer by reversing the condition and returning 'no_value' right after the "if" statement.

jan-cerny · 2023-02-13T12:44:26Z

openscap_report/scap_results_parser/parsers/shared_static_methods_of_parser.py

+import uuid
+
+
+class SharedStaticMethodsOfParser:


we strongly need unit tests, and these methos would be a nice start with unit testing

jan-cerny · 2023-02-13T12:49:55Z

openscap_report/scap_results_parser/parsers/oval_state_parser.py

+            operation = element.get("operation")
+            if operation is not None:
+                out.append(("operation", operation))
+        return [SharedStaticMethodsOfParser.transform_tuples_to_dict(out)]


What is the benefit of creating a list of tuples and then converting it to a dict instead of creating a dictionary from the beginning? It isn't obvious. It would be great to have a better naming or a comment so that the things would become more clear.

jan-cerny · 2023-02-13T13:08:12Z

openscap_report/report_generators/html_templates/js/oval_graph_generation_script.js

+    div.appendChild(get_label("", test_info.test_id));
+    div.appendChild(get_label("pf-m-blue", test_info.comment));
+    if (test_info.check) {
+        div.appendChild(get_label("pf-m-cyan", `Check atribute: ${test_info.check}`));


It's nice that it shows now the value of the check and check_existence attributes. But I'm afraid that no user except from the content authors know what this means. It would be better to show also the meaning of that attribute.

It can, for example check_existence="none_exist", say: This check requires that no such object can exist on the system.

You can find the possible values and their meaning in the OVAL specification. The report generate could translat attributus values to some helpful text.

jan-cerny · 2023-02-13T13:10:39Z

openscap_report/report_generators/html_templates/js/oval_graph_generation_script.js

+    const div = DIV.cloneNode();
+    div.className = "pf-c-accordion__expanded-content-body";
+    div.appendChild(get_label("", test_info.test_id));
+    div.appendChild(get_label("pf-m-blue", test_info.comment));


The comment becames a tooltip also for the check and check_existence rectangles. I don't think that it's useful to repeat it over and over. I think they could have a different tool tip, maybe related to the maining of the elements (see also the comment below).

jan-cerny · 2023-02-16T14:02:05Z

@Honny1 If you do some changes you need to summarize them in a comment in the PR so that the visitors know what changed.

Honny1 · 2023-02-16T14:33:13Z

@jan-cerny Okay.

Changes:

Removed shared static method for transition list of tuples to dictionary and create a dictionary in the first place,
Created unit tests for shared methods,
Reversed if condition in method _get_ref_var,
Added tooltip info to check, check_existence and to id of objects and state. See the image below.

jan-cerny · 2023-02-16T14:19:34Z

openscap_report/report_generators/html_templates/js/oval_graph_generation_script.js

+    "all": "collect all values",
+    "at least one": "collect at least one value",
+    "none exist": "not collect any of the values",
+    "none satisfy": "not collect any of the values",
+    "only one": "collect only one of the values"


I find this very confusing. The point of the "check" attribute isn't the data collection process. It defines the relationship between objects and states. The value of the check attribute defines how many of the collected objects must satisfy the given state for the test to return true.

Please read the CheckEnumeration type definition in the OVAL common schema.

jan-cerny · 2023-02-16T14:24:35Z

openscap_report/report_generators/html_templates/js/oval_graph_generation_script.js

+    "only one": "collect only one of the values"
+};
+const CHECK_EXISTENCE_ATTRIBUTE_TO_TEXT = {
+    "all_exist": "The test requires that all distinct groups of information as defined by the OVAL object exist in the endpoint in order for the OVAL test to evaluate to \"true\".",


I find this too complex and difficult to understand. I like the definition in the specification more than this. Try to make it more clear. For example "all distinct groups of information as defined by the OVAL object" can be simplified to "objects defined by the test".

jan-cerny · 2023-02-16T15:38:08Z

openscap_report/scap_results_parser/parsers/oval_test_info_parser.py

+                oval_object = self.objects_parser.get_object(object_id)
+            state_id = item.attrib.get("state_ref")
+            if state_id is not None:
+                oval_state = self.states_parser.get_state(state_id)


I don't know the details but I'm curious: Why is this a loop? In each iteration it overwrites the oval_object and oval_state. Wouldn't it be better to just pick the last item? And why is the self.tests[test_id] an iterable? Does that mean that there are more tests with the same ID?

Yes, you are right. This for loop is useless. The value contained in self.tests is a dictionary where the key is test_id and the value is an XML element. This loop is used to iterate through the children of the test XML element to get objects and states with a different namespaces. I will simplify this code to make it more readable and replace the unnecessary loop with xPath.

Honny1 · 2023-02-17T12:57:56Z

Changes:

Simplified get_test_info method,
Improved tooltip text for check and check_existence attributes.

jan-cerny · 2023-02-20T14:00:24Z

openscap_report/scap_results_parser/parsers/oval_object_parser.py

+                out[id_in_dict] = self._get_ref_var(element, xml_collected_object)
+        return [out]
+
+    def _get_item(self, item_ref):


I find the name confusing, instead of "item" it returns some dictionary

out is almost never a good name for anything

jan-cerny · 2023-02-20T14:04:25Z

openscap_report/scap_results_parser/parsers/oval_object_parser.py

+                out[id_in_dict] = element.text
+            else:
+                out[id_in_dict] = self._get_ref_var(element, xml_collected_object)
+        return [out]


Why does this function return a list that contains a single item and that is a dictionary? That sounds suspicious and is a sign of a bad design. Please investigate this. If you really need this wrapping to a list it probably should be done one level of abstaraction above.

Also I suggest giving the variable a different name than out. Think what it actually contains and give it a descriptive name.

This wrapping is caused by extending the abstraction of the OVAL objects. The OVAL object class has an oval_data attribute that stores a list that contains dictionaries of attributes and values. These dictionaries are stored in a list because they are used to populate more instances of objects that have been collected. In this case, the usual object approach has been changed to be more capable of using different types of OVAL objects and storing multiple instances of OVAL objects.

jan-cerny · 2023-02-20T14:05:37Z

openscap_report/scap_results_parser/parsers/oval_object_parser.py

+        var_id = element.get('var_ref')
+        for item in xml_collected_object:
+            if var_id == item.get('variable_id'):
+                variable_value += item.text


More pythonic way of doing this would be to collect these in a list and after that merge the items in the list using .join() method.

jan-cerny · 2023-02-20T14:10:08Z

openscap_report/scap_results_parser/parsers/oval_state_parser.py

+            operation = element.get("operation")
+            if operation is not None:
+                out["operation"] = operation
+        return [out]


this method has also similar understandability problems as the other similar methods - return value is a list that contains a single item which is a dict that has a generic name

Honny1 · 2023-02-22T13:36:11Z

Changes:

Improved tooltip text for check and check_existence attributes,
renamed out variables,
removed wrapping array in return,
Used a more pythonic way to contact strings.

jan-cerny

I have generated a report for a fedora scan and I have checked the chekd and check_existence boxes in multiple rules and triggered their tooltips.

Great job!

Honny1 force-pushed the improve-oval-test-info branch from ab3abc8 to 9d8aa34 Compare January 20, 2023 10:42

Honny1 marked this pull request as ready for review January 30, 2023 16:12

Honny1 force-pushed the improve-oval-test-info branch from 9d8aa34 to 65c32ec Compare January 31, 2023 12:25

Honny1 force-pushed the improve-oval-test-info branch from 65c32ec to fcf41f2 Compare February 7, 2023 16:38

Honny1 force-pushed the improve-oval-test-info branch 2 times, most recently from da9abf6 to f918805 Compare February 10, 2023 10:04

jan-cerny self-assigned this Feb 13, 2023

jan-cerny requested changes Feb 13, 2023

View reviewed changes

Honny1 force-pushed the improve-oval-test-info branch 3 times, most recently from b8ff2aa to 8738eea Compare February 15, 2023 15:24

Honny1 force-pushed the improve-oval-test-info branch from 8738eea to 1a05a3d Compare February 16, 2023 14:21

Honny1 requested a review from jan-cerny February 16, 2023 15:00

jan-cerny reviewed Feb 16, 2023

View reviewed changes

Honny1 force-pushed the improve-oval-test-info branch from 1a05a3d to a589abc Compare February 17, 2023 12:54

Honny1 requested a review from jan-cerny February 17, 2023 12:58

jan-cerny reviewed Feb 20, 2023

View reviewed changes

Honny1 added 5 commits February 22, 2023 10:27

Add info about comparison of endpoint values in OVAL test

7618638

Add OVAL state class

db6dfb1

Parse OVAL state

13a67cb

Decompose OVALTestInfoParser class

9232535

Add information about OVAL state to report

b161fb2

Honny1 force-pushed the improve-oval-test-info branch from a589abc to dc069a1 Compare February 22, 2023 11:48

Honny1 added 2 commits February 22, 2023 14:26

Add check and check_existence atributes to report

1ae156b

Add unit tests for SharedStaticMethodsOfParser

3867e0f

Honny1 force-pushed the improve-oval-test-info branch from dc069a1 to 3867e0f Compare February 22, 2023 13:27

jan-cerny approved these changes Feb 22, 2023

View reviewed changes

jan-cerny merged commit 10c75c1 into OpenSCAP:main Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve OVAL test info #126

Improve OVAL test info #126

Honny1 commented Jan 20, 2023

evgenyz commented Feb 7, 2023

jan-cerny left a comment

jan-cerny Feb 13, 2023

jan-cerny Feb 13, 2023

jan-cerny Feb 13, 2023

jan-cerny Feb 13, 2023

jan-cerny Feb 13, 2023

jan-cerny commented Feb 16, 2023

Honny1 commented Feb 16, 2023

jan-cerny Feb 16, 2023

jan-cerny Feb 16, 2023

jan-cerny Feb 16, 2023

Honny1 Feb 17, 2023 •

edited

Loading

Honny1 commented Feb 17, 2023

jan-cerny Feb 20, 2023

jan-cerny Feb 20, 2023

Honny1 Feb 22, 2023

jan-cerny Feb 20, 2023

jan-cerny Feb 20, 2023

Honny1 commented Feb 22, 2023

jan-cerny left a comment

Improve OVAL test info #126

Improve OVAL test info #126

Conversation

Honny1 commented Jan 20, 2023

evgenyz commented Feb 7, 2023

jan-cerny 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

jan-cerny commented Feb 16, 2023

Honny1 commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Honny1 Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Honny1 commented Feb 17, 2023

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

Honny1 commented Feb 22, 2023

jan-cerny left a comment

Choose a reason for hiding this comment

Honny1 Feb 17, 2023 •

edited

Loading