Adding _remove_permissions_from_roles() and extracting parts of repo_role() into shared methods. #213

scriptsrc · 2019-03-18T23:05:42Z

Adding _remove_permissions_from_roles() and extracting parts of repo_role() into shared methods.

Also updates the policy size checking to correctly remove all whitespace before checking the size.

Need a code review with @mcpeak.

mcpeak · 2019-03-19T16:10:23Z

repokid/cli/repokid_cli.py

@@ -48,6 +49,7 @@
 from cloudaux.aws.iam import (delete_role_policy, get_account_authorization_details, get_role_inline_policies,
                              put_role_policy)
 from cloudaux.aws.sts import sts_conn
+from policyuniverse import ARN


please keep this in alphabetical order grouped by import type (stdlib, third party libs, module imports)

mcpeak · 2019-03-19T16:14:15Z

repokid/cli/repokid_cli.py

+
+
+def _check_inline_policies_size(policies):
+    """Validate the policies, when converted to JSON without whitespace, remain under the size limit."""


The comment says validate policies remain under the limit but you return True when the size exceeds the limit.

mcpeak · 2019-03-19T16:48:01Z

repokid/cli/repokid_cli.py

+        role_name = arn.name.split('/')[-1]
+
+        role_id = find_role_in_cache(dynamo_table, account_number, role_name)
+        _remove_permissions_from_role(account_number, permissions, role_name, role_id, dynamo_table, config, hooks, commit=commit)


I'd prefer putting the hook calling here so it's easier to see that it's happening.

mcpeak · 2019-03-19T16:50:03Z

repokid/cli/repokid_cli.py

+
+
+def _remove_permissions_from_role(account_number, permissions, role_name, role_id, dynamo_table, config, hooks, commit=False):
+    """Remove the given permissions from the given role.


Is permissions here a single permission? A list of permissions? It's called permissions but below you reference "the selected permission".

mcpeak · 2019-03-19T16:50:53Z

repokid/cli/repokid_cli.py

@@ -1052,6 +1155,12 @@ def main():
    if args.get('find_roles_with_permission'):
        return find_roles_with_permission(args.get('<permission>'), dynamo_table)

+    if args.get('remove_permissions_from_roles'):
+        permissions = args.get('--permissions')
+        roles = args.get('--role-filename')


roles seems like a list of roles but it's a filename, make the variable name consistent.

mcpeak · 2019-03-19T16:52:19Z

repokid/cli/repokid_cli.py

@@ -1052,6 +1155,12 @@ def main():
    if args.get('find_roles_with_permission'):
        return find_roles_with_permission(args.get('<permission>'), dynamo_table)

+    if args.get('remove_permissions_from_roles'):
+        permissions = args.get('--permissions')
+        roles = args.get('--role-filename')


From a user perspective, where is the file supposed to come from? Do we want to provide other options to find the roles since we have a database?

JohnVonNeumann · 2019-03-21T12:49:25Z

In line with #44 should all new functions be following a standard around docstrings? Is this a situation where development should start as it means to go on?

JohnVonNeumann · 2019-03-21T12:58:34Z

repokid/cli/repokid_cli.py

@@ -48,6 +49,7 @@
 from cloudaux.aws.iam import (delete_role_policy, get_account_authorization_details, get_role_inline_policies,
                              put_role_policy)
 from cloudaux.aws.sts import sts_conn
+from policyuniverse.arn import ARN


nit: alphabetise the imports

Should probably have flake8 enforce import order if it's something we care about.

But yes, as @mcpeak pointed this out, I will fix.

JohnVonNeumann · 2019-03-21T13:04:03Z

repokid/cli/repokid_cli.py

+def _check_access_advisor_age(role, role_name, account_number, max_days_old):
+    """Ensure access advisor data has been recently refreshed."""
+    old_aa_data_services = []
+    for aa_service in role.aa_data:


can this block be cleaned up using some variables for the various times/periods so that the arithmetic is easier to understand from a first look.

ie:

last_updated_time = datetime.datetime.strptime(aa_service['lastUpdated'], '%a, %d %b %Y %H:%M:%S %Z') current_time = datetime.datetime.now() expiration_time = datetime.timedelta(days=max_days_old) if( last_updated_time < current_time - expiration_time): old_aa_data_services.append(aa_service['serviceName'])

Probably need to play with var names, I'm tired and not picking the correct words/context for the block, but you get the idea of what I'm trying to convey.

This was originally inline for the repo_role() method. I refactored it into it's own method thinking I would need this code for the main method that I am adding, but it turns out that I don't. I'll move this back into repo_role(), but won't be otherwise fixing the naming.

Not in this PR, anyways.

scriptsrc · 2019-03-21T17:06:44Z

Thank you @JohnVonNeumann and @mcpeak for the review. I'll work on many of these items for this PR. Others are broader concerns that I may follow up on in other PRs.

Again, thanks.

…role() into shared methods.

coveralls · 2019-03-27T00:48:52Z

Coverage increased (+3.4%) to 58.221% when pulling 0dc1149 on remove_specific_permission into ea37f65 on master.

scriptsrc · 2019-03-27T00:56:51Z

…ons. Other various fixes.

scriptsrc · 2019-03-28T22:41:16Z

This PR has been put through it's paces in our test environment.

update_role_cache
display_role_cache
display_role
repo_role
rollback_role
find_roles_with_permissions
remove_permissions_from_roles

Seems to work as expected.

Future work may be to have the remove_permissions_from_roles flow not touch statements or policies that don't have the provided permission. Right now it will expand the wildcards on those otherwise unrelated statements, which cause problems with the policy size limit.

mcpeak reviewed Mar 19, 2019

View reviewed changes

JohnVonNeumann reviewed Mar 21, 2019

View reviewed changes

pkelley added 4 commits March 26, 2019 16:38

Adding _remove_permissions_from_roles() and extracting parts of repo_…

c618354

…role() into shared methods.

Fixing import.

0da59de

Fixing flake8 problems.

0086c1d

Updating PR based on Code Review. Still need to update documentation.

42d097c

scriptsrc force-pushed the remove_specific_permission branch from 4f1b6c3 to 42d097c Compare March 27, 2019 00:46

pkelley added 3 commits March 27, 2019 09:22

Adding find/remove instructions to README.

776bfc5

Adding some unit tests for the new methods.

5bbb0cf

Making find_roles_with_permission capable of taking multiple permissi…

0dc1149

…ons. Other various fixes.

mcpeak approved these changes Mar 28, 2019

View reviewed changes

JohnVonNeumann approved these changes Mar 28, 2019

View reviewed changes

scriptsrc merged commit 439f346 into master Mar 28, 2019

scriptsrc deleted the remove_specific_permission branch March 28, 2019 23:19

scriptsrc mentioned this pull request Mar 28, 2019

Repokid should provide the ability to remove a specified permission. #2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding _remove_permissions_from_roles() and extracting parts of repo_role() into shared methods. #213

Adding _remove_permissions_from_roles() and extracting parts of repo_role() into shared methods. #213

scriptsrc commented Mar 18, 2019

mcpeak Mar 19, 2019

mcpeak Mar 19, 2019

mcpeak Mar 19, 2019

mcpeak Mar 19, 2019

mcpeak Mar 19, 2019

mcpeak Mar 19, 2019

JohnVonNeumann commented Mar 21, 2019

JohnVonNeumann Mar 21, 2019

scriptsrc Mar 21, 2019

scriptsrc Mar 21, 2019

JohnVonNeumann Mar 21, 2019

scriptsrc Mar 21, 2019

scriptsrc commented Mar 21, 2019

coveralls commented Mar 27, 2019 •

edited

Loading

scriptsrc commented Mar 27, 2019 •

edited

Loading

scriptsrc commented Mar 28, 2019



		def _check_inline_policies_size(policies):
		"""Validate the policies, when converted to JSON without whitespace, remain under the size limit."""



		def _remove_permissions_from_role(account_number, permissions, role_name, role_id, dynamo_table, config, hooks, commit=False):
		"""Remove the given permissions from the given role.

Adding _remove_permissions_from_roles() and extracting parts of repo_role() into shared methods. #213

Adding _remove_permissions_from_roles() and extracting parts of repo_role() into shared methods. #213

Conversation

scriptsrc commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnVonNeumann commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scriptsrc commented Mar 21, 2019

coveralls commented Mar 27, 2019 • edited Loading

scriptsrc commented Mar 27, 2019 • edited Loading

scriptsrc commented Mar 28, 2019

coveralls commented Mar 27, 2019 •

edited

Loading

scriptsrc commented Mar 27, 2019 •

edited

Loading