Add config file support for control omission #431
Conversation
There was a problem hiding this comment.
Looks good so far, below are the testing results:
- rationale left blank: warning on config file omitting drivedocs.7.1 but no rationale provided; report reflects behavior
- rationale empty: no warning; report details "Test omitted by user. Rationale not provided."
- neither rationale or expiration date provided: warning on config file omitting drivedocs.7.1 but no rationale provided; report reflects behavior
- rationale provided: functions as expected
- omitpolicies left blank: no error, report generated normally without omissions
- expiration date in past: warning on provided exp. date already passed and control not omitted; setting exp. date in mm-dd-yyyy format throws warning, expected format is yyyy-mm-dd
- expiration date in future: functions as expected
- expiration date empty: functions as expected with valid exp. date or without the field
- expiration date blank: functions as expected with valid exp. date or without the field
- expiration date malformed: warning on malformed expiration date, policy not omitted
I've referenced this PR in the python unit testing epic so we can get config file coverage on the above cases.
For number 2, can you confirm that no warning is printed? There should be a warning, I just checked again and for me it printed. |
Reran and the warning is displayed. "Test omitted by user. Rationale not provided." is shown in the report details. |
There was a problem hiding this comment.
Ran against all test cases and overall looks good. A couple final considerations:
No warning is thrown if a policy ID is not formatted correctly (v0.3 not included):
omitpolicy:
# the omission is excluded from the report but no indication of this action
GWS.COMMONCONTROLS.13.1:
rationale: "exclude this policy"
When I excluded the GWS.COMMONCONTROLS.13.1v0.3 (policy group for system defined rules) ScubaResults.json/BaselineReports.html indicates 1 omission for the Rules baseline. Should we update this to indicate "39 omissions" as to accurately reflect all of the rules?
Good call about warning for bad IDs, I'll add that. As for Rules, interesting question. My first thought was that it should be counted as 1 omission for the common controls report but 39 for rules. That's what it currently does for common controls, but not rules. This is what it currently does: Which...isn't exactly wrong per se. But you're probably right that this rules should have specially handling. I'll work on that too. |
|
|
||
| @classmethod | ||
| def _validate_config(cls, args : argparse.Namespace) -> None: | ||
| if 'omitpolicy' in args: |
There was a problem hiding this comment.
This entire function right now only executes if there's an 'omitpolicy' in the args.
For future proofing should _validate_config only contain the conditions and the validation check for omitpolicy be moved to a separate function (i.e _validate_omitpolicy) that is called when if 'omitpolicy' is true in the original function?
Co-authored-by: David Bui <[email protected]>
Co-authored-by: mitchelbaker-cisa <[email protected]>
🗣 Description
Added config file support for omitting controls. See documentation here: https://github.com/cisagov/ScubaGoggles/blob/351-add-config-file-support-for-control-omission/docs/usage/Config.md#omit-policies.
💭 Motivation and context
Closes #351.
📷 Screenshots (if appropriate)
🧪 Testing
Rationale left blank:
Rationale empty:
Neither rationale nor expiration date provided:
Rationale provided:
omitpolicies left blank:
expiration date in past:
expiration date in future:
expiration date empty:
expiration date blank:
expiration date malformed:
also omit_controls.yaml as is.
✅ Pre-approval checklist
✅ Pre-merge Checklist
Squash and mergebutton.✅ Post-merge Checklist