r114117 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r114116‎ \| r114117 \| r114118 >
Date:	22:19, 18 March 2012
Author:	ialex
Status:	ok (Comments)
Tags:
Comment:	* (bug 35303) Make proxy and DNS blacklist blocking work again
Modified paths:	/branches/REL1_19/phase3/RELEASE-NOTES-1.19 (modified) (history) /branches/REL1_19/phase3/includes/User.php (modified) (history) /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history) /trunk/phase3/includes/User.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/User.php
—	—	@@ -1273,10 +1273,6 @@
1274	1274	// overwriting mBlockedby, surely?
1275	1275	$this->load();
1276	1276
1277		~~- $this->mBlockedby = 0;~~
1278		~~- $this->mHideName = 0;~~
1279		~~- $this->mAllowUsertalk = 0;~~
1280		-
1281	1277	# We only need to worry about passing the IP address to the Block generator if the
1282	1278	# user is not immune to autoblocks/hardblocks, and they are the current user so we
1283	1279	# know which IP address they're actually coming from
—	—	@@ -1287,30 +1283,37 @@
1288	1284	}
1289	1285
1290	1286	# User/IP blocking
1291		~~- $this->mBlock = Block::newFromTarget( $this->getName(), $ip, !$bFromSlave );~~
1292		~~- if ( $this->mBlock instanceof Block ) {~~
1293		~~- wfDebug( __METHOD__ . ": Found block.\n" );~~
1294		~~- $this->mBlockedby = $this->mBlock->getByName();~~
1295		~~- $this->mBlockreason = $this->mBlock->mReason;~~
1296		~~- $this->mHideName = $this->mBlock->mHideName;~~
1297		~~- $this->mAllowUsertalk = !$this->mBlock->prevents( 'editownusertalk' );~~
1298		~~- }~~
	1287	+ $block = Block::newFromTarget( $this->getName(), $ip, !$bFromSlave );
1299	1288
1300	1289	# Proxy blocking
1301		~~- if ( $ip !== null && !$this->isAllowed( 'proxyunbannable' ) && !in_array( $ip, $wgProxyWhitelist ) ) {~~
	1290	+ if ( !$block instanceof Block && $ip !== null && !$this->isAllowed( 'proxyunbannable' )
	1291	+ && !in_array( $ip, $wgProxyWhitelist ) )
	1292	+ {
1302	1293	# Local list
1303	1294	if ( self::isLocallyBlockedProxy( $ip ) ) {
1304		~~- $this->mBlockedby = wfMsg( 'proxyblocker' );~~
1305		~~- $this->mBlockreason = wfMsg( 'proxyblockreason' );~~
	1295	+ $block = new Block;
	1296	+ $block->setBlocker( wfMsg( 'proxyblocker' ) );
	1297	+ $block->mReason = wfMsg( 'proxyblockreason' );
	1298	+ $block->setTarget( $ip );
	1299	+ } elseif ( $this->isAnon() && $this->isDnsBlacklisted( $ip ) ) {
	1300	+ $block = new Block;
	1301	+ $block->setBlocker( wfMsg( 'sorbs' ) );
	1302	+ $block->mReason = wfMsg( 'sorbsreason' );
	1303	+ $block->setTarget( $ip );
1306	1304	}
	1305	+ }
1307	1306
1308		~~- # DNSBL~~
1309		~~- if ( !$this->mBlockedby && !$this->getID() ) {~~
1310		~~- if ( $this->isDnsBlacklisted( $ip ) ) {~~
1311		~~- $this->mBlockedby = wfMsg( 'sorbs' );~~
1312		~~- $this->mBlockreason = wfMsg( 'sorbsreason' );~~
1313		~~- }~~
1314		~~- }~~
	1307	+ if ( $block instanceof Block ) {
	1308	+ wfDebug( __METHOD__ . ": Found block.\n" );
	1309	+ $this->mBlock = $block;
	1310	+ $this->mBlockedby = $block->getByName();
	1311	+ $this->mBlockreason = $block->mReason;
	1312	+ $this->mHideName = $block->mHideName;
	1313	+ $this->mAllowUsertalk = !$block->prevents( 'editownusertalk' );
	1314	+ } else {
	1315	+ $this->mBlockedby = '';
	1316	+ $this->mHideName = 0;
	1317	+ $this->mAllowUsertalk = false;
1315	1318	}
1316	1319
1317	1320	# Extensions
Index: trunk/phase3/RELEASE-NOTES-1.19
—	—	@@ -24,6 +24,7 @@
25	25	preloaded into edit summary on section edit
26	26	* (bug 31417) New ID mw-content-text around the actual page text, without categories,
27	27	contentSub, ... The same div often also contains the class mw-content-ltr/rtl.
	28	+* (bug 35303) Proxy and DNS blacklist blocking works again
28	29
29	30	=== Configuration changes in 1.19 ===
30	31	* Removed SkinTemplateSetupPageCss hook; use BeforePageDisplay instead.
Index: branches/REL1_19/phase3/includes/User.php
—	—	@@ -1268,10 +1268,6 @@
1269	1269	// overwriting mBlockedby, surely?
1270	1270	$this->load();
1271	1271
1272		~~- $this->mBlockedby = 0;~~
1273		~~- $this->mHideName = 0;~~
1274		~~- $this->mAllowUsertalk = 0;~~
1275		-
1276	1272	# We only need to worry about passing the IP address to the Block generator if the
1277	1273	# user is not immune to autoblocks/hardblocks, and they are the current user so we
1278	1274	# know which IP address they're actually coming from
—	—	@@ -1282,30 +1278,37 @@
1283	1279	}
1284	1280
1285	1281	# User/IP blocking
1286		~~- $this->mBlock = Block::newFromTarget( $this->getName(), $ip, !$bFromSlave );~~
1287		~~- if ( $this->mBlock instanceof Block ) {~~
1288		~~- wfDebug( __METHOD__ . ": Found block.\n" );~~
1289		~~- $this->mBlockedby = $this->mBlock->getByName();~~
1290		~~- $this->mBlockreason = $this->mBlock->mReason;~~
1291		~~- $this->mHideName = $this->mBlock->mHideName;~~
1292		~~- $this->mAllowUsertalk = !$this->mBlock->prevents( 'editownusertalk' );~~
1293		~~- }~~
	1282	+ $block = Block::newFromTarget( $this->getName(), $ip, !$bFromSlave );
1294	1283
1295	1284	# Proxy blocking
1296		~~- if ( $ip !== null && !$this->isAllowed( 'proxyunbannable' ) && !in_array( $ip, $wgProxyWhitelist ) ) {~~
	1285	+ if ( !$block instanceof Block && $ip !== null && !$this->isAllowed( 'proxyunbannable' )
	1286	+ && !in_array( $ip, $wgProxyWhitelist ) )
	1287	+ {
1297	1288	# Local list
1298	1289	if ( self::isLocallyBlockedProxy( $ip ) ) {
1299		~~- $this->mBlockedby = wfMsg( 'proxyblocker' );~~
1300		~~- $this->mBlockreason = wfMsg( 'proxyblockreason' );~~
	1290	+ $block = new Block;
	1291	+ $block->setBlocker( wfMsg( 'proxyblocker' ) );
	1292	+ $block->mReason = wfMsg( 'proxyblockreason' );
	1293	+ $block->setTarget( $ip );
	1294	+ } elseif ( $this->isAnon() && $this->isDnsBlacklisted( $ip ) ) {
	1295	+ $block = new Block;
	1296	+ $block->setBlocker( wfMsg( 'sorbs' ) );
	1297	+ $block->mReason = wfMsg( 'sorbsreason' );
	1298	+ $block->setTarget( $ip );
1301	1299	}
	1300	+ }
1302	1301
1303		~~- # DNSBL~~
1304		~~- if ( !$this->mBlockedby && !$this->getID() ) {~~
1305		~~- if ( $this->isDnsBlacklisted( $ip ) ) {~~
1306		~~- $this->mBlockedby = wfMsg( 'sorbs' );~~
1307		~~- $this->mBlockreason = wfMsg( 'sorbsreason' );~~
1308		~~- }~~
1309		~~- }~~
	1302	+ if ( $block instanceof Block ) {
	1303	+ wfDebug( __METHOD__ . ": Found block.\n" );
	1304	+ $this->mBlock = $block;
	1305	+ $this->mBlockedby = $block->getByName();
	1306	+ $this->mBlockreason = $block->mReason;
	1307	+ $this->mHideName = $block->mHideName;
	1308	+ $this->mAllowUsertalk = !$block->prevents( 'editownusertalk' );
	1309	+ } else {
	1310	+ $this->mBlockedby = '';
	1311	+ $this->mHideName = 0;
	1312	+ $this->mAllowUsertalk = false;
1310	1313	}
1311	1314
1312	1315	# Extensions
Index: branches/REL1_19/phase3/RELEASE-NOTES-1.19
—	—	@@ -24,6 +24,7 @@
25	25	preloaded into edit summary on section edit
26	26	* (bug 31417) New ID mw-content-text around the actual page text, without categories,
27	27	contentSub, ... The same div often also contains the class mw-content-ltr/rtl.
	28	+* (bug 35303) Proxy and DNS blacklist blocking works again
28	29
29	30	=== Configuration changes in 1.19 ===
30	31	* Removed SkinTemplateSetupPageCss hook; use BeforePageDisplay instead.

Comments

#Comment by Nikerabbit (talk | contribs) 09:30, 19 March 2012

It's not immediately obvious why this fixes the bug.

#Comment by Krinkle (talk | contribs) 14:39, 19 March 2012

Could you please stop committing to trunk and REL1_19 at the same time?

I noticed it a few times now. It makes it harder to filter things for mediawiki-core (since common-rootpath is no longer /trunk/phase3) and puts unreviewed code in a release branch. Everybody gets reviewed, no exceptions please.

#Comment by Krinkle (talk | contribs) 14:43, 19 March 2012

It also makes it less obvious to merge to 1.19wmf1.

btw: should this be backported to REL1_18 (for a potential 1.18.x release) or is this a 1.19 regression?

#Comment by Bawolff (talk | contribs) 18:07, 19 March 2012

Thanks for fixing this.

The block notice the user sees for this is a little confusing. However its quite possible it was always like that. For reference the user sees (assuming its localhost that was blocked, as was the case on my test wiki. Note: the DNSBL in the text actually links to user:DNSBL).

You do not have permission to edit this page, for the following reason:

Your username or IP address has been blocked.

The block was made by DNSBL. The reason given is Your IP address is listed as an open proxy in the DNSBL used by Test..

    * Start of block: 15:01, 19 March 2012
    * Expiry of block: 15:01, 19 March 2012
    * Intended blockee: 0:0:0:0:0:0:0:1 

You can contact DNSBL or another administrator to discuss the block. You cannot use the 'e-mail this user' feature unless a valid e-mail address is specified in your account preferences and you have not been blocked from using it. Your current IP address is ::1, and the block ID is #. Please include all above details in any queries you make.

Perhaps having a FakeBlock object that various classes can detect and do something more intelligent when displaying the error message would be a good approach.

Should probably also have a regression test. (DNS blacklist would be difficult, but checking against $wgProxyList should be straightforward). I'll try to get around to adding a regression test for that at some point or another.

#Comment by Aaron Schulz (talk | contribs) 21:57, 20 March 2012

Block could use some more accessors.

Status & tagging log

23:58, 20 March 2012 Catrope (talk | contribs) changed the tags for r114117 [removed: gerritmigration]
21:57, 20 March 2012 Aaron Schulz (talk | contribs) changed the status of r114117 [removed: new added: ok]
21:13, 20 March 2012 Catrope (talk | contribs) changed the tags for r114117 [added: gerritmigration]