Yurik has uploaded a new change for review.
https://gerrit.wikimedia.org/r/197384
Change subject: Unit tests and a number of minor bug fixes
......................................................................
Unit tests and a number of minor bug fixes
To run, from vagrant's /vagrant/mediawiki dir:
$ php tests/phpunit/phpunit.php
extensions/Gather/tests/phpunit/api/ApiQueryLists.php
Bug fixes:
* watchlist list row is no longer created unless action=editlist
tries to change something to non-default value
(description, privacy, etc)
* Security fix - incorrect check with ids= parameter allowed
access to private list under a certain condition
* float -> int for counts (apparent in unit tests, not output)
Change-Id: I392fd6e44c7fc4117fdab8c9c9dfe02fcdbfa2e1
---
M includes/api/ApiEditList.php
M includes/api/ApiQueryLists.php
A tests/phpunit/api/ApiQueryLists.php
3 files changed, 363 insertions(+), 18 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Gather
refs/changes/84/197384/1
diff --git a/includes/api/ApiEditList.php b/includes/api/ApiEditList.php
index 89c4b7e..42c1fdc 100644
--- a/includes/api/ApiEditList.php
+++ b/includes/api/ApiEditList.php
@@ -287,27 +287,43 @@
* @param $isWatchlist
*/
private function createRow( DatabaseBase $dbw, User $user, array
$params, &$isWatchlist ) {
- $id = $dbw->nextSequenceValue( 'gather_list_gl_id_seq' );
$label = $isWatchlist ? '' : $params['label'];
- $dbw->insert( 'gather_list', array(
- 'gl_id' => $id,
- 'gl_user' => $user->getId(),
- 'gl_label' => $label,
- 'gl_info' => $this->updateInfo( new stdClass(), $params
),
- ), __METHOD__, 'IGNORE' );
- $id = $dbw->insertId();
+ $info = $this->updateInfo( new stdClass(), $params );
+ $createRow = !$isWatchlist || $info;
+
+ if ( $createRow ) {
+ $id = $dbw->nextSequenceValue( 'gather_list_gl_id_seq'
);
+ $dbw->insert( 'gather_list', array(
+ 'gl_id' => $id,
+ 'gl_user' => $user->getId(),
+ 'gl_label' => $label,
+ 'gl_info' => $info,
+ ), __METHOD__, 'IGNORE' );
+ $id = $dbw->insertId();
+ } else {
+ $id = 0;
+ }
+
if ( $id === 0 ) {
- // Already exists, update instead
+ // List already exists, update instead, or might not
need it
$row = $dbw->selectRow( 'gather_list',
array( 'gl_id', 'gl_user', 'gl_label',
'gl_info' ),
array( 'gl_user' => $user->getId(), 'gl_label'
=> $label ),
__METHOD__
);
if ( $row === false ) {
- // If creation failed, the second query should
have succeeded
- $this->dieDebug( "List was not found", 'badid'
);
+ if ( $createRow ) {
+ // If creation failed, the second query
should have succeeded
+ $this->dieDebug( "List was not found",
'badid' );
+ }
+ $this->getResult()->addValue( null,
$this->getModuleName(), array(
+ 'status' => 'nochanges',
+ 'id' => 0,
+ ) );
+ } else {
+ $isWatchlist = $row->gl_label === '';
+ $this->updateListDb( $dbw, $params, $row );
}
- $this->updateListDb( $dbw, $params, $row );
} else {
$this->getResult()->addValue( null,
$this->getModuleName(), array(
'status' => 'created',
@@ -325,7 +341,7 @@
*/
private function updateListDb( DatabaseBase $dbw, array $params, $row )
{
$update = array();
- if ( $params['label'] && $row->gl_label !== $params['label'] ) {
+ if ( $params['label'] !== null && $row->gl_label !==
$params['label'] ) {
$update['gl_label'] = $params['label'];
}
$info = self::parseListInfo( $row->gl_info, $row->gl_id, true );
diff --git a/includes/api/ApiQueryLists.php b/includes/api/ApiQueryLists.php
index 33b8b6d..89e3d23 100644
--- a/includes/api/ApiQueryLists.php
+++ b/includes/api/ApiQueryLists.php
@@ -44,7 +44,17 @@
public function execute() {
$params = $this->extractRequestParams();
+
$ids = $params['ids'];
+ if ( $ids ) {
+ $findWatchlist = array_search( 0, $ids );
+ if( $findWatchlist !== false) {
+ unset( $ids[$findWatchlist] );
+ $findWatchlist = true;
+ }
+ } else {
+ $findWatchlist = false;
+ }
/** @var User $owner */
list( $owner, $showPrivate ) = $this->calcPermissions( $params,
$ids );
@@ -57,8 +67,15 @@
if ( $owner ) {
$this->addWhereFld( 'gl_user', $owner->getId() );
}
- if ( $ids ) {
- $this->addWhereFld( 'gl_id', $ids );
+ if ( $ids || $findWatchlist ) {
+ $cond = array();
+ if ( $ids ) {
+ $cond['gl_id'] = $ids;
+ }
+ if ( $findWatchlist ) {
+ $cond['gl_label'] = '';
+ }
+ $this->addWhere( $db->makeList( $cond, LIST_OR ) );
}
$continue = $params['continue'];
@@ -106,7 +123,7 @@
// Determine if this gather_list row is viewable by the
current user
if ( $showPrivate === false && !ApiEditList::isPublic(
$info ) ) {
return true;
- } elseif ( $showPrivate === null && $row->gl_user ===
$currUserId ) {
+ } elseif ( $showPrivate === null && $row->gl_user !==
$currUserId ) {
return true;
}
@@ -326,9 +343,9 @@
// TODO: estimateRowCount() might be much faster, TBD
if ok
$db = $this->getQuery()->getNamedDB( 'watchlist',
DB_SLAVE, 'watchlist' );
// Must divide in two because of duplicate talk pages
(same as the special page)
- $counts[$wlListId] = floor(
+ $counts[$wlListId] = intval( floor(
$db->selectRowCount( 'watchlist', '*', array(
'wl_user' => $wlUserId ),
- __METHOD__ ) / 2 );
+ __METHOD__ ) / 2 ) );
}
if ( count( $ids ) > 0 ) {
$db = $this->getDB();
diff --git a/tests/phpunit/api/ApiQueryLists.php
b/tests/phpunit/api/ApiQueryLists.php
new file mode 100644
index 0000000..c0bfeef
--- /dev/null
+++ b/tests/phpunit/api/ApiQueryLists.php
@@ -0,0 +1,312 @@
+<?php
+/**
+ *
+ * Created on Feb 6, 2013
+ *
+ * Copyright © 2013 Yuri Astrakhan "<Firstname><Lastname>@gmail.com"
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ */
+
+/**
+ * These tests validate basic functionality of the api query module
+ *
+ * @group API
+ * @group Database
+ * @group medium
+ * @covers ApiQuery
+ */
+class ApiQueryLists extends ApiQueryTestBase {
+ protected $exceptionFromAddDBData;
+ /** @var TestUser */
+ private static $noWlUser, $wlUser, $wlOnly;
+
+ static function initUsers() {
+ if ( !self::$wlUser ) {
+ self::$wlUser = new TestUser( 'GatherWL', 'GatherWL',
'[email protected]' );
+ self::$noWlUser = new TestUser( 'GatherNoWL',
'GatherNoWL', '[email protected]' );
+ self::$wlOnly = new TestUser( 'GatherNoLst',
'GatherNoLst', '[email protected]' );
+
+ User::createNew( self::$wlUser->getUser()->getName() );
+ User::createNew( self::$noWlUser->getUser()->getName()
);
+ User::createNew( self::$wlOnly->getUser()->getName() );
+ }
+ self::$users['GatherWL'] = self::$wlUser;
+ self::$users['GatherNoWL'] = self::$noWlUser;
+ self::$users['GatherNoLst'] = self::$wlOnly;
+ }
+
+ /**
+ * Create a set of pages. These must not change, otherwise the tests
might give wrong results.
+ * @see MediaWikiTestCase::addDBData()
+ */
+ public function addDBData() {
+ try {
+ if ( !Title::newFromText( 'Gather-All' )->exists() ) {
+ $this->editPage( 'Gather-ListA', 'a' );
+ $this->editPage( 'Gather-ListB', 'b' );
+ $this->editPage( 'Gather-ListAB', 'ab' );
+ $this->editPage( 'Gather-ListW', 'w' );
+ $this->editPage( 'Gather-ListWA', 'wa' );
+ $this->editPage( 'Gather-ListWAB', 'wab' );
+ }
+ } catch ( Exception $e ) {
+ $this->exceptionFromAddDBData = $e;
+ }
+ }
+
+ protected function setUp() {
+ parent::setUp();
+ self::initUsers();
+ }
+
+ public function testAnonymous() {
+ $a = User::newFromId( 0 );
+
+ $this->assertUsage( 'an-0', '{ "list": "lists" }', $a );
+ $this->assertUsage( 'an-1', '{ "list": "lists", "lstids": 0 }',
$a );
+ }
+
+ public function testProps() {
+ $wl = self::$wlUser->getUser();
+
+ $res = $this->callApi( 'p-a0', '{ "list": "lists" }', $wl );
+ $this->assertListsEquals( 'p-a0', $res, '[{"id":0,
"watchlist":true, "label":"Watchlist"}]' );
+
+ $token = $this->getToken( $wl );
+ $this->legacyAddToWatchlist( 'p-a1',
'Gather-ListW|Gather-ListWA|Gather-ListWAB', $wl, $token );
+ $res = $this->callApi( 'p-a1', '{ "list": "lists" }', $wl );
+ $this->assertListsEquals( 'p-a1', $res, '[{"id":0,
"watchlist":true, "label":"Watchlist"}]' );
+
+ $this->addToList( 'p-a2a', 'A', 'Gather-ListWA|Gather-ListWAB',
$wl, $token );
+ $this->addToList( 'p-a2b', 'B', 'Gather-ListWAB', $wl, $token );
+ $res = $this->callApi( 'p-a2', '{ "list": "lists" }', $wl );
+ $this->assertListsEquals( 'p-a2', $res,
+ '[{"id":0, "watchlist":true,"label":"Watchlist"},
{"label":"A"}, {"label":"B"}]' );
+ }
+
+ public function testWatchlistOnly() {
+ $u = self::$wlOnly->getUser();
+ $token = $this->getToken( $u );
+ $wlOnly = '[{"id":0, "watchlist":true, "label":"Watchlist"}]';
+
+ $res = $this->callApi( 'nc-a0', '{ "list": "lists" }', $u );
+ $this->assertListsEquals( 'nc-a0', $res, $wlOnly );
+
+ $res = $this->callApi( 'nc-a1', '{ "list": "lists", "lstids": 0
}', $u );
+ $this->assertListsEquals( 'nc-a1', $res, $wlOnly );
+
+ $res = $this->callApi( 'nc-a2', '{ "list": "lists", "lstlimit":
1 }', $u );
+ $this->assertListsEquals( 'nc-a2', $res, $wlOnly );
+
+ $res = $this->callApi( 'nc-a3',
+ '{ "list": "lists", "lstprop":
"label|description|public|count" }', $u );
+ $this->assertListsEquals( 'nc-a3',$res,
+
'[{"id":0,"watchlist":true,"count":0,"label":"Watchlist","description":"","public":false}]'
+ );
+
+
+
+ $this->legacyAddToWatchlist( 'nc-b0', 'Gather-ListW', $u,
$token );
+ $res = $this->callApi( 'nc-b0', '{ "list": "lists", "lstprop":
"count" }', $u );
+ $this->assertListsEquals( 'nc-b0', $res, '[{"id": 0,
"watchlist":true, "count": 1}]' );
+
+
+ $this->addToList( 'nc-c0', 0, 'Gather-ListW', $u, $token );
+ $res = $this->callApi( 'nc-c0', '{ "list": "lists" }', $u );
+ $this->assertListsEquals( 'nc-c0', $res, $wlOnly );
+
+ $res = $this->callApi( 'nc-c1', '{ "list": "lists", "lstids": 0
}', $u );
+ $this->assertListsEquals( 'nc-c1', $res, $wlOnly );
+
+ $res = $this->callApi( 'nc-c3', '{ "list": "lists", "lstprop":
"count" }', $u );
+ $this->assertListsEquals( 'nc-c3', $res, '[{"id":0,
"watchlist":true, "count": 1}]' );
+
+
+ // What can others see from this user
+ $n = $u->getName();
+ $u2 = self::$wlUser->getUser();
+
+ $res = $this->callApi( 'nc-e0', '{ "list": "lists", "lstowner":
"' . $n . '" }', $u2 );
+ $this->assertListsEquals( 'nc-e0', $res, '[]' );
+
+ $res = $this->callApi( 'nc-e1', '{ "list": "lists", "lstowner":
"' . $n .
+ '", "lstids": 0 }', $u2 );
+ $this->assertListsEquals( 'nc-e1', $res, '[]' );
+
+
+ // Create watchlist list DB record
+ $res = $this->updateList( 'nc-f0', '{ "id":0,
"description":"aa" }', $u, $token );
+ $this->assertEquals( 'created', $res['status'], 'nc-f0' );
+ $this->assertNotEquals( 0, $res['id'], 'nc-f1' );
+ $id = $res['id'];
+ $wlOnly = array( array( 'id' => $id, 'watchlist' => true,
'label' => 'Watchlist' ) );
+
+ $res = $this->callApi( 'nc-f2', '{ "list": "lists" }', $u );
+ $this->assertListsEquals( 'nc-f2', $res, $wlOnly, false );
+
+ $res = $this->callApi( 'nc-f3', '{ "list": "lists", "lstids": 0
}', $u );
+ $this->assertListsEquals( 'nc-f3', $res, $wlOnly, false );
+
+ $res = $this->callApi( 'nc-f4',
+ '{ "list": "lists", "lstprop":
"label|description|public|count" }', $u );
+ $this->assertListsEquals( 'nc-f4',$res,
+ '[{"id":' . $id .
+
',"watchlist":true,"count":1,"label":"Watchlist","description":"aa","public":false}]',
+ false );
+
+
+ // Others still can't see the watchlist
+ $res = $this->callApi( 'nc-g0', '{ "list": "lists", "lstowner":
"' . $n . '" }', $u2 );
+ $this->assertListsEquals( 'nc-g0', $res, '[]' );
+
+ $res = $this->callApi( 'nc-g1', '{ "list": "lists", "lstowner":
"' . $n .
+ '", "lstids": 0 }', $u2 );
+ $this->assertListsEquals( 'nc-g1', $res, '[]' );
+
+
+ $res = $this->callApi( 'nc-g2', '{ "list": "lists", "lstids": '
. $id . ' }', $u2 );
+ $this->assertListsEquals( 'nc-g2', $res, '[]' );
+
+ $res = $this->callApi( 'nc-g3', '{ "list": "lists", "lstowner":
"' . $n . '", "lstids": ' .
+ $id . ' }', $u2 );
+ $this->assertListsEquals( 'nc-g3', $res, '[]' );
+
+ $this->assertUsage( 'nc-i0', '{ "action": "editlist", "id":0,
"label":"bb" }', $u );
+ $this->assertUsage( 'nc-i1', '{ "action": "editlist", "id":' .
$id . ', "label":"bb" }', $u );
+ }
+
+ private function assertListsEquals( $message, $actual, $expected,
$removeIds = true ) {
+ $actual = $this->checkResult( $message, $message, '"query",
"lists"', $actual );
+ $expected = $this->toArray( $message, $expected );
+ if ( $removeIds ) {
+ $actual = self::removeIds( $actual );
+ }
+ $this->assertArrayEquals( $expected, $actual, true, true,
$message );
+ }
+
+ private function updateList( $message, $params, $user, $token ) {
+ $params = $this->toArray( $message, $params );
+ if ( !isset( $params['action'] ) ) {
+ $params['action'] = 'editlist';
+ }
+ if ( !isset( $params['token'] ) ) {
+ $params['token'] = $token;
+ }
+ return $this->callApi( $message, $params, $user, array(
$params['action'] ) );
+ }
+
+ private function addToList( $message, $label, $titles, $user, $token ) {
+ $params = array(
+ 'action' => 'editlist',
+ 'titles' => $titles,
+ 'token' => $token,
+ );
+ $params[is_string( $label ) ? 'label' : 'id'] = $label;
+ $this->callApi( $message, $params, $user, '"editlist", "pages",
0, "added"' );
+ }
+
+ private function legacyAddToWatchlist( $message, $titles, $user, $token
) {
+ $params = array(
+ 'action' => 'watch',
+ 'titles' => $titles,
+ 'token' => $token,
+ );
+ $this->callApi( $message, $params, $user, '"watch", 0,
"watched"' );
+ }
+
+ private function getToken( User $user ) {
+ return $this->callApi( 'token-' . $user->getName(), array(
+ 'meta' => 'tokens',
+ 'type' => 'watch',
+ ), $user, '"query", "tokens", "watchtoken"' );
+ }
+
+ private function assertUsage( $message, $params, User $user = null ) {
+ try {
+ $params = $this->toApiParams( $message, $params );
+ $result = $this->callApi( $message, $params, $user );
+ $params = $this->toStr( $params );
+ $this->fail( "No UsageException for $params,
received:\n" .
+ $this->toStr( $result[0], true ) );
+ } catch ( UsageException $e ) {
+ $this->assertTrue( true );
+ }
+ }
+
+ private function callApi( $message, $params, User $user = null, $path =
'' ) {
+ $params = $this->toApiParams( $message, $params );
+ $result = $this->doApiRequest( $params, null, false, $user );
+ return $path ? $this->checkResult( $message, $params, $path,
$result ) : $result;
+ }
+
+ private function toApiParams( $message, $params ) {
+ $params = $this->toArray( $message, $params );
+ if ( !isset( $params['action'] ) ) {
+ $params['action'] = 'query';
+ }
+ if ( $params['action'] === 'query' && !isset(
$params['continue'] ) ) {
+ $params['continue'] = '';
+ }
+ return $params;
+ }
+
+ private function toArray( $message, $params ) {
+ if ( is_string( $params ) && $params ) {
+ $p = $params[0] !== '[' && $params[0] !== '{' ?
"[$params]" : $params;
+ $st = FormatJson::parse( $p, FormatJson::FORCE_ASSOC );
+ $this->assertTrue( $st->isOK(), 'invalid JSON value ' .
$params, $message );
+ $params = $st->getValue();
+ return $params;
+ }
+ return $params;
+ }
+
+ private function toStr( $params, $pretty = false ) {
+ if ( is_string( $params ) ) {
+ return $params;
+ }
+ return FormatJson::encode( $params, $pretty, FormatJson::ALL_OK
);
+ }
+
+ private static function removeIds( $arr ) {
+ foreach ( $arr as &$v ) {
+ if ( array_key_exists( 'id', $v ) && $v['id'] !== 0 ) {
+ unset( $v['id'] );
+ }
+ }
+ return $arr;
+ }
+
+ private function checkResult( $message, $params, $path, $result ) {
+ $path = $this->toArray( $message, $path );
+ array_unshift( $path, 0 );
+ $res = $result;
+ foreach ( $path as $p ) {
+ if ( !array_key_exists( $p, $res ) ) {
+ $params = $this->toStr( $params );
+ $path = $this->toStr( $path );
+ $this->fail( "$message: Request $params has no
key $p of $path in result\n" .
+ $this->toStr( $result,
true ) );
+ }
+ $res = $res[$p];
+ }
+ $result = $res;
+ return $result;
+ }
+}
--
To view, visit https://gerrit.wikimedia.org/r/197384
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I392fd6e44c7fc4117fdab8c9c9dfe02fcdbfa2e1
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Gather
Gerrit-Branch: master
Gerrit-Owner: Yurik <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
