FIX: Effect on Statement is required (#101)

* Effect on Statement is required * add PR number to changelog * update cloudformation_actions.py * update cloudformation_actions.py * PR suggestions Co-authored-by: Ramon <[email protected]>
Skyscanner · Apr 19, 2022 · abddd5e · abddd5e
1 parent f84db2a
commit abddd5e
Show file tree

Hide file tree

Showing 7 changed files with 106 additions and 28 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,12 @@
 # Change Log
 All notable changes to this project will be documented in this file.
 
+## 0.19.1
+### Fixes
+- `Effect` on `Statement` is **required**. [#101](https://github.com/Skyscanner/pycfmodel/pull/97)
+### Updates
+- Update `CLOUDFORMATION_ACTIONS`.
+
 ## 0.19.0
 ### Improvements
 - Able to parse PolicyDocument that are implicit in string properties. [#97](https://github.com/Skyscanner/pycfmodel/pull/97)

diff --git a/pycfmodel/cloudformation_actions.py b/pycfmodel/cloudformation_actions.py
@@ -483,20 +483,24 @@
     "apprunner:AssociateCustomDomain",
     "apprunner:CreateAutoScalingConfiguration",
     "apprunner:CreateConnection",
+    "apprunner:CreateObservabilityConfiguration",
     "apprunner:CreateService",
     "apprunner:CreateVpcConnector",
     "apprunner:DeleteAutoScalingConfiguration",
     "apprunner:DeleteConnection",
+    "apprunner:DeleteObservabilityConfiguration",
     "apprunner:DeleteService",
     "apprunner:DeleteVpcConnector",
     "apprunner:DescribeAutoScalingConfiguration",
     "apprunner:DescribeCustomDomains",
+    "apprunner:DescribeObservabilityConfiguration",
     "apprunner:DescribeOperation",
     "apprunner:DescribeService",
     "apprunner:DescribeVpcConnector",
     "apprunner:DisassociateCustomDomain",
     "apprunner:ListAutoScalingConfigurations",
     "apprunner:ListConnections",
+    "apprunner:ListObservabilityConfigurations",
     "apprunner:ListOperations",
     "apprunner:ListServices",
     "apprunner:ListTagsForResource",
@@ -1014,7 +1018,7 @@
     "billingconductor:ListPricingRules",
     "billingconductor:ListPricingRulesAssociatedToPricingPlan",
     "billingconductor:ListResourcesAssociatedToCustomLineItem",
-    "billingconductor:ListTagsResource",
+    "billingconductor:ListTagsForResource",
     "billingconductor:TagResource",
     "billingconductor:UntagResource",
     "billingconductor:UpdateBillingGroup",
@@ -1116,13 +1120,17 @@
     "chatbot:CreateSlackChannelConfiguration",
     "chatbot:DeleteChimeWebhookConfiguration",
     "chatbot:DeleteSlackChannelConfiguration",
+    "chatbot:DeleteSlackUserIdentity",
     "chatbot:DeleteSlackWorkspaceAuthorization",
     "chatbot:DescribeChimeWebhookConfigurations",
     "chatbot:DescribeSlackChannelConfigurations",
     "chatbot:DescribeSlackChannels",
+    "chatbot:DescribeSlackUserIdentities",
     "chatbot:DescribeSlackWorkspaces",
+    "chatbot:GetAccountPreferences",
     "chatbot:GetSlackOauthParameters",
     "chatbot:RedeemSlackOauthCode",
+    "chatbot:UpdateAccountPreferences",
     "chatbot:UpdateChimeWebhookConfiguration",
     "chatbot:UpdateSlackChannelConfiguration",
     "chime:AcceptDelegate",
@@ -2788,6 +2796,7 @@
     "datasync:CreateAgent",
     "datasync:CreateLocationEfs",
     "datasync:CreateLocationFsxLustre",
+    "datasync:CreateLocationFsxOpenZfs",
     "datasync:CreateLocationFsxWindows",
     "datasync:CreateLocationHdfs",
     "datasync:CreateLocationNfs",
@@ -2801,6 +2810,7 @@
     "datasync:DescribeAgent",
     "datasync:DescribeLocationEfs",
     "datasync:DescribeLocationFsxLustre",
+    "datasync:DescribeLocationFsxOpenZfs",
     "datasync:DescribeLocationFsxWindows",
     "datasync:DescribeLocationHdfs",
     "datasync:DescribeLocationNfs",
@@ -3062,6 +3072,7 @@
     "devicefarm:UpdateUpload",
     "devicefarm:UpdateVPCEConfiguration",
     "devops-guru:AddNotificationChannel",
+    "devops-guru:DeleteInsight",
     "devops-guru:DescribeAccountHealth",
     "devops-guru:DescribeAccountOverview",
     "devops-guru:DescribeAnomaly",
@@ -4558,19 +4569,22 @@
     "events:CreateApiDestination",
     "events:CreateArchive",
     "events:CreateConnection",
+    "events:CreateEndpoint",
     "events:CreateEventBus",
     "events:CreatePartnerEventSource",
     "events:DeactivateEventSource",
     "events:DeauthorizeConnection",
     "events:DeleteApiDestination",
     "events:DeleteArchive",
     "events:DeleteConnection",
+    "events:DeleteEndpoint",
     "events:DeleteEventBus",
     "events:DeletePartnerEventSource",
     "events:DeleteRule",
     "events:DescribeApiDestination",
     "events:DescribeArchive",
     "events:DescribeConnection",
+    "events:DescribeEndpoint",
     "events:DescribeEventBus",
     "events:DescribeEventSource",
     "events:DescribePartnerEventSource",
@@ -4582,6 +4596,7 @@
     "events:ListApiDestinations",
     "events:ListArchives",
     "events:ListConnections",
+    "events:ListEndpoints",
     "events:ListEventBuses",
     "events:ListEventSources",
     "events:ListPartnerEventSourceAccounts",
@@ -4605,6 +4620,7 @@
     "events:UpdateApiDestination",
     "events:UpdateArchive",
     "events:UpdateConnection",
+    "events:UpdateEndpoint",
     "evidently:BatchEvaluateFeature",
     "evidently:CreateExperiment",
     "evidently:CreateFeature",
@@ -4687,25 +4703,29 @@
     "fis:UntagResource",
     "fis:UpdateExperimentTemplate",
     "fms:AssociateAdminAccount",
+    "fms:AssociateThirdPartyFirewall",
     "fms:DeleteAppsList",
     "fms:DeleteNotificationChannel",
     "fms:DeletePolicy",
     "fms:DeleteProtocolsList",
     "fms:DisassociateAdminAccount",
+    "fms:DisassociateThirdPartyFirewall",
     "fms:GetAdminAccount",
     "fms:GetAppsList",
     "fms:GetComplianceDetail",
     "fms:GetNotificationChannel",
     "fms:GetPolicy",
     "fms:GetProtectionStatus",
     "fms:GetProtocolsList",
+    "fms:GetThirdPartyFirewallAssociationStatus",
     "fms:GetViolationDetails",
     "fms:ListAppsLists",
     "fms:ListComplianceStatus",
     "fms:ListMemberAccounts",
     "fms:ListPolicies",
     "fms:ListProtocolsLists",
     "fms:ListTagsForResource",
+    "fms:ListThirdPartyFirewallFirewallPolicies",
     "fms:PutAppsList",
     "fms:PutNotificationChannel",
     "fms:PutPolicy",
@@ -6083,6 +6103,7 @@
     "iot:ListJobTemplates",
     "iot:ListJobs",
     "iot:ListManagedJobTemplates",
+    "iot:ListMetricValues",
     "iot:ListMitigationActions",
     "iot:ListNamedShadowsForThing",
     "iot:ListOTAUpdates",
@@ -7905,6 +7926,34 @@
     "mgn:UpdateReplicationConfiguration",
     "mgn:UpdateReplicationConfigurationTemplate",
     "mgn:UpdateSourceServerReplicationType",
+    "migrationhub-orchestrator:CreateWorkflow",
+    "migrationhub-orchestrator:CreateWorkflowStep",
+    "migrationhub-orchestrator:CreateWorkflowStepGroup",
+    "migrationhub-orchestrator:DeleteWorkflow",
+    "migrationhub-orchestrator:DeleteWorkflowStep",
+    "migrationhub-orchestrator:DeleteWorkflowStepGroup",
+    "migrationhub-orchestrator:GetMessage",
+    "migrationhub-orchestrator:GetTemplate",
+    "migrationhub-orchestrator:GetTemplateStep",
+    "migrationhub-orchestrator:GetTemplateStepGroup",
+    "migrationhub-orchestrator:GetWorkflow",
+    "migrationhub-orchestrator:GetWorkflowStep",
+    "migrationhub-orchestrator:GetWorkflowStepGroup",
+    "migrationhub-orchestrator:ListPlugins",
+    "migrationhub-orchestrator:ListTemplateStepGroups",
+    "migrationhub-orchestrator:ListTemplateSteps",
+    "migrationhub-orchestrator:ListTemplates",
+    "migrationhub-orchestrator:ListWorkflowStepGroups",
+    "migrationhub-orchestrator:ListWorkflowSteps",
+    "migrationhub-orchestrator:ListWorkflows",
+    "migrationhub-orchestrator:RegisterPlugin",
+    "migrationhub-orchestrator:RetryWorkflowStep",
+    "migrationhub-orchestrator:SendMessage",
+    "migrationhub-orchestrator:StartWorkflow",
+    "migrationhub-orchestrator:StopWorkflow",
+    "migrationhub-orchestrator:UpdateWorkflow",
+    "migrationhub-orchestrator:UpdateWorkflowStep",
+    "migrationhub-orchestrator:UpdateWorkflowStepGroup",
     "migrationhub-strategy:GetAntiPattern",
     "migrationhub-strategy:GetApplicationComponentDetails",
     "migrationhub-strategy:GetApplicationComponentStrategies",
@@ -8939,7 +8988,6 @@
     "rds:CopyDBParameterGroup",
     "rds:CopyDBSnapshot",
     "rds:CopyOptionGroup",
-    "rds:CreateCustomAvailabilityZone",
     "rds:CreateCustomDBEngineVersion",
     "rds:CreateDBCluster",
     "rds:CreateDBClusterEndpoint",
@@ -8957,7 +9005,6 @@
     "rds:CreateGlobalCluster",
     "rds:CreateOptionGroup",
     "rds:CrossRegionCommunication",
-    "rds:DeleteCustomAvailabilityZone",
     "rds:DeleteCustomDBEngineVersion",
     "rds:DeleteDBCluster",
     "rds:DeleteDBClusterEndpoint",
@@ -8973,12 +9020,10 @@
     "rds:DeleteDBSubnetGroup",
     "rds:DeleteEventSubscription",
     "rds:DeleteGlobalCluster",
-    "rds:DeleteInstallationMedia",
     "rds:DeleteOptionGroup",
     "rds:DeregisterDBProxyTargets",
     "rds:DescribeAccountAttributes",
     "rds:DescribeCertificates",
-    "rds:DescribeCustomAvailabilityZones",
     "rds:DescribeDBClusterBacktracks",
     "rds:DescribeDBClusterEndpoints",
     "rds:DescribeDBClusterParameterGroups",
@@ -9007,7 +9052,6 @@
     "rds:DescribeEvents",
     "rds:DescribeExportTasks",
     "rds:DescribeGlobalClusters",
-    "rds:DescribeInstallationMedia",
     "rds:DescribeOptionGroupOptions",
     "rds:DescribeOptionGroups",
     "rds:DescribeOrderableDBInstanceOptions",
@@ -9022,7 +9066,6 @@
     "rds:DownloadDBLogFilePortion",
     "rds:FailoverDBCluster",
     "rds:FailoverGlobalCluster",
-    "rds:ImportInstallationMedia",
     "rds:ListTagsForResource",
     "rds:ModifyCertificates",
     "rds:ModifyCurrentDBClusterCapacity",

diff --git a/pycfmodel/model/resources/properties/policy_document.py b/pycfmodel/model/resources/properties/policy_document.py
@@ -35,6 +35,10 @@ def _statement_as_list(self) -> List[Statement]:
             return [self.Statement]
         return self.Statement
 
+    @staticmethod
+    def _is_statement_effect_allow(statement_effect: str) -> bool:
+        return statement_effect.lower() == "allow"
+
     def statements_with(self, pattern: Pattern) -> List[Statement]:
         """
         Finds all statements which have at least one resource with the pattern.
@@ -60,7 +64,7 @@ def allowed_actions_with(self, pattern: Pattern) -> List[Statement]:
         return [
             statement
             for statement in self._statement_as_list()
-            if statement.actions_with(pattern) and statement.Effect == "Allow"
+            if statement.actions_with(pattern) and self._is_statement_effect_allow(statement.Effect)
         ]
 
     def allowed_principals_with(self, pattern: Pattern) -> List[str]:
@@ -75,7 +79,7 @@ def allowed_principals_with(self, pattern: Pattern) -> List[str]:
         """
         principals = set()
         for statement in self._statement_as_list():
-            if statement.Effect == "Allow":
+            if self._is_statement_effect_allow(statement.Effect):
                 principals.update(statement.principals_with(pattern))
         return list(principals)
 
@@ -91,7 +95,7 @@ def non_whitelisted_allowed_principals(self, whitelist: List[str]) -> List[str]:
         """
         principals = set()
         for statement in self._statement_as_list():
-            if statement.Effect == "Allow":
+            if self._is_statement_effect_allow(statement.Effect):
                 principals.update(statement.non_whitelisted_principals(whitelist))
         return list(principals)
 
@@ -130,6 +134,6 @@ def get_allowed_actions(self) -> List[str]:
         """
         actions = set()
         for statement in self._statement_as_list():
-            if statement.Effect.lower() == "allow":
+            if self._is_statement_effect_allow(statement.Effect):
                 actions.update(statement.get_expanded_action_list())
         return sorted(actions)
diff --git a/pycfmodel/model/resources/properties/statement.py b/pycfmodel/model/resources/properties/statement.py
@@ -25,7 +25,7 @@ class Principal(Property):
 
 class Statement(Property):
     """
-    Contains information about an attached policy.
+    Contains information about an statement of a policy document.
 
     Properties:
 
@@ -43,7 +43,7 @@ class Statement(Property):
     """
 
     Sid: Optional[ResolvableStr] = None
-    Effect: Optional[ResolvableStr] = None
+    Effect: ResolvableStr
     Principal: Optional[PrincipalTypes] = None
     NotPrincipal: Optional[PrincipalTypes] = None
     Action: Optional[ResolvableStrOrList] = None
@@ -53,9 +53,11 @@ class Statement(Property):
     Condition: Optional[StatementCondition] = None
 
     @validator("Effect")
-    def capitalize_if_str(cls, v: ResolvableStr):
+    def allowed_values_for_effect_and_capitalized(cls, v: ResolvableStr):
         if isinstance(v, str):
-            return v.capitalize()
+            v = v.capitalize()
+            if v not in ["Allow", "Deny"]:
+                raise ValueError("Effect on Statement must either be Allow or Deny")
         return v
 
     def get_action_list(self, include_action=True, include_not_action=True) -> List[ResolvableStr]:

diff --git a/setup.py b/setup.py
@@ -21,7 +21,7 @@
 
 setup(
     name="pycfmodel",
-    version="0.19.0",
+    version="0.19.1",
     description="A python model for CloudFormation scripts",
     author="Skyscanner Product Security",
     author_email="[email protected]",

diff --git a/tests/resources/properties/test_policy_document.py b/tests/resources/properties/test_policy_document.py
@@ -1,6 +1,7 @@
 import re
 from ipaddress import IPv4Network
 
+import pytest
 from pytest import fixture
 
 from pycfmodel.cloudformation_actions import CLOUDFORMATION_ACTIONS
@@ -34,7 +35,7 @@ def policy_document_multi_statement():
                     "Action": ["sts:AssumeRole"],
                 },
                 {
-                    "Effect": "bar",
+                    "Effect": "Allow",
                     "Principal": {"Service": ["ec2.amazonaws.com"], "AWS": "arn:aws:iam::324320755747:root"},
                     "Action": ["sts:AssumeRole"],
                 },
@@ -179,7 +180,7 @@ def test_one_statement(policy_document_one_statement):
 
 def test_multi_statements(policy_document_multi_statement):
     assert policy_document_multi_statement.Statement[0].Effect == "Allow"
-    assert policy_document_multi_statement.Statement[1].Effect == "Bar"
+    assert policy_document_multi_statement.Statement[1].Effect == "Allow"
 
 
 def test_star_resource(policy_document_star_resource):
@@ -337,6 +338,17 @@ def policy_document_condition_with_source_vpce():
     )
 
 
+@pytest.mark.parametrize(
+    "statement_effect, is_allow",
+    [
+        ("Allow", True),
+        ("Deny", False),
+    ],
+)
+def test_policy_document_chan_check_if_the_statement_effect_is_allow_or_deny(statement_effect: str, is_allow: bool):
+    assert PolicyDocument._is_statement_effect_allow(statement_effect=statement_effect) == is_allow
+
+
 def test_policy_document_condition_with_source_ip(policy_document_condition_with_source_ip: PolicyDocument):
     assert policy_document_condition_with_source_ip.Statement[0].Condition.IpAddress == {
         "aws:SourceIp": [IPv4Network("116.202.65.160/32"), IPv4Network("116.202.68.32/27")]