Page MenuHomePhabricator

Clean up how ConfirmEdit tracks modules associated with CAPTCHA
Closed, ResolvedPublic

Description

There is a public getForm method that can be used to access the CAPTCHA's HTML. However, there is not a clean way to access the associated modules. Currently, they are added directly to the OutputPage passed in. However, this makes it hard to pull them out, causing issues (see e.g. T140472: Flow: Update to stop using buildCssLinks and for related ConfirmEdit change).

I suggest we should remove the OutputPage parameter. We can use an associative array like https://github.com/wikimedia/mediawiki/blob/master/includes/api/ApiParse.php#L374-L386 (plus a key for the HTML itself).

This would be a breaking change, and require callers to register the modules themselves.

Related Objects

Event Timeline

Restricted Application added a subscriber: Florian. · View Herald TranscriptJul 25 2016, 8:09 PM

It's maybe a good idea to change the registration of modules, removing the OutputPage completely (or the context itself) could lead into problems. ReCaptchaNoCaptcha for example uses the OutputPage object to add a required JS module (which is loaded from Google dierctly buhhhh!!!!), see https://github.com/wikimedia/mediawiki-extensions-ConfirmEdit/blob/master/ReCaptchaNoCaptcha/ReCaptchaNoCaptcha.class.php#L22-L25.

So we should probably just change the return value of getForm to an array and return the HTML and the modules. If we do it as an associative array, instead of a numbered one, we could also be prepared for the future, where probably other information needs to be returned, too :)

So we should probably just change the return value of getForm to an array and return the HTML and the modules. If we do it as an associative array, instead of a numbered one, we could also be prepared for the future, where probably other information needs to be returned, too :)

We can remove the OutputPage parameter and still support the weird ReCAPTCHA case, using "headitems".

Good argument!

Change 301147 had a related patch set uploaded (by Florianschmidtwelzow):
Deprecate getForm() and replace by getFormInformation()

https://gerrit.wikimedia.org/r/301147

I was working on this and had code.

However, it's basically consistent with yours (except I was leaning towards not doing a getForm shim and will explain why), so I will reassign and review yours.

Mattflaschen-WMF added a comment.EditedJul 27 2016, 2:14 AM

From reviewing https://github.com/search?q=org%3Awikimedia+getForm&type=Code , the only Gerrit-hosted extensions affected are:

I'll fix these, but only the WMF-installed ones absolutely must be merged at the same time (the others would be good so they don't languish, though).

@Krinkle Can you check that the ConfirmEdit patch doesn't cause a regression for VisualEditor. Stand-alone VE isn't working for me locally at the moment.

At least from my testing with VE, there're no differences or problems. But it would be good to have someone, who is more familar with VE, to recheck, as I simply tried to edit and solved the CAPTCHA, once with master and once with the change applied :)

Change 301147 merged by jenkins-bot:
Remove getForm() and replace by getFormInformation()

https://gerrit.wikimedia.org/r/301147

Checking in betalabs - cannot trigger Captcha for edits.
Although Special:Version shows that ConfirmEdit is installed in beta. @Mattflaschen-WMF - something should be added/enabled in CommonSettings.php ?
The scenarios that I'd like to check are in
https://www.mediawiki.org/wiki/Extension:ConfirmEdit/Test_Plan

General scenario:

  • as a user that belongs only to Users group, edit a page in VE mode
  • click 'Save page' - Captcha should appear.

On English Wikipedia on Beta Cluster, we have:

var_export( $wgCaptchaTriggers );
array (
  'edit' => false,
  'create' => false,
  'sendemail' => false,
  'addurl' => true,
  'createaccount' => true,
  'badlogin' => true,
  'badloginperuser' => true,
)

This is the same as production.

You can test:

  • Adding a URL when editing
  • Creating an account
  • Repeatedly getting the login password wrong.

It is not configured to trigger on all edits.

Checked Captcha in betalabs for creating accounts/multiple login attmepts - all works as expected.

jmatazzoni closed this task as Resolved.Aug 8 2016, 7:38 PM