Added more detailed verification of UpdateHandler (#46)

aws-cloudformation · Jan 30, 2024 · 36b5d75 · 36b5d75
1 parent a1f00a6
commit 36b5d75
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 18 deletions.
diff --git a/docs/BASIC_LINTING.md b/docs/BASIC_LINTING.md
@@ -55,12 +55,14 @@
 | | `PER002` | `"Resource MUST NOT specify wildcard permissions for create handler"` |
 | `ensure_resource_read_handler_exists_and_have_permissions` | `PER003` | `"Resource MUST implement read handler"` |
 | | `PER004` | `"Resource MUST NOT specify wildcard permissions for read handler"` |
-| `ensure_resource_update_handler_exists_and_have_permissions` | `PER005` | `"Resource MUST implement update handler"` |
-| | `PER006` | `"Resource MUST NOT specify wildcard permissions for update handler"` |
-| `ensure_resource_delete_handler_exists_and_have_permissions` | `PER007` | `"Resource MUST implement delete handler"` |
-| | `PER008` | `"Resource MUST NOT specify wildcard permissions for delete handler"` |
-| `ensure_resource_list_handler_exists_and_have_permissions` | `PER009` | `"Resource MUST implement list handler"` |
-| | `PER010` | `"Resource MUST NOT specify wildcard permissions for list handler"` |
+| `ensure_resource_update_handler_exists_and_have_permissions` | `PER005` | `"Resource SHOULD implement update handler"` |
+| | `PER006` | `"Resource update handler MUST have permissions list specified"` |
+| | `PER006` | `"Resource update handler MUST have non-empty permissions"` |
+| | `PER007` | `"Resource MUST NOT specify wildcard permissions for update handler"` |
+| `ensure_resource_delete_handler_exists_and_have_permissions` | `PER008` | `"Resource MUST implement delete handler"` |
+| | `PER009` | `"Resource MUST NOT specify wildcard permissions for delete handler"` |
+| `ensure_resource_list_handler_exists_and_have_permissions` | `PER010` | `"Resource MUST implement list handler"` |
+| | `PER011` | `"Resource MUST NOT specify wildcard permissions for list handler"` |
 
 #### Other Checks
 | Rule Name   |      Check Id      |  Message |

diff --git a/src/rpdk/guard_rail/core/templates/guard-result-pojo.output b/src/rpdk/guard_rail/core/templates/guard-result-pojo.output
@@ -19,15 +19,17 @@
 {% endfor %}
 
 
+{% if failed_rules%}
 {{failed_header}}
 {% for rule, checks in failed_rules.items() %}
 {{rule.upper()}}:
     {%- for check in checks %}
-    {% if check.path or check.path != "unidentified"%}check-id: {{check.check_id}}
+    {% if check.path and check.path != "unidentified"%}check-id: {{check.check_id}}
     message: {{check.message}}
     path: {{check.path}}
     {% else %}check-id: {{check.check_id}}
     message: {{check.message}}
     {% endif %}
     {%- endfor -%}
 {% endfor %}
+{% endif %}
diff --git a/src/rpdk/guard_rail/rule_library/permissions/schema-linter-core-permission-rules.guard b/src/rpdk/guard_rail/rule_library/permissions/schema-linter-core-permission-rules.guard
@@ -50,21 +50,45 @@ rule ensure_resource_update_handler_exists_and_have_permissions {
     handlers.update exists
     <<
     {
-        "result": "NON_COMPLIANT",
+        "result": "WARNING",
         "check_id": "PER005",
-        "message": "Resource MUST implement update handler"
+        "message": "Resource SHOULD implement update handler"
     }
     >>
 
-    handlers.update.permissions.* {
-        this != %wildcard_notation
+    when handlers.update exists {
+        handlers.update.permissions exists
         <<
         {
             "result": "NON_COMPLIANT",
             "check_id": "PER006",
-            "message": "Resource MUST NOT specify wildcard permissions for update handler"
+            "message": "Resource update handler MUST have permissions list specified"
         }
         >>
+
+        when handlers.update.permissions exists {
+            handlers.update.permissions !empty
+            <<
+            {
+                "result": "NON_COMPLIANT",
+                "check_id": "PER006",
+                "message": "Resource update handler MUST have non-empty permissions"
+            }
+            >>
+        }
+
+        when handlers.update.permissions !empty {
+            handlers.update.permissions.* {
+                this != %wildcard_notation
+                <<
+                {
+                    "result": "NON_COMPLIANT",
+                    "check_id": "PER007",
+                    "message": "Resource MUST NOT specify wildcard permissions for update handler"
+                }
+                >>
+            }
+        }
     }
 }
 
@@ -73,7 +97,7 @@ rule ensure_resource_delete_handler_exists_and_have_permissions {
     <<
     {
         "result": "NON_COMPLIANT",
-        "check_id": "PER007",
+        "check_id": "PER008",
         "message": "Resource MUST implement delete handler"
     }
     >>
@@ -83,7 +107,7 @@ rule ensure_resource_delete_handler_exists_and_have_permissions {
         <<
         {
             "result": "NON_COMPLIANT",
-            "check_id": "PER008",
+            "check_id": "PER009",
             "message": "Resource MUST NOT specify wildcard permissions for delete handler"
         }
         >>
@@ -95,7 +119,7 @@ rule ensure_resource_list_handler_exists_and_have_permissions {
     <<
     {
         "result": "NON_COMPLIANT",
-        "check_id": "PER009",
+        "check_id": "PER010",
         "message": "Resource MUST implement list handler"
     }
     >>
@@ -105,7 +129,7 @@ rule ensure_resource_list_handler_exists_and_have_permissions {
         <<
         {
             "result": "NON_COMPLIANT",
-            "check_id": "PER010",
+            "check_id": "PER011",
             "message": "Resource MUST NOT specify wildcard permissions for list handler"
         }
         >>

diff --git a/tests/integ/data/schema-update-handler-empty-permissions.json b/tests/integ/data/schema-update-handler-empty-permissions.json
@@ -0,0 +1,8 @@
+{
+    "typeName": "SAMPLE::UNITTEST::SCHEMA",
+    "handlers": {
+        "update": {
+            "permissions": []
+        }
+    }
+}
diff --git a/tests/integ/data/schema-update-handler-no-permissions.json b/tests/integ/data/schema-update-handler-no-permissions.json
@@ -0,0 +1,6 @@
+{
+    "typeName": "SAMPLE::UNITTEST::SCHEMA",
+    "handlers": {
+        "update": {}
+    }
+}
diff --git a/tests/integ/runner/test_integ_runner.py b/tests/integ/runner/test_integ_runner.py
@@ -111,16 +111,63 @@
                         path="/primaryIdentifier/0",
                     )
                 },
+            },
+            {
                 "ensure_resource_update_handler_exists_and_have_permissions": {
                     GuardRuleResult(
                         check_id="PER005",
-                        message="Resource MUST implement update handler",
+                        message="Resource SHOULD implement update handler",
+                        path="",
+                    )
+                }
+            },
+        ),
+        (
+            collect_schemas(
+                schemas=[
+                    "file:/"
+                    + str(
+                        Path(os.path.dirname(os.path.realpath(__file__))).joinpath(
+                            "../data/schema-update-handler-no-permissions.json"
+                        )
+                    )
+                ]
+            ),
+            [],
+            {
+                "ensure_resource_update_handler_exists_and_have_permissions": {
+                    GuardRuleResult(
+                        check_id="PER006",
+                        message="Resource update handler MUST have permissions list specified",
                         path="",
                     )
                 },
             },
             {},
         ),
+        (
+            collect_schemas(
+                schemas=[
+                    "file:/"
+                    + str(
+                        Path(os.path.dirname(os.path.realpath(__file__))).joinpath(
+                            "../data/schema-update-handler-empty-permissions.json"
+                        )
+                    )
+                ]
+            ),
+            [],
+            {
+                "ensure_resource_update_handler_exists_and_have_permissions": {
+                    GuardRuleResult(
+                        check_id="PER006",
+                        message="Resource update handler MUST have non-empty permissions",
+                        path="/handlers/update/permissions",
+                    )
+                },
+            },
+            {},
+        ),
     ],
 )
 def test_exec_compliance_stateless(

diff --git a/tests/unit/core/test_data_types.py b/tests/unit/core/test_data_types.py
@@ -52,5 +52,5 @@ def test_success_result_str():
                 }
             )
         )
-        == "---------\n[SKIPPED]:\n\n\n\x1b[32m[PASSED]:\x1b[39m\n\n\n\x1b[33m[WARNING]:\x1b[39m\n\n\n\n\x1b[31m[FAILED]:\x1b[39m\n\nENSURE_OLD_PROPERTY_NOT_TURNED_IMMUTABLE:\n    check-id: MI007\n    message: cannot remove minimum from properties\n    path: /minimum/removed\n    "  # pylint: disable=C0301
+        == "---------\n[SKIPPED]:\n\n\n\x1b[32m[PASSED]:\x1b[39m\n\n\n\x1b[33m[WARNING]:\x1b[39m\n\n\n\n\n\x1b[31m[FAILED]:\x1b[39m\n\nENSURE_OLD_PROPERTY_NOT_TURNED_IMMUTABLE:\n    check-id: MI007\n    message: cannot remove minimum from properties\n    path: /minimum/removed\n    \n"  # pylint: disable=C0301
     )