Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IAM definition update code #254

Merged
merged 8 commits into from
Oct 16, 2020
Merged

Fix IAM definition update code #254

merged 8 commits into from
Oct 16, 2020

Conversation

reetasingh
Copy link
Contributor

@reetasingh reetasingh commented Oct 16, 2020

What does this PR do?

Address #253

as per the instruction in the issue itself

  1. first updated policy_sentry/shared/awsdocs.py
  2. ran https://github.com/salesforce/policy_sentry/blob/master/utils/download_docs.py .this updated the policy_sentry/shared/data/iam-definition.json and other files under policy_sentry/shared/data/docs

What gif best describes this PR or how it makes you feel?

1_jpM16hr5Bgh-0v3xJBJRxA

updating the code which updates the IAM data locally

Completion checklist

@reetasingh reetasingh changed the title update IAM data Fix IAM definition update code Oct 16, 2020
@reetasingh
Copy link
Contributor Author

reetasingh commented Oct 16, 2020

@kmcquade let me know if this is what is expected and what will be the best way to test
I am especially interested in regression testing

@kmcquade
Copy link
Collaborator

kmcquade commented Oct 16, 2020

Awesome!! This helps so much @reetasingh.

let me know if this is what is expected

Yes, this is perfect.

I am especially interested in regression testing

Generally, my testing strategy has gone like this so far:

  • File mapping: Unit test files map directly to the .py files in the Policy Sentry package
  • Folder mapping: Folder structure of the test files maps directly to the folder structure in the Policy Sentry package
  • GitHub issue mapping: As users have opened up new bugs, I add unit tests that correspond directly to those GitHub issues to show that my fix work specifically for their ask. I prefixed them with test_gh_999 if 999 is the GitHub issue number. For example:
    def test_gh_211_write_with_empty_access_level_lists(self):
  • Adaptable to IAM Definition changes: When I started unit tests for the write-policy command, I initially wrote my tests so that they would compare the results with the expected_results.
    • Over time, as AWS added new IAM actions, that meant that there would be more actions per access level
    • Example: let's say that hypothetically the secretsmanager service had 4 actions at the "write" access level that could be restricted to the secret resource type. If I wrote the tests so that they tested the value of the generated policy, that works for the initial tests. But if AWS adds two new actions that can be restricted to that access level and resource type, then the tests would fail.
    • This only really applies for where the expected test values would be dependent on the current state of the IAM definition (aka the IAM Database, iam_definition.json) and the actions in there. So, validating the output of write-policy related commands, and query related commands.
    • After I realized that, I started writing my tests so that the expected output that showed what actions you'd want would be in the comments instead of the expected_results, so the human knows what's going on. And then I would just check that the size of the array holding the actions is greater than or equal to the size that it is currently at. So, in the secretsmanager example above, the test would probably just check that the size of the actions is greater than or equal to 4.

I'm going to paste this very informative text about the testing strategy in the relevant GitHub issue so others can find it later lol.

At one point, I am pretty sure I achieved 100% test coverage 😁 I did that for the learning experience and because I was frustrated with when my tests would fail.

Examples of how to do these tests well:

expected_statement_ids = [
"RdsReadDb",
"MultMultNone",
"RdsWriteDb",
"RdsListDb"
]
policy = write_policy_with_template(cfg)
for statement in policy.get("Statement"):
self.assertTrue(statement.get("Sid") in expected_statement_ids)

@kmcquade kmcquade merged commit e6d275b into salesforce:master Oct 16, 2020
saikirankv pushed a commit to saikirankv/policy_sentry that referenced this pull request Nov 18, 2020
update awsdocs.py for new AWS formatting changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants