Admins should be asked for confirmation when Antispoof applies
Closed, ResolvedPublic

Description

Author: gdonato

Description:
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.


Version: unspecified
Severity: enhancement

bzimport added a subscriber: Unknown Object (MLST).
bzimport set Reference to bz13426.
bzimport created this task.Via LegacyMar 18 2008, 10:01 PM
bzimport added a comment.Via ConduitMay 1 2008, 4:38 PM

gdonato wrote:

*** Bug 13917 has been marked as a duplicate of this bug. ***

Parent5446 added a comment.Via ConduitJun 19 2008, 7:19 PM

Created attachment 5002
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.

attachment AntiSpoof.diff ignored as obsolete

Ilmari_Karonen added a comment.Via ConduitJun 19 2008, 9:50 PM

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.

Parent5446 added a comment.Via ConduitJun 20 2008, 12:38 AM

Created attachment 5004
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.

attachment AntiSpoof.diff ignored as obsolete

brion added a comment.Via ConduitJun 20 2008, 6:29 PM

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.

Parent5446 added a comment.Via ConduitJun 20 2008, 7:58 PM

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.

Parent5446 added a comment.Via ConduitJun 21 2008, 3:11 AM

Created attachment 5006
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.

attachment AntiSpoof.diff ignored as obsolete

Parent5446 added a comment.Via ConduitJun 21 2008, 3:12 AM

Created attachment 5007
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.

attachment SpecialUserlogin.diff ignored as obsolete

Parent5446 added a comment.Via ConduitJun 21 2008, 3:18 AM

Created attachment 5008
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.

attachment Userlogin.diff ignored as obsolete

Parent5446 added a comment.Via ConduitJun 21 2008, 3:44 AM

Created attachment 5009
Adds option for extra input in mainloginform function. See other diffs for better description. (Now checks for array) - Fixed capitalization.

attachment SpecialUserlogin.diff ignored as obsolete

Ilmari_Karonen added a comment.Via ConduitJun 25 2008, 8:18 PM

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.

bzimport added a comment.Via ConduitJun 25 2008, 9:58 PM

Bryan.TongMinh wrote:

Please watch your case sensitivity on $extraInput, keep variable names with the same capitalization everywhere you use them.

Parent5446 added a comment.Via ConduitJun 26 2008, 12:56 AM

Comment on attachment 5006
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();
Parent5446 added a comment.Via ConduitJun 26 2008, 12:59 AM

Comment on attachment 5008
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>
Parent5446 added a comment.Via ConduitJun 26 2008, 1:00 AM

Comment on attachment 5009
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 ) );
+ }
+ }
+

  1. Prepare language selection links as needed if( $wgLoginLanguageSelector ) { $template->set( 'languages', $this->makeLanguageSelector() );
Parent5446 added a comment.Via ConduitJun 26 2008, 1:01 AM

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.

Parent5446 added a comment.Via ConduitJun 27 2008, 2:13 PM

Created attachment 5025
Combined patch.

I combined the patches into one diff for easier viewing.

Attached: AntiSpoof.diff

bzimport added a comment.Via ConduitJun 27 2008, 7:51 PM

Bryan.TongMinh wrote:

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.

Add Comment