-
Notifications
You must be signed in to change notification settings - Fork 320
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
Okta 719439 https loopback #3650
Conversation
51b3ca3
to
1e02094
Compare
.requestHooks(loopbackSuccessLogger, loopbackSuccessWithHttpsMock)('in loopback server approach, https loopback succeeds', async t => { | ||
failureCount = 0; | ||
// after OKTA-715718 is fixed, should use ".eql(0)" for ".lte(otherProbeCount)" assertions | ||
const otherProbeCount = process.env.OKTA_SIW_GEN3 === 'true' ? 0 : 1; |
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.
bacon test flakiness
@denysoblohin-okta has a fix in this PR
According to my local test, it is indeed flaky, especially when I add break points
SIW V2, should be only two /probe
call like V3
but actually, four /probe
calls
SIW V3, it is using simple async/await, so only two /probe
calls, which is expected
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.
any specific reasons we see different number of probe calls in V2 and V3? I'm fine with asserting different number of calls but would like to understand the root cause
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 believe the reason is in this PR's description. Mainly because Promisified $.ajax in BaseOktaVerifyChallengeView were not chained correctly
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 get some review from SIW team as well, especially on V3 code and tests
.requestHooks(loopbackSuccessLogger, loopbackSuccessWithHttpsMock)('in loopback server approach, https loopback succeeds', async t => { | ||
failureCount = 0; | ||
// after OKTA-715718 is fixed, should use ".eql(0)" for ".lte(otherProbeCount)" assertions | ||
const otherProbeCount = process.env.OKTA_SIW_GEN3 === 'true' ? 0 : 1; |
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.
any specific reasons we see different number of probe calls in V2 and V3? I'm fine with asserting different number of calls but would like to understand the root cause
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.
lgtm!
4e776de
to
26ee3ec
Compare
// if https domain are included, max number of ports to be probed should be doubled | ||
Logger.info('httpsDomain enabled, will probe and challenge https first'); | ||
maxNumberOfPorts += maxNumberOfPorts; | ||
ports.forEach(port => { |
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.
move this probing logic into a function? looks similar with the one used for regular domain.
@@ -124,11 +125,19 @@ const LoopbackProbe: FunctionComponent<{ uischema: LoopbackProbeElement }> = ({ | |||
useEffect(() => { | |||
const doLoopback = async () => { | |||
let foundPort = false; | |||
// loop over each port | |||
|
|||
let urls = ports.map((port) => `${domain}:${port}`); |
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.
nit: urls -> baseUrls
1119376
OKTA-719439 OKTA-719439 Support HTTPS loopback server call support v3 https loopback Fix review comments fix review comments
Description:
Add https loopback support to SIW v2 and v3
PR Checklist
Issue:
Reviewers:
Screenshot/Video:
Downstream Monolith Build: