[IAMRISK-3010] Added support for auth0_v2 captcha failOpen #2507
Conversation
alexkoumarianos-okta
had a problem deploying
to
external
GitHub Actions
Failure
alexkoumarianos-okta
had a problem deploying
to
external
GitHub Actions
Failure
| attributes['error-callback'] = () => { | ||
| if (this.state.retryCount < MAX_RETRY) { | ||
| removeScript(scriptUrl); | ||
| loadScript(scriptUrl, attributes); | ||
| this.setState(prevState => ({ | ||
| retryCount: prevState.retryCount + 1 | ||
| })); | ||
| return; | ||
| } | ||
| removeScript(scriptUrl); | ||
| this.changeHandler('BYPASS_CAPTCHA'); | ||
| }; | ||
| window[callbackName] = () => { | ||
| delete window[callbackName]; | ||
| callback(); |
There was a problem hiding this comment.
Can we reuse the code we already have for this, as it seems to be identical apart from two things:
onerrorvserror-callback- arkose is not doing
delete window[callbackName]. Why is that different here?
There was a problem hiding this comment.
The naming comes directly from the APIs so we can't align them directly. I didn't work on the arkose changes but aside from skipping the delete they also send in the arkose script and I know they handle the callback a bit differently further up in the tree. I will look into it to get some more insight and get back to you!
There was a problem hiding this comment.
I didnt mean to align the names. I just mean that it may be possible to reuse the same code. Along the lines of:
if (provider === ARKOSE_PROVIDER || provider === AUTH0_V2_CAPTCHA_PROVIDER) {
const errorCallbackName = provider === ARKOSE_PROVIDER ? 'onerror' : 'error-callback';
attributes[errorCallbackName] = () => {
if (this.state.retryCount < MAX_RETRY) {
removeScript(scriptUrl);
loadScript(scriptUrl, attributes);
this.setState(prevState => ({
retryCount: prevState.retryCount + 1
}));
return;
}
removeScript(scriptUrl);
this.changeHandler('BYPASS_CAPTCHA');
};
window[callbackName] = callback;
}Anyway, not saying you have to make this change. But it felt like it was doing the exact same thing so maybe worth considering but can approve if you believe it makes no sense.
There was a problem hiding this comment.
The above doesnt do the delete window[callbackName], so maybe it needs to be added again. But anyway, I just wanted to show what I meant.
There was a problem hiding this comment.
See my other comment but we can definitely combine the Arkose & Auth0 V2 error handling here. The attributes here refers to the attributes of the script tag and we're trying to add error handling in the case the script fails to load to the DOM and this is not the same as error-callback param from the provider API which is after the script is loaded and when there are provider API related errors.
| callback(arkose); | ||
| }; | ||
| } else if (provider === AUTH0_V2_CAPTCHA_PROVIDER) { | ||
| attributes['error-callback'] = () => { |
There was a problem hiding this comment.
Same thing as auth0.js PR (https://github.com/auth0/auth0.js/pull/1382/files#r1433514962). It should be attached to the onerror attribute of the script tag.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2507 +/- ##
==========================================
+ Coverage 42.19% 42.35% +0.15%
==========================================
Files 120 120
Lines 3076 3081 +5
Branches 334 335 +1
==========================================
+ Hits 1298 1305 +7
+ Misses 1684 1682 -2
Partials 94 94 ☔ View full report in Codecov by Sentry. |
**Added** - [IAMRISK-2916] Added support for Auth0 v2 captcha provider [\#2503](#2503) ([alexkoumarianos-okta](https://github.com/alexkoumarianos-okta)) **Changed** - [IAMRISK-3010] Added support for auth0_v2 captcha failOpen [\#2507](#2507) ([alexkoumarianos-okta](https://github.com/alexkoumarianos-okta)) [IAMRISK-2916]: https://auth0team.atlassian.net/browse/IAMRISK-2916?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [IAMRISK-3010]: https://auth0team.atlassian.net/browse/IAMRISK-3010?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
alexkoumarianos-okta
had a problem deploying
to
external
GitHub Actions
Failure
alexkoumarianos-okta
had a problem deploying
to
external
GitHub Actions
Failure
Changes
Made changes to allow auth0_v2 to handle failOpen by passing a distinct dummy token.
References
https://auth0team.atlassian.net/browse/IAMRISK-3010
Testing