r86814 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r86813‎ | r86814 | r86815 >
Date:11:47, 24 April 2011
Author:happy-melon
Status:ok (Comments)
Tags:
Comment:
Beginnings of a rewrite of the captcha system in a more object-oriented fashion; currently although there are classes for different types of captcha, they don't actually represent a single captcha object, they're just confused frontend/backend messes. With the places captchas are inserted increasingly using HTMLForm, it makes eminent sense to introduce a HTMLCaptchaField, but that needs a more OOP implementation of the actual captcha, which I've started here. This is not finished and isn't actually implemented anywhere, but a) it's harmless, b) I don't want it to bitrot, and c) some feedback would always be appreciated, so here it is.
Modified paths:
  • /trunk/extensions/ConfirmEdit/Captcha.php (modified) (history)
  • /trunk/extensions/ConfirmEdit/ConfirmEdit.php (modified) (history)
  • /trunk/extensions/ConfirmEdit/HTMLCaptchaField.php (added) (history)

Diff [purge]

Index: trunk/extensions/ConfirmEdit/HTMLCaptchaField.php
——@@ -0,0 +1,84 @@
 2+<?php
 3+/**
 4+ * HTMLFormField for inserting Captchas into a form.
 5+ *
 6+ * This program is free software; you can redistribute it and/or modify
 7+ * it under the terms of the GNU General Public License as published by
 8+ * the Free Software Foundation; either version 2 of the License, or
 9+ * (at your option) any later version.
 10+ *
 11+ * This program is distributed in the hope that it will be useful,
 12+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
 13+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 14+ * GNU General Public License for more details.
 15+ *
 16+ * You should have received a copy of the GNU General Public License along
 17+ * with this program; if not, write to the Free Software Foundation, Inc.,
 18+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 19+ * http://www.gnu.org/copyleft/gpl.html
 20+ *
 21+ * @class
 22+ */
 23+class HTMLCaptchaField extends HTMLFormField {
 24+
 25+ /**
 26+ * @var Captcha
 27+ */
 28+ private $captcha;
 29+
 30+ public $prefix = '';
 31+
 32+ /**
 33+ * @var Bool|Array
 34+ */
 35+ private $validationResult;
 36+
 37+ public function __construct( $params ){
 38+ parent::__construct( $params );
 39+
 40+ // For differentiating the type of form, mainly
 41+ if( isset( $params['prefix'] ) ){
 42+ $this->prefix = $params['prefix'];
 43+ }
 44+ }
 45+
 46+ /**
 47+ * Get the captcha body. Don't include any of the surrounding table cells/rows
 48+ *
 49+ * @param $value String
 50+ * @return String
 51+ */
 52+ public function getInputHTML( $value ){
 53+ # TODO
 54+ }
 55+
 56+ public function validate( $data, $alldata ){
 57+ // We sent back the exists status of the captcha before. If it *doesn't* exist
 58+ // we actually want to validate this as true, because we don't want an angry red
 59+ // error message, just for the user to put the captcha in again
 60+ if( $data === false ){
 61+ return true;
 62+ }
 63+
 64+
 65+ }
 66+
 67+ /**
 68+ * @param $request WebRequest
 69+ * @return void
 70+ */
 71+ public function loadDataFromRequest( $request ){
 72+ $this->captcha = Captcha::factory();
 73+ $this->captcha->loadFromRequest( $request, $this );
 74+ if( !$this->captcha->exists() ){
 75+ // The captcha doesn't exist; probably because it's already been used and
 76+ // then deleted for security. Load the field up with a new captcha which
 77+ // will be shown to the user when the validation of said new object fails
 78+ $this->captcha = Captcha::newRandom();
 79+ }
 80+
 81+ // This will be useful as the difference between "the captcha doesn't exist" and
 82+ // "you answered the captcha wrongly"
 83+ return $this->captcha->exists();
 84+ }
 85+}
\ No newline at end of file
Property changes on: trunk/extensions/ConfirmEdit/HTMLCaptchaField.php
___________________________________________________________________
Added: svn:eol-style
186 + native
Index: trunk/extensions/ConfirmEdit/Captcha.php
——@@ -1,5 +1,221 @@
22 <?php
33
 4+/**
 5+ * Object encapsulating a captcha process. The captcha has two elements: it must be able
 6+ * to generate a frontend HTML representation of itself which can be presented to the user,
 7+ * which provides inputs for users to provide their interpretation of the captcha; and it
 8+ * must be able to retrieve that data from a subsequently-submitted request and validate
 9+ * whether the user got the data correct.
 10+ */
 11+abstract class Captcha {
 12+
 13+ /**
 14+ * @var String
 15+ */
 16+ protected $id;
 17+
 18+ /**
 19+ * Information about the captcha, in array form
 20+ * @var $info Array
 21+ */
 22+ protected $info;
 23+
 24+ /**
 25+ * Whether this captcha exists in the storage
 26+ * @var Bool
 27+ */
 28+ protected $exists;
 29+
 30+ /**
 31+ * Generate a new empty Captcha. This is guaranteed to return a Captcha object if it
 32+ * does not throw an exception
 33+ *
 34+ * @return Captcha subclass
 35+ */
 36+ public final static function factory() {
 37+ global $wgCaptchaClass;
 38+ $obj = new $wgCaptchaClass;
 39+ if ( $obj instanceof Captcha ) {
 40+ return $obj;
 41+ } else {
 42+ throw new MWException( "Invalid Captcha class $wgCaptchaClass, must extend Captcha" );
 43+ }
 44+ }
 45+
 46+ /**
 47+ * Instantiate a new Captcha object for a given Id
 48+ *
 49+ * @param $id Int
 50+ * @return Captcha
 51+ */
 52+ public final static function newFromId( $id ){
 53+ $obj = self::factory();
 54+ $obj->setId( $id );
 55+ return $obj->exists()
 56+ ? $obj
 57+ : null;
 58+ }
 59+
 60+ /**
 61+ * Instantiate a brand new captcha, never seen before.
 62+ *
 63+ * @return Captcha
 64+ */
 65+ public final static function newRandom(){
 66+ $obj = self::factory();
 67+ $obj->generateNew();
 68+ return $obj;
 69+ }
 70+
 71+ /**
 72+ * Protected constructor - use only the factory methods above to instantiate captchas,
 73+ * or you may end up with the wrong type of object
 74+ */
 75+ protected function __construct(){}
 76+
 77+ /**
 78+ * Get the captcha Id
 79+ *
 80+ * @return String
 81+ */
 82+ public function getId(){
 83+ return $this->id;
 84+ }
 85+
 86+ /**
 87+ * Set the Id internally. Don't include wierd things like entities or characters that
 88+ * need to be HTML-escaped, you'll just be creating more work and pain for yourself...
 89+ *
 90+ * @param $id String
 91+ */
 92+ protected function setId( $id ){
 93+ $this->id = $id;
 94+ }
 95+
 96+ /**
 97+ * Initialise $this->info etc with information needed to make this object a new,
 98+ * (ideally) never-seen-before captcha. Implementations should not save the data in
 99+ * the store in this function, as the captcha may not ever be used.
 100+ *
 101+ * @return Array of captcha info
 102+ */
 103+ # FIXME: detail
 104+ protected abstract function generateNew();
 105+
 106+ /**
 107+ * Save a generated captcha in storage somewhere where it won't be lost between
 108+ * requests. A random ID is used so legit users can make edits in multiple tabs
 109+ * or windows without being unnecessarily hobbled by a serial order requirement.
 110+ */
 111+ protected function store() {
 112+ // Assign random index if we're not udpating
 113+ if ( !isset( $this->info['index'] ) ) {
 114+ if( !$this->getId() ){
 115+ $this->setId( strval( mt_rand() ) );
 116+ }
 117+ $this->info['index'] = $this->getId();
 118+ }
 119+ CaptchaStore::get()->store( $this->info['index'], $this->info );
 120+ }
 121+
 122+ /**
 123+ * Fetch the data for this captcha from the CaptchaStore. This requires $this->id
 124+ * to be set.
 125+ *
 126+ * @return Array|Bool: Array of info, or false if missing
 127+ */
 128+ protected function retrieve() {
 129+ if( $this->getId() === null ){
 130+ return null;
 131+ }
 132+ if( $this->info === null ){
 133+ $this->info = CaptchaStore::get()->retrieve( $this->getId() );
 134+ $this->exists = $this->info !== false;
 135+ }
 136+ return $this->info;
 137+ }
 138+
 139+ /**
 140+ * Clear the information about this captcha from the CaptchaStore, so it cannot
 141+ * be reused at a later date.
 142+ */
 143+ protected function delete() {
 144+ if( $this->getId() !== null ){
 145+ CaptchaStore::get()->clear( $this->getId() );
 146+ }
 147+ }
 148+
 149+ /**
 150+ * Whether this captcha exists. $this->setId() must have been called from some context
 151+ *
 152+ * @return Bool
 153+ */
 154+ public function exists(){
 155+ if( $this->exists === null ){
 156+ $this->retrieve();
 157+ }
 158+ return $this->exists;
 159+ }
 160+
 161+ /**
 162+ * Load some data from a WebRequest. Implementations must load all data they need
 163+ * from the request in this function, they must not use the global $wgRequest, as
 164+ * in the post-1.18 environment they may not necessarily be the same.
 165+ *
 166+ * @param $request WebRequest
 167+ * @param $field HTMLCaptchaField will be passed if the captcha is part of an HTMLForm
 168+ */
 169+ public abstract function loadFromRequest( WebRequest $request, HTMLCaptchaField $field = null );
 170+
 171+ /**
 172+ * Return the data that would be needed to pass the captcha challenge through the API.
 173+ * Implementations must return an array with at least the following parameters:
 174+ * 'type' - a unique description of the type of challenge. This could be
 175+ * the class name
 176+ * 'mime' - the MIME type of the challenge
 177+ * 'id' - the captcha Id produced by getId()
 178+ * Implementations should document how the user should use the provided data to answer
 179+ * the captcha.
 180+ *
 181+ * Implementations may return False to indicate that it is not possible to represent
 182+ * the challenge via the API. API actions protected by such a captcha will be disabled.
 183+ *
 184+ * @return Array|Bool
 185+ */
 186+ public abstract function getApiParams();
 187+
 188+ /**
 189+ * Return the HTML which will be placed in the 'input' table cell of an HTMLForm.
 190+ * Implementations must include input fields which will perpetuate the captcha Id and
 191+ * any special data, as well as providing a means for the user to answer the captcha.
 192+ * Implementations should not include any help or label text, as these will be set in
 193+ * the label-message and help-message attributes of the HTMLCaptchafield.
 194+ * Implementations should honour the options set in the HTMLFormField such as
 195+ * $field->mName and $field->mReadonly.
 196+ *
 197+ * @param $field HTMLCaptchaField
 198+ * @return String raw HTML
 199+ */
 200+ public abstract function getFormHTML( HTMLCaptchaField $field );
 201+
 202+ /**
 203+ * Return the HTML which will be used in legacy forms which do not implement HTMLForm
 204+ * Implementations must include input fields which will perpetuate the captcha Id and
 205+ * any other necessary data, as well as providing a means for the user to answer the
 206+ * captcha, and any relevant descriptions and instructions.
 207+ *
 208+ * @return String raw HTML
 209+ */
 210+ public abstract function getFreeflowHTML();
 211+
 212+ /**
 213+ * Using the parameters loaded from the web request, check the captcha, maybe delete
 214+ * it if that's desirable, do any other necessary cleanup, and return Bool
 215+ * @return Bool whether the captcha was successfully answered
 216+ */
 217+ public abstract function checkCaptcha();
 218+}
 219+
4220 class SimpleCaptcha {
5221
6222 /**
Index: trunk/extensions/ConfirmEdit/ConfirmEdit.php
——@@ -201,11 +201,13 @@
202202 $wgHooks['APIEditBeforeSave'][] = 'ConfirmEditHooks::confirmEditAPI';
203203
204204 $wgAutoloadClasses['ConfirmEditHooks'] = "$wgConfirmEditIP/ConfirmEditHooks.php";
 205+$wgAutoloadClasses['Captcha']= "$wgConfirmEditIP/Captcha.php";
205206 $wgAutoloadClasses['SimpleCaptcha']= "$wgConfirmEditIP/Captcha.php";
206207 $wgAutoloadClasses['CaptchaStore']= "$wgConfirmEditIP/CaptchaStore.php";
207208 $wgAutoloadClasses['CaptchaSessionStore']= "$wgConfirmEditIP/CaptchaStore.php";
208209 $wgAutoloadClasses['CaptchaCacheStore']= "$wgConfirmEditIP/CaptchaStore.php";
209210 $wgAutoloadClasses['CaptchaSpecialPage'] = "$wgConfirmEditIP/ConfirmEditHooks.php";
 211+$wgAutoloadClasses['HTMLCaptchaField']= "$wgConfirmEditIP/HTMLCaptchaField.php";
210212
211213 /**
212214 * Set up $wgWhitelistRead

Comments

#Comment by Nikerabbit (talk | contribs)   12:26, 24 April 2011

wierd -> weird

Is it a good idea to tie captcha to the WebQequest class? Could there be simpler interface to achieve the same?

#Comment by Happy-melon (talk | contribs)   13:56, 24 April 2011

It's tricky because different captchas expose different inputs for collecting user data (ReCaptcha, for instance, is loaded entirely through javascript), and also vary in the data they need to pass on. So you either need to get the Captcha to get the data from the request itself, or it needs to provide a way to tell the HTMLForm how to get the data it needs from the request. Which is all much of a muchness, really...

#Comment by Aaron Schulz (talk | contribs)   20:29, 13 September 2011

More or less OK. Not sure where getApiParams() is going. The WebRequest thing stood out to me too, but I see why that's hard to work around.

Status & tagging log