Last modified: 2008-06-27 19:51:02 UTC
When an administrator creates an account which would normally be blocked by the Antispoof extension, they should be asked for confirmation that they really want to create the account before it is actually created (the confirmation should include which username(s) the account is similar to) Note: Multiple results wanted in bug 12232. Rationale: Without this, administrators can accidentally create accounts (on behalf of someone else) which are confusing as they are similar to another user. At worst, this would mean one extra click if the admin already knew about the conflict but would prevent the admin accidentally creating a confusing situation.
*** Bug 13917 has been marked as a duplicate of this bug. ***
Created attachment 5002 [details] trunk/extensions/AntiSpoof/AntiSpoof.php - Adds warning to sysops before creating similar username. Patch against trunk. The patch makes it where if a sysop attempts to create a similar username, the creation fails with a message at the top of the login form. This message includes a built-in form that goes through with the creation. The message is "antispoof-name-warning", and a draft of the message can be found at [[User:Parent5446/MediaWiki/antispoof-name-warning]]. The warning is surpressed by POSTing "wpOverride" to the login form. Note: Users still must have the "override-antispoof" right in order to override.
I don't think we want to embed form HTML in messages like that, even if it technically works. Better to construct the form in code and just pull the strings from messages.
Created attachment 5004 [details] Incorporates HTML form in code instead of in message. OK, now the form is located in the code, and only the beginning text is in the message.
This patch has some basic security problems: arbitrary user input is placed directly into HTML output without escaping, making it an HTML injection / cross-site scripting vulnerability. It also appears to rely on client-side JavaScript to submit a form via clicking a link instead of using a regular form button. This practice should be avoided, as it doesn't follow standard user interface behavior and it will fail if JavaScript is disabled. The action path in the form is hardcoded, and will fail on most MediaWiki sites including Wikimedia's on the secure.wikimedia.org interface.
OK, sorry about that. I am not exactly the most experienced PHP user. I'll see what I can do and get back to this. It may require a bit more tweaking than usual, but that's OK.
Created attachment 5006 [details] Adds option for extra input. See other diffs for better description. - Capitalization fixed. Adds extrainput array when running hook. See later diffs for more info.
Created attachment 5007 [details] Adds option for extra input in mainloginform function. See other diffs for better description. This adds the option for the $extrainput array, which is passed from the AntiSpoof extension (or any other extension that wants to use it) to the next diff, which is in the Userlogin.php file.
Created attachment 5008 [details] Adds support for an extra <input> tag in the login form. - Fixed capitalization and used htmlspeciachars() directly. This adds support for another variable in the account creation form. If a function passes an array $extrainput, with a 'name' and 'value', the account creation form will have a hidden variable with the following syntax: <input type="hidden" name="$extrainput['name']" value="$extrainput['value']" /> In the AntiSpoof extension specifically, here is a conclusion of what happens. If the extension sets the conflict mode to 'OVERRIDE' (which it does for users with the 'override-antispoof' right), then the function will return false, and cause the Special:Userlogin page (along with classes in the Userlogin.php file) to generate the account creation form with a notice on top. The concept is basically like when the software warns a user about not putting in an edit summary. If the username is similar, it pops up with a notice saying "The accounts are similar, press Create account (or by-email) again to continue." When they press it a second time, the form now POSTs the wpOverride variable, which was inserted using the $extrainput array, which allows the creation to go through. Look at the code itself for more details, since my explanation is a little confusing.
Created attachment 5009 [details] Adds option for extra input in mainloginform function. See other diffs for better description. (Now checks for array) - Fixed capitalization.
See http://en.wikipedia.org/wiki/Wikipedia_talk:Request_an_account#Discussion_on_giving_accountcreators_override-antispoof_right for consensus on en wikipedia.
Looks pretty good, though you should be using htmlspecialchars() directly instead of $this->msg() in templates/Userlogin.php. Also maybe make it a checkbox instead, and add a custom label (for which you _should_ use $this->msg())? Also, in the future, I'd suggest submitting all the relevant changes in one patch unless some parts are genuinely alternative or optional -- it's less confusing for the devs, who don't need to puzzle out which patches you want them to apply.
Please watch your case sensitivity on $extraInput, keep variable names with the same capitalization everywhere you use them.
Comment on attachment 5006 [details] Adds option for extra input. See other diffs for better description. - Capitalization fixed. >Index: extensions/AntiSpoof/AntiSpoof.php >=================================================================== >--- extensions/AntiSpoof/AntiSpoof.php (revision 36517) >+++ extensions/AntiSpoof/AntiSpoof.php (working copy) >@@ -60,7 +60,7 @@ > * @param string $message > * @return bool true to continue, false to abort user creation > */ >-function asAbortNewAccountHook( $user, &$message ) { >+function asAbortNewAccountHook( $user, &$message, &$extraInput ) { > global $wgAntiSpoofAccounts, $wgUser; > wfLoadExtensionMessages( 'AntiSpoof' ); > >@@ -84,6 +84,7 @@ > wfDebugLog( 'antispoof', "{$mode}PASS new account '$name' [$normalized]" ); > } else { > wfDebugLog( 'antispoof', "{$mode}CONFLICT new account '$name' [$normalized] spoofs '$conflict'" ); >+ global $wgRequest; > if( $active ) { > $message = wfMsg( 'antispoof-name-conflict', $name, $conflict ); > return false; >@@ -88,6 +89,11 @@ > $message = wfMsg( 'antispoof-name-conflict', $name, $conflict ); > return false; > } >+ elseif( $mode = 'OVERRIDE' && !$wgRequest->getText( 'wpOverride' ) ) { >+ $message = wfMsg( 'antispoof-name-warning', $name, $conflict ); >+ $extrainput = array( 'name' => 'wpOverride', 'value' => true ); >+ return false; >+ } > } > } else { > $error = $spoof->getError();
Comment on attachment 5008 [details] Adds support for an extra <input> tag in the login form. - Fixed capitalization and used htmlspeciachars() directly. >Index: phase3/includes/templates/Userlogin.php >=================================================================== >--- phase3/includes/templates/Userlogin.php (revision 36517) >+++ phase3/includes/templates/Userlogin.php (working copy) >@@ -208,6 +208,11 @@ > tabindex="9" > value="<?php $this->msg('createaccountmail') ?>" /> > <?php } ?> >+ <?php if( is_array( $this->data['extraInput'] ) && $this->data['extraInput']['name'] ) { ?> >+ <input type='hidden' name="<?php $this->htmlspecialchars( $this->data['extraInput']['name'] ); ?>" id="<?php $this->msg( $this->data['extraInput']['name'] ); ?>" >+ value="<?php if( $this->data['extraInput']['value'] ) { $this->msg( $this->data['extraInput']['value'] ); } >+ else { echo 0; } ?>" /> >+ <?php } ?> > </td> > </tr> > </table>
Comment on attachment 5009 [details] Adds option for extra input in mainloginform function. See other diffs for better description. (Now checks for array) - Fixed capitalization. >Index: phase3/includes/specials/SpecialUserlogin.php >=================================================================== >--- phase3/includes/specials/SpecialUserlogin.php (revision 36517) >+++ phase3/includes/specials/SpecialUserlogin.php (working copy) >@@ -297,10 +297,14 @@ > $u->setRealName( $this->mRealName ); > > $abortError = ''; >- if( !wfRunHooks( 'AbortNewAccount', array( $u, &$abortError ) ) ) { >+ if( !wfRunHooks( 'AbortNewAccount', array( $u, &$abortError, &$extraInput ) ) ) { > // Hook point to add extra creation throttles and blocks > wfDebug( "LoginForm::addNewAccountInternal: a hook blocked creation\n" ); >- $this->mainLoginForm( $abortError ); >+ if( is_array( $extraInput ) ) { >+ $this->mainLoginForm( $abortError, $extraInput ); >+ } else { >+ $this->mainLoginForm( $abortError ); >+ } > return false; > } > >@@ -710,7 +714,7 @@ > /** > * @private > */ >- function mainLoginForm( $msg, $msgtype = 'error' ) { >+ function mainLoginForm( $msg, $msgtype = 'error', $extraInput ) { > global $wgUser, $wgOut, $wgAllowRealName, $wgEnableEmail; > global $wgCookiePrefix, $wgAuth, $wgLoginLanguageSelector; > global $wgAuth, $wgEmailConfirmToEdit; >@@ -793,6 +797,15 @@ > $template->set( 'canreset', $wgAuth->allowPasswordChange() ); > $template->set( 'remember', $wgUser->getOption( 'rememberpassword' ) or $this->mRemember ); > >+ if( is_array( $extraInput ) && isset( $extraInput['name'] ) ) { >+ if( isset( $extraInput['value'] ) ) { >+ $template->set( 'extraInput', array( 'name' => $extraInput['name'], 'value' => $extraInput['value'] ) ); >+ } >+ else { >+ $template->set( 'extraInput', array( 'name' => $extraInput['name'], 'value' => false ) ); >+ } >+ } >+ > # Prepare language selection links as needed > if( $wgLoginLanguageSelector ) { > $template->set( 'languages', $this->makeLanguageSelector() );
I fixed capitalization in all the patches so they all have the $extraInput version. Furthermore, it now uses htmlspecialchars() directly instead of the msg() function.
Created attachment 5025 [details] Combined patch. I combined the patches into one diff for easier viewing.
Implemented a more flexible way of modifying the user login form in r36758 based on Tyler Romeo's patch. Only override the anti spoof if the wpIgnoreAntiSpoof checkbox is set in r36760.
