From 3205762160dab68f5766d58446cde74617ce9b9d Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Fri, 23 Feb 2024 15:36:18 +0100 Subject: [PATCH] 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 --- README.md | 1 + bundle/regal/ast/ast.rego | 4 +- bundle/regal/config/config_test.rego | 4 +- bundle/regal/config/provided/data.yaml | 2 + .../rules/imports/circular_import_test.rego | 6 +- .../regal/rules/imports/ignored_import.rego | 34 ++++++++++ .../rules/imports/ignored_import_test.rego | 63 +++++++++++++++++++ docs/rules/imports/ignored-import.md | 59 +++++++++++++++++ 8 files changed, 167 insertions(+), 6 deletions(-) create mode 100644 bundle/regal/rules/imports/ignored_import.rego create mode 100644 bundle/regal/rules/imports/ignored_import_test.rego create mode 100644 docs/rules/imports/ignored-import.md diff --git a/README.md b/README.md index 75134c68..5d44ebe5 100644 --- a/README.md +++ b/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 | diff --git a/bundle/regal/ast/ast.rego b/bundle/regal/ast/ast.rego index 1b520323..2e8e08d0 100644 --- a/bundle/regal/ast/ast.rego +++ b/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]) diff --git a/bundle/regal/config/config_test.rego b/bundle/regal/config/config_test.rego index c1e986a2..381f2fd4 100644 --- a/bundle/regal/config/config_test.rego +++ b/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] } diff --git a/bundle/regal/config/provided/data.yaml b/bundle/regal/config/provided/data.yaml index 9d798282..7ec2b0c2 100644 --- a/bundle/regal/config/provided/data.yaml +++ b/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: diff --git a/bundle/regal/rules/imports/circular_import_test.rego b/bundle/regal/rules/imports/circular_import_test.rego index 64a4047a..4ddd292b 100644 --- a/bundle/regal/rules/imports/circular_import_test.rego +++ b/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() } diff --git a/bundle/regal/rules/imports/ignored_import.rego b/bundle/regal/rules/imports/ignored_import.rego new file mode 100644 index 00000000..049a0850 --- /dev/null +++ b/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)])}, + )) +} diff --git a/bundle/regal/rules/imports/ignored_import_test.rego b/bundle/regal/rules/imports/ignored_import_test.rego new file mode 100644 index 00000000..473e1986 --- /dev/null +++ b/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() +} diff --git a/docs/rules/imports/ignored-import.md b/docs/rules/imports/ignored-import.md new file mode 100644 index 00000000..20612d5e --- /dev/null +++ b/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)!