r64826 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r64825‎ | r64826 | r64827 >
Date:20:05, 9 April 2010
Author:reedy
Status:resolved (Comments)
Tags:
Comment:
Refactor code in ApiRollback to fix bug 23117

Rollback uses salted token, but as this is dealt with in the Article object, it was missed by myself when refactoring API code

Refactor code to generate salt, and save variables as needed (so not needlessly regenerated)

Hence fix for r62482 and r62557
Modified paths:
  • /trunk/phase3/includes/api/ApiRollback.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiRollback.php
——@@ -35,35 +35,15 @@
3636 public function __construct( $main, $action ) {
3737 parent::__construct( $main, $action );
3838 }
 39+
 40+ private $mTitleObj = null;
3941
4042 public function execute() {
4143 $params = $this->extractRequestParams();
4244
43 - $titleObj = null;
44 - if ( !isset( $params['title'] ) ) {
45 - $this->dieUsageMsg( array( 'missingparam', 'title' ) );
46 - }
47 - if ( !isset( $params['user'] ) ) {
48 - $this->dieUsageMsg( array( 'missingparam', 'user' ) );
49 - }
 45+ // User and title already validated in call to getTokenSalt from Main
5046
51 - $titleObj = Title::newFromText( $params['title'] );
52 - if ( !$titleObj ) {
53 - $this->dieUsageMsg( array( 'invalidtitle', $params['title'] ) );
54 - }
55 - if ( !$titleObj->exists() ) {
56 - $this->dieUsageMsg( array( 'notanarticle' ) );
57 - }
58 -
59 - // We need to be able to revert IPs, but getCanonicalName rejects them
60 - $username = User::isIP( $params['user'] )
61 - ? $params['user']
62 - : User::getCanonicalName( $params['user'] );
63 - if ( !$username ) {
64 - $this->dieUsageMsg( array( 'invaliduser', $params['user'] ) );
65 - }
66 -
67 - $articleObj = new Article( $titleObj );
 47+ $articleObj = new Article( $this->mTitleObj );
6848 $summary = ( isset( $params['summary'] ) ? $params['summary'] : '' );
6949 $details = null;
7050 $retval = $articleObj->doRollback( $username, $summary, $params['token'], $params['markbot'], $details );
——@@ -73,7 +53,7 @@
7454 $this->dieUsageMsg( reset( $retval ) );
7555 }
7656
77 - $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj );
 57+ $watch = $this->getWatchlistValue( $params['watchlist'], $this->mTitleObj );
7858
7959 if ( $watch !== null) {
8060 if ( $watch ) {
——@@ -84,7 +64,7 @@
8565 }
8666
8767 $info = array(
88 - 'title' => $titleObj->getPrefixedText(),
 68+ 'title' => $this->mTitleObj->getPrefixedText(),
8969 'pageid' => intval( $details['current']->getPage() ),
9070 'summary' => $details['summary'],
9171 'revid' => intval( $details['newid'] ),
——@@ -151,7 +131,33 @@
152132 }
153133
154134 public function getTokenSalt() {
155 - return '';
 135+ $params = $this->extractRequestParams();
 136+
 137+ if ( !isset( $params['user'] ) ) {
 138+ $this->dieUsageMsg( array( 'missingparam', 'user' ) );
 139+ }
 140+
 141+ // We need to be able to revert IPs, but getCanonicalName rejects them
 142+ $this->username = User::isIP( $params['user'] )
 143+ ? $params['user']
 144+ : User::getCanonicalName( $params['user'] );
 145+ if ( !$this->username ) {
 146+ $this->dieUsageMsg( array( 'invaliduser', $params['user'] ) );
 147+ }
 148+
 149+ if ( !isset( $params['title'] ) ) {
 150+ $this->dieUsageMsg( array( 'missingparam', 'title' ) );
 151+ }
 152+
 153+ $this->mTitleObj = Title::newFromText( $params['title'] );
 154+ if ( !$this->mTitleObj ) {
 155+ $this->dieUsageMsg( array( 'invalidtitle', $params['title'] ) );
 156+ }
 157+ if ( !$this->mTitleObj->exists() ) {
 158+ $this->dieUsageMsg( array( 'notanarticle' ) );
 159+ }
 160+
 161+ return array( $this->mTitleObj->getPrefixedText(), $this->username );
156162 }
157163
158164 protected function getExamples() {

Follow-up revisions

RevisionCommit summaryAuthorDate
r648681.16wmf4: Commit live hack for bug 23117catrope12:08, 10 April 2010
r65371(bug 23117) Fix totally broken action=rollback, by doing a temporary hack alo...tstarling10:41, 21 April 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r62482Start of "Bug 21991 - Move common query parameter (uc, rc) validation, token...reedy22:20, 14 February 2010
r62557Refactor requiresToken to getTokenSalt - Returns salt if exists, null if no s...reedy23:53, 15 February 2010

Comments

#Comment by Catrope (talk | contribs)   12:07, 10 April 2010

I don't like the way this relies on calling order to set $this->mTitleObj and $this->username. I feel functions like getTitle() that cache their return value would make for cleaner code.

#Comment by Reedy (talk | contribs)   12:28, 10 April 2010

Mmm

Need to do something similar then for Userrights iirc

#Comment by Reedy (talk | contribs)   12:28, 10 April 2010

Mmm

Need to do something similar then for Userrights iirc

#Comment by Reedy (talk | contribs)   12:35, 10 April 2010

Something like (excuse the horrendous formatting)

private $mTitleObj = null;

private void getTitle() {

if ( $this->mTitle !== null ) { return $this->mTitle; }

$params = $this->extractRequestParams();

+ if ( !isset( $params['title'] ) ) { + $this->dieUsageMsg( array( 'missingparam', 'title' ) ); + } + + $this->mTitleObj = Title::newFromText( $params['title'] ); + if ( !$this->mTitleObj ) { + $this->dieUsageMsg( array( 'invalidtitle', $params['title'] ) ); + } + if ( !$this->mTitleObj->exists() ) { + $this->dieUsageMsg( array( 'notanarticle' ) ); + }

return $mTitle;

}


Which could almost go into base..

#Comment by Catrope (talk | contribs)   12:37, 10 April 2010

Sounds good.

#Comment by Catrope (talk | contribs)   10:06, 21 April 2010

This was done in r64875.

#Comment by Reedy (talk | contribs)   11:18, 9 June 2010

Is this still broken? I thought i'd fixed it up...

#Comment by Catrope (talk | contribs)   13:02, 9 June 2010

Yes, was fixed, marking as resolved.

Status & tagging log