saml2aws Fix infinite loop with Okta push

This PR fixes Okta infinite loop with MFA push, raised in issue #714.

I have tested this on my machine, and have been using it for several months now.

Please note that I'm a complete Go newbie, so any suggestions and improvements are welcome.

Feb 11 '22 22:02 resnikb

@wolfeidau Any chance this might make it into a release? The infinite loop in the current release is a blocker for us

May 07 '22 12:05 resnikb

@wolfeidau Bump for review, please?

Jul 24 '22 06:07 resnikb

This PR fixes okta+duo login issues for me too.

Nov 29 '22 07:11 jck

@resnikb could you merge/rebase this branch with master for CI to get triggered again?

Dec 18 '22 16:12 jck

@jck No worries, done

Dec 18 '22 21:12 resnikb

@wolfeidau We've been using this fix at my workplace for a few months and can confirm that it fixes okta infinite loop issues we were experiencing. Any chance you could review?

Jan 11 '23 12:01 jck

@wolfeidau @mapkon any chance we could get this merged in?

Mar 13 '23 12:03 jck

@wolfeidau @mapkon any chance we could get this merged in?

I indeed can. Any chance you can give me some unit tests?

Mar 13 '23 21:03 mapkon

Codecov Report

Merging #773 (6944b39) into master (4903e2f) will increase coverage by 0.47%. The diff coverage is 92.30%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #773      +/-   ##
==========================================
+ Coverage   31.86%   32.33%   +0.47%     
==========================================
  Files          52       52              
  Lines        7492     7502      +10     
==========================================
+ Hits         2387     2426      +39     
+ Misses       4787     4753      -34     
- Partials      318      323       +5

Flag	Coverage Δ
unittests	`32.33% <92.30%> (+0.47%)`	:arrow_up:

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

Impacted Files	Coverage Δ
pkg/provider/okta/okta.go	`11.63% <92.30%> (+4.19%)`	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Mar 21 '23 23:03 codecov-commenter

@jck @resnikb

I was going to merge this until I noticed that it would bump the code coverage down. Please consider adding a test.

Thanks

Mar 21 '23 23:03 mapkon

Thanks, @mapkon - I'll try to get something together over the weekend.

Mar 22 '23 01:03 resnikb

@resnikb There are conflicts with master :-( - Let's fix those and roll the tests (as suggested). I want to merge this

Mar 24 '23 22:03 mapkon

@mapkon Thanks - I've added a test for the rememberDevice functionality, but I was unable to create one that works for preserving cookie values. Any suggestions are appreciated

Mar 24 '23 23:03 resnikb

@briantist / @gliptak - thoughts?

Mar 24 '23 23:03 mapkon

consider merging #1004 making this corner testable

after this PR can be rebased and particular signature updated to

func (oc *Client) getStateToken(req *http.Request, loginDetails *creds.LoginDetails) (string, error) {

and test updated to assert cookie persistence

Mar 25 '23 15:03 gliptak

Thanks, @mapkon and @gliptak, I've made the requested changes, please take a look.

Mar 26 '23 09:03 resnikb

@resnikb thank you for the updates :+1:

Mar 26 '23 14:03 gliptak