r76344 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r76343‎ \| r76344 \| r76345 >
Date:	21:55, 8 November 2010
Author:	vasilievvv
Status:	resolved (Comments)
Tags:
Comment:	Title blacklist: * (bug 22141) Introduce a separate user right for overriding title blacklist on account creations only * Some minor refactorings
Modified paths:	/trunk/extensions/TitleBlacklist/TitleBlacklist.hooks.php (modified) (history) /trunk/extensions/TitleBlacklist/TitleBlacklist.i18n.php (modified) (history) /trunk/extensions/TitleBlacklist/TitleBlacklist.list.php (modified) (history) /trunk/extensions/TitleBlacklist/TitleBlacklist.php (modified) (history)

Diff [purge]

Index: trunk/extensions/TitleBlacklist/TitleBlacklist.i18n.php
—	—	@@ -25,7 +25,9 @@
26	26	It matches the following blacklist entry: <code>$1</code>',
27	27	'titleblacklist-invalid' => 'The following {{PLURAL:$1\|line\|lines}} in the title blacklist {{PLURAL:$1\|is\|are}} invalid;
28	28	please correct {{PLURAL:$1\|it\|them}} before saving:',
	29	+ 'titleblacklist-override' => 'Ignore the blacklist',
29	30	'right-tboverride' => 'Override the title blacklist',
	31	+ 'right-tboverride-account' => 'Override the username blacklist',
30	32	);
31	33
32	34	/** Message documentation (Message documentation)
Index: trunk/extensions/TitleBlacklist/TitleBlacklist.php
—	—	@@ -12,7 +12,7 @@
13	13	$wgExtensionCredits['other'][] = array(
14	14	'path' => __FILE__,
15	15	'name' => 'Title Blacklist',
16		~~- 'author' => array( 'VasilievVV', 'Fran Rogers' ),~~
	16	+ 'author' => array( 'Victor Vasiliev', 'Fran Rogers' ),
17	17	'version' => '1.4.2',
18	18	'url' => 'http://www.mediawiki.org/wiki/Extension:Title_Blacklist',
19	19	'descriptionmsg' => 'titleblacklist-desc',
—	—	@@ -40,7 +40,8 @@
41	41	'warningexpiry' => 600,
42	42	);
43	43
44		~~-$wgAvailableRights[] = 'tboverride';~~
	44	+$wgAvailableRights[] = 'tboverride'; // Implies tboverride-account
	45	+$wgAvailableRights[] = 'tboverride-account'; // For account creation
45	46	$wgGroupPermissions['sysop']['tboverride'] = true;
46	47
47	48	$wgHooks['getUserPermissionsErrorsExpensive'][] = 'TitleBlacklistHooks::userCan';
—	—	@@ -49,6 +50,7 @@
50	51	$wgHooks['CentralAuthAutoCreate'][] = 'TitleBlacklistHooks::centralAuthAutoCreate';
51	52	$wgHooks['EditFilter'][] = 'TitleBlacklistHooks::validateBlacklist';
52	53	$wgHooks['ArticleSaveComplete'][] = 'TitleBlacklistHooks::clearBlacklist';
	54	+$wgHooks['UserCreateForm'][] = 'TitleBlacklistHooks::addOverrideCheckbox';
53	55
54	56	/**
55	57	* Initialize the title blacklist
Index: trunk/extensions/TitleBlacklist/TitleBlacklist.hooks.php
—	—	@@ -1,8 +1,8 @@
2	2	<?php
3	3	/**
4	4	* Hooks for Title Blacklist
5		~~- * @author VasilievVV~~
6		~~- * @copyright © 2007 VasilievVV~~
	5	+ * @author Victor Vasiliev
	6	+ * @copyright © 2007-2010 Victor Vasiliev et al
7	7	* @license GNU General Public License 2.0 or later
8	8	*/
9	9
—	—	@@ -17,12 +17,9 @@
18	18	global $wgTitleBlacklist;
19	19	if( $action == 'create' \|\| $action == 'edit' \|\| $action == 'upload' ) {
20	20	efInitTitleBlacklist();
21		~~- $blacklisted = $wgTitleBlacklist->isBlacklisted( $title, $action );~~
	21	+ $blacklisted = $wgTitleBlacklist->userCannot( $title, $user, $action );
22	22	if( $blacklisted instanceof TitleBlacklistEntry ) {
23		~~- $message = $blacklisted->getCustomMessage();~~
24		~~- if( is_null( $message ) )~~
25		~~- $message = 'titleblacklist-forbidden-edit';~~
26		~~- $result = array( $message,~~
	23	+ $result = array( $blacklisted->getErrorMessage( 'edit' ),
27	24	htmlspecialchars( $blacklisted->getRaw() ),
28	25	$title->getFullText() );
29	26	return false;
—	—	@@ -35,14 +32,11 @@
36	33	public static function abortMove( $old, $nt, $user, &$err ) {
37	34	global $wgTitleBlacklist;
38	35	efInitTitleBlacklist();
39		~~- $blacklisted = $wgTitleBlacklist->isBlacklisted( $nt, 'move' );~~
	36	+ $blacklisted = $wgTitleBlacklist->userCannot( $nt, $user, 'move' );
40	37	if( !$blacklisted )
41		~~- $blacklisted = $wgTitleBlacklist->isBlacklisted( $old, 'edit' );~~
	38	+ $blacklisted = $wgTitleBlacklist->userCannot( $old, $user, 'edit' );
42	39	if( $blacklisted instanceof TitleBlacklistEntry ) {
43		~~- $message = $blacklisted->getCustomMessage();~~
44		~~- if( is_null( $message ) )~~
45		~~- $message = 'titleblacklist-forbidden-move';~~
46		~~- $err = wfMsgWikiHtml( $message,~~
	40	+ $err = wfMsgWikiHtml( $blacklisted->getErrorMessage( 'move' ),
47	41	htmlspecialchars( $blacklisted->getRaw() ),
48	42	htmlspecialchars( $old->getFullText() ),
49	43	htmlspecialchars( $nt->getFullText() ) );
—	—	@@ -59,17 +53,13 @@
60	54	*
61	55	* @return bool Acceptable
62	56	*/
63		~~- private static function acceptNewUserName( $userName, &$err ) {~~
64		~~- global $wgTitleBlacklist;~~
	57	+ private static function acceptNewUserName( $userName, &$err, $override = true ) {
	58	+ global $wgTitleBlacklist, $wgUser;
65	59	efInitTitleBlacklist();
66	60	$title = Title::makeTitleSafe( NS_USER, $userName );
67		~~- $blacklisted = $wgTitleBlacklist->isBlacklisted( $title, 'new-account' );~~
68		~~- if( !( $blacklisted instanceof TitleBlacklistEntry ) )~~
69		~~- $blacklisted = $wgTitleBlacklist->isBlacklisted( $title, 'create' );~~
	61	+ $blacklisted = $wgTitleBlacklist->userCannot( $title, $wgUser, 'new-account', $override );
70	62	if( $blacklisted instanceof TitleBlacklistEntry ) {
71		~~- $message = $blacklisted->getCustomMessage();~~
72		~~- if ( is_null( $message ) )~~
73		~~- $message = 'titleblacklist-forbidden-new-account';~~
	63	+ $message = $blacklisted->getErrorMessage( 'new-account' );
74	64	$err = wfMsgWikiHtml( $message, $blacklisted->getRaw(), $userName );
75	65	return false;
76	66	}
—	—	@@ -78,10 +68,9 @@
79	69
80	70	/** AbortNewAccount hook */
81	71	public static function abortNewAccount( $user, &$message ) {
82		~~- global $wgUser;~~
83		~~- if( $wgUser->isAllowed( 'tboverride' ) )~~
84		~~- return true;~~
85		~~- return self::acceptNewUserName( $user->getName(), $message );~~
	72	+ global $wgUser, $wgRequest;
	73	+ $override = $wgRequest->getCheck( 'wpIgnoreTitleBlacklist' );
	74	+ return self::acceptNewUserName( $user->getName(), $message, $override );
86	75	}
87	76
88	77	/** CentralAuthAutoCreate hook */
—	—	@@ -92,7 +81,7 @@
93	82
94	83	/** EditFilter hook */
95	84	public static function validateBlacklist( $editor, $text, $section, $error ) {
96		~~- global $wgTitleBlacklist;~~
	85	+ global $wgTitleBlacklist, $wgUser;
97	86	efInitTitleBlacklist();
98	87	$title = $editor->mTitle;
99	88	if( $title->getNamespace() == NS_MEDIAWIKI && $title->getDBkey() == 'Titleblacklist' ) {
—	—	@@ -118,7 +107,7 @@
119	108	# Block redirects to nonexistent blacklisted titles
120	109	$retitle = Title::newFromRedirect( $text );
121	110	if( $retitle !== null && !$retitle->exists() ) {
122		~~- $blacklisted = $wgTitleBlacklist->isBlacklisted( $retitle, 'create' );~~
	111	+ $blacklisted = $wgTitleBlacklist->userCannot( $retitle, $wgUser, 'create' );
123	112	if( $blacklisted instanceof TitleBlacklistEntry ) {
124	113	$error = Html::openElement( 'div', array( 'class' => 'errorbox' ) ) .
125	114	wfMsg( 'titleblacklist-forbidden-edit',
—	—	@@ -146,4 +135,15 @@
147	136	}
148	137	return true;
149	138	}
	139	+
	140	+ /** UserCreateForm hook based on the one from AntiSpoof extension */
	141	+ public static function addOverrideCheckbox( &$template ) {
	142	+ global $wgRequest, $wgUser;
	143	+
	144	+ if ( TitleBlacklist::userCanOverride( 'new-account' ) )
	145	+ $template->addInputItem( 'wpIgnoreTitleBlacklist',
	146	+ $wgRequest->getCheck( 'wpIgnoreTitleBlacklist' ),
	147	+ 'checkbox', 'titleblacklist-override' );
	148	+ return true;
	149	+ }
150	150	}
Index: trunk/extensions/TitleBlacklist/TitleBlacklist.list.php
—	—	@@ -1,8 +1,8 @@
2	2	<?php
3	3	/**
4	4	* Title Blacklist class
5		~~- * @author VasilievVV~~
6		~~- * @copyright © 2007 VasilievVV~~
	5	+ * @author Victor Vasiliev
	6	+ * @copyright © 2007-2010 Victor Vasiliev et al
7	7	* @license GNU General Public License 2.0 or later
8	8	* @file
9	9	*/
—	—	@@ -129,22 +129,39 @@
130	130	}
131	131
132	132	/**
133		~~- * Check whether the blacklist restricts the current User from~~
	133	+ * Check whether the blacklist restricts giver nuser
134	134	* performing a specific action on the given Title
135	135	*
136	136	* @param $title Title to check
	137	+ * @param $user User to check
137	138	* @param $action Action to check; 'edit' if unspecified
	139	+ * @param $override If set to true, overrides work
138	140	* @return The corresponding TitleBlacklistEntry if blacklisted;
139	141	* otherwise FALSE
140	142	*/
	143	+ public function userCannot( $title, $user, $action = 'edit', $override = true ) {
	144	+ if( $override && self::userCanOverride( $action ) )
	145	+ return false;
	146	+ else
	147	+ return $this->isBlacklisted( $title, $action );
	148	+ }
	149	+
	150	+ /**
	151	+ * Check whether the blacklist restricts
	152	+ * performing a specific action on the given Title
	153	+ *
	154	+ * @param $title Title to check
	155	+ * @param $action Action to check; 'edit' if unspecified
	156	+ * @return The corresponding TitleBlacklistEntry if blacklisted;
	157	+ * otherwise FALSE
	158	+ */
141	159	public function isBlacklisted( $title, $action = 'edit' ) {
142		~~- global $wgUser;~~
143	160	if( !($title instanceof Title) ) {
144	161	$title = Title::newFromText( $title );
145	162	}
146	163	$blacklist = $this->getBlacklist();
147	164	foreach ( $blacklist as $item ) {
148		~~- if( !$item->userCan( $title, $wgUser, $action ) ) {~~
	165	+ if( !$item->matches( $title, $action ) ) {
149	166	if( $this->isWhitelisted( $title, $action ) ) {
150	167	return false;
151	168	}
—	—	@@ -169,7 +186,7 @@
170	187	}
171	188	$whitelist = $this->getWhitelist();
172	189	foreach( $whitelist as $item ) {
173		~~- if( !$item->userCan( $title, $wgUser, $action ) ) {~~
	190	+ if( !$item->matches( $title, $action ) ) {
174	191	return true;
175	192	}
176	193	}
—	—	@@ -245,6 +262,17 @@
246	263	}
247	264	return $badEntries;
248	265	}
	266	+
	267	+ /**
	268	+ * Inidcates whether user can override blacklist on certain action.
	269	+ *
	270	+ * @param $action Action
	271	+ */
	272	+ public static function userCanOverride( $action ) {
	273	+ global $wgUser;
	274	+ return $wgUser->isAllowed( 'tboverride' ) \|\|
	275	+ ( $action == 'new-account' && $wgUser->isAllowed( 'tboverride-account' ) );
	276	+ }
249	277	}
250	278
251	279
—	—	@@ -273,18 +301,14 @@
274	302	}
275	303
276	304	/**
277		~~- * Check whether the specified User can perform the specified action~~
	305	+ * Check whether a user can perform the specified action
278	306	* on the specified Title
279	307	*
280	308	* @param $title Title to check
281		~~- * @param $user User to check~~
282	309	* @param $action %Action to check
283	310	* @return TRUE if the user can; otherwise FALSE
284	311	*/
285		~~- public function userCan( $title, $user, $action ) {~~
286		~~- if( $user->isAllowed( 'tboverride' ) ) {~~
287		~~- return true;~~
288		~~- }~~
	312	+ public function matches( $title, $action ) {
289	313	wfSuppressWarnings();
290	314	$match = preg_match( "/^(?:{$this->mRegex})$/us" . ( isset( $this->mParams['casesensitive'] ) ? '' : 'i' ), $title->getFullText() );
291	315	wfRestoreWarnings();
—	—	@@ -419,6 +443,18 @@
420	444	* @param $v New version to set
421	445	*/
422	446	public function setFormatVersion( $v ) { $this->mFormatVersion = $v; }
	447	+
	448	+ /**
	449	+ * Return the error message name for the blacklist entry.
	450	+ *
	451	+ * @param $operation Operation name (as in titleblacklist-forbidden message name)
	452	+ *
	453	+ * @returns The error message name
	454	+ */
	455	+ public function getErrorMessage( $operation ) {
	456	+ $message = $this->getCustomMessage();
	457	+ return $message ? $message : "titleblacklist-forbidden-{$operation}";
	458	+ }
423	459	}
424	460
425		~~-//@}~~
\ No newline at end of file
	461	+//@}

Follow-up revisions

Revision	Commit summary	Author	Date
r81751	1.17wmf1: Superficially fix fatal caused by r76344	catrope	15:26, 8 February 2011
r84575	Followup r76344, fix another inversed logic...	reedy	22:30, 22 March 2011
r97794	Fix for r76344: you can't access $wgUser during the CentralAuthAutoCreate hoo...	tstarling	06:10, 22 September 2011

Comments

#Comment by Catrope (talk | contribs) 15:28, 8 February 2011

-	public function userCan( $title, $user, $action ) {
-		if( $user->isAllowed( 'tboverride' ) ) {
-			return true;
-		}
+	public function matches( $title, $action ) {

This causes a fatal because $user is used again in the function ( if( isset( $this->mParams['autoconfirmed'] ) && $user->isAllowed( 'autoconfirmed' ) ) {). Superficially fixed this in 1.17wmf1, but this needs a proper fix.

#Comment by Aaron Schulz (talk | contribs) 05:33, 22 September 2011

$wgUser not used anymore in abortNewAccount().

#Comment by Tim Starling (talk | contribs) 06:00, 22 September 2011

Was the $user parameter to TitleBlacklist::userCannot() meant to be used for something?

Status & tagging log

13:54, 15 June 2011 Reedy (talk | contribs) changed the status of r76344 [removed: new added: resolved]
21:24, 21 April 2011 Happy-melon (talk | contribs) changed the tags for r76344 [removed: 1.17ok]
22:51, 22 March 2011 Reedy (talk | contribs) changed the status of r76344 [removed: fixme added: new]
21:13, 11 February 2011 RobLa-WMF (talk | contribs) changed the tags for r76344 [added: 1.17ok]
15:28, 8 February 2011 Catrope (talk | contribs) changed the status of r76344 [removed: ok added: fixme]
16:20, 31 January 2011 Reedy (talk | contribs) changed the tags for r76344 [removed: sam]
16:20, 31 January 2011 Reedy (talk | contribs) changed the status of r76344 [removed: new added: ok]
03:28, 26 January 2011 MarkAHershberger (talk | contribs) changed the tags for r76344 [added: sam]