search

Versent / saml2aws

CLI tool which enables you to login and retrieve AWS temporary credentials using a SAML IDP
https://github.com/Versent/saml2aws
MIT License
2.09k stars 563 forks source link

Update the googleapps IDP provider to work with changes to Google log… #1285

Closed aaronthebaron closed 5 months ago

aaronthebaron commented 5 months ago

…in page while maintaining gaialogin compatibility

Attempting to fix the issues with the GoogleApps IdP provider noted in the issue #1259

The google login system has changed for most but not all users causing this provider to no longer work for a majority of our in house users.

I have tested this with the new pages, in a configuration requiring 2-factor authentication with a phone app. I have not been able to regression test this with the current login, but I have tried to keep that legacy flow intact with my changes. I have also not been able to test each challenge but I have modified the challenges in a way that I hope to be backwards compatible.

edwardrf commented 5 months ago

Awesome PR, but it seems I am still unable to login with the following error:

Using IdP Account default to access GoogleApps https://accounts.google.com/o/saml2/initsso?idpid=C00mmafdv&spid=XXXXXXXXXXXX&forceauthn=false
Authenticating as XXX@XXXXXX.XXX ...
Error authenticating to IdP.: error loading challenge page: unable to extract skip form: could not find form with query "form[action$=\"skip\"]"
aaronthebaron commented 5 months ago

Awesome PR, but it seems I am still unable to login with the following error:

Using IdP Account default to access GoogleApps https://accounts.google.com/o/saml2/initsso?idpid=C00mmafdv&spid=XXXXXXXXXXXX&forceauthn=false
Authenticating as XXX@XXXXXX.XXX ...
Error authenticating to IdP.: error loading challenge page: unable to extract skip form: could not find form with query "form[action$=\"skip\"]"

Thanks for testing @edwardrf can you run the command with DUMP_CONTENT=true and --verbose flag and give me the results?

Specifically the results page of /challenge/sk

codecov-commenter commented 5 months ago

Codecov Report

Attention: Patch coverage is 32.00000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 42.19%. Comparing base (54e1e97) to head (6c82206). Report is 10 commits behind head on master.

:exclamation: Current head 6c82206 differs from pull request most recent head 99d6fe4

Please upload reports for the commit 99d6fe4 to get more accurate results.

Files Patch % Lines
pkg/provider/googleapps/googleapps.go 32.00% 15 Missing and 2 partials :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1285 +/- ## ========================================== + Coverage 42.08% 42.19% +0.11% ========================================== Files 54 54 Lines 6442 6456 +14 ========================================== + Hits 2711 2724 +13 + Misses 3288 3283 -5 - Partials 443 449 +6 ``` | [Flag](https://app.codecov.io/gh/Versent/saml2aws/pull/1285/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Versent) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/Versent/saml2aws/pull/1285/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Versent) | `42.19% <32.00%> (+0.11%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Versent#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

edwardrf commented 5 months ago

@aaronthebaron Please see the log in the attachment test.log

aaronthebaron commented 5 months ago

@mapkon

@aaronthebaron is this ready for review?

Also, lots of changes here - do you reckon we should cover some of the missing lines with tests?

Yes, this is ready for review.

Of all the lines I've added, most of them are under existing testing. I've added a test and modified another test to cover the two main changes.

There are so many paths possible in this provider that I don't have access to, but current this is passing regression.

More paths will be breaking that I won't be able to fix and should probably be handed in another pull reqest.

aaronthebaron commented 5 months ago

@edwardrf Thank you for providing the output. Unfortunately you are entering a part of the flow that handles multiple challenge possibilities (TOTP, U2F, etc) and that page has completely changed.

Originally it must've been a form with multiple actions but now looks to be divs with different annotations. I think a lot more work will need to happen to get this page working again and I'm not sure I can spend more time on it.

The code for that page can be found here.

A quick fix for you personally might be to remove some possible authentication methods. Sorry I can't do more at this time.