Add support for alternative captchas

[MarkPugner79]

…il.com

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[MarkPugner79]

I was looking through the test results and fixed the sample config check, but I think the other one is a build system error.

[clokep]

Generally the code seems pretty reasonable, is there an MSC for using hcaptcha?

[ptman]

There's MSC2745: matrix-org/matrix-spec-proposals#2745

[aaronraimist]

@clokep what changes are you looking for?

[tulir]

It probably requires a proper MSC to be made first and then some vendor-prefixing added here until the MSC lands.

[MarkPugner79]

@clokep @tulir have any examples of vendor prefixing that have been done previously or what you are looking for?

[tulir]

I think the only place here where it's needed is the login type: it can't use the m. before it's in the spec, so it should be something like org.matrix.login.hcaptcha (or using your own domain, or maybe a MSC number like org.matrix.msc2745.login.hcaptcha)

[MarkPugner79]

@tulir Gotcha, I'll update it to reflect that, thanks for the feed back! ( I'll double check that's the only spot too.)

[MarkPugner79]

@tulir @clokep The only other thing I noticed is that there is a twister test for the current recaptcha implementation. Should I update that test to add testing support for hcaptcha? Auth Test Implementation Where the test is actually used Line 34

I'm still looking through the current test implementation for what it does exactly.

[clokep]

Oops, sorry about that! I think I failed to comment after moving around some labels.

First off, thanks for putting this together, overall it looks fairly reasonable!

Some questions/thoughts in my mind:

• MSC2745 seems relatively stagnant.
  • There's a lot of discussion on matrix-org/matrix-doc#1281 about what the "right" service is to use, although it really seems like this should be made more abstract so that a homeserver implementation can use whatever service they want.
  • I think we'd want to MSC to be a bit more fleshed out before merging this.
• It seems like the vast majority of this is copied and pasted from the reCAPTCHA implementation. Is there an opportunity to share any code?

Thanks for making the changes to use an unstable endpoints / identifiers! I'm not really sure what to do about the fallback endpoint here. It seems hard implementation wise to only have this show up as unstable, so probably ignore that for the moment.

[MarkPugner79]

@clokep No problem, I was hoping that this would skate by without a more abstraction implementation. I was thinking about making it more of a template style of implementation, but that's only really good for captcha's that are implemented in almost the same exact way as recaptcha.

The ideal thing would be to template out all of the special call back names as an additional config param and make it a one size fits all solution? (since that is the only thing that seems to be hard coded in the template for recaptcha and related tests).

I'm going to take a look at trying to template that out and maybe do a nice clean up of the config.

I'll get back to you in a bit and update my production environment to test it out and see if it works.

Looks like the script, callback data/class name need to be made into a template in synapse/res/templates/recaptcha.html and the other spot that needs made into a template would be in the testing scripts, tests/rest/client/v2_alpha/test_auth.py and a few other spots.

And synapse/rest/client/v2_alpha/auth.py for response = parse_string(request, "g-recaptcha-response")

I'll follow up with some updates if everything seems to work.

[clokep]

@MarkPugner79 that sounds about like what I was thinking! 👍 If it turns out to be extremely difficult, please do shout, but I think it is mostly making a few more things configurable!

[MarkPugner79]

Brief update:

Tracked down that I need to be able to template the main reg page and found it synapse/static/client/register/index.html I need to find where that is hooked in the api and add the new stuff that I've been working on to that.

The short list of stuff I was looking at adding are:
recaptcha_callback_class_target Example: "g-recaptcha" or "h-captcha"
recaptcha_response_template Example: g-recaptcha-response or h-captcha-response
recaptcha_template_script Example: https://www.recaptcha.net/recaptcha/api.js or https://hcaptcha.com/1/api.js

Those need to be peppered into a few of the registration flows and a few other spots that I've tracked down.

If you have some pointers or feedback let me know. Thanks!

... synapse/static/client/register/index.html is served from the static content folder :(
and has all the recaptcha stuff hardcoded into it. :(

[clokep]

Tracked down that I need to be able to template the main reg page and found it synapse/static/client/register/index.html I need to find where that is hooked in the api and add the new stuff that I've been working on to that.

This isn't the main registration page, it is an unspecced fallback page. Looking at it...it seems to assume an awful lot about the registration flow. I'm not sure we would require changes to that to accept this. See #7676 for more info (which says this page is straight-up broken so 🤷 ).

[MarkPugner79]

I'm still trying to track down how https://riot.yourdomain/#/register is generated. (more about this at the end, found it)

Well this seems to be what the current implementation is: #6865

What looks relevant in Element line 29
script-src 'self' 'unsafe-eval' https://www.recaptcha.net https://www.gstatic.com;

Also this seems to be the most relevant issue trying to track down everything that needs modified

The recaptcha part of the spec covers very little

To make this work as expected we would/should add a altcaptcha config with params as I've outlined(similar to my current hcaptcha commits), since it isn't ReCaptcha, it uses the fallback api's and works for authentication by opening a new tab/window and integrates cleanly with the auth flows.

I'm probably going to close this PR after further investigation into what it would take to retool the current recaptcha implementation it's too tightly integrated into element to abstract out and use other captcha's at this point. It seems like adding another auth/registration flow similar to my hcaptcha work flow with some of the abstractions will be the more correct answer of how to implement generic captcha support.

ReCaptcha is baked into https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/components/views/auth/CaptchaForm.js and several other parts for those in the future that want to work on this problem.

[clokep]

ReCaptcha is baked into https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/components/views/auth/CaptchaForm.js and several other parts for those in the future that want to work on this problem.

Right, so this brings us back around to the conversation in matrix-org/matrix-doc#1281. It might be reasonable to simply add hcaptcha support to matrix-js-sdk also...but it might make sense to get a bit more decision on the MSC first.

[MarkPugner79]

@clokep I need to look at how much can be passed from matrix to the ui/client, might be able to make a generic captcha interface for cleaner integration of other captchas.
I haven't shelved this entirely but might open another PR for a more generic solution to add alternative captchas even if they are via the fall back style of auth for registration.

[clokep]

might be able to make a generic captcha interface for cleaner integration of other captchas.

This would be my hope!

I believe the fallback page is already implemented in this PR, so hopefully that Just Works without client support, but 🤷

[MarkPugner79]

@clokep the fall back does work from testing it in production 👍

[MarkPugner79]

I might shoe horn hcaptcha as a suggested alternative captcha and keep working on this PR after doing some more testing with some other captcha systems. Will update after more testing.

[MarkPugner79]

Good news, added a bunch of new stuff that I've been testing such as:

altcaptcha_public_key
altcaptcha_private_key
altcaptcha_siteverify_api
enable_registration_altcaptcha

New stuff to support most recaptcha/hcaptcha like api's:

altcaptcha_callback_class_target (The class used by the captcha to bind/display in the page)
altcaptcha_template_script (This is the main script used to run the captcha)
altcaptcha_template_script2 (Some captcha's have two scripts, one being a wasm module and one being the main logic script)
altcaptcha_response_template ( response name used to handle the data passed back to matrix to validate the captcha)
altcaptcha_siteverify_api_response (some api's use a special response param for validation instead of "response" like "solution")

What I have tested it with:
Friendly Captcha Also works fine using the more advanced config options.

hCaptcha Works fine

MtCaptcha DOES NOT WORK They use a GET request api, which means splitting out and doing a one off param for their api.

Aside from adding some type of lambda functions for everything in the config that's about as good as I think it's going to get.

I need to clean up a bunch of stuff and I'll commit the changes for comment.

[clokep]

@MarkPugner79 Just shout when you think this is ready for someone to take a look at.

[MarkPugner79]

@clokep I think it's at a good functional point for initial feedback. I haven't added any tests since I don't think there is a graceful way to handle testing.

[karolyi]

+1, waiting for this to enter the release.

[clokep]

I haven't done an in-depth review, but I think it looks reasonable overall. The main blocker is that there's no MSC for the "altcaptcha" proposed in this PR.

[erikjohnston]

As @clokep said this seems reasonable. However, we're not comfortable merging this until the MSC has been merged, or at least further along. (This is to ensure that we don't end up a) merging code that doesn't really get used and b) to ensure that proposals don't get de-facto standardised by being in implementations.)

I think the next steps here are to address the comments in MSC 2745 and flesh it out, and then the hopefully it can be progressed there. (If the original author isn't around to update it then it can be taken over or new MSCs can be created).

Thanks for spending the time here! Having a working implementation is a prerequisite of an MSC being merged, so this isn't wasted effort at all :)

[erikjohnston]

As per my comment above, I think this is blocked on responding to the MSC, so I'm going to close this for now.