Address issues mentioned in review

Dunedan · Dunedan · commit 3939aee4a377 · 2017-01-03T19:05:49.000+01:00
diff --git a/pre_commit_hooks/detect_aws_credentials.py b/pre_commit_hooks/detect_aws_credentials.py
@@ -10,24 +10,23 @@
 def get_aws_credential_files_from_env():
     """Extract credential file paths from environment variables."""
     files = set()
-    for env_var in {'AWS_CONFIG_FILE', 'AWS_CREDENTIAL_FILE',
-                    'AWS_SHARED_CREDENTIALS_FILE', 'BOTO_CONFIG'}:
-        try:
+    for env_var in (
+        'AWS_CONFIG_FILE', 'AWS_CREDENTIAL_FILE', 'AWS_SHARED_CREDENTIALS_FILE',
+        'BOTO_CONFIG'
+    ):
+        if env_var in os.environ:
             files.add(os.environ[env_var])
-        except KeyError:
-            pass
     return files
 
 
 def get_aws_secrets_from_env():
     """Extract AWS secrets from environment variables."""
     keys = set()
-    for env_var in {'AWS_SECRET_ACCESS_KEY', 'AWS_SECURITY_TOKEN',
-                    'AWS_SESSION_TOKEN'}:
-        try:
+    for env_var in (
+        'AWS_SECRET_ACCESS_KEY', 'AWS_SECURITY_TOKEN', 'AWS_SESSION_TOKEN'
+    ):
+        if env_var in os.environ:
             keys.add(os.environ[env_var])
-        except KeyError:
-            pass
     return keys
 
 
@@ -49,8 +48,10 @@ def get_aws_secrets_from_file(credentials_file):
 
     keys = set()
     for section in parser.sections():
-        for var in {'aws_secret_access_key', 'aws_security_token',
-                    'aws_session_token'}:
+        for var in (
+            'aws_secret_access_key', 'aws_security_token',
+            'aws_session_token'
+        ):
             try:
                 keys.add(parser.get(section, var))
             except configparser.NoOptionError:
@@ -74,7 +75,7 @@ def check_file_for_aws_keys(filenames, keys):
                 # collision
                 if key in text_body:
                     bad_files.append({'filename': filename,
-                                      'key': key[:4].ljust(32, str('*'))})
+                                      'key': key[:4] + '*' * 28})
     return bad_files
 
 
@@ -109,16 +110,17 @@ def main(argv=None):
     keys |= get_aws_secrets_from_env()
 
     if not keys:
-        print('No AWS keys were found in the configured credential files and '
-              'environment variables.\nPlease ensure you have the correct '
-              'setting for --credentials-file')
+        print(
+            'No AWS keys were found in the configured credential files and '
+            'environment variables.\nPlease ensure you have the correct '
+            'setting for --credentials-file'
+        )
         return 2
 
     bad_filenames = check_file_for_aws_keys(args.filenames, keys)
     if bad_filenames:
         for bad_file in bad_filenames:
-            print('AWS secret found in {filename}: {key}'.format(
-                **bad_file))
+            print('AWS secret found in {filename}: {key}'.format(**bad_file))
         return 1
     else:
         return 0
diff --git a/tests/detect_aws_credentials_test.py b/tests/detect_aws_credentials_test.py
@@ -1,4 +1,5 @@
 import pytest
+from mock import patch
 
 from pre_commit_hooks.detect_aws_credentials import get_aws_credential_files_from_env
 from pre_commit_hooks.detect_aws_credentials import get_aws_secrets_from_env
@@ -7,72 +8,100 @@
 from testing.util import get_resource_path
 
 
-def test_get_aws_credentials_file_from_env(monkeypatch):
+@pytest.mark.parametrize(
+    ('env_vars', 'values'),
+    (
+        ({}, set()),
+        ({'AWS_DUMMY_KEY': '/foo'}, set()),
+        ({'AWS_CONFIG_FILE': '/foo'}, {'/foo'}),
+        ({'AWS_CREDENTIAL_FILE': '/foo'}, {'/foo'}),
+        ({'AWS_SHARED_CREDENTIALS_FILE': '/foo'}, {'/foo'}),
+        ({'BOTO_CONFIG': '/foo'}, {'/foo'}),
+        ({'AWS_DUMMY_KEY': '/foo', 'AWS_CONFIG_FILE': '/bar'}, {'/bar'}),
+        (
+            {
+                'AWS_DUMMY_KEY': '/foo', 'AWS_CONFIG_FILE': '/bar',
+                'AWS_CREDENTIAL_FILE': '/baz'
+            },
+            {'/bar', '/baz'}
+        ),
+        (
+            {
+                'AWS_CONFIG_FILE': '/foo', 'AWS_CREDENTIAL_FILE': '/bar',
+                'AWS_SHARED_CREDENTIALS_FILE': '/baz'
+            },
+            {'/foo', '/bar', '/baz'}
+        ),
+    ),
+)
+def test_get_aws_credentials_file_from_env(env_vars, values):
     """Test that reading credential files names from environment variables works."""
-    monkeypatch.delenv('AWS_CREDENTIAL_FILE', raising=False)
-    monkeypatch.delenv('AWS_SHARED_CREDENTIALS_FILE', raising=False)
-    monkeypatch.delenv('BOTO_CONFIG', raising=False)
-    assert get_aws_credential_files_from_env() == set()
-    monkeypatch.setenv('AWS_CREDENTIAL_FILE', '/foo')
-    assert get_aws_credential_files_from_env() == {'/foo'}
-    monkeypatch.setenv('AWS_SHARED_CREDENTIALS_FILE', '/bar')
-    assert get_aws_credential_files_from_env() == {'/foo', '/bar'}
-    monkeypatch.setenv('BOTO_CONFIG', '/baz')
-    assert get_aws_credential_files_from_env() == {'/foo', '/bar', '/baz'}
-    monkeypatch.setenv('AWS_CONFIG_FILE', '/xxx')
-    assert get_aws_credential_files_from_env() == {'/foo', '/bar', '/baz', '/xxx'}
-    monkeypatch.setenv('AWS_DUMMY_KEY', 'foobar')
-    assert get_aws_credential_files_from_env() == {'/foo', '/bar', '/baz', '/xxx'}
+    with patch.dict('os.environ', env_vars, clear=True):
+        assert get_aws_credential_files_from_env() == values
 
 
-def test_get_aws_secrets_from_env(monkeypatch):
+@pytest.mark.parametrize(
+    ('env_vars', 'values'),
+    (
+        ({}, set()),
+        ({'AWS_DUMMY_KEY': 'foo'}, set()),
+        ({'AWS_SECRET_ACCESS_KEY': 'foo'}, {'foo'}),
+        ({'AWS_SECURITY_TOKEN': 'foo'}, {'foo'}),
+        ({'AWS_SESSION_TOKEN': 'foo'}, {'foo'}),
+        ({'AWS_DUMMY_KEY': 'foo', 'AWS_SECRET_ACCESS_KEY': 'bar'}, {'bar'}),
+        (
+            {'AWS_SECRET_ACCESS_KEY': 'foo', 'AWS_SECURITY_TOKEN': 'bar'},
+            {'foo', 'bar'}
+        ),
+    ),
+)
+def test_get_aws_secrets_from_env(env_vars, values):
     """Test that reading secrets from environment variables works."""
-    monkeypatch.delenv('AWS_SECRET_ACCESS_KEY', raising=False)
-    monkeypatch.delenv('AWS_SESSION_TOKEN', raising=False)
-    assert get_aws_secrets_from_env() == set()
-    monkeypatch.setenv('AWS_SECRET_ACCESS_KEY', 'foo')
-    assert get_aws_secrets_from_env() == {'foo'}
-    monkeypatch.setenv('AWS_SESSION_TOKEN', 'bar')
-    assert get_aws_secrets_from_env() == {'foo', 'bar'}
-    monkeypatch.setenv('AWS_SECURITY_TOKEN', 'baz')
-    assert get_aws_secrets_from_env() == {'foo', 'bar', 'baz'}
-    monkeypatch.setenv('AWS_DUMMY_KEY', 'baz')
-    assert get_aws_secrets_from_env() == {'foo', 'bar', 'baz'}
+    with patch.dict('os.environ', env_vars, clear=True):
+        assert get_aws_secrets_from_env() == values
 
 
-@pytest.mark.parametrize(('filename', 'expected_keys'), (
-    ('aws_config_with_secret.ini', {
-        'z2rpgs5uit782eapz5l1z0y2lurtsyyk6hcfozlb'}),
-    ('aws_config_with_session_token.ini', {'foo'}),
-    ('aws_config_with_secret_and_session_token.ini',
-     {'z2rpgs5uit782eapz5l1z0y2lurtsyyk6hcfozlb', 'foo'}),
-    ('aws_config_with_multiple_sections.ini', {
-        '7xebzorgm5143ouge9gvepxb2z70bsb2rtrh099e',
-        'z2rpgs5uit782eapz5l1z0y2lurtsyyk6hcfozlb',
-        'ixswosj8gz3wuik405jl9k3vdajsnxfhnpui38ez',
-        'foo'}),
-    ('aws_config_without_secrets.ini', set()),
-    ('nonsense.txt', set()),
-    ('ok_json.json', set()),
-))
+@pytest.mark.parametrize(
+    ('filename', 'expected_keys'),
+    (
+        (
+            'aws_config_with_secret.ini',
+            {'z2rpgs5uit782eapz5l1z0y2lurtsyyk6hcfozlb'}
+        ),
+        ('aws_config_with_session_token.ini', {'foo'}),
+        ('aws_config_with_secret_and_session_token.ini',
+         {'z2rpgs5uit782eapz5l1z0y2lurtsyyk6hcfozlb', 'foo'}),
+        (
+            'aws_config_with_multiple_sections.ini',
+            {
+                '7xebzorgm5143ouge9gvepxb2z70bsb2rtrh099e',
+                'z2rpgs5uit782eapz5l1z0y2lurtsyyk6hcfozlb',
+                'ixswosj8gz3wuik405jl9k3vdajsnxfhnpui38ez',
+                'foo'
+            }
+        ),
+        ('aws_config_without_secrets.ini', set()),
+        ('nonsense.txt', set()),
+        ('ok_json.json', set()),
+    ),
+)
 def test_get_aws_secrets_from_file(filename, expected_keys):
     """Test that reading secrets from files works."""
     keys = get_aws_secrets_from_file(get_resource_path(filename))
     assert keys == expected_keys
 
 
-# Input filename, expected return value
-TESTS = (
-    ('aws_config_with_secret.ini', 1),
-    ('aws_config_with_session_token.ini', 1),
-    ('aws_config_with_multiple_sections.ini', 1),
-    ('aws_config_without_secrets.ini', 0),
-    ('nonsense.txt', 0),
-    ('ok_json.json', 0),
+@pytest.mark.parametrize(
+    ('filename', 'expected_retval'),
+    (
+        ('aws_config_with_secret.ini', 1),
+        ('aws_config_with_session_token.ini', 1),
+        ('aws_config_with_multiple_sections.ini', 1),
+        ('aws_config_without_secrets.ini', 0),
+        ('nonsense.txt', 0),
+        ('ok_json.json', 0),
+    ),
 )
-
-
-@pytest.mark.parametrize(('filename', 'expected_retval'), TESTS)
 def test_detect_aws_credentials(filename, expected_retval):
     """Test if getting configured AWS secrets from files to be checked in works."""
 
@@ -84,20 +113,20 @@ def test_detect_aws_credentials(filename, expected_retval):
     assert ret == expected_retval
 
 
-def test_non_existent_credentials(capsys, monkeypatch):
+@patch('pre_commit_hooks.detect_aws_credentials.get_aws_secrets_from_file')
+@patch('pre_commit_hooks.detect_aws_credentials.get_aws_secrets_from_env')
+def test_non_existent_credentials(mock_secrets_env, mock_secrets_file, capsys):
     """Test behavior with no configured AWS secrets."""
-    monkeypatch.setattr(
-        'pre_commit_hooks.detect_aws_credentials.get_aws_secrets_from_env',
-        lambda: set())
-    monkeypatch.setattr(
-        'pre_commit_hooks.detect_aws_credentials.get_aws_secrets_from_file',
-        lambda x: set())
+    mock_secrets_env.return_value = set()
+    mock_secrets_file.return_value = set()
     ret = main((
         get_resource_path('aws_config_without_secrets.ini'),
         "--credentials-file=testing/resources/credentailsfilethatdoesntexist"
     ))
     assert ret == 2
     out, _ = capsys.readouterr()
-    assert out == ('No AWS keys were found in the configured credential files '
-                   'and environment variables.\nPlease ensure you have the '
-                   'correct setting for --credentials-file\n')
+    assert out == (
+        'No AWS keys were found in the configured credential files '
+        'and environment variables.\nPlease ensure you have the '
+        'correct setting for --credentials-file\n'
+    )
