Rule: ignored-import (#577)

Flagging any reference in a policy that does not make use of
an import where applicable.

Fixes #540

Signed-off-by: Anders Eknert <anders@styra.com>

README.md

@@ -220,6 +220,7 @@ The following rules are currently available:
 | idiomatic   | [use-some-for-output-vars](https://docs.styra.com/regal/rules/idiomatic/use-some-for-output-vars)     | Use `some` to declare output variables                    |
 | imports     | [avoid-importing-input](https://docs.styra.com/regal/rules/imports/avoid-importing-input)             | Avoid importing input                                     |
 | imports     | [circular-import](https://docs.styra.com/regal/rules/imports/circular-import)                         | Circular import                                           |
+| imports     | [ignored-import](https://docs.styra.com/regal/rules/imports/ignored-import)                           | Reference ignores import                                  |
 | imports     | [implicit-future-keywords](https://docs.styra.com/regal/rules/imports/implicit-future-keywords)       | Use explicit future keyword imports                       |
 | imports     | [import-after-rule](https://docs.styra.com/regal/rules/imports/import-after-rule)                     | Import declared after rule                                |
 | imports     | [import-shadows-builtin](https://docs.styra.com/regal/rules/imports/import-shadows-builtin)           | Import shadows built-in namespace                         |

bundle/regal/ast/ast.rego

@@ -321,12 +321,14 @@ is_ref(value) if value.type == "ref"
 
 is_ref(value) if value[0].type == "ref"
 
-all_refs contains value if {
+all_rules_refs contains value if {
 	walk(input.rules, [_, value])
 
 	is_ref(value)
 }
 
+all_refs contains value if some value in all_rules_refs
+
 all_refs contains value if {
 	walk(input.imports, [_, value])
 

bundle/regal/config/config_test.rego

@@ -161,7 +161,7 @@ test_all_rules_are_in_provided_configuration if {
 		some category, title
 		data.regal.rules[category][title]
 		not endswith(title, "_test")
-		not data.regal.config.provided.rules[category][title]
+		not config.provided.rules[category][title]
 	}
 
 	count(missing_config) == 0
@@ -172,7 +172,7 @@ test_all_configured_rules_exist if {
 
 	missing_rules := {title |
 		some category, title
-		data.regal.config.provided.rules[category][title]
+		config.provided.rules[category][title]
 		not data.regal.rules[category][title]
 	}
 

bundle/regal/config/provided/data.yaml

@@ -65,6 +65,8 @@ rules:
       level: error
     circular-import:
       level: error
+    ignored-import:
+      level: error
     implicit-future-keywords:
       level: error
     import-after-rule:

bundle/regal/rules/imports/circular_import_test.rego

@@ -118,7 +118,7 @@ test_import_graph_self_import if {
 }
 
 test_self_reachable if {
-	r := rule.self_reachable with data.regal.rules.imports["circular-import"].import_graph as {
+	r := rule.self_reachable with rule.import_graph as {
 		"data.policy.a": {"data.policy.b"},
 		"data.policy.b": {"data.policy.c"}, "data.policy.c": {"data.policy.a"},
 	}
@@ -127,7 +127,7 @@ test_self_reachable if {
 }
 
 test_groups if {
-	r := rule.groups with data.regal.rules.imports["circular-import"].import_graph as {
+	r := rule.groups with rule.import_graph as {
 		"data.policy.a": {"data.policy.b"},
 		"data.policy.b": {"data.policy.c"},
 		"data.policy.c": {"data.policy.a"},
@@ -145,7 +145,7 @@ test_groups if {
 }
 
 test_groups_empty_graph if {
-	r := rule.groups with data.custom.regal.rules.imports["circular-import"].import_graph as {"data.policy.a": {}}
+	r := rule.groups with rule.import_graph as {"data.policy.a": {}}
 
 	r == set()
 }

bundle/regal/rules/imports/ignored_import.rego

@@ -0,0 +1,34 @@
+# METADATA
+# description: Reference ignores import
+package regal.rules.imports["ignored-import"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+
+import_paths contains path if {
+	some imp in input.imports
+	path := [p.value | some p in imp.path.value]
+
+	path[0] in {"data", "input"}
+}
+
+report contains violation if {
+	some ref in ast.all_rules_refs
+
+	ref.value[0].type == "var"
+	ref.value[0].value in {"data", "input"}
+
+	most_specific_match := regal.last(sort([ip |
+		ref_path := [p.value | some p in ref.value]
+
+		some ip in import_paths
+		array.slice(ref_path, 0, count(ip)) == ip
+	]))
+
+	violation := result.fail(rego.metadata.chain(), object.union(
+		result.location(ref),
+		{"description": sprintf("Reference ignores import of %s", [concat(".", most_specific_match)])},
+	))
+}

bundle/regal/rules/imports/ignored_import_test.rego

@@ -0,0 +1,63 @@
+package regal.rules.imports["ignored-import_test"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.imports["ignored-import"] as rule
+
+test_fail_ignored_import if {
+	module := ast.policy(`
+	import data.foo
+
+	bar := data.foo
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "imports",
+		"description": "Reference ignores import of data.foo",
+		"level": "error",
+		"location": {"col": 9, "file": "policy.rego", "row": 6, "text": "\tbar := data.foo"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/ignored-import", "imports"),
+		}],
+		"title": "ignored-import",
+	}}
+}
+
+test_fail_ignored_most_specific_import if {
+	module := ast.policy(`
+	import data.foo
+	import data.foo.bar
+
+	bar := data.foo.bar
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "imports",
+		"description": "Reference ignores import of data.foo.bar",
+		"level": "error",
+		"location": {"col": 9, "file": "policy.rego", "row": 7, "text": "\tbar := data.foo.bar"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/ignored-import", "imports"),
+		}],
+		"title": "ignored-import",
+	}}
+}
+
+test_success_import_not_ignored if {
+	module := ast.policy(`
+	import data.foo.bar
+
+	foo := bar
+	baz := bar.baz
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}

docs/rules/imports/ignored-import.md

@@ -0,0 +1,59 @@
+# ignored-import
+
+**Summary**: Reference ignores import
+
+**Category**: Imports
+
+**Avoid**
+```rego
+package policy
+
+import rego.v1
+
+import data.authz.roles
+
+allow if {
+    some role in input.user.roles
+    # data.authz.roles has been imported, but the import is ignored here
+    role in data.authz.roles.admin_roles
+}
+```
+
+**Prefer**
+```rego
+package policy
+
+import rego.v1
+
+import data.authz.roles
+
+allow if {
+    some role in input.user.roles
+    # imported data.authz.roles used
+    role in roles.admin_roles
+}
+```
+
+## Rationale
+
+Imports tend to make long, nested references more readable, and encourages reuse of common logic. Using a full reference
+(like `data.users.permissions`) despite having previously imported the reference, or parts of it (like `data.users`)
+defeats the purpose of the import, and you're better off referring to the import directly.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  imports:
+    ignored-import:
+      # one of "error", "warning", "ignore"
+      level: error
+```
+
+## Community
+
+If you think you've found a problem with this rule or its documentation, would like to suggest improvements, new rules,
+or just talk about Regal in general, please join us in the `#regal` channel in the Styra Community
+[Slack](https://communityinviter.com/apps/styracommunity/signup)!