-
Notifications
You must be signed in to change notification settings - Fork 562
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
Yet Another Trusted Device PR #1039
Conversation
my employer doesn't have this one, so someone else will need to check it still works
@@ -1236,3 +1381,214 @@ func fidoWebAuthn(oc *Client, oktaOrgHost string, challengeContext *mfaChallenge | |||
|
|||
return gjson.GetBytes(body, "sessionToken").String(), nil | |||
} | |||
|
|||
func verifyTrustedCert(oc *Client, doc *goquery.Document, duoHost string, duoSubmitURL string, q url.Values) (*goquery.Document, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same as the existing PR, just broken out into helper functions to make testing easier
q.Add("parent", fmt.Sprintf("https://%s/signin/verify/duo/web", host)) | ||
q.Add("v", "2.8") | ||
|
||
fakeForm := fmt.Sprintf(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
originally this was a constant, but it needs the http server address and it's only available at runtime
|
||
// anonymised from actual endpoint | ||
const fakeEndpointForm = ` | ||
<form method="post" id="endpoint-health-form"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this out of dev tools and changed all the PII, so it's reasonably accurate
parent, _ := doc.Find("input[name=\"parent\"]").Attr("value") | ||
duoAppUrl, _ := doc.Find("input[name=\"duo_app_url\"]").Attr("value") | ||
ehDownloadLink, _ := doc.Find("input[name=\"eh_download_link\"]").Attr("value") | ||
// isSilentCollection, _ := doc.Find("input[name=\"is_silent_collection\"]").Attr("value") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wasn't present in my debug session and seems to not be necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove it it? Commented out code is dead code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests to this. Thanks
there's tests for the two helper functions introduced. Did you mean for the U2F flow as well? |
@mapkon please let me know if there's any other tests you'd like |
Codecov Report
📣 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 #1039 +/- ##
==========================================
+ Coverage 36.40% 37.20% +0.80%
==========================================
Files 52 53 +1
Lines 7546 7926 +380
==========================================
+ Hits 2747 2949 +202
- Misses 4415 4580 +165
- Partials 384 397 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@gliptak Got it. Please ignore my previous comment |
Basically just rebased #1027 and fixed the tests