[IAMRISK-2916] Added support for Auth0 v2 captcha provider

[alexkoumarianos-okta]

Changes

Added support for Auth0 v2 (Cloudflare turnstile) captcha provider.

References

https://auth0team.atlassian.net/browse/IAMRISK-2916

Testing

• This change adds unit test coverage
• This change adds integration test coverage
• This change has been tested on the latest version of the platform/language

Checklist

• I have read the Auth0 general contribution guidelines
• I have read the Auth0 Code of Conduct
• All code quality tools/guidelines have been run/followed
• All relevant assets have been compiled

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (003339c) 41.74% compared to head (c140f86) 42.19%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2503      +/-   ##
==========================================
+ Coverage   41.74%   42.19%   +0.44%     
==========================================
  Files         120      120              
  Lines        3066     3076      +10     
  Branches      332      334       +2     
==========================================
+ Hits         1280     1298      +18     
+ Misses       1694     1684      -10     
- Partials       92       94       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[srijonsaha]

Can we move this as part of the React component? We shouldn't be passing around this outside the component when we can have it as a class method for the component.

[alexkoumarianos-okta]

Code coverage made me do it! I can pass the individual params instead of this but there was no good way (that I could find) to get coverage without pulling it out. Any suggestions?

[srijonsaha]

Hmmm I feel like we shouldn't really do this just to pass code coverage check. My suggestion would be to not create the getRenderParams function just for code coverage and see if we can bypass the code coverage requirement to merge this and see what SDKs has to say because most of the code here is untested anyway. If we do want to test for code coverage sake, I was looking this up and found this. I guess one thing you could do to test it is if there's a mock window object, then you can follow that to do what the stackoverflow answer does and that would call componentDidMount -> injectCaptchaScript -> attaches callback function that calls getRenderParams to window -> call callback from window function to unit test. But I like your current approach better. Only thing I would change is instead of individually passing the render params, can we pass an object that contains the render params so it's extensible for the future if we add new providers that have new params.

[alexkoumarianos-okta]

Let me know if this implementation works for you!

[frederikprijck]

I added a review comment around this specifically. I do not want to be blocking, but I believe this can be improved.

[frederikprijck]

because most of the code here is untested anyway

I think this shouldnt mean we can not try our best to improve the quality and add tests for new added functionality. Leaving that to you, as you own the functionality, but I would recommend ensuring things are tested.

[frederikprijck]

Thanks for this, this looks good for the most part. Left one comment about code written specifically to test and the fact that I believe we should not test implementation details.

[frederikprijck]

I don't think we should have this comment here.

Additionally, I am not sure I understand why we need this.getRenderParams in the first place, we shouldnt have to spy on it. Instead, we should verify what gets passed to provider.render as that's what counts. getRenderParams is an implementation details we shouldnt test, meaning that if we revert this to the original, tests should still succeed.

I am not saying we cant have this.getRenderParams, but I am saying we should ensure we do not need it in order for the tests to succeed. Whether you keep it in that case is up to you.

[frederikprijck]

Thanks for making all the changes here, appreciate it.

Currently, the code relies on this.getRenderParams to have been called on mount, and then just uses this.renderParams.

How do you feel about dropping this.renderParams, but instead call this.getRenderParams() where u need the params, and inline the current defaultRenderParams in the getRenderParams. It would also remove the need to do delete this.renderPrams, which I always try to avoid. Additionally we would also no longer be constantly mutating the same this.renderParams.

 getRenderParams() {
    if (this.props.provider === ARKOSE_PROVIDER) {
      return;
    }
    
    const defaultRenderParams = {
      sitekey: this.props.sitekey,
    };

    if (this.props.provider === FRIENDLY_CAPTCHA_PROVIDER) {
      return {
        ...this.defaultRenderParams,
        language: this.props.hl,
        doneCallback: this.changeHandler,
        errorCallback: this.erroredHandler
      };
    }

    if (this.props.provider === AUTH0_V2_CAPTCHA_PROVIDER) {
      return {
        ...this.defaultRenderParams,
        callback: this.changeHandler,
        'expired-callback': this.expiredHandler,
        'error-callback': this.erroredHandler
        language: this.props.hl,
        theme: 'light'
      };
    } else {
      return {
      ...this.defaultRenderParams,
      callback: this.changeHandler,
      'expired-callback': this.expiredHandler,
      'error-callback': this.erroredHandler
    };
    }
  }

[frederikprijck]

Can we address #2503 (comment) ?

[alexkoumarianos-okta]

@frederikprijck Can I get a merge as well whenever you get a chance?