Refactor rule metadata logic

Some additional refactoring made possible now that each rule resides in its own file. Extend this further to have each rule in its own package, and move the metadata from the `report` rule in each package and to the package declaration. We may then use `rego.metadata.chain()` instead, which includes the path of the package containing both the category and the title, thus allowing us to skip having to redeclare that in annotations. This has a number of benefits: - Tests are now proper _unit_ tests, and no longer test the whole category. - Checking if a rule is ignored can now be the responsibility of the main rule, and linter rules need no longer be concerned with that unless they have unique configuration options. - Reduce the amount of clutter in each rule related to metadata, and with that the possibility of mistakes. Signed-off-by: Anders Eknert <anders@styra.com>
StyraInc · Jun 26, 2023 · 8bf7f62 · 8bf7f62
1 parent 54907c0
commit 8bf7f62
Show file tree

Hide file tree

Showing 65 changed files with 486 additions and 710 deletions.
diff --git a/bundle/regal/main.rego b/bundle/regal/main.rego
@@ -4,6 +4,8 @@ import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
+import data.regal.config
+
 report contains violation if {
 	not is_object(input)
 
@@ -24,14 +26,28 @@ report contains violation if {
 	}
 }
 
+to_meta(category, title) := {
+	"custom": {"category": category},
+	"title": title,
+}
+
+# Check bundled rules
 report contains violation if {
-	violation := data.regal.rules[_].report[_]
+	some category, title
+	violation := data.regal.rules[category][title].report[_]
+
+	config.for_rule(to_meta(category, title)).level != "ignore"
 
 	not ignored(violation, ignore_directives)
 }
 
+# Check custom rules
 report contains violation if {
-	violation := data.custom.regal.rules[_].report[_]
+	some category, title
+
+	violation := data.custom.regal.rules[category][title].report[_]
+
+	config.for_rule(to_meta(category, title)).level != "ignore"
 
 	not ignored(violation, ignore_directives)
 }

diff --git a/bundle/regal/main_test.rego b/bundle/regal/main_test.rego
@@ -5,7 +5,7 @@ test_main_basic_input_success {
 	report == set()
 }
 
-test_main_multiple_failues {
+test_main_multiple_failures {
 	policy := `package p
 
 	# both camel case and unification operator

diff --git a/bundle/regal/result.rego b/bundle/regal/result.rego
@@ -5,7 +5,58 @@ import future.keywords.in
 
 import data.regal.config
 
-fail(metadata, details) := violation if {
+# Provided rules, i.e. regal.rules.category.title
+fail(chain, details) := violation if {
+	is_array(chain) # from rego.metadata.chain()
+
+	some link in chain
+	link.annotations.scope == "package"
+
+	some category, title
+	["regal", "rules", category, title] = link.path
+
+	# NOTE: consider allowing annotation to override any derived values?
+	annotation := object.union(link.annotations, {
+		"custom": {"category": category},
+		"title": title,
+		"related_resources": _related_resources(link.annotations, category, title),
+	})
+
+	violation := _fail_annotated(annotation, details)
+}
+
+# Custom rules, i.e. custom.regal.rules.category.title
+fail(chain, details) := violation if {
+	is_array(chain) # from rego.metadata.chain()
+
+	some link in chain
+	link.annotations.scope == "package"
+
+	some category, title
+	["custom", "regal", "rules", category, title] = link.path
+
+	# NOTE: consider allowing annotation to override any derived values?
+	annotation := object.union(link.annotations, {
+		"custom": {"category": category},
+		"title": title,
+		"related_resources": _related_resources(link.annotations, category, title),
+	})
+
+	violation := _fail_annotated(annotation, details)
+}
+
+_related_resources(annotations, _, _) := annotations.related_resources
+
+_related_resources(annotations, category, title) := rr if {
+	not annotations.related_resources
+	rr := [{
+		"description": "documentation",
+		"ref": sprintf("%s/%s/%s", [config.docs.base_url, category, title]),
+	}]
+}
+
+_fail_annotated(metadata, details) := violation if {
+	is_object(metadata) # from rego.metadata.rule()
 	with_location := object.union(metadata, details)
 	category := with_location.custom.category
 	with_category := object.union(with_location, {
@@ -22,6 +73,8 @@ fail(metadata, details) := violation if {
 	)
 }
 
+fail(metadata, details) := _fail_annotated(metadata, details)
+
 resource_urls(related_resources, category) := [r |
 	some item in related_resources
 	r := object.union(object.remove(item, ["ref"]), {"ref": config.docs.resolve_url(item.ref, category)})

diff --git a/bundle/regal/rules/bugs/common_test.rego b/bundle/regal/rules/bugs/common_test.rego
diff --git a/bundle/regal/rules/bugs/constant_condition.rego b/bundle/regal/rules/bugs/constant_condition.rego
@@ -1,10 +1,11 @@
-package regal.rules.bugs
+# METADATA
+# description: Constant condition
+package regal.rules.bugs["constant-condition"]
 
 import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
-import data.regal.config
 import data.regal.result
 
 # NOTE: The constant condition checks currently don't do nesting!
@@ -31,36 +32,16 @@ probably_no_body(rule) if {
 	rule.body[0].terms.value == true
 }
 
-# METADATA
-# title: constant-condition
-# description: Constant condition
-# related_resources:
-# - description: documentation
-#   ref: $baseUrl/$category/constant-condition
-# custom:
-#   category: bugs
 report contains violation if {
-	config.for_rule(rego.metadata.rule()).level != "ignore"
-
 	some rule in _rules_with_bodies
 	some expr in rule.body
 
 	expr.terms.type in _scalars
 
-	violation := result.fail(rego.metadata.rule(), result.location(expr))
+	violation := result.fail(rego.metadata.chain(), result.location(expr))
 }
 
-# METADATA
-# title: constant-condition
-# description: Constant condition
-# related_resources:
-# - description: documentation
-#   ref: $baseUrl/$category/constant-condition
-# custom:
-#   category: bugs
 report contains violation if {
-	config.for_rule(rego.metadata.rule()).level != "ignore"
-
 	some rule in _rules_with_bodies
 	some expr in rule.body
 
@@ -70,5 +51,5 @@ report contains violation if {
 	expr.terms[1].type in _scalars
 	expr.terms[2].type in _scalars
 
-	violation := result.fail(rego.metadata.rule(), result.location(expr))
+	violation := result.fail(rego.metadata.chain(), result.location(expr))
 }
diff --git a/bundle/regal/rules/bugs/constant_condition_test.rego b/bundle/regal/rules/bugs/constant_condition_test.rego
@@ -2,11 +2,13 @@ package regal.rules.bugs_test
 
 import future.keywords.if
 
+import data.regal.ast
 import data.regal.config
-import data.regal.rules.bugs.common_test.report
+
+import data.regal.rules.bugs["constant-condition"] as rule
 
 test_fail_simple_constant_condition if {
-	r := report(`allow {
+	r := rule.report with input as ast.policy(`allow {
 	1
 	}`)
 	r == {{
@@ -23,11 +25,12 @@ test_fail_simple_constant_condition if {
 }
 
 test_success_static_condition_probably_generated if {
-	report(`allow { true }`) == set()
+	r := rule.report with input as ast.policy(`allow { true }`)
+	r == set()
 }
 
 test_fail_operator_constant_condition if {
-	r := report(`allow {
+	r := rule.report with input as ast.policy(`allow {
 	1 == 1
 	}`)
 	r == {{
@@ -44,5 +47,6 @@ test_fail_operator_constant_condition if {
 }
 
 test_success_non_constant_condition if {
-	report(`allow { 1 == input.one }`) == set()
+	r := rule.report with input as ast.policy(`allow { 1 == input.one }`)
+	r == set()
 }
diff --git a/bundle/regal/rules/bugs/not_equals_in_loop.rego b/bundle/regal/rules/bugs/not_equals_in_loop.rego
@@ -1,23 +1,14 @@
-package regal.rules.bugs
+# METADATA
+# description: Use of != in loop
+package regal.rules.bugs["not-equals-in-loop"]
 
 import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
-import data.regal.config
 import data.regal.result
 
-# METADATA
-# title: not-equals-in-loop
-# description: Use of != in loop
-# related_resources:
-# - description: documentation
-#   ref: $baseUrl/$category/not-equals-in-loop
-# custom:
-#   category: bugs
 report contains violation if {
-	config.for_rule(rego.metadata.rule()).level != "ignore"
-
 	some rule in input.rules
 	some expr in rule.body
 
@@ -32,5 +23,5 @@ report contains violation if {
 	neq_term.value[i].type == "var"
 	startswith(neq_term.value[i].value, "$")
 
-	violation := result.fail(rego.metadata.rule(), result.location(expr.terms[0]))
+	violation := result.fail(rego.metadata.chain(), result.location(expr.terms[0]))
 }
diff --git a/bundle/regal/rules/bugs/not_equals_in_loop_test.rego b/bundle/regal/rules/bugs/not_equals_in_loop_test.rego
@@ -2,11 +2,12 @@ package regal.rules.bugs_test
 
 import future.keywords.if
 
-import data.regal.rules.bugs.common_test.report
-import data.regal.rules.bugs.common_test.report_with_fk
+import data.regal.ast
+import data.regal.config
+import data.regal.rules.bugs["not-equals-in-loop"] as rule
 
 test_fail_neq_in_loop if {
-	r := report(`deny {
+	r := rule.report with input as ast.policy(`deny {
 		"admin" != input.user.groups[_]
 		input.user.groups[_] != "admin"
 	}`)
@@ -18,7 +19,7 @@ test_fail_neq_in_loop if {
 			"location": {"col": 11, "file": "policy.rego", "row": 4, "text": "\t\t\"admin\" != input.user.groups[_]"},
 			"related_resources": [{
 				"description": "documentation",
-				"ref": "https://github.com/StyraInc/regal/blob/main/docs/rules/bugs/not-equals-in-loop.md",
+				"ref": config.docs.resolve_url("$baseUrl/$category/not-equals-in-loop", "bugs"),
 			}],
 			"title": "not-equals-in-loop",
 		},
@@ -29,23 +30,23 @@ test_fail_neq_in_loop if {
 			"location": {"col": 24, "file": "policy.rego", "row": 5, "text": "\t\tinput.user.groups[_] != \"admin\""},
 			"related_resources": [{
 				"description": "documentation",
-				"ref": "https://github.com/StyraInc/regal/blob/main/docs/rules/bugs/not-equals-in-loop.md",
+				"ref": config.docs.resolve_url("$baseUrl/$category/not-equals-in-loop", "bugs"),
 			}],
 			"title": "not-equals-in-loop",
 		},
 	}
 }
 
 test_fail_neq_in_loop_one_liner if {
-	r := report_with_fk(`deny if "admin" != input.user.groups[_]`)
+	r := rule.report with input as ast.with_future_keywords(`deny if "admin" != input.user.groups[_]`)
 	r == {{
 		"category": "bugs",
 		"description": "Use of != in loop",
 		"level": "error",
 		"location": {"col": 17, "file": "policy.rego", "row": 8, "text": "deny if \"admin\" != input.user.groups[_]"},
 		"related_resources": [{
 			"description": "documentation",
-			"ref": "https://github.com/StyraInc/regal/blob/main/docs/rules/bugs/not-equals-in-loop.md",
+			"ref": config.docs.resolve_url("$baseUrl/$category/not-equals-in-loop", "bugs"),
 		}],
 		"title": "not-equals-in-loop",
 	}}

diff --git a/bundle/regal/rules/bugs/rule_named_if.rego b/bundle/regal/rules/bugs/rule_named_if.rego
@@ -1,25 +1,16 @@
-package regal.rules.bugs
+# METADATA
+# description: Rule named "if"
+package regal.rules.bugs["rule-named-if"]
 
 import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
-import data.regal.config
 import data.regal.result
 
-# METADATA
-# title: rule-named-if
-# description: Rule named "if"
-# related_resources:
-# - description: documentation
-#   ref: $baseUrl/$category/rule-named-if
-# custom:
-#   category: bugs
 report contains violation if {
-	config.for_rule(rego.metadata.rule()).level != "ignore"
-
 	some rule in input.rules
 	rule.head.name == "if"
 
-	violation := result.fail(rego.metadata.rule(), result.location(rule.head))
+	violation := result.fail(rego.metadata.chain(), result.location(rule.head))
 }
diff --git a/bundle/regal/rules/bugs/rule_named_if_test.rego b/bundle/regal/rules/bugs/rule_named_if_test.rego
@@ -2,11 +2,12 @@ package regal.rules.bugs_test
 
 import future.keywords.if
 
+import data.regal.ast
 import data.regal.config
-import data.regal.rules.bugs.common_test.report
+import data.regal.rules.bugs["rule-named-if"] as rule
 
 test_fail_rule_named_if if {
-	r := report(`
+	r := rule.report with input as ast.policy(`
 	allow := true if {
         input.foo
     }`)

diff --git a/bundle/regal/rules/bugs/rule_shadows_builtin.rego b/bundle/regal/rules/bugs/rule_shadows_builtin.rego
@@ -1,26 +1,17 @@
-package regal.rules.bugs
+# METADATA
+# description: Rule name shadows built-in
+package regal.rules.bugs["rule-shadows-builtin"]
 
 import future.keywords.contains
 import future.keywords.if
 import future.keywords.in
 
 import data.regal.ast
-import data.regal.config
 import data.regal.result
 
-# METADATA
-# title: rule-shadows-builtin
-# description: Rule name shadows built-in
-# related_resources:
-# - description: documentation
-#   ref: $baseUrl/$category/rule-shadows-builtin
-# custom:
-#   category: bugs
 report contains violation if {
-	config.for_rule(rego.metadata.rule()).level != "ignore"
-
 	some rule in input.rules
 	rule.head.name in ast.builtin_names
 
-	violation := result.fail(rego.metadata.rule(), result.location(rule.head))
+	violation := result.fail(rego.metadata.chain(), result.location(rule.head))
 }