Address Rego Linter (Regal) Rules (#640)

* Change ignore to warning for specified rules in issue * update style guide for new linter rules * change to warning for test-outside-test-package and rule-length * Update .regal/config.yaml Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> * Update .regal/config.yaml Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> * Update CONTENTSTYLEGUIDE.md Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> * Update CONTENTSTYLEGUIDE.md Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> * updated opa rego intro paragraph * removed extra sentance * removed bulletpoint * fix capitalization * add punctuation * word smithing * Update CONTENTSTYLEGUIDE.md Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> --------- Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>
cisagov · Nov 28, 2023 · 7a11343 · 7a11343
1 parent 8e46600
commit 7a11343
Show file tree

Hide file tree

Showing 2 changed files with 110 additions and 64 deletions.
diff --git a/.regal/config.yaml b/.regal/config.yaml
@@ -10,7 +10,7 @@ rules:
       # redundant, as that's the implied rule body when
       # none is provided. It's harmless, but should be
       # fixed anyway so real issues aren't missed.
-      level: ignore
+      level: warning
   custom:
     # https://docs.styra.com/regal/rules/custom/naming-convention
     naming-convention:
@@ -60,11 +60,12 @@ rules:
       # Most of these seem fixable by concatenating strings
       # and by using the non-breakable-word-threshold option
       # for when that doesn't work.
-      level: ignore
+      non-breakable-word-threshold: 100
+      level: warning
     # https://docs.styra.com/regal/rules/style/no-whitespace-comment
     no-whitespace-comment:
       # This repo is actually good about this, but frequently
-      # uses '#--' as a delimeter of sorts. That should be OK,
+      # uses '#--' as a delimiter of sorts. That should be OK,
       # and the next version of Regal will allow for exceptions
       # like this: https://github.com/StyraInc/regal/issues/379
       level: ignore
@@ -73,7 +74,7 @@ rules:
       level: ignore
     # https://docs.styra.com/regal/rules/style/prefer-some-in-iteration
     prefer-some-in-iteration:
-      level: ignore
+      level: warning
     # https://docs.styra.com/regal/rules/style/prefer-snake-case
     prefer-snake-case:
       # This is the default style preference for Rego, but since
@@ -83,16 +84,18 @@ rules:
       level: ignore
     # https://docs.styra.com/regal/rules/style/rule-length
     rule-length:
-      level: ignore
+      level: warning
+      max-rule-length: 30
+      count-comments: false      
     # https://docs.styra.com/regal/rules/style/todo-comment
     todo-comment:
       level: ignore
   testing:
     # https://docs.styra.com/regal/rules/testing/identically-named-tests
     identically-named-tests:
-      # Only a few of these — woulde be easy to fix
-      level: ignore
+      # Only a few of these — would be easy to fix
+      level: warning
     # https://docs.styra.com/regal/rules/testing/test-outside-test-package
     test-outside-test-package:
       # This is just a style preference
-      level: ignore
+      level: warning
diff --git a/CONTENTSTYLEGUIDE.md b/CONTENTSTYLEGUIDE.md
@@ -1,6 +1,6 @@
 # Content style guide for SCuBA <!-- omit in toc -->
 
-Welcome to the content style guide for ScubaGear
+Welcome to the content style guide for ScubaGear!
 
 These guidelines are specific to style rules for PowerShell and OPA Rego code. For general style questions or guidance on topics not covered here, ask or go with best guess and bring up at a later meeting.
 
@@ -10,20 +10,20 @@ Use menu icon on the top left corner of this document to get to a specific secti
 
 - Our style guide aims for simplicity. Guidelines should be easy to apply to a range of scenarios.
 - Decisions aren’t about what’s right or wrong according to the rules, but about what’s best practice and improves readability. We're flexible and open to change while maintaining consistency.
-- When making a style or structure decision, we consider the readability, maintainability and ability for consitancy in a range of situations.
+- When making a style or structure decision, we consider the readability, maintainability and ability for consistency in a range of situations.
 - When a question specific to help documentation isn’t covered by the style guide, we think it through using these principles, then make a decision and bring it up in the next meeting for deliberation.
 
 ## OPA Rego
 
-Because there isn't a standard style guide for the Rego language, we are creating one from scratch. For consistency, we will be using many of the same style rules as PowerShell. There are also a few best practice rules that this program will follow. These best practices were deliberated on and chosen to enhance readability. We recognize that the code is in a constant state of improvement, so the best practices are subject to change.
+The project is adopting the following public Rego [style guide](https://docs.styra.com/opa/rego-style-guide), except where our guide specifically notes an exception (e.g., variable name case). For consistency, we will be using many of the same style rules as PowerShell. There are also a few best practice rules that this project will follow. These best practices were chosen to enhance readability. We recognize that the code is in a constant state of improvement, so the best practices are subject to change. The project is also integrating the [Regal](https://github.com/StyraInc/regal) linter into its automated checks to promote style guide adherence.
 
 ### Test Cases
 
 Test names will use the syntax `test_mainVar_In/correct_*V#` to support brevity in naming that highlights the primary variable being tested. Furthermore, for tests with more than one version, the first test will also include a version as `_V1`. Consistent use of a version number promotes clarity and signals the presence of multiple test versions to reviewers. Version numbers are not used if there is only a single test of a given variable and type (Correct/Incorrect)
 
 ```
 test_ExampleVar_Correct_V1 if {
-    PolicyId := "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>"
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
 
     Output := tests with input as {
         "example_policies" : [
@@ -46,7 +46,7 @@ test_ExampleVar_Correct_V2 if {
 }
 
 test_ExampleVar_Incorrect if {
-    PolicyId := "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>"
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
 
     Output := tests with input as {
         "example_policies" : [
@@ -67,45 +67,40 @@ test_ExampleVar_Incorrect if {
 
 ### Not Implemented
 
-If the policy bullet point is untestable at this time, use the templates below.
+If the policy is untestable at this time, use the templates below.
 
 #### Config
 
 The first one directs the user to the baseline document for manual checking. The second instructs the user to run a different script because the test is in another version. However, if they are unable to run the other script, they are also directed to the baseline like in the first template.
 
 ```
 # At this time we are unable to test for X because of Y
-tests[{
-    "PolicyId" : PolicyId,
-    "Criticality" : "Shall/Not-Implemented",
-    "Commandlet" : [],
-    "ActualValue" : [],
-    "ReportDetails" : NotCheckedDetails(PolicyId),
-    "RequirementMet" : false
-}] {
-    PolicyId := "MS.<Product>.<Control Group #>.<Control #>v<Version #>"
-    true
+tests contains {
+    "PolicyId": "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",
+    "Criticality": "Should/Not-Implemented",
+    "Commandlet": [],
+    "ActualValue": [],
+    "ReportDetails": NotCheckedDetails("MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"),
+    "RequirementMet": false,
 }
 ```
 
 ```
 # At this time we are unable to test for X because of Y
-tests[{
-    "PolicyId" : "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>",
-    "Criticality" : "Shall/3rd Party",
-    "Commandlet" : [],
-    "ActualValue" : [],
-    "ReportDetails" : "Custom implementation allowed. If you are using Defender to fulfill this requirement, run the Defender version of this script. Otherwise, use a 3rd party tool OR manually check",
-    "RequirementMet" : false
-}] {
-    true
+tests contains {
+    "PolicyId": "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>,
+    "Criticality": "Shall/3rd Party",
+    "Commandlet": [],
+    "ActualValue": [],
+    "ReportDetails": DefenderMirrorDetails("MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"),
+    "RequirementMet": false,
 }
 ```
 #### Testing
 
 ```
 test_NotImplemented_Correct if {
-    PolicyId := "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>"
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
 
     Output := tests with input as { }
 
@@ -118,15 +113,15 @@ test_NotImplemented_Correct if {
 ```
 ```
 test_3rdParty_Correct_V1 if {
-    PolicyId := "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>"
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
 
     Output := tests with input as { }
 
     RuleOutput := [Result | Result = Output[_]; Result.PolicyId == PolicyId]
 
     count(RuleOutput) == 1
     not RuleOutput[0].RequirementMet
-    RuleOutput[0].ReportDetails == "Custom implementation allowed. If you are using Defender to fulfill this requirement, run the Defender version of this script. Otherwise, use a 3rd party tool OR manually check"
+    RuleOutput[0].ReportDetails == DefenderMirrorDetails(PolicyId)
 }
 ```
 
@@ -144,7 +139,7 @@ One True Brace - requires that every braceable statement should have the opening
 
 ```
 test_Example_Correct if {
-    PolicyId := "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>"
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
 
     Output := tests with input as {
         "example_tag" : {
@@ -175,7 +170,7 @@ Example[Example.Id] {
 }
 
 tests[{
-    "PolicyId" : "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>",
+    "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",
     "Criticality" : "Shall",
     "Commandlet" : "Example-Command",
     "ActualValue" : ExampleVar.ExampleSetting,
@@ -187,15 +182,15 @@ tests[{
 }
 
 tests[{
-    "PolicyId" : "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>",,
+    "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
 ...
 ```
 
 2) Two blank lines between subsections
 
 ```
 tests[{
-    "PolicyId" : "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>",,
+    "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
     "ActualValue" : ExampleVar.ExampleSetting,
@@ -222,15 +217,15 @@ tests[{
 # MS.<Product>.1  #
 ###################
 ```
-2) Indicate the beginning of every policy bullet point.
+2) Indicate the beginning of every policy.
 
 ```
 #
-# MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>
+# MS.<Product>.<Policy Group #>.<Policy #>v<Version #>
 #--
 ```
 
-3) Indicate the end of every policy bullet point.
+3) Indicate the end of every policy.
 
 ```
 #--
@@ -244,13 +239,13 @@ tests[{
 
 ### Booleans
 
-In the interest of consistency across policy tests and human readability of the test, boolean-valued variables should be set via a comparison test against a boolean constant (true/false).
+In the interest of consistency across policy tests and human readability of the test, boolean-valued variables should be set via a comparison test against a boolean constant (true/false) for variables.
 
 #### Correct
 
 ```
 tests[{
-    "PolicyId" : "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>",,
+    "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
     "ActualValue" : ExampleVar.ExampleSetting,
@@ -262,7 +257,7 @@ tests[{
 }
 
 tests[{
-    "PolicyId" : "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>",,
+    "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
     "ActualValue" : ExampleVar.ExampleSetting,
@@ -281,7 +276,7 @@ tests[{
     ...
 }] {
     ExampleVar := input.ExampleVar
-    Status := ExampleVar # Mising == true
+    Status := ExampleVar # Missing == true
 }
 
 tests[{
@@ -292,58 +287,106 @@ tests[{
 }
 ```
 
+Because methods can return undefined, use `not` instead of false comparison when dealing with booleans. `not` will still pass if the you want the false result, i.e. `== false`, but will also pass if the result is undefined. This is important because the default keyword does not work for methods, only variables. So you write your methods for the true cases & treat false/undefined results as failing cases.
+
+#### Correct
+
+
+```
+ExampleVariable contains SomeVariable.DisplayName if {
+    some X
+    SomeVariable := input.example_key[X]
+    not is_null(SomeVariable.OnPremisesImmutableId)
+}
+```
+
+#### Incorrect
+
+```
+ExampleVariable contains SomeVariable.DisplayName if {
+    some X
+    SomeVariable := input.example_key[X]
+    is_null(SomeVariable.OnPremisesImmutableId) == false
+}
+```
+
+Opa will use implicit true in some cases. For example, if the variable contained a true/false boolean, you did not need `== true` to check if the boolean contained is true. This is also true in method returns. Opa assumes methods return true/false & if not specified, will return true. For readability purposes, we enforce explicitly stating `:= true`.
+
+#### Correct
+```
+ExampleMethod(Variable) := true if {
+    count({x | some x in Variable}) == 0
+}
+```
+
+#### Incorrect
+
+```
+ExampleMethod(Variable) if {
+    count({x | some x in Variable}) == 0
+}
+```
+
 ### Taking input
 
-We will always store the input in a variable first thing. It can sometimes be easier to only use `input.ExampleVar` repeatedly, but for consistancy this is best practice. the `[_]` is added to the end for when you are anticipating an array, so the program has to loop through the input. There are other ways to take in input, but OPA Documents states `input.VariableName` is recommended. As such we will only use this method for consistancy. If there is a problem, it can be taken up on a case by case basis for disscussion.
+We will always store the input in a variable first thing. It can sometimes be easier to only use `input.ExampleVar` repeatedly, but for consistency this is best practice. the `some var in` is added to the end for when you are anticipating an array, so the program has to loop through the input. There are other ways to take in input, but OPA Documents states `input.VariableName` is recommended. As such we will only use this method for consistency. If there is a problem, it can be taken up on a case by case basis for discussion.
 
 ```
-tests[{
+tests contains {
     ...
-}] {
-    ExampleVar := input.ExampleVar[_]
+} if {
+    some ExampleVar in input.example_var
     Status := "Example" in ExampleVar
 }
 
-tests[{
+tests contains {
     ...
-}] {
-    ExampleVar := input.ExampleVar
+} if {
+    some ExampleVar in input.example_var[_].nested_var
+    Status := "Example" in ExampleVar
+}
+
+tests contains {
+    ...
+} if {
+    ExampleVar := input.example_var
     Status := ExampleVar == true
 }
 ```
 
 ### ActualValue
 
-It can be tempting to put the status variable in the ActualValue spot when you are anticipating a boolean. DON'T! For consistancy and as best practice put `ExampleVar.ExampleSetting`.
+It can be tempting to put the status variable in the ActualValue spot when you are anticipating a boolean. DON'T! For consistency and as best practice put `ExampleVar.ExampleSetting`.
 
-#### InCorrect
+#### Incorrect
 ```
-tests[{
-    "PolicyId" : "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>",,
+tests contains {
+    "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
     "ActualValue" : Status,
     "ReportDetails" : ReportDetailsBoolean(Status),
     "RequirementMet" : Status
-}] {
-    ExampleVar := input.ExampleVar
+} if {
+    ExampleVar := input.example_var
     Status := ExampleVar == true
 }
 ```
 
 #### Correct
 ```
-tests[{
-    "PolicyId" : "MS.<Product>.<Policy #>.<Bulletpoint #>v<Version #>",,
+tests contains {
+    "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
     "ActualValue" : ExampleVar.ExampleSetting,
     "ReportDetails" : ReportDetailsBoolean(Status),
     "RequirementMet" : Status
-}] {
-    ExampleVar := input.ExampleVar
+} if {
+    ExampleVar := input.example_var
     Status := ExampleVar == true
 }
 ```
+
 ## PowerShell
 [PoshCode's The PowerShell Best Practices and Style Guide](https://github.com/PoshCode/PowerShellPracticeAndStyle)