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

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

Merged
merged 3 commits into from
Jun 7, 2024

Conversation

aaronthebaron
Copy link
Contributor

@aaronthebaron aaronthebaron commented Jun 5, 2024

…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
Copy link
Contributor

edwardrf commented Jun 5, 2024

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
Copy link
Contributor Author

aaronthebaron commented Jun 5, 2024

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
Copy link

codecov-commenter commented Jun 5, 2024

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.

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 ⚠️

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

Additional details and impacted files
@@            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 Coverage Δ
unittests 42.19% <32.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mapkon mapkon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronthebaron is this ready for review?

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

@edwardrf
Copy link
Contributor

edwardrf commented Jun 6, 2024

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

…in page while maintaining gaialogin compatibility
@aaronthebaron
Copy link
Contributor Author

@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 aaronthebaron requested a review from mapkon June 6, 2024 15:39
@aaronthebaron
Copy link
Contributor Author

@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.

@mapkon mapkon merged commit c7eb46d into Versent:master Jun 7, 2024
8 checks passed
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.

4 participants