r70440 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r70439‎ | r70440 | r70441 >
Date:00:05, 4 August 2010
Author:kaldari
Status:resolved (Comments)
Tags:
Comment:
Fix for form issues
Modified paths:
  • /trunk/extensions/CentralNotice/SpecialCentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/SpecialNoticeTemplate.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/SpecialCentralNotice.php
——@@ -42,8 +42,8 @@
4343
4444 $method = $wgRequest->getVal( 'method' );
4545 // Handle form sumissions
46 - if ( $this->editable && $wgRequest->wasPosted() ) {
47 -
 46+ if ( $this->editable && $wgRequest->wasPosted() && $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
 47+
4848 // Handle removing
4949 $toRemove = $wgRequest->getArray( 'removeNotices' );
5050 if ( isset( $toRemove ) ) {
——@@ -185,28 +185,26 @@
186186
187187 // Handle adding of notice
188188 $this->showAll = $wgRequest->getVal( 'showAll' );
189 - if ( $this->editable && $method == 'addNotice' ) {
 189+ if ( $this->editable && $method == 'addNotice' && $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
190190 $noticeName = $wgRequest->getVal( 'noticeName' );
191191 $start = $wgRequest->getArray( 'start' );
192192 $project_name = $wgRequest->getVal( 'project_name' );
193193 $project_languages = $wgRequest->getArray( 'project_languages' );
194194 if ( $noticeName == '' ) {
195 - //$wgOut->addWikiMsg ( 'centralnotice-null-string' );
196195 $wgOut->addHTML( Xml::element( 'div', array( 'class' => 'cn-error' ), wfMsg( 'centralnotice-null-string' ) ) );
197 - }
198 - else {
 196+ } else {
199197 $this->addNotice( $noticeName, '0', $start, $project_name, $project_languages );
200198 }
201199 }
202200
203201 // Handle removing of notice
204 - if ( $this->editable && $method == 'removeNotice' ) {
 202+ if ( $this->editable && $method == 'removeNotice' && $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
205203 $noticeName = $wgRequest->getVal ( 'noticeName' );
206204 $this->removeNotice ( $noticeName );
207205 }
208206
209207 // Handle adding of template
210 - if ( $this->editable && $method == 'addTemplateTo' ) {
 208+ if ( $this->editable && $method == 'addTemplateTo' && $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
211209 $noticeName = $wgRequest->getVal( 'noticeName' );
212210 $templateName = $wgRequest->getVal( 'templateName' );
213211 $templateWeight = $wgRequest->getVal ( 'weight' );
——@@ -217,7 +215,7 @@
218216 }
219217
220218 // Handle removing of template
221 - if ( $this->editable && $method == 'removeTemplateFor' ) {
 219+ if ( $this->editable && $method == 'removeTemplateFor' && $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
222220 $noticeName = $wgRequest->getVal ( 'noticeName' );
223221 $templateName = $wgRequest->getVal ( 'templateName ' );
224222 $this->removeTemplateFor( $noticeName , $templateName );
——@@ -520,6 +518,7 @@
521519 $htmlOut .= $this->tableRow( $fields );
522520 }
523521 $htmlOut .= Xml::closeElement( 'table' );
 522+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
524523 if ( $this->editable ) {
525524 $htmlOut .= Xml::openElement( 'div', array( 'class' => 'cn-buttons' ) );
526525 $htmlOut .= Xml::submitButton( wfMsg( 'centralnotice-modify' ),
——@@ -585,6 +584,7 @@
586585 $htmlOut .= Xml::closeElement( 'tr' );
587586 $htmlOut .= Xml::closeElement( 'table' );
588587 $htmlOut .= Xml::hidden( 'change', 'weight' );
 588+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
589589
590590 // Submit button
591591 $htmlOut .= Xml::tags( 'div',
——@@ -604,7 +604,8 @@
605605
606606 function listNoticeDetail( $notice ) {
607607 global $wgOut, $wgRequest, $wgUser;
608 - if ( $wgRequest->wasPosted() ) {
 608+ if ( $wgRequest->wasPosted() && $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
 609+
609610 // Handle removing of templates
610611 $templateToRemove = $wgRequest->getArray( 'removeTemplates' );
611612 if ( isset( $templateToRemove ) ) {
——@@ -688,6 +689,8 @@
689690 }
690691 }
691692 if ( $this->editable ) {
 693+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
 694+
692695 // Submit button
693696 $htmlOut .= Xml::tags( 'div',
694697 array( 'class' => 'cn-buttons' ),
Index: trunk/extensions/CentralNotice/SpecialNoticeTemplate.php
——@@ -39,7 +39,7 @@
4040 // Begin Banners tab content
4141 $wgOut->addHTML( Xml::openElement( 'div', array( 'id' => 'preferences' ) ) );
4242
43 - if ( $this->editable ) {
 43+ if ( $this->editable && $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
4444 // Handle forms
4545 if ( $wgRequest->wasPosted() ) {
4646
——@@ -85,42 +85,39 @@
8686 }
8787 }
8888
89 - switch ( $sub ) {
90 -
91 - // Handle viewing or editing a specific banner
92 - case 'view':
93 - if ( $wgRequest->getVal( 'wpUserLanguage' ) == 'all' ) {
94 - // Handle viewing a banner in all languages
95 - $template = $wgRequest->getVal( 'template' );
96 - $this->showViewAvailable( $template );
97 - } elseif ( $wgRequest->getText( 'template' ) != '' ) {
98 - $this->showView();
99 - }
100 - break;
101 -
102 - // Handle adding a banner
103 - case 'add':
104 - if ( $this->editable ) {
105 - $this->showAdd();
106 - }
107 - break;
108 -
109 - // Handle cloning a banner
110 - case 'clone':
111 - if ( $this->editable ) {
112 - $oldTemplate = $wgRequest->getVal( 'oldTemplate' );
113 - $newTemplate = $wgRequest->getVal( 'newTemplate' );
114 - // We use the returned name in case any special characters had to be removed
115 - $template = $this->cloneTemplate( $oldTemplate, $newTemplate );
116 - $wgOut->redirect( SpecialPage::getTitleFor( 'NoticeTemplate', 'view' )->getLocalUrl( "template=$template" ) );
117 - }
118 - break;
119 -
120 - // Show list of banners by default
121 - default:
122 - $this->showList();
123 -
 89+ // Handle viewing of a template in all languages
 90+ if ( $sub == 'view' && $wgRequest->getVal( 'wpUserLanguage' ) == 'all' ) {
 91+ $template = $wgRequest->getVal( 'template' );
 92+ $this->showViewAvailable( $template );
 93+ return;
12494 }
 95+
 96+ // Handle viewing a specific template
 97+ if ( $sub == 'view' && $wgRequest->getText( 'template' ) != '' ) {
 98+ $this->showView();
 99+ return;
 100+ }
 101+
 102+ if ( $this->editable ) {
 103+ // Handle "Add a banner" link
 104+ if ( $sub == 'add' ) {
 105+ $this->showAdd();
 106+ return;
 107+ }
 108+
 109+ // Handle cloning a specific template
 110+ if ( $sub == 'clone' && $wgUser->matchEditToken( $wgRequest->getVal( 'authtoken' ) ) ) {
 111+ $oldTemplate = $wgRequest->getVal( 'oldTemplate' );
 112+ $newTemplate = $wgRequest->getVal( 'newTemplate' );
 113+ // We use the returned name in case any special characters had to be removed
 114+ $template = $this->cloneTemplate( $oldTemplate, $newTemplate );
 115+ $wgOut->redirect( SpecialPage::getTitleFor( 'NoticeTemplate', 'view' )->getLocalUrl( "template=$template" ) );
 116+ return;
 117+ }
 118+ }
 119+
 120+ // Show list by default
 121+ $this->showList();
125122
126123 // End Banners tab content
127124 $wgOut->addHTML( Xml::closeElement( 'div' ) );
——@@ -175,7 +172,7 @@
176173 }
177174
178175 function showAdd() {
179 - global $wgOut;
 176+ global $wgOut, $wgUser;
180177
181178 // Build HTML
182179 $htmlOut = '';
——@@ -194,6 +191,7 @@
195192 $htmlOut .= Xml::tags( 'p', null,
196193 Xml::textarea( 'templateBody', '', 60, 20 )
197194 );
 195+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
198196
199197 // Submit button
200198 $htmlOut .= Xml::tags( 'div',
——@@ -337,7 +335,7 @@
338336 $htmlOut .= Xml::closeElement( 'tr' );
339337 }
340338 if ( $this->editable ) {
341 - $htmlOut .= Xml::hidden( 'token', $token );
 339+ $htmlOut .= Xml::hidden( 'authtoken', $token );
342340 $htmlOut .= Xml::hidden( 'wpUserLanguage', $wpUserLang );
343341 $htmlOut .= Xml::openElement( 'tr' );
344342 $htmlOut .= Xml::tags( 'td', array( 'colspan' => 4 ),
——@@ -347,6 +345,7 @@
348346 }
349347
350348 $htmlOut .= Xml::closeElement( 'table' );
 349+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
351350 $htmlOut .= Xml::closeElement( 'fieldset' );
352351
353352 if ( $this->editable ) {
——@@ -375,6 +374,7 @@
376375 Xml::tags( 'td', null, $sk->makeLinkObj( $newPage, wfMsgHtml( 'centralnotice-preview-all-template-translations' ), "template=$currentTemplate&wpUserLanguage=all" ) )
377376 );
378377 $htmlOut .= Xml::closeElement( 'table' );
 378+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
379379 $htmlOut .= Xml::closeElement( 'fieldset' );
380380 $htmlOut .= Xml::closeElement( 'form' );
381381 }
——@@ -403,6 +403,7 @@
404404 );
405405 }
406406 $htmlOut .= Xml::closeElement( 'table' );
 407+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
407408 $htmlOut .= Xml::closeElement( 'fieldset' );
408409 if ( $this->editable ) {
409410 $htmlOut .= Xml::closeElement( 'form' );
——@@ -428,6 +429,7 @@
429430
430431 $htmlOut .= Xml::closeElement( 'tr' );
431432 $htmlOut .= Xml::closeElement( 'table' );
 433+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
432434 $htmlOut .= Xml::closeElement( 'fieldset' );
433435 $htmlOut .= Xml::closeElement( 'form' );
434436 }
——@@ -539,7 +541,7 @@
540542 global $wgOut;
541543
542544 if ( $body == '' || $name == '' ) {
543 - $wgOut->addWikiMsg( 'centralnotice-null-string' );
 545+ $wgOut->addHTML( Xml::element( 'div', array( 'class' => 'cn-error' ), wfMsg( 'centralnotice-null-string' ) ) );
544546 return;
545547 }
546548
——@@ -555,7 +557,7 @@
556558 );
557559
558560 if ( $dbr->numRows( $res ) > 0 ) {
559 - $wgOut->addWikiMsg( 'centralnotice-template-exists' );
 561+ $wgOut->addHTML( Xml::element( 'div', array( 'class' => 'cn-error' ), wfMsg( 'centralnotice-template-exists' ) ) );
560562 return false;
561563 } else {
562564 $dbw = wfGetDB( DB_MASTER );
——@@ -582,7 +584,7 @@
583585 global $wgOut;
584586
585587 if ( $body == '' || $name == '' ) {
586 - $wgOut->addWikiMsg( 'centralnotice-null-string' );
 588+ $wgOut->addHTML( Xml::element( 'div', array( 'class' => 'cn-error' ), wfMsg( 'centralnotice-null-string' ) ) );
587589 return;
588590 }
589591
——@@ -765,6 +767,7 @@
766768 }
767769
768770 function getEndBody() {
 771+ global $wgUser;
769772 $htmlOut = '';
770773 if ( $this->editable ) {
771774 $htmlOut .= Xml::tags( 'tr', null,
——@@ -774,6 +777,7 @@
775778 );
776779 }
777780 $htmlOut .= Xml::closeElement( 'table' );
 781+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
778782 return $htmlOut;
779783 }
780784 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r70441Fix for error in r70440. Two different tokens.kaldari00:11, 4 August 2010
r70442Better fix for r70440. Actually it looks like the old token stuff was left ov...kaldari00:26, 4 August 2010
r70446Fixing getTitle calls per comment in r70440kaldari01:25, 4 August 2010

Comments

#Comment by Kaldari (talk | contribs)   00:16, 4 August 2010

The change from the switch statement is actually a reversion of myself to the previous scheme. The error referred to in r70441 is that there are 2 different tokens in SpecialNoticeTemplate.php - one for translation tokens, the other for authentication. I accidentally confused them at line 338.

#Comment by Kaldari (talk | contribs)   00:31, 4 August 2010

Actually it looks like the old tokens were just left over from a previous auth scheme that had been abandoned. Removed in r70442 now that they are redundant.

#Comment by Nikerabbit (talk | contribs)   00:31, 4 August 2010 
+$wgOut->redirect( SpecialPage::getTitleFor( 'NoticeTemplate', 'view' )->getLocalUrl( "template=$template" ) );

I think you can replace SpecialPage::getTitleFor with $this->getTitle 

-$wgOut->addWikiMsg( 'centralnotice-null-string' );
+$wgOut->addHTML( Xml::element( 'div', array( 'class' => 'cn-error' ), wfMsg( 'centralnotice-null-string' ) ) );

Use $wgOut->wrapWikiMsg

#Comment by Kaldari (talk | contribs)   01:27, 4 August 2010

Cleaned up the getTitle calls in r70446. I'll clean up the error messages as well some time soon.

#Comment by Catrope (talk | contribs)   23:49, 23 August 2010

This code doesn't treat token mismatches nicely at all. It doesn't show an explanatory message (such as the sessionfailure message) and it doesn't restore the form state.

On an unrelated note, you should be using explicit indexes (campaign IDs, preferably) for the enabled[] checkboxes and its friends. This prevents issues with campaigns being added or removed while the user is viewing the form or the sortable table JS messing up the order.

#Comment by Ryan Kaldari (WMF) (talk | contribs)   00:38, 27 August 2010

Session failure error messages were added in r71606. State restore is mostly implemented by r71676, r71688, r71690, r71711, r71716, r71718, etc. There are still a couple pieces that I have yet to add, but all the important pieces are handled now.

Explicit indexes for the checkboxes aren't needed since this is already handled by giving them unique values per campaign.

Status & tagging log