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(425): parse NextJS data for role aliases #426

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

joepurdy
Copy link
Contributor

@joepurdy joepurdy commented Aug 21, 2023

Description

This PR updates the parsing in gimme_aws_creds/aws.py to support the new NextJS SAML login page that AWS plans to release. The changes from this PR are backwards compatible by first asserting whether the new NextJS metadata is present in the HTML response for the sign-in page. In the event this metadata is present a new approach is used to parse the JSON data for the necessary alias names. If not, the old codepath is used for parsing this data from the HTML content of the legacy page.

Related Issue

Fixes #425 for new AWS SAML login page

Motivation and Context

While AWS did rollback their recent change to the SAML sign-in page, it's likely they will move forward with the new NextJS application at an, at this time, uncertain future date. Applying this fix proactively will ensure when this occurs that the aliases can be parsed from the new metadata.

How Has This Been Tested?

I've empirically tested this locally using a response I saved from the new SAML page while it was deployed earlier today. I plan to update tests/test_aws_resolver.py prior to merging this PR with a new test case using mock data in the new format (I still need to redact my local response of private data).

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document. (See note)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Note to maintainers:

The link to submit the Individual Contributor License Agreement in this project's CONTRIBUTING document is not working. I'd be happy to submit a CLA prior to this PR merging so long as a maintainer could point me in the right direction for that.

Additionally, as mentioned previously I don't love introducing new functionality without tests so I'm going to spend some time this afternoon mocking the new response HTML that I have locally to turn it into an additional test case prior to merging.

EDIT: I finished adding a new unit test and cleaning up the mock HTML data by creating a fixtures directory in a599592 and 531baa2 and confirmed all new and existing tests pass. This is ready for review/merge.

@joepurdy joepurdy marked this pull request as ready for review August 21, 2023 23:08
@joepurdy
Copy link
Contributor Author

@epierce Wanted to flag this for your review since you're the most active maintainer from Nike. Feel free to checkout the linked issue for more context on the upcoming AWS SAML sign-in page change. I've kept this backwards compatible to allow implementing the fix before AWS rolls out their new page again.

Appreciate your time on this, we're big fans/users of GAC over at @ArcadiaPower so we're happy to help keep it working great when AWS throws a curveball.

@epierce
Copy link
Member

epierce commented Aug 22, 2023

This looks good - it may have to be updated when AWS releases the new version of the role selection screen again, but this puts us in a good position to handle that. I have to go through Change Control here before cutting a new release, so it'll probably be early next week, but I'll get it out as soon as possible.

@epierce epierce merged commit 36c4187 into Nike-Inc:master Aug 22, 2023
7 checks passed
@joepurdy joepurdy deleted the fix/425/aws-saml-parsing branch August 22, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing broken on new AWS SAML login page if resolve_aws_alias true
2 participants