From 635b96288f4e6c368bcccb239f000aaa2f2e5ee4 Mon Sep 17 00:00:00 2001
From: Kevin DeJong <kddejong@amazon.com>
Date: Mon, 2 Dec 2019 13:18:08 -0800
Subject: [PATCH] Look at top level function in a condition (#1235)

---
 src/cfnlint/rules/conditions/Configuration.py | 11 ++-
 test/fixtures/templates/bad/conditions.yaml   | 73 +++++++++----------
 .../rules/conditions/test_configuration.py    |  2 +-
 test/unit/rules/conditions/test_used.py       |  2 +-
 4 files changed, 45 insertions(+), 43 deletions(-)

diff --git a/src/cfnlint/rules/conditions/Configuration.py b/src/cfnlint/rules/conditions/Configuration.py
index 9870272cbb..a7b4bb9deb 100644
--- a/src/cfnlint/rules/conditions/Configuration.py
+++ b/src/cfnlint/rules/conditions/Configuration.py
@@ -17,9 +17,8 @@ class Configuration(CloudFormationLintRule):
     condition_keys = [
         'Fn::And',
         'Fn::Equals',
-        'Fn::If',
         'Fn::Not',
-        'Fn::Or'
+        'Fn::Or',
     ]
 
     def match(self, cfn):
@@ -43,5 +42,13 @@ def match(self, cfn):
                             ['Conditions', condname],
                             message.format(condname)
                         ))
+                    else:
+                        for k, _ in condobj.items():
+                            if k not in self.condition_keys:
+                                message = 'Condition {0} has invalid property {1}'
+                                matches.append(RuleMatch(
+                                    ['Conditions', condname] + [k],
+                                    message.format(condname, k)
+                                ))
 
         return matches
diff --git a/test/fixtures/templates/bad/conditions.yaml b/test/fixtures/templates/bad/conditions.yaml
index ef01d444f9..e4dc79b473 100644
--- a/test/fixtures/templates/bad/conditions.yaml
+++ b/test/fixtures/templates/bad/conditions.yaml
@@ -37,47 +37,47 @@ Parameters:
   EnableGeoBlocking:
     Type: String
 Conditions:
-  CreateProdResources: !Equals [ !Ref EnvType, prod ]
+  CreateProdResources: !Equals [!Ref EnvType, prod]
   BadCondition: String
-  UnusedCondition: !Equals [ !Ref EnvType, prod ]
+  UnusedCondition: !Equals [!Ref EnvType, prod]
   TooManyConditions:
-    Fn::Equals: [ !Ref EnvType, prod ]
-    Fn::Not: !Equals [ !Ref EnvType, prod ]
-  EnableGeoBlocking: !Equals [ !Ref EnableGeoBlocking, "true" ]
+    Fn::Equals: [!Ref EnvType, prod]
+    Fn::Not: !Equals [!Ref EnvType, prod]
+  EnableGeoBlocking: !Equals [!Ref EnableGeoBlocking, "true"]
+  HasParam:
+    "Fn::Of":
+      - !Not [!Equals [!Ref EnvType, ""]]
+      - !Not [!Equals [!Ref EnableGeoBlocking, ""]]
 Resources:
   EC2Instance:
     Type: "AWS::EC2::Instance"
     Properties:
       ImageId: !FindInMap [RegionMap, !Ref "AWS::Region", AMI]
       Tags:
-      -
-        Key: TestKey
-        Value: TestValue
-      - Fn::If:
-        - isProd
-        - Key: Environment1
-          Value: Prod
+        - Key: TestKey
+          Value: TestValue
         - Fn::If:
-          - isDev
-          - BadKey: Environment2
-            BadValue: Dev
-          - !Ref AWS::NoValue
+            - isProd
+            - Key: Environment1
+              Value: Prod
+            - Fn::If:
+                - isDev
+                - BadKey: Environment2
+                  BadValue: Dev
+                - !Ref AWS::NoValue
   MountPoint:
     Type: "AWS::EC2::VolumeAttachment"
     Condition: CreateProdResources
     Properties:
-      InstanceId:
-        !Ref EC2Instance
-      VolumeId:
-        !Ref NewVolume
+      InstanceId: !Ref EC2Instance
+      VolumeId: !Ref NewVolume
       Device: /dev/sdh
   NewVolume:
     Type: "AWS::EC2::Volume"
     Condition: CreateProdResources
     Properties:
       Size: 100
-      AvailabilityZone:
-        !GetAtt EC2Instance.AvailabilityZone
+      AvailabilityZone: !GetAtt EC2Instance.AvailabilityZone
   CloudFrontDistribution:
     Type: "AWS::CloudFront::Distribution"
     Condition: False
@@ -85,23 +85,18 @@ Resources:
       DistributionConfig:
         Enabled: true
         Restrictions:
-          GeoRestriction:
-            !If
-              - EnableGeoBlocking
-              -
-                RestrictionType:
-                  !If
-                    - EnableGeoBlocking
-                    -
-                      - whitelist
-                    - whitelist
-                BadLocations:
-                  - BE
-                  - LU
-                  - NL
-              - RestrictionType: none
+          GeoRestriction: !If
+            - EnableGeoBlocking
+            - RestrictionType: !If
+                - EnableGeoBlocking
+                - - whitelist
+                - whitelist
+              BadLocations:
+                - BE
+                - LU
+                - NL
+            - RestrictionType: none
 Outputs:
   VolumeId:
     Condition: CreateProdResources
-    Value:
-      !Ref NewVolume
+    Value: !Ref NewVolume
diff --git a/test/unit/rules/conditions/test_configuration.py b/test/unit/rules/conditions/test_configuration.py
index 730fa194af..dfc3beffcb 100644
--- a/test/unit/rules/conditions/test_configuration.py
+++ b/test/unit/rules/conditions/test_configuration.py
@@ -20,4 +20,4 @@ def test_file_positive(self):
 
     def test_file_negative(self):
         """Test failure"""
-        self.helper_file_negative('test/fixtures/templates/bad/conditions.yaml', 2)
+        self.helper_file_negative('test/fixtures/templates/bad/conditions.yaml', 3)
diff --git a/test/unit/rules/conditions/test_used.py b/test/unit/rules/conditions/test_used.py
index 830a929cc7..ce741f730d 100644
--- a/test/unit/rules/conditions/test_used.py
+++ b/test/unit/rules/conditions/test_used.py
@@ -24,4 +24,4 @@ def test_file_positive(self):
 
     def test_file_negative(self):
         """Test failure"""
-        self.helper_file_negative('test/fixtures/templates/bad/conditions.yaml', 3)
+        self.helper_file_negative('test/fixtures/templates/bad/conditions.yaml', 4)