Repository: salesforce/policy_sentry
Pull request #246 (2020-10-11 20:47:50)

fix None type scenario in get_actions_for_service
## What does this PR do?
Address NoneType error raised as part of issue https://github.com/salesforce/policy_sentry/issues/239

[//]: # (Required: Describe the effects of your pull request in detail. If
multiple changes are involved, a bulleted list is often useful.)

## What gif best describes this PR or how it makes you feel?

[//]: # (Encouraged: Insert an appropriate gif/meme to brighten up your reviewer's day.)

## Completion checklist

- [x] Additions and changes have unit tests
- [x] [Unit tests, Pylint, security testing, and Integration tests are passing.](https://github.com/salesforce/policy_sentry/actions). GitHub actions does this automatically
- [ ] The pull request has been appropriately labeled using the provided PR labels


reetasingh: @kmcquade please review , this is my first PR for this repo. Hoping to make more contributions soon
reetasingh: ```
Python 3.9.0 (tags/v3.9.0:9cf6752, Oct  5 2020, 15:34:40) [MSC v.1927 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>>
>>> from policy_sentry.querying.actions import get_actions_for_service
>>>
>>>
>>> def example():
...     actions = get_actions_for_service('abc')  # Then you can leverage any method that requires access to the database.
...     for action in actions:
...         print(action)
...
>>> if __name__ == '__main__':
...     example()
...
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 2, in example
  File "C:\Users\rakes\git\policy_sentry\policy_sentry\querying\actions.py", line 26, in get_actions_for_service
    service_prefix_data = get_service_prefix_data(service_prefix)
  File "C:\Users\rakes\git\policy_sentry\policy_sentry\shared\iam_data.py", line 27, in get_service_prefix_data
    raise Exception(f"Service prefix {service_prefix} not found.")
Exception: Service prefix abc not found.
>>>
```
kmcquade: Hey @reetasingh! Thanks for making a PR.

I think that you might have made the fix in the wrong file. 

```
  File "/usr/local/Cellar/policy_sentry/0.8.7/libexec/lib/python3.8/site-packages/policy_sentry/command/query.py", line 84, in action_table
    query_action_table(name, service, access_level, condition, wildcard_only, fmt)
  File "/usr/local/Cellar/policy_sentry/0.8.7/libexec/lib/python3.8/site-packages/policy_sentry/command/query.py", line 163, in query_action_table
    output = get_actions_for_service(service)
  File "/usr/local/Cellar/policy_sentry/0.8.7/libexec/lib/python3.8/site-packages/policy_sentry/querying/actions.py", line 28, in get_actions_for_service
    for item in service_prefix_data["privileges"]:
TypeError: 'NoneType' object is not subscriptable
```

The above indicates two different options to investigate:
1. `policy_sentry.shared.iam_data.get_service_prefix_data`
2. `policy_sentry.querying.actions.get_actions_for_service`

### get_service_prefix_data

The section you chose - `policy_sentry.shared.iam_data.get_service_prefix_data` - is working as intended, actually. Some background:

I use Policy Sentry as a library, as do several others that I'm aware of - most notably in Cloudsplaining. Cloudsplaining pulls live AWS IAM Policies for analysis. Sometimes these IAM Policies reference AWS Services that have not yet been included in the IAM Definition that we store at [policy_sentry/shared/data/iam-definition.json](https://github.com/salesforce/policy_sentry/blob/master/policy_sentry/shared/data/iam-definition.json). Our IAM definition gets updated once a month. 

But for Cloudsplaining, we need to make sure that if a service does not exist, that Policy Sentry just logs the issue and does not break the scan by throwing an exception. I've encountered this issue a few times - where Policy Sentry gets a new version at the beginning of the month, and AWS releases 4 new service prefixes within that same month, before we have time to release a new version of Policy Sentry. That would break the scan.

So, given the above - `get_service_prefix_data` is working as intended.

### get_actions_for_service

I think this is the one that actually needs fixing. The function below looks like it will still try to add an object of type `None` to the list of results. I think that is what causes the issue I mentioned.

https://github.com/salesforce/policy_sentry/blob/a8502ce66e75b42adaa7091910948e3bb72f9f09/policy_sentry/querying/actions.py#L26-L30

I think this means you would have to change your PR to be focused on `get_actions_for_service` instead of `get_service_prefix_data`. Might want to just start with a fresh PR though and close this one - that should be cleaner.
reetasingh: > service_prefix_data["privileges"]

Thanks for the background @kmcquade 

actually, service_prefix_data["privileges"] is the one which is failing because service_prefix_data is None and we are trying to find priveleges in a None type

do you want me to add a check for not None before calling service_prefix_data["privileges"]

 
kmcquade: Yeah that works! Thank you!
reetasingh: @kmcquade addressed the review comments

Patch:
diff --git a/policy_sentry/querying/actions.py b/policy_sentry/querying/actions.py
index f5df5ded..306c7620 100644
--- a/policy_sentry/querying/actions.py
+++ b/policy_sentry/querying/actions.py
@@ -25,8 +25,9 @@ def get_actions_for_service(service_prefix):
     """
     service_prefix_data = get_service_prefix_data(service_prefix)
     results = []
-    for item in service_prefix_data["privileges"]:
-        results.append(f"{service_prefix}:{item}")
+    if isinstance(service_prefix_data, dict):
+        for item in service_prefix_data["privileges"]:
+            results.append(f"{service_prefix}:{item}")
     return results
 
 


Test patch:
diff --git a/test/querying/test_query_actions.py b/test/querying/test_query_actions.py
index 9280abcb..fcd94574 100644
--- a/test/querying/test_query_actions.py
+++ b/test/querying/test_query_actions.py
@@ -72,6 +72,13 @@ class QueryActionsTestCase(unittest.TestCase):
         # old: 0.021s
         # this one: 0.005s
 
+    def test_get_actions_for_invalid_service(self):
+        """querying.actions.get_actions_for_service
+           for invalid service
+        """
+        output = get_actions_for_service("invalid_service")
+        self.assertListEqual([], output)
+
     def test_get_privilege_info(self):
         expected_results_file = os.path.abspath(
             os.path.join(