Major Codebase Refactor for Easier Maintenance #38

mide · 2018-01-08T14:44:15Z

Breaking Changes

Removed Python 2.6 Support. (Also see Remove Support for Python 2.6 #41 for this specific issue)
- pip doesn't support 2.6 since fall 2016 Drop support for Python 2.6? pypa/pip#3955
- pytestdoesn't support 2.6 since fall 2017 finally drop python2.6 support pytest-dev/pytest#2812
- setuptools doesn't support 2.6 since fall 2017 Drop support for Python 2.6 pypa/setuptools#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 Major Codebase Refactor for Easier Maintenance #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 Error in case of extra comma #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 · 2018-01-08T14:52:20Z

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

mide · 2018-01-08T14:53:32Z

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

stevemac007 · 2018-01-08T21:31:27Z

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 · 2018-01-08T21:39:23Z

+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 · 2018-01-09T14:17:25Z

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.

- Changes configuration object to allow for empty creation, and added "raise_if_invalid()" function to determine if the config is good. - Added the ability to read from ~/.aws/config cached values.

coveralls · 2018-01-10T13:25:04Z

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

coveralls · 2018-01-10T13:25:04Z

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

coveralls · 2018-01-10T13:25:04Z

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

coveralls · 2018-01-10T13:28:08Z

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

coveralls · 2018-01-10T13:28:08Z

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

coveralls · 2018-01-10T13:28:09Z

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

coveralls · 2018-01-10T13:28:09Z

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

mide · 2018-01-10T13:30:12Z

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

coveralls · 2018-01-10T17:53:48Z

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

coveralls · 2018-01-10T17:53:48Z

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

coveralls · 2018-01-10T18:23:37Z

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

coveralls · 2018-01-10T18:29:05Z

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

coveralls · 2018-01-10T18:29:05Z

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

coveralls · 2018-01-10T18:29:05Z

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

coveralls · 2018-01-10T18:29:05Z

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

mide · 2018-01-10T18:33:20Z

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

nonspecialist · 2018-01-11T21:51:34Z

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 · 2018-01-11T21:53:25Z

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

nonspecialist · 2018-01-15T22:02:51Z

I think my description is confusing, so I'll adjust it.

Missing any default value for the profile argument results in a NoneType being used to load, and re-load the profile, instead of using the profile "default"

mide · 2018-01-16T02:37:41Z

Hmmm, okay. That's an interesting case. I'll take a look at that in the morning @nonspecialist. (I'm in the US - New York time zone)

coveralls · 2018-01-16T16:45:48Z

Coverage increased (+8.8%) to 41.23% when pulling 6226626 on mide:code-cleanup into 50bcd11 on cevoaustralia:master.

mide · 2018-01-16T17:00:41Z

@nonspecialist I was able to reproduce the problem and I feel that 2a53e49 should solve your problem. That being said, I don't think we want the default to be None and instead be sts. So now when you don't specify a profile, it should use sts.

coveralls · 2018-01-16T17:01:39Z

Coverage increased (+8.7%) to 41.136% when pulling 6bcc759 on mide:code-cleanup into 50bcd11 on cevoaustralia:master.

coveralls · 2018-01-16T17:01:40Z

Coverage increased (+8.7%) to 41.136% when pulling 6bcc759 on mide:code-cleanup into 50bcd11 on cevoaustralia:master.

coveralls · 2018-01-16T17:01:40Z

Coverage increased (+8.7%) to 41.136% when pulling 6bcc759 on mide:code-cleanup into 50bcd11 on cevoaustralia:master.

coveralls · 2018-01-16T17:01:40Z

Coverage increased (+8.7%) to 41.136% when pulling 6bcc759 on mide:code-cleanup into 50bcd11 on cevoaustralia:master.

When no profile was specified, the code would take "None" as the default, instead of "sts", which caused issues as None is not a string. See #38 (comment)

coveralls · 2018-01-16T17:06:38Z

Coverage increased (+8.8%) to 41.23% when pulling 2a53e49 on mide:code-cleanup into 50bcd11 on cevoaustralia:master.

coveralls · 2018-01-16T17:06:38Z

Coverage increased (+8.8%) to 41.23% when pulling 2a53e49 on mide:code-cleanup into 50bcd11 on cevoaustralia:master.

coveralls · 2018-01-16T17:06:38Z

Coverage increased (+8.8%) to 41.23% when pulling 2a53e49 on mide:code-cleanup into 50bcd11 on cevoaustralia:master.

coveralls · 2018-01-16T17:06:38Z

Coverage increased (+8.8%) to 41.23% when pulling 2a53e49 on mide:code-cleanup into 50bcd11 on cevoaustralia:master.

SamBarker · 2018-01-16T21:12:00Z

@mide sorry for the slow reply.

Being back in front of the computer again I've just outlined our cross account usage a bit more on #42 (comment) hopefully that makes our current use case a bit clearer.

mide · 2018-01-16T21:18:40Z

@SamBarker - I think I'm a little lost, is that just for context, or do you expect changes from that comment?

SamBarker · 2018-01-16T21:19:38Z

Just for context! Not expecting anything else at all.

On Wed, 17 Jan 2018 at 10:18, Mark Ide ***@***.***> wrote: @SamBarker <https://github.com/sambarker> - I think I'm a little lost, is that just for context, or do you expect changes from that comment? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACs8qkgdhcqIP9nf6u8oXi8t3NJYm1oDks5tLRIxgaJpZM4RWd6w> .

-- [image: MahiFX] Sam Barker Senior Developer

…

------------------------------ MahiFX Level 3, 50 Victoria Street, Christchurch, 8013

------------------------------ Mobile: +64 (0)21 1917157 Skype: sammahifx [image: MFX Compass]

--

------------------------------ *IMPORTANT NOTICE*: MahiFX Limited (*MahiFX Ltd*) and MahiFX (UK) Limited (*MahiFX (UK)*) are operating subsidiaries within the MahiFX group of companies (collectively, the MahiFX Group). All references to “*MahiFX*” refer to the MahiFX Group. MahiFX Limited is registered in New Zealand (Company no. 2446590, NZBusNo 9429031595070) and Australia (Australian registered body number ARBN 152-535-085). MahiFX Limited is authorised and regulated under the Australian Securities and Investment Commission (AFSL number 414198) and the New Zealand Financial Markets Authority (FSPR number FSP197465). MahiFX (UK) Limited is registered in the United Kingdom, (registered company number 08107062). MahiFX (UK) Limited is authorised and regulated under the Financial Conduct Authority (reference number 751019). This email, its attachments and any rights attaching hereto are confidential and intended exclusively for the person to whom the email is addressed. If you are not the intended recipient, do not read, copy, disclose or use the contents in any way. Please notify the sender by return email and destroy the email and attachments immediately. MahiFX does not accept any liability for any changes made to this email or attachments after sending by MahiFX. You must scan this email and attachments for viruses. The opinions expressed are not necessarily those of MahiFX. MahiFX accepts no liability for any loss, damage or consequence, whether caused by our own negligence or not, resulting directly or indirectly from the use of this email and attachments. For more information about MahiFX Limited see mahifx.com.

mide · 2018-01-16T21:21:46Z

Great! Thank you!

nonspecialist · 2018-01-16T22:22:38Z

Excellent! Works for my bizarre-o ~/.aws/config with and without specifying a profile. I like the coalesce cleanup too.

👍

nonspecialist

Looks good to me. This should make it much easier to work on too.

mide · 2018-01-16T22:25:57Z

Thanks!

stevemac007 · 2018-01-17T10:26:07Z

Looks great @mide - I've confirmed that my existing config works well.
Lets get this merged and then we can look at updating the remaining open PR's.

cliveza · 2018-01-17T21:02:51Z

@stevemac007 Could we get a new python package please?

nonspecialist · 2018-01-17T21:10:36Z

@cliveza the travis build should publish a new package automatically, but TravisCI has had some problems over the past 24 hours. I'll check the build and kick it manually if need be

nonspecialist · 2018-01-17T21:42:47Z

@cliveza pypi and Docker tags have been updated

Major Codebase Refactor for Easier Maintenance

0535c37

PR Feedback #38

7f57028

- Changes configuration object to allow for empty creation, and added "raise_if_invalid()" function to determine if the config is good. - Added the ability to read from ~/.aws/config cached values.

Add more tests, only write credentials if Amazon object provided.

0008520

Adds more tests

0388b28

Python version differences.

55d91fd

This was referenced Jan 11, 2018

Resolve AWS account aliases (if possible) and print them to the users #39

Merged

Prompt the user to insert their U2F Device #36

Closed

Cleanup configuration logic; makes setting variables more clear.

6226626

BUGFIX: Fix issue when no profile specified.

2a53e49

When no profile was specified, the code would take "None" as the default, instead of "sts", which caused issues as None is not a string. See #38 (comment)

nonspecialist self-requested a review January 16, 2018 22:22

nonspecialist approved these changes Jan 16, 2018

View reviewed changes

stevemac007 approved these changes Jan 17, 2018

View reviewed changes

stevemac007 merged commit 6f3357a into cevoaustralia:master Jan 17, 2018

mide deleted the code-cleanup branch January 17, 2018 15:06

cliveza mentioned this pull request Jan 17, 2018

Updated deprecated python 2 StandardError exception #43

Closed

Major Codebase Refactor for Easier Maintenance #38

Major Codebase Refactor for Easier Maintenance #38

Conversation

mide commented Jan 8, 2018 • edited Loading

Breaking Changes

Under-the-hood Changes

Added the following tests

Needed Work

Note

coveralls commented Jan 8, 2018 • edited Loading

mide commented Jan 8, 2018

stevemac007 commented Jan 8, 2018

SamBarker commented Jan 8, 2018

mide commented Jan 9, 2018

coveralls commented Jan 10, 2018 • edited Loading

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

mide commented Jan 10, 2018

coveralls commented Jan 10, 2018 • edited Loading

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

mide commented Jan 10, 2018

nonspecialist commented Jan 11, 2018

mide commented Jan 11, 2018

nonspecialist commented Jan 15, 2018

mide commented Jan 16, 2018

coveralls commented Jan 16, 2018 • edited Loading

mide commented Jan 16, 2018 • edited Loading

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

SamBarker commented Jan 16, 2018

mide commented Jan 16, 2018

SamBarker commented Jan 16, 2018 via email

mide commented Jan 16, 2018

nonspecialist commented Jan 16, 2018

nonspecialist left a comment

Choose a reason for hiding this comment

mide commented Jan 16, 2018

stevemac007 commented Jan 17, 2018

cliveza commented Jan 17, 2018

nonspecialist commented Jan 17, 2018

nonspecialist commented Jan 17, 2018

mide commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 10, 2018 •

edited

Loading

coveralls commented Jan 10, 2018 •

edited

Loading

coveralls commented Jan 16, 2018 •

edited

Loading

mide commented Jan 16, 2018 •

edited

Loading