Major Codebase Refactor for Easier Maintenance

mide commented 6 years ago

Breaking Changes

Removed Python 2.6 Support. (Also see #41 for this specific issue)
- pip doesn't support 2.6 since fall 2016 https://github.com/pypa/pip/issues/3955
- pytestdoesn't support 2.6 since fall 2017 https://github.com/pytest-dev/pytest/issues/2812
- setuptools doesn't support 2.6 since fall 2017 https://github.com/pypa/setuptools/issues/878
- Many other tools look like they're moving in this direction (and eventually moving to remove all 2.* support, but I'm not going that far yet.)
~~Remove persistent (to file) profiles.~~
- Previously, aws-google-auth would write to a user's ~/.aws/config file and read the values when run the next time. I feel this adds a complexity in trying to configure the tool to determine defaults. aws-google-auth supports command line params, user input, environment variables and adding another may be too much to maintain. Of course, we can add this back if desired. (Edit: Per feedback in #38, I will add this back in)

Under-the-hood Changes

Broke apart __init__.py into the following files (along with their purproses):
- google.py is for all Google related functions. It includes the logic to perform the page scraping and SAML fetching.
- amazon.py for AWS related fucntions. This performs the role extraction from the SAML and performs the AWS API call to get the access tokens.
- configuration.py - This only maintains user options (anything the user specifies). Breaking this into it's own object allows us to perform much more robust testing and just pass around a single object instead of a handful of options.
- util.py for common tooling

Added the following tests

Flake8 Python style testing. This is added into the TravisCI build script (.travis.yml).
test_configuration.py:
- Test that invalid duration values get rejected.
- Test that valid duration values are accepted.
- Test that the default value of duration is max_duration
- Test that invalid ask_role values get rejected.
- Test that valid ask_role values are accepted.
- Test that ask_role is an optional setting.
- Test that invalid idp_id values get rejected.
- Test that valid idp_id values are accepted.
- Test that invalid sp_id values get rejected.
- Test that valid username values are accepted.
- Test that invalid username values get rejected.
- Test that valid sp_id values are accepted.
- Test that the default value of profile is sts
- Test that invalid profile values get rejected.
- Test that valid profile values are accepted.
- Test all profile_defaults.
- Test that invalid region values get rejected.
- Test that valid region values are accepted.
- Test that the default value of region is ap_southeast_2
- Test that invalid role_arn values get rejected.
- Test that role_arn_is is an optional setting.
- Test that valid role_arn values are accepted.
- Test that invalid u2f_disabled values get rejected.
- Test that valid u2f_disabled values are accepted.
- Test that u2f_disabled_is is an optional setting.
test_amazon.py
- Test that the sts boto client properly returns an STS client object.
- Test that role extraction happens correctly given a valid SAML response.
- Test that role extraction happens correctly given a SAML response with too many commas (See #12)

Needed Work

In order to get tests to pass, I had to ignore Flake8 rule E722. We should go back and determine what the correct exceptions are to catch and only catch those.

Note

Please don't feel you need to accept this, but do please let me know. If this breaks Cevo's workflows, I may just end up maintaining my own fork.

coveralls commented 6 years ago

Coverage increased (+4.8%) to 37.26% when pulling 0535c3748ea1089df21ace887edbb46b6286f80f on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

mide commented 6 years ago

Wow, only 4.8% increase? That's disappointing.

stevemac007 commented 6 years ago

I can't see any reason to not accept this work - looks really good, and setup much better to improve the quality of the testing. Only 4.8% is a good start, and will hopefully allow future enhancements to only improve on that value.

I personally prefer storing the details in the config file, makes re-auth easier if you've done it from the input prompts, especially seeing as locating the IDP and SP value in the first place is tricky.

What effort is there for adding that back in?

SamBarker commented 6 years ago

+1 for maintaining AWS/config support. We have started using google auth with a read only profile and elevating to a rw role as needed, ala sudo. We use AWS-google-auth for the read only login, naming the profile, we then have additional profiles in the config file which are based off the ro one ( we use this between organisations as well)

mide commented 6 years ago

Sounds good @stevemac007 and @SamBarker - I will add the ~/.aws/config persistence back in. It shouldn't be too much work.

I do want to clarify though (for @SamBarker) the feature that I did remove (and will put back) is not writing the STS (aws_access_key_id and aws_secret_access_key) tokens to ~/.aws/credentials. It's writing various user settings (like email and role_arn) to ~/.aws/config.

👍 Thanks for the quick feedback folks. I'll get right on that change.

coveralls commented 6 years ago

Coverage decreased (-5.3%) to 27.098% when pulling 31a4932264443b40fab995eae8e382aa063b3908 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage decreased (-5.3%) to 27.098% when pulling 31a4932264443b40fab995eae8e382aa063b3908 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage decreased (-5.3%) to 27.098% when pulling 31a4932264443b40fab995eae8e382aa063b3908 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage decreased (-5.3%) to 27.098% when pulling 7f570282418689803d6a455a1eda584bf12aa803 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage decreased (-5.3%) to 27.098% when pulling 7f570282418689803d6a455a1eda584bf12aa803 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage decreased (-5.3%) to 27.098% when pulling 7f570282418689803d6a455a1eda584bf12aa803 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage decreased (-5.3%) to 27.098% when pulling 7f570282418689803d6a455a1eda584bf12aa803 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

mide commented 6 years ago

Implemented feedback; coverage is looking sad, which is a reminder to write tests for the new persistence logic.

coveralls commented 6 years ago

Coverage increased (+5.6%) to 38.038% when pulling 0008520dc8b8992b84038cd7fc4fb7fea14ae384 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+5.6%) to 38.038% when pulling 0008520dc8b8992b84038cd7fc4fb7fea14ae384 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.1%) to 38.534% when pulling 0388b284ab583800c8c254a899f7db4d09f35b8b on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.6%) to 39.007% when pulling 55d91fdfbeb047c8afcee31e013b74baa97d92ef on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.6%) to 39.007% when pulling 55d91fdfbeb047c8afcee31e013b74baa97d92ef on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.6%) to 39.007% when pulling 55d91fdfbeb047c8afcee31e013b74baa97d92ef on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.6%) to 39.007% when pulling 55d91fdfbeb047c8afcee31e013b74baa97d92ef on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

mide commented 6 years ago

Okay, this should be ready for another set of feedback. Please let me know your thoughts.

nonspecialist commented 6 years ago

Dropping 2.6 is fine so long as we can make sure that people running 2.6 get a Helpful Message which lets them know why the tool stopped working, and what to do about it.

Otherwise, this PR looks shmick (which is a Good Thing)

mide commented 6 years ago

Okay, good feedback @nonspecialist - I will have it display a note if the user is on Python 2.6.

nonspecialist commented 6 years ago

Once we merge this, I'd like to get #35 refactored so we can merge it as well

stevemac007 commented 6 years ago

Test of this branch doesn't seem to read the existing ~/.aws/config settings.

I have this in my config:

[profile cevo-demo]
region = ap-southeast-2
output = json
google_config.role_arn = arn:aws:iam::1234:role/Cevo-Demo
google_config.provider = arn:aws:iam::1234:saml-provider/GoogleApps
google_config.google_idp_id = <IDP>
google_config.google_sp_id = <SPID>
google_config.google_username = steve.mactaggart@email
google_config.duration = 3600

But when I run I'm still prompted for my details.

$ aws-google-auth -p cevo-demo
Failed to import U2F libraries, U2F login unavailable. Other methods can still continue.
Google username:

I'm reading now why this has changed, but seems to have broken backwards compatibility.

mide commented 6 years ago

@stevemac007 That's likely because I changed the namespace from google_config to aws_google_auth. But you're right, that did break backwards compatibility.

I can revert it back to google_config all together OR have the application read from aws_google_auth.* if its there, and if not, try google_config. thoughts?

nonspecialist commented 6 years ago

hmmm, running in a python2.7 virtualenv and specifying a profile gets me:

(aws-google-auth) cmp@tak.local aws-google-auth $ bin/aws-google-auth -p cevo-dev
Failed to import U2F libraries, U2F login unavailable. Other methods can still continue.
Traceback (most recent call last):
  File "bin/aws-google-auth", line 11, in <module>
    load_entry_point('aws-google-auth==0.0.16', 'console_scripts', 'aws-google-auth')()
  File "/home/cmp/Build/Cevo/aws-google-auth/lib/python2.7/site-packages/aws_google_auth-0.0.16-py2.7.egg/aws_google_auth/__init__.py", line 41, in main
    cli(sys.argv[1:])
  File "/home/cmp/Build/Cevo/aws-google-auth/lib/python2.7/site-packages/aws_google_auth-0.0.16-py2.7.egg/aws_google_auth/__init__.py", line 53, in cli
    config.read(args.profile)
  File "/home/cmp/Build/Cevo/aws-google-auth/lib/python2.7/site-packages/aws_google_auth-0.0.16-py2.7.egg/aws_google_auth/configuration.py", line 139, in read
    self.idp_id = util.Util.unicode_to_string_if_needed(util.Util.default_if_none(config_parser[profile].get('aws_google_auth_idp_id', None), self.idp_id))
  File "/home/cmp/Build/Cevo/aws-google-auth/lib/python2.7/site-packages/aws_google_auth-0.0.16-py2.7.egg/aws_google_auth/util.py", line 54, in unicode_to_string_if_needed
    return string.encode('utf-8')
AttributeError: 'NoneType' object has no attribute 'encode'

where the ~/.aws/config contains:

[cevo-dev]
region = ap-southeast-2
output = json
google_config.role_arn = arn:aws:iam::123456789012:role/Cevo-Dev-Administrator
google_config.provider = arn:aws:iam::123456789012:saml-provider/GoogleApps
google_config.google_idp_id = [elided]
google_config.google_sp_id = [elided]
google_config.google_username = colin.panisset@cevo.com.au
google_config.duration = 3600

mide commented 6 years ago

That's interesting; I'll take a look at that.

nonspecialist commented 6 years ago

It looks to be because python2.7 doesn't have string imported by default

stevemac007 commented 6 years ago

Happy for it to up convert the config on the fly to the new namespace, but also wary of carrying lots of additional code for the purpose.

Other than it aligns with the tool name, is there great benefit of changing the namespace?

mide commented 6 years ago

@stevemac007 None - I just figured if a user looked at their ~/.aws/config file it'd be more obvious what put those values there and what uses them. But it's not important, I can cut it back.

mide commented 6 years ago

My open tasks:

[x] Make message for Python 2.6 users, describing why we did away with 2.6 support. (7ab39a3f2ef5e748d5cbfd686af4f1b6fc1aab93, 6f3ed9d645f0230bd2aa87d5971b49c351fcdbdc)
[x] Cut back to google_config namespace in ~/.aws/config. (e1a05b23be8f035beac217f7865eed5abab593fd)
[x] Look into Python 2.7 errors with .encode() and add new tests to catch this. (5dba49925239dadce985a1a1a0b3dfb54c2dfa89)

coveralls commented 6 years ago

Coverage increased (+6.6%) to 39.007% when pulling e1a05b23be8f035beac217f7865eed5abab593fd on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.4%) to 38.863% when pulling 5dba49925239dadce985a1a1a0b3dfb54c2dfa89 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.4%) to 38.863% when pulling 5dba49925239dadce985a1a1a0b3dfb54c2dfa89 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+5.9%) to 38.283% when pulling 7ab39a3f2ef5e748d5cbfd686af4f1b6fc1aab93 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+5.9%) to 38.283% when pulling 7ab39a3f2ef5e748d5cbfd686af4f1b6fc1aab93 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+5.9%) to 38.283% when pulling 7ab39a3f2ef5e748d5cbfd686af4f1b6fc1aab93 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+5.9%) to 38.283% when pulling 7ab39a3f2ef5e748d5cbfd686af4f1b6fc1aab93 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+5.9%) to 38.283% when pulling 6f3ed9d645f0230bd2aa87d5971b49c351fcdbdc on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+5.9%) to 38.283% when pulling 6f3ed9d645f0230bd2aa87d5971b49c351fcdbdc on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+5.9%) to 38.283% when pulling 6f3ed9d645f0230bd2aa87d5971b49c351fcdbdc on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

mide commented 6 years ago

Alright, implemented feedback. I think we're good for another round.

I opened https://github.com/cevoaustralia/aws-google-auth/issues/41 so that people that see the message regarding Python 2.6 are given a quick description - so they don't need to read this whole PR.

mide commented 6 years ago

Oh yikes. That’s shameful of me.

I’ll fix the backwards compatibility issues and then add tests to make sure they stay static.

Sorry for the extra churn.

coveralls commented 6 years ago

Coverage increased (+6.6%) to 38.991% when pulling 00c97787b55c0401a450a2f3a1207f720c5d093b on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.6%) to 38.991% when pulling be0f04e94a1ef33ac65892458d78d21148cc5fd1 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.6%) to 38.991% when pulling be0f04e94a1ef33ac65892458d78d21148cc5fd1 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.6%) to 38.991% when pulling be0f04e94a1ef33ac65892458d78d21148cc5fd1 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

coveralls commented 6 years ago

Coverage increased (+6.6%) to 38.991% when pulling be0f04e94a1ef33ac65892458d78d21148cc5fd1 on mide:code-cleanup into 50bcd11da1ddf11f6cb7788d09e6ab10a00a1e70 on cevoaustralia:master.

mide commented 6 years ago

Okay:

I've addressed the backwards compatibility concerns in be0f04e94a1ef33ac65892458d78d21148cc5fd1, and added tests to ensure things don't change unexpectedly.
I fixed the whole named profile issue, and added tests in 00c97787b55c0401a450a2f3a1207f720c5d093b.

My apologies for the extra churn here, I think we're getting closer.

cevoaustralia / aws-google-auth