r74213 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r74212‎ \| r74213 \| r74214 >
Date:	15:24, 3 October 2010
Author:	reedy
Status:	resolved (Comments)
Tags:
Comment:	Revert r74122, making r74117 live again Some refactoring, to encapsulate users. Added a UserWrapper, contains the setup username/password and user object. Allows for ease of adding of multiple users (and groups) Per Brion, if logging in user == wguser, we skip login tests, so use a secondary user Fixup testApiLoginBadPass, we should get a WrongPass when the passwords wrong, but we've given a token
Modified paths:	/trunk/phase3/maintenance/tests/phpunit/includes/api/ApiSetup.php (modified) (history) /trunk/phase3/maintenance/tests/phpunit/includes/api/ApiTest.php (modified) (history) /trunk/phase3/maintenance/tests/phpunit/includes/api/ApiWatchTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/tests/phpunit/includes/api/ApiTest.php
—	—	@@ -79,7 +79,7 @@
80	80	*/
81	81	function testApiLoginNoName() {
82	82	$data = $this->doApiRequest( array( 'action' => 'login',
83		~~- 'lgname' => '', 'lgpassword' => self::$passWord~~
	83	+ 'lgname' => '', 'lgpassword' => self::$user->password,
84	84	) );
85	85	$this->assertEquals( 'NoName', $data[0]['login']['result'] );
86	86	}
—	—	@@ -90,32 +90,35 @@
91	91	if ( !isset( $wgServer ) ) {
92	92	$this->markTestIncomplete( 'This test needs $wgServer to be set in LocalSettings.php' );
93	93	}
94		~~- $resp = Http::post( self::$apiUrl . "?action=login&format=xml",~~
95		~~- array( "postData" => array(~~
96		~~- "lgname" => self::$userName,~~
97		~~- "lgpassword" => "bad" ) ) );~~
98		~~- libxml_use_internal_errors( true );~~
99		~~- $sxe = simplexml_load_string( $resp );~~
100		~~- $this->assertNotType( "bool", $sxe );~~
101		~~- $this->assertThat( $sxe, $this->isInstanceOf( "SimpleXMLElement" ) );~~
102		~~- $a = $sxe->login[0]->attributes()->result[0];~~
103		~~- $this->assertEquals( ' result="NeedToken"', $a->asXML() );~~
	94	+ $ret = $this->doApiRequest( array(
	95	+ "action" => "login",
	96	+ "lgname" => self::$sysopUser->userName,
	97	+ "lgpassword" => "bad",
	98	+ )
	99	+ );
	100	+
	101	+ $result = $ret[0];
104	102
105		~~- $token = (string)$sxe->login[0]->attributes()->token;~~
	103	+ $this->assertNotType( "bool", $result );
	104	+ $a = $result["login"]["result"];
	105	+ $this->assertEquals( "NeedToken", $a );
106	106
107		~~- $resp = Http::post( self::$apiUrl . "?action=login&format=xml",~~
108		~~- array( "postData" => array(~~
109		~~- "lgtoken" => $token,~~
110		~~- "lgname" => self::$userName,~~
111		~~- "lgpassword" => "bad" ) ) );~~
	107	+ $token = $result["login"]["token"];
112	108
	109	+ $ret = $this->doApiRequest( array(
	110	+ "action" => "login",
	111	+ "lgtoken" => $token,
	112	+ "lgname" => self::$sysopUser->userName,
	113	+ "lgpassword" => "bad",
	114	+ )
	115	+ );
113	116
114		~~- $sxe = simplexml_load_string( $resp );~~
115		~~- $this->assertNotType( "bool", $sxe );~~
116		~~- $this->assertThat( $sxe, $this->isInstanceOf( "SimpleXMLElement" ) );~~
117		~~- $a = $sxe->login[0]->attributes()->result[0];~~
	117	+ $result = $ret[0];
118	118
119		~~- $this->assertEquals( ' result="NeedToken"', $a->asXML() );~~
	119	+ $this->assertNotType( "bool", $result );
	120	+ $a = $result["login"]["result"];
	121	+
	122	+ $this->assertEquals( "WrongPass", $a );
120	123	}
121	124
122	125	function testApiLoginGoodPass() {
—	—	@@ -127,12 +130,11 @@
128	131
129	132	$ret = $this->doApiRequest( array(
130	133	"action" => "login",
131		~~- "lgname" => self::$userName,~~
132		~~- "lgpassword" => self::$passWord,~~
	134	+ "lgname" => self::$user->userName,
	135	+ "lgpassword" => self::$user->password,
133	136	)
134	137	);
135	138
136		~~- libxml_use_internal_errors( true );~~
137	139	$result = $ret[0];
138	140	$this->assertNotType( "bool", $result );
139	141	$this->assertNotType( "null", $result["login"] );
—	—	@@ -144,8 +146,8 @@
145	147	$ret = $this->doApiRequest( array(
146	148	"action" => "login",
147	149	"lgtoken" => $token,
148		~~- "lgname" => self::$userName,~~
149		~~- "lgpassword" => self::$passWord,~~
	150	+ "lgname" => self::$user->userName,
	151	+ "lgpassword" => self::$user->password,
150	152	)
151	153	);
152	154
—	—	@@ -166,8 +168,8 @@
167	169	$req = HttpRequest::factory( self::$apiUrl . "?action=login&format=xml",
168	170	array( "method" => "POST",
169	171	"postData" => array(
170		~~- "lgname" => self::$userName,~~
171		~~- "lgpassword" => self::$passWord ) ) );~~
	172	+ "lgname" => self::$user->userName,
	173	+ "lgpassword" => self::$user->password ) ) );
172	174	$req->execute();
173	175
174	176	libxml_use_internal_errors( true );
—	—	@@ -182,8 +184,8 @@
183	185
184	186	$req->setData( array(
185	187	"lgtoken" => $token,
186		~~- "lgname" => self::$userName,~~
187		~~- "lgpassword" => self::$passWord ) );~~
	188	+ "lgname" => self::$user->userName,
	189	+ "lgpassword" => self::$user->password ) );
188	190	$req->execute();
189	191
190	192	$cj = $req->getCookieJar();
—	—	@@ -191,7 +193,7 @@
192	194	$this->assertNotEquals( false, $serverName );
193	195	$serializedCookie = $cj->serializeToHttpRequest( $wgScriptPath, $serverName );
194	196	$this->assertNotEquals( '', $serializedCookie );
195		~~- $this->assertRegexp( '/_session=[^;]; .UserID=[0-9]; .UserName=' . self::$userName . '; .*Token=/', $serializedCookie );~~
	197	+ $this->assertRegexp( '/_session=[^;]; .UserID=[0-9]; .UserName=' . self::$user->userName . '; .*Token=/', $serializedCookie );
196	198
197	199	return $cj;
198	200	}
Index: trunk/phase3/maintenance/tests/phpunit/includes/api/ApiSetup.php
—	—	@@ -1,9 +1,8 @@
2	2	<?php
3	3
4	4	abstract class ApiTestSetup extends PHPUnit_Framework_TestCase {
5		~~- protected static $userName;~~
6		~~- protected static $passWord;~~
7	5	protected static $user;
	6	+ protected static $sysopUser;
8	7	protected static $apiUrl;
9	8
10	9	function setUp() {
—	—	@@ -33,22 +32,12 @@
34	33	}
35	34
36	35	static function setupUser() {
37		~~- if ( self::$user == NULL ) {~~
38		~~- self::$userName = "Useruser";~~
39		~~- self::$passWord = 'Passpass';~~
40		-
41		~~- self::$user = User::newFromName( self::$userName );~~
42		~~- if ( !self::$user->getID() ) {~~
43		~~- self::$user = User::createNew( self::$userName, array(~~
44		~~- "email" => "test@example.com",~~
45		~~- "real_name" => "Test User" ) );~~
46		~~- }~~
47		~~- self::$user->setPassword( self::$passWord );~~
48		~~- self::$user->addGroup( 'sysop' );~~
49		~~- self::$user->saveSettings();~~
	36	+ if ( self::$user == NULL \|\| self::$sysopUser == NULL ) {
	37	+ self::$user = new UserWrapper( 'Useruser', 'Passpass' );
	38	+ self::$sysopUser = new UserWrapper( 'Useruser1', 'Passpass1', 'sysop' );
50	39	}
51	40
52		~~- $GLOBALS['wgUser'] = self::$user;~~
	41	+ $GLOBALS['wgUser'] = self::$user->user;
53	42	}
54	43
55	44	function tearDown() {
—	—	@@ -56,3 +45,25 @@
57	46	$wgMemc = null;
58	47	}
59	48	}
	49	+
	50	+class UserWrapper {
	51	+ var $userName, $password, $user;
	52	+
	53	+ public function __construct( $userName, $password, $group = '' ) {
	54	+ $this->userName = $userName;
	55	+ $this->password = $password;
	56	+
	57	+ $this->user = User::newFromName( $this->userName );
	58	+ if ( !$this->user->getID() ) {
	59	+ $this->user = User::createNew( $this->userName, array(
	60	+ "email" => "test@example.com",
	61	+ "real_name" => "Test User" ) );
	62	+ }
	63	+ $this->user->setPassword( $this->password );
	64	+
	65	+ if ( $group !== '' ) {
	66	+ $this->user->addGroup( $group );
	67	+ }
	68	+ $this->user->saveSettings();
	69	+ }
	70	+}
Index: trunk/phase3/maintenance/tests/phpunit/includes/api/ApiWatchTest.php
—	—	@@ -11,8 +11,8 @@
12	12	function testLogin() {
13	13	$data = $this->doApiRequest( array(
14	14	'action' => 'login',
15		~~- 'lgname' => self::$userName,~~
16		~~- 'lgpassword' => self::$passWord ) );~~
	15	+ 'lgname' => self::$user->userName,
	16	+ 'lgpassword' => self::$user->password ) );
17	17
18	18	$this->assertArrayHasKey( "login", $data[0] );
19	19	$this->assertArrayHasKey( "result", $data[0]['login'] );
—	—	@@ -22,8 +22,8 @@
23	23	$data = $this->doApiRequest( array(
24	24	'action' => 'login',
25	25	"lgtoken" => $token,
26		~~- "lgname" => self::$userName,~~
27		~~- "lgpassword" => self::$passWord ), $data );~~
	26	+ "lgname" => self::$user->userName,
	27	+ "lgpassword" => self::$user->password ), $data );
28	28
29	29	$this->assertArrayHasKey( "login", $data[0] );
30	30	$this->assertArrayHasKey( "result", $data[0]['login'] );
—	—	@@ -163,7 +163,7 @@
164	164	$data = $this->doApiRequest( array(
165	165	'action' => 'rollback',
166	166	'title' => 'Main Page',
167		~~- 'user' => self::$userName,~~
	167	+ 'user' => self::$user->userName,
168	168	'token' => $pageinfo['rollbacktoken'],
169	169	'watchlist' => 'watch' ), $data );
170	170	} catch( UsageException $ue ) {

Follow-up revisions

Revision	Commit summary	Author	Date
r74223	Fiuxp issues from r74213 CR...	reedy	16:32, 3 October 2010
r75588	Add feature to block common (weak) passwords....	platonides	22:26, 27 October 2010
r75589	Do NOT use hardcoded passwords! Not even if the user agreed to run destructiv...	platonides	22:27, 27 October 2010

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r74117	Fixup testApiLoginBadPass per r74113, still failing, it's checking for "NeedT...	reedy	22:27, 1 October 2010
r74122	Revert r74117. Breaks the test. Using an internal request it doesn't fail wit...	platonides	23:11, 1 October 2010

Comments

#Comment by Nikerabbit (talk | contribs) 16:25, 3 October 2010

+		if ( self::$user == NULL || self::$sysopUser == NULL ) {

Lowercase null is preferred.

+		$this->userName = $userName;
+	    $this->password = $password;

Inconsistent indentation.

+	var $userName, $password, $user;

Var is discouraged. protected should do fine here?

Status & tagging log

21:28, 3 October 2010 Brion VIBBER (talk | contribs) changed the status of r74213 [removed: new added: resolved]