Page MenuHomePhabricator

Turn ConfirmEdit captcha implementations into a family of services
Open, Needs TriagePublic


The <Something>Captcha classes in ConfirmEdit are awkward - it's hard to tell what gets called when, what gets called externally, and the whole "many extensions in a single repo" thing is weird. IMO the nicest approach would be to turn it into a service with a clean interface, rewrite each captcha class as a different implementation of the service (also fix the class inhertance - captchas should inherit from an abtsract base class, not SimpleCaptcha which is an actual captcha implementation, even if not particularly useful), and make the service wiring decide based on some config flag which service to load. (This would also make it very easy to implement things like different captcha types on different interfaces, or for different user groups...)

Event Timeline

Tgr created this task.Sep 30 2017, 3:14 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 30 2017, 3:14 AM

That sounds like a really good idea, thanks for bringing this up, @Tgr. Without looking into it deeper, I'll create some subtasks to split the work down and prevent "massive" changes in gerrit. I'll also start working on that :)

Tgr added a comment.EditedNov 3 2017, 4:43 AM

Here is how I would go about it:

  • have to services, CaptchaService and CaptchaRendererService for the backend and presentation (or could be one if you don't care much about interface segregation)
  • A CaptchaContext value object contains the relevant context (e.g. username, or sender for EmailUser). There are a bunch of subclasses for the various contexts.
  • CaptchaService::generateCaptcha( $action, $trigger, CaptchaContext $context ) returns a Captcha object (and stores it in the backend if the service deems that useful). For classic captchas the object mostly just contains a session key to identify the correct solution. For webservice-based captchas it probably contains the site's API key. There should be some way to return a timeout instead (ie. tell that the user has been blocked from further attempts and avoid the UX horror of T158823#3048666). Probably also a way to return "no captcha needed" as the caller might want to completely skip form generation in that case (e.g. autologin), or maybe that can be handled by the renderer not returning anything. Captcha can be serialized into the session and the various ConfirmEdit hooks do that to pass it between the the form display logic and the form submit processing logic if that's needed (I don't think it is).
  • CaptchaRendererService::renderCaptcha( Captcha $captcha, $format ) takes a Captcha and returns some kind of output - a HTML string for the edit form, a metadata array for the API, an AuthenticationRequest` for authentication. (Or those could be separate methods if preferred.)
  • CaptchaRenderService::loadCaptcha( $data, $action?, $trigger? ) takes request data (or an AuthenticationRequest) and returns a CaptchaAttempt object. (Or this and Captcha could be replace with arrays if we prefer stringly typed data over lots of trivial subclasses.)
  • CaptchaService::verifyCaptchaAttempt( CaptchaAttempt $captcha ) checks the attempt, and returns a StatusValue

This takes care of most the existing public methods:

  • setAction, setTrigger are replaced by passing these as arguments - services are stateless
  • getError is replaced by returning a StatusValue
  • getCaptcha and storeCaptcha aren't ever used separately so they can be replaced by generateCaptcha
  • addCaptchaAPI, getFormInformation and createAuthenticationRequest are replaced by generateCaptcha + renderCaptcha
  • retrieveCaptcha will be replaced by loadCaptcha
  • getCaptchaInfo, getMessage will be internal to CaptchaRendererService
  • isBadLogin[PerUser]Triggered can be handled by the captcha generator returning an error. increase/resetBadLoginCounter needs to be kept (could be made a little more generic such as increase/resetAttempts, or even more generic by having the captcha service subscribe to some sort of login event stream instead - the longterm vision being that eventually we could have an ORES-like system for predicting the probability of a user being a spammer, with failed logins being just with signal along with suspicious edits and AbuseFilter triggers and whatnot - probably should be its own task, though).
  • describeCaptchaType can be kept.
  • As far as I see all else can be killed / turned into internal detail of the services, making the interface a whole lot saner.
Tgr renamed this task from Turn ConfirmEdit captcha into a service to Turn ConfirmEdit captcha implementations into a family of services.May 24 2018, 12:23 PM
Tgr added a comment.May 24 2018, 12:26 PM

It would be nice if the properties of a captcha could be adjusted dynamically (see T189546: Add a hook for altering captcha strength in FancyCaptcha) so either the service would have to expose setters for that (seems like a bad idea) or there should be some sort of factory service which takes a configuration object and creates a captcha renderer / validator, instead of making those renderers/validators themselves into services.