From 4ed85a5479b9b4343ed4c7244dc05179a994715b Mon Sep 17 00:00:00 2001
From: Kinnaird McQuade <kmcquade@salesforce.com>
Date: Wed, 6 May 2020 18:39:08 -0400
Subject: [PATCH] Fixes #23 - issue arising from policies with "Deny" with no
 resource constraints

---
 CHANGELOG.md                            |  3 +++
 cloudsplaining/bin/cloudsplaining       |  2 +-
 cloudsplaining/scan/policy_document.py  |  6 ++++--
 cloudsplaining/scan/statement_detail.py |  8 ++++----
 test/scanning/test_policy_document.py   | 20 ++++++++++++++++++++
 5 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d23e40df..fe945e45 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,8 @@
 # CHANGELOG
 
+## 0.0.11 (2020-05-06)
+* Fixed an issue arising from policies where "Deny" was used in effect with no resource constraints. Fixes #23.
+
 ## 0.0.10 (2020-05-05)
 * Removed the recursive credentials method from the `download` command.
 * Fixed occasional installation error occurring from outdated Policy Sentry versions.
diff --git a/cloudsplaining/bin/cloudsplaining b/cloudsplaining/bin/cloudsplaining
index 71876209..b09c0248 100755
--- a/cloudsplaining/bin/cloudsplaining
+++ b/cloudsplaining/bin/cloudsplaining
@@ -7,7 +7,7 @@
 """
     Cloudsplaining is an AWS IAM Assessment tool that identifies violations of least privilege and generates a risk-prioritized HTML report with a triage worksheet.
 """
-__version__ = "0.0.10"
+__version__ = "0.0.11"
 import click
 from cloudsplaining import command
 
diff --git a/cloudsplaining/scan/policy_document.py b/cloudsplaining/scan/policy_document.py
index 046b6c0f..b523257c 100644
--- a/cloudsplaining/scan/policy_document.py
+++ b/cloudsplaining/scan/policy_document.py
@@ -40,7 +40,8 @@ def all_allowed_actions(self):
         """Output all allowed IAM Actions, regardless of resource constraints"""
         allowed_actions = []
         for statement in self.statements:
-            allowed_actions.extend(statement.expanded_actions)
+            if statement.expanded_actions:
+                allowed_actions.extend(statement.expanded_actions)
         allowed_actions = list(dict.fromkeys(allowed_actions))
         return allowed_actions
 
@@ -50,7 +51,8 @@ def all_allowed_unrestricted_actions(self):
         allowed_actions = []
         for statement in self.statements:
             if not statement.has_resource_constraints:
-                allowed_actions.extend(statement.expanded_actions)
+                if statement.expanded_actions:
+                    allowed_actions.extend(statement.expanded_actions)
         allowed_actions = list(dict.fromkeys(allowed_actions))
         return allowed_actions
 
diff --git a/cloudsplaining/scan/statement_detail.py b/cloudsplaining/scan/statement_detail.py
index e8006b59..2529c0ae 100644
--- a/cloudsplaining/scan/statement_detail.py
+++ b/cloudsplaining/scan/statement_detail.py
@@ -81,7 +81,7 @@ def _not_action_effective_actions(self):
         """If NotAction is used, calculate the allowed actions - i.e., what it would be """
         effective_actions = []
         if not self.not_action:
-            return False
+            return None
         not_actions_expanded = determine_actions_to_expand(self.not_action)
         not_actions_expanded_lowercase = [x.lower() for x in not_actions_expanded]
 
@@ -116,12 +116,12 @@ def _not_action_effective_actions(self):
             return effective_actions
         elif self.has_resource_constraints and self.effect_deny:
             logger.debug("NOTE: Haven't decided if we support Effect Deny here?")
-            return False
+            return None
         elif not self.has_resource_constraints and self.effect_deny:
             logger.debug("NOTE: Haven't decided if we support Effect Deny here?")
-            return False
+            return None
         else:
-            return False
+            return None
 
     @property
     def has_not_resource_with_allow(self):
diff --git a/test/scanning/test_policy_document.py b/test/scanning/test_policy_document.py
index dd390feb..d2443b7a 100644
--- a/test/scanning/test_policy_document.py
+++ b/test/scanning/test_policy_document.py
@@ -182,3 +182,23 @@ def test_allows_specific_actions(self):
         ]
         results = policy_document.allows_specific_actions_without_constraints(high_priority_read_only_actions)
         self.assertListEqual(results, high_priority_read_only_actions)
+
+    def test_policy_document_not_action_deny_gh_23(self):
+        test_policy = {
+            "Version": "2012-10-17",
+            "Statement": [{
+                "Sid": "DenyAllUsersNotUsingMFA",
+                "Effect": "Deny",
+                "NotAction": "iam:*",
+                "Resource": "*",
+                "Condition": {"BoolIfExists": {"aws:MultiFactorAuthPresent": "false"}}
+            }]
+        }
+        policy_document = PolicyDocument(test_policy)
+        allowed_actions = []
+        for statement in policy_document.statements:
+            if not statement.has_resource_constraints:
+                if statement.expanded_actions:
+                    allowed_actions.extend(statement.expanded_actions)
+        self.assertListEqual(allowed_actions, [])
+        self.assertListEqual(policy_document.all_allowed_unrestricted_actions, [])