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

Adds support for Okta Verify risky login challenge #793

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

duckfez
Copy link
Contributor

@duckfez duckfez commented Mar 23, 2022

Should fix #643

@duckfez duckfez force-pushed the bugfix/dw_add_number_challeng branch from b88af47 to 3ce8b8e Compare March 23, 2022 12:26
@duckfez duckfez force-pushed the bugfix/dw_add_number_challeng branch from 3ce8b8e to 42abee5 Compare May 28, 2022 23:42
@duckfez
Copy link
Contributor Author

duckfez commented May 28, 2022

Rebased onto current master

@mapkon
Copy link
Member

mapkon commented Mar 26, 2023

@duckfez This is such an important change, and I am looking at merging it. Could you please add a test for this change?

@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2023

Codecov Report

Merging #793 (792eace) into master (f8af25e) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #793      +/-   ##
==========================================
- Coverage   36.29%   36.25%   -0.05%     
==========================================
  Files          53       53              
  Lines        7941     7950       +9     
==========================================
  Hits         2882     2882              
- Misses       4686     4695       +9     
  Partials      373      373              
Flag Coverage Δ
unittests 36.25% <0.00%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
pkg/provider/okta/okta.go 22.03% <0.00%> (-0.17%) ⬇️

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

@duckfez
Copy link
Contributor Author

duckfez commented Mar 27, 2023

I will take a look at adding tests. Thanks!

@mapkon
Copy link
Member

mapkon commented Mar 27, 2023

I will take a look at adding tests. Thanks!

Thank you! The aim is to keep the current coverage or at least increase. You can view the effect of your PR on the codecov report above.

@duckfez
Copy link
Contributor Author

duckfez commented Apr 3, 2023

I am struggling to figure out how to write tests for this. I've started by (very small) factoring of the verifyMfa function to put the Okta Push part into its own function. (Writing a test for all of verifyMfa was a lot). The primary issue I'm finding in the testing is that the real provider has to make 3+ round-trip calls to the Okta HTTP API, and I don't quite know how to mock that up. Advice / examples welcome

@mapkon
Copy link
Member

mapkon commented Apr 3, 2023

I am struggling to figure out how to write tests for this. I've started by (very small) factoring of the verifyMfa function to put the Okta Push part into its own function. (Writing a test for all of verifyMfa was a lot). The primary issue I'm finding in the testing is that the real provider has to make 3+ round-trip calls to the Okta HTTP API, and I don't quite know how to mock that up. Advice / examples welcome

Is it possible to mock the round trips? @gliptak has been using that technique in some of his recent commits

@gliptak
Copy link
Contributor

gliptak commented Apr 3, 2023

@duckfez please see #1015 for HTTP lifecycle testing

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.

Okta Verify with Push issue
6 participants