r89545 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r89544‎ | r89545 | r89546 >
Date:00:09, 6 June 2011
Author:krinkle
Status:resolved
Tags:
Comment:
WatchAction requires token (BREAKING CHANGE)
* (bug 27655) Require token for watching/unwatching pages
* Previously done for API (bug 29070) in r88522
* As with markpatrolled, the tokens are not compatible and made that way on purpose. The API requires the POST method and uses a universal token per-session. Since the front-end is all GET based (also per convention like in markpatrolled and rollback) they are stronger salted (title / action specific)
* ajax.watch used the API already and was switched in r88554.
* The actual watching/unwatching code was moved from WatchAction->onView to WatchAction::doWatch. This was done to allow the API to do the action without needing to generate a token like the front-end needs (or having to duplicate code). It is now similar to RecentChange::markPatrolled (in that it also a "central" function that does not care about tokens, it's called after the token-handling)
* JavaScript / Gadgets that utilize action=watch in their scripts:
** Effects should be minimal as they should be using the API (see r88522 and wikitech-l)
** If they use index.php and scrap the link from the page, they can continue to do so.

* There are links to the watch action all over the place. I've tried to catch most of them, but there may be some I miss. Migration in most cases is just a matter of adding an array item to the $query for:
'token' => WatchAction::getWatchToken( $title, $user [, $action] )
or changing:
Action::factory( 'watch', $article )->execute();
to:
WatchAction::doWatch( $title, $user );

While replacing the usages in some cases an instance of Article() no longer had to be created, in others $wgUser had to be retrieved from global (which was implied before but needs to be given directly now)

Other notes:
* Article->unwatch() and Article->watch(), which were deprecated as of 1.18 and are no longer used in core, may be broken in scenarios where the Request does not have a 'token' but is making a call to $article->watch()
* Some extensions need to be fixed, I'm currently running a grep search and will fix them a.s.a.p


[1] http://www.mediawiki.org/wiki/ResourceLoader/Default_modules?mw.user#tokens
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/EditPage.php (modified) (history)
  • /trunk/phase3/includes/FileDeleteForm.php (modified) (history)
  • /trunk/phase3/includes/ProtectionForm.php (modified) (history)
  • /trunk/phase3/includes/SkinLegacy.php (modified) (history)
  • /trunk/phase3/includes/SkinTemplate.php (modified) (history)
  • /trunk/phase3/includes/actions/DeleteAction.php (modified) (history)
  • /trunk/phase3/includes/actions/WatchAction.php (modified) (history)
  • /trunk/phase3/includes/api/ApiBase.php (modified) (history)
  • /trunk/phase3/includes/api/ApiWatch.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/ProtectionForm.php
——@@ -318,10 +318,10 @@
319319 return false;
320320 }
321321
322 - if( $wgRequest->getCheck( 'mwProtectWatch' ) && $wgUser->isLoggedIn() ) {
323 - Action::factory( 'watch', $this->mArticle )->execute();
324 - } elseif( $this->mTitle->userIsWatching() ) {
325 - Action::factory( 'unwatch', $this->mArticle )->execute();
 322+ if ( $wgRequest->getCheck( 'mwProtectWatch' ) && $wgUser->isLoggedIn() ) {
 323+ WatchAction::doWatch( $this->mTitle, $wgUser );
 324+ } elseif ( $this->mTitle->userIsWatching() ) {
 325+ WatchAction::doUnwatch( $this->mTitle, $wgUser );
326326 }
327327 return $ok;
328328 }
Index: trunk/phase3/includes/Article.php
——@@ -2382,7 +2382,8 @@
23832383 }
23842384
23852385 /**
2386 - * User-interface handler for the "watch" action
 2386+ * User-interface handler for the "watch" action.
 2387+ * Requires Request to pass a token as of 1.19.
23872388 * @deprecated since 1.18
23882389 */
23892390 public function watch() {
——@@ -2398,11 +2399,13 @@
23992400 * @deprecated since 1.18
24002401 */
24012402 public function doWatch() {
2402 - return Action::factory( 'watch', $this )->execute();
 2403+ global $wgUser;
 2404+ return WatchAction:doWatch( $this->mTitle, $wgUser );
24032405 }
24042406
24052407 /**
24062408 * User interface handler for the "unwatch" action.
 2409+ * Requires Request to pass a token as of 1.19.
24072410 * @deprecated since 1.18
24082411 */
24092412 public function unwatch() {
——@@ -2415,7 +2418,8 @@
24162419 * @deprecated since 1.18
24172420 */
24182421 public function doUnwatch() {
2419 - return Action::factory( 'unwatch', $this )->execute();
 2422+ global $wgUser;
 2423+ return WatchAction:doUnwatch( $this->mTitle, $wgUser );
24202424 }
24212425
24222426 /**
Index: trunk/phase3/includes/EditPage.php
——@@ -1176,13 +1176,14 @@
11771177 * Commit the change of watch status
11781178 */
11791179 protected function commitWatch() {
 1180+ global $wgUser;
11801181 if ( $this->watchthis xor $this->mTitle->userIsWatching() ) {
11811182 $dbw = wfGetDB( DB_MASTER );
11821183 $dbw->begin();
11831184 if ( $this->watchthis ) {
1184 - Action::factory( 'watch', $this->mArticle )->execute();
 1185+ WatchAction::doWatch( $this->mTitle, $wgUser );
11851186 } else {
1186 - Action::factory( 'unwatch', $this->mArticle )->execute();
 1187+ WatchAction::doUnwatch( $this->mTitle, $wgUser );
11871188 }
11881189 $dbw->commit();
11891190 }
Index: trunk/phase3/includes/actions/DeleteAction.php
——@@ -1,6 +1,6 @@
22 <?php
33 /**
4 - * Performs the watch and unwatch actions on a page
 4+ * Performs the delete action on a page
55 *
66 * This program is free software; you can redistribute it and/or modify
77 * it under the terms of the GNU General Public License as published by
——@@ -196,9 +196,9 @@
197197 public function onSuccess(){
198198 // Watch or unwatch, if requested
199199 if( $this->getRequest()->getCheck( 'wpWatch' ) && $this->getUser()->isLoggedIn() ) {
200 - Action::factory( 'watch', $this->page )->execute();
 200+ WatchAction::doWatch( $this->getTitle(), $this->getUser() );
201201 } elseif ( $this->getTitle()->userIsWatching() ) {
202 - Action::factory( 'unwatch', $this->page )->execute();
 202+ WatchAction::doUnwatch( $this->getTitle(), $this->getUser() );
203203 }
204204
205205 $this->getOutput()->setPagetitle( wfMsg( 'actioncomplete' ) );
Index: trunk/phase3/includes/actions/WatchAction.php
——@@ -39,9 +39,20 @@
4040 }
4141
4242 protected function checkCanExecute( User $user ) {
 43+
 44+ // Must be logged in
4345 if ( $user->isAnon() ) {
4446 throw new ErrorPageError( 'watchnologin', 'watchnologintext' );
4547 }
 48+
 49+ // Must have valid token for this action/title
 50+ $salt = array( $this->getName(), $this->getTitle()->getDBkey() );
 51+
 52+ if ( !$user->matchEditToken( $this->getRequest()->getVal( 'token' ), $salt ) ) {
 53+ throw new ErrorPageError( 'sessionfailure-title', 'sessionfailure' );
 54+ return;
 55+ }
 56+
4657 return parent::checkCanExecute( $user );
4758 }
4859
——@@ -49,15 +60,66 @@
5061 wfProfileIn( __METHOD__ );
5162
5263 $user = $this->getUser();
53 - if ( wfRunHooks( 'WatchArticle', array( &$user, &$this->page ) ) ) {
54 - $this->getUser()->addWatch( $this->getTitle() );
55 - wfRunHooks( 'WatchArticleComplete', array( &$user, &$this->page ) );
56 - }
 64+ self::doWatch( $this->getTitle(), $user );
5765
5866 wfProfileOut( __METHOD__ );
5967
6068 return wfMessage( 'addedwatchtext', $this->getTitle()->getPrefixedText() )->parse();
6169 }
 70+
 71+ public static function doWatch( Title $title, User $user ) {
 72+ $page = new Article( $title );
 73+
 74+ if ( wfRunHooks( 'WatchArticle', array( &$user, &$page ) ) ) {
 75+ $user->addWatch( $title );
 76+ wfRunHooks( 'WatchArticleComplete', array( &$user, &$page ) );
 77+ }
 78+ return true;
 79+ }
 80+
 81+ public static function doUnwatch( Title $title, User $user ) {
 82+ $page = new Article( $title );
 83+
 84+ if ( wfRunHooks( 'UnwatchArticle', array( &$user, &$page ) ) ) {
 85+ $user->removeWatch( $title );
 86+ wfRunHooks( 'UnwatchArticleComplete', array( &$user, &$page ) );
 87+ }
 88+ return true;
 89+ }
 90+
 91+ /**
 92+ * Get token to watch (or unwatch) a page for a user
 93+ *
 94+ * @param Title $title Title object of page to watch
 95+ * @param User $title User for whom the action is going to be performed
 96+ * @param string $action Optionally override the action to 'unwatch'
 97+ * @return string Token
 98+ * @since 1.19
 99+ */
 100+ public static function getWatchToken( Title $title, User $user, $action = 'watch' ) {
 101+ if ( $action != 'unwatch' ) {
 102+ $action = 'watch';
 103+ }
 104+ $salt = array( $action, $title->getDBkey() );
 105+
 106+ // This token stronger salted and not compatible with ApiWatch
 107+ // It's title/action specific because index.php is GET and API is POST
 108+ return $user->editToken( $salt );
 109+ }
 110+
 111+ /**
 112+ * Get token to unwatch (or watch) a page for a user
 113+ *
 114+ * @param Title $title Title object of page to unwatch
 115+ * @param User $title User for whom the action is going to be performed
 116+ * @param string $action Optionally override the action to 'watch'
 117+ * @return string Token
 118+ * @since 1.19
 119+ */
 120+ public static function getUnwatchToken( Title $title, User $user, $action = 'unwatch' ) {
 121+ return self::getWatchToken( $title, $user, $action );
 122+ }
 123+
62124 }
63125
64126 class UnwatchAction extends WatchAction {
——@@ -74,10 +136,7 @@
75137 wfProfileIn( __METHOD__ );
76138
77139 $user = $this->getUser();
78 - if ( wfRunHooks( 'UnwatchArticle', array( &$user, &$this->page ) ) ) {
79 - $this->getUser()->removeWatch( $this->getTitle() );
80 - wfRunHooks( 'UnwatchArticleComplete', array( &$user, &$this->page ) );
81 - }
 140+ self::doUnwatch( $this->getTitle(), $user );
82141
83142 wfProfileOut( __METHOD__ );
84143
Index: trunk/phase3/includes/api/ApiWatch.php
——@@ -59,11 +59,11 @@
6060 if ( $params['unwatch'] ) {
6161 $res['unwatched'] = '';
6262 $res['message'] = wfMsgExt( 'removedwatchtext', array( 'parse' ), $title->getPrefixedText() );
63 - $success = Action::factory( 'unwatch', $article )->execute();
 63+ $success = WatchAction::doWatch( $title, $wgUser );
6464 } else {
6565 $res['watched'] = '';
6666 $res['message'] = wfMsgExt( 'addedwatchtext', array( 'parse' ), $title->getPrefixedText() );
67 - $success = Action::factory( 'watch', $article )->execute();
 67+ $success = UnwatchAction::doUnwatch( $title, $wgUser );
6868 }
6969 if ( !$success ) {
7070 $this->dieUsageMsg( 'hookaborted' );
Index: trunk/phase3/includes/api/ApiBase.php
——@@ -645,17 +645,17 @@
646646 * @param $titleObj Title the article's title to change
647647 * @param $userOption String The user option to consider when $watch=preferences
648648 */
649 - protected function setWatch ( $watch, $titleObj, $userOption = null ) {
 649+ protected function setWatch( $watch, $titleObj, $userOption = null ) {
650650 $value = $this->getWatchlistValue( $watch, $titleObj, $userOption );
651651 if ( $value === null ) {
652652 return;
653653 }
654654
655 - $articleObj = new Article( $titleObj );
 655+ global $wgUser;
656656 if ( $value ) {
657 - Action::factory( 'watch', $articleObj )->execute();
 657+ WatchAction::doWatch( $titleObj, $wgUser );
658658 } else {
659 - Action::factory( 'unwatch', $articleObj )->execute();
 659+ WatchAction::doUnwatch( $titleObj, $wgUser );
660660 }
661661 }
662662
Index: trunk/phase3/includes/FileDeleteForm.php
——@@ -123,10 +123,10 @@
124124 // delete the associated article first
125125 if( $article->doDeleteArticle( $reason, $suppress, $id, false ) ) {
126126 global $wgRequest;
127 - if( $wgRequest->getCheck( 'wpWatch' ) && $wgUser->isLoggedIn() ) {
128 - Action::factory( 'watch', $article )->execute();
129 - } elseif( $title->userIsWatching() ) {
130 - Action::factory( 'unwatch', $article )->execute();
 127+ if ( $wgRequest->getCheck( 'wpWatch' ) && $wgUser->isLoggedIn() ) {
 128+ WatchAction::doWatch( $title, $wgUser );
 129+ } elseif ( $title->userIsWatching() ) {
 130+ WatchAction::doUnwatch( $title, $wgUser );
131131 }
132132 $status = $file->delete( $reason, $suppress );
133133 if( $status->ok ) {
Index: trunk/phase3/includes/SkinLegacy.php
——@@ -672,22 +672,31 @@
673673 }
674674
675675 function watchThisPage() {
676 - global $wgOut;
 676+ global $wgOut, $wgUser;
677677 ++$this->mWatchLinkNum;
678678
 679+ // Cache
 680+ $title = $this->getSkin()->getTitle();
 681+
679682 if ( $wgOut->isArticleRelated() ) {
680 - if ( $this->getSkin()->getTitle()->userIsWatching() ) {
 683+ if ( $title->userIsWatching() ) {
681684 $text = wfMsg( 'unwatchthispage' );
682 - $query = array( 'action' => 'unwatch' );
 685+ $query = array(
 686+ 'action' => 'unwatch',
 687+ 'token' => UnwatchAction::getUnwatchToken( $title, $wgUser ),
 688+ );
683689 $id = 'mw-unwatch-link' . $this->mWatchLinkNum;
684690 } else {
685691 $text = wfMsg( 'watchthispage' );
686 - $query = array( 'action' => 'watch' );
 692+ $query = array(
 693+ 'action' => 'watch',
 694+ 'token' => WatchAction::getWatchToken( $title, $wgUser ),
 695+ );
687696 $id = 'mw-watch-link' . $this->mWatchLinkNum;
688697 }
689698
690699 $s = $this->getSkin()->link(
691 - $this->getSkin()->getTitle(),
 700+ $title,
692701 $text,
693702 array( 'id' => $id ),
694703 $query,
Index: trunk/phase3/includes/SkinTemplate.php
——@@ -1015,10 +1015,11 @@
10161016 * the global versions.
10171017 */
10181018 $mode = $title->userIsWatching() ? 'unwatch' : 'watch';
 1019+ $token = WatchAction::getWatchToken( $title, $wgUser, $mode );
10191020 $content_navigation['actions'][$mode] = array(
10201021 'class' => $onPage && ( $action == 'watch' || $action == 'unwatch' ) ? 'selected' : false,
10211022 'text' => wfMsg( $mode ), // uses 'watch' or 'unwatch' message
1022 - 'href' => $title->getLocalURL( 'action=' . $mode )
 1023+ 'href' => $title->getLocalURL( array( 'action' => $mode, 'token' => $token ) )
10231024 );
10241025 }
10251026

Follow-up revisions

RevisionCommit summaryAuthorDate
r89546Fix syntax error (Follow-up r89545)krinkle00:12, 6 June 2011
r89547Use tokens where needed for WatchAction (as of r89545)...krinkle00:32, 6 June 2011
r89604Release notes for bug 27655 (r89545 and related revs)krinkle20:52, 6 June 2011
r90041FU r89545: change broke api watching by switching watch and unwatch around....nikerabbit13:10, 14 June 2011
r94630Followup r89545: add ,0 to Article constructor callcatrope12:48, 16 August 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r88522* (bug 29070) Add token to action=watchreedy16:38, 21 May 2011
r88554Passing token paremeter in mw.action.watch.ajax since this is required as of ...krinkle23:14, 21 May 2011

Status & tagging log