r92924 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r92923‎ | r92924 | r92925 >
Date:00:48, 23 July 2011
Author:aaron
Status:resolved (Comments)
Tags:
Comment:
* Moved email changing from sp:Preferences to new sp:ChangeEmail, which requires confirming the user password. This reduces the impact of session hijacking, which was increased slightly with r86482. Changing a password already required confirming the old one. This change closes the loophole of changing the email address and then doing a reset.
* Parse 'mailerror' message correctly
Modified paths:
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/Preferences.php (modified) (history)
  • /trunk/phase3/includes/SpecialPageFactory.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialChangeEmail.php (added) (history)
  • /trunk/phase3/includes/specials/SpecialPreferences.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/language/messages.inc
@@ -525,6 +525,16 @@
526526 'passwordreset-emailelement',
527527 'passwordreset-emailsent',
528528 ),
 529+ 'changeemail' => array(
 530+ 'changeemail',
 531+ 'changeemail-header',
 532+ 'changeemail-text',
 533+ 'changeemail-no-info',
 534+ 'changeemail-oldemail',
 535+ 'changeemail-newemail',
 536+ 'changeemail-submit',
 537+ 'changeemail-cancel',
 538+ ),
529539 'toolbar' => array(
530540 'bold_sample',
531541 'bold_tip',
@@ -935,6 +945,8 @@
936946 'prefs-watchlist-token',
937947 'prefs-misc', // continue checking if used from here on (r49916)
938948 'prefs-resetpass',
 949+ 'prefs-changeemail',
 950+ 'prefs-setemail',
939951 'prefs-email',
940952 'prefs-rendering',
941953 'saveprefs',
@@ -3516,6 +3528,7 @@
35173529 'mail' => 'E-mail sending',
35183530 'passwordstrength' => 'JavaScript password checks',
35193531 'resetpass' => 'Change password dialog',
 3532+ 'changeemail' => 'Change E-mail dialog',
35203533 'passwordreset' => 'Special:PasswordReset',
35213534 'toolbar' => 'Edit page toolbar',
35223535 'edit' => 'Edit pages',
Index: trunk/phase3/includes/SpecialPageFactory.php
@@ -160,6 +160,7 @@
161161 static function getList() {
162162 global $wgSpecialPages;
163163 global $wgDisableCounters, $wgDisableInternalSearch, $wgEmailAuthentication;
 164+ global $wgEnableEmail;
164165
165166 if ( !is_object( self::$mList ) ) {
166167 wfProfileIn( __METHOD__ );
@@ -177,6 +178,10 @@
178179 self::$mList['Invalidateemail'] = 'EmailInvalidation';
179180 }
180181
 182+ if ( $wgEnableEmail ) {
 183+ self::$mList['ChangeEmail'] = 'SpecialChangeEmail';
 184+ }
 185+
181186 // Add extension special pages
182187 self::$mList = array_merge( self::$mList, $wgSpecialPages );
183188
@@ -512,7 +517,7 @@
513518 static function getLocalNameFor( $name, $subpage = false ) {
514519 global $wgContLang;
515520 $aliases = $wgContLang->getSpecialPageAliases();
516 -
 521+
517522 if ( isset( $aliases[$name][0] ) ) {
518523 $name = $aliases[$name][0];
519524 } else {
Index: trunk/phase3/includes/AutoLoader.php
@@ -743,6 +743,7 @@
744744 'SpecialBlockme' => 'includes/specials/SpecialBlockme.php',
745745 'SpecialBookSources' => 'includes/specials/SpecialBooksources.php',
746746 'SpecialCategories' => 'includes/specials/SpecialCategories.php',
 747+ 'SpecialChangeEmail' => 'includes/specials/SpecialChangeEmail.php',
747748 'SpecialChangePassword' => 'includes/specials/SpecialChangePassword.php',
748749 'SpecialComparePages' => 'includes/specials/SpecialComparePages.php',
749750 'SpecialContributions' => 'includes/specials/SpecialContributions.php',
Index: trunk/phase3/includes/specials/SpecialPreferences.php
@@ -61,11 +61,6 @@
6262 );
6363 }
6464
65 - if ( $wgRequest->getCheck( 'eauth' ) ) {
66 - $wgOut->wrapWikiMsg( "<div class='error' style='clear: both;'>\n$1\n</div>",
67 - 'eauthentsent', $wgUser->getName() );
68 - }
69 -
7065 $htmlForm = Preferences::getFormObject( $wgUser );
7166 $htmlForm->setSubmitCallback( array( 'Preferences', 'tryUISubmit' ) );
7267
Index: trunk/phase3/includes/specials/SpecialChangeEmail.php
@@ -0,0 +1,206 @@
 2+<?php
 3+/**
 4+ * Implements Special:ChangeEmail
 5+ *
 6+ * This program is free software; you can redistribute it and/or modify
 7+ * it under the terms of the GNU General Public License as published by
 8+ * the Free Software Foundation; either version 2 of the License, or
 9+ * (at your option) any later version.
 10+ *
 11+ * This program is distributed in the hope that it will be useful,
 12+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
 13+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 14+ * GNU General Public License for more details.
 15+ *
 16+ * You should have received a copy of the GNU General Public License along
 17+ * with this program; if not, write to the Free Software Foundation, Inc.,
 18+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 19+ * http://www.gnu.org/copyleft/gpl.html
 20+ *
 21+ * @file
 22+ * @ingroup SpecialPage
 23+ */
 24+
 25+/**
 26+ * Let users change their email address.
 27+ *
 28+ * @ingroup SpecialPage
 29+ */
 30+class SpecialChangeEmail extends SpecialPage {
 31+ public function __construct() {
 32+ parent::__construct( 'ChangeEmail' );
 33+ }
 34+
 35+ /**
 36+ * Main execution point
 37+ */
 38+ function execute( $par ) {
 39+ global $wgRequest;
 40+
 41+ $out = $this->getOutput();
 42+ if ( wfReadOnly() ) {
 43+ $out->readOnlyPage();
 44+ return;
 45+ }
 46+
 47+ $user = $this->getUser();
 48+
 49+ $this->mPassword = $wgRequest->getVal( 'wpPassword' );
 50+ $this->mNewEmail = $wgRequest->getVal( 'wpNewEmail' );
 51+
 52+ $this->setHeaders();
 53+ $this->outputHeader();
 54+ $out->disallowUserJs();
 55+
 56+ if ( !$wgRequest->wasPosted() && !$user->isLoggedIn() ) {
 57+ $this->error( wfMsg( 'changeemail-no-info' ) );
 58+ return;
 59+ }
 60+
 61+ if ( $wgRequest->wasPosted() && $wgRequest->getBool( 'wpCancel' ) ) {
 62+ $this->doReturnTo();
 63+ return;
 64+ }
 65+
 66+ if ( $wgRequest->wasPosted()
 67+ && $user->matchEditToken( $wgRequest->getVal( 'token' ) ) )
 68+ {
 69+ $info = $this->attemptChange( $user, $this->mPassword, $this->mNewEmail );
 70+ if ( $info === true ) {
 71+ $this->doReturnTo();
 72+ } elseif ( $info === 'eauth' ) {
 73+ # Notify user that a confirmation email has been sent...
 74+ $out->wrapWikiMsg( "<div class='error' style='clear: both;'>\n$1\n</div>",
 75+ 'eauthentsent', $user->getName() );
 76+ $this->doReturnTo( 'soft' ); // just show the link to go back
 77+ return; // skip form
 78+ }
 79+ }
 80+
 81+ $this->showForm();
 82+ }
 83+
 84+ protected function doReturnTo( $type = 'hard' ) {
 85+ global $wgRequest;
 86+ $titleObj = Title::newFromText( $wgRequest->getVal( 'returnto' ) );
 87+ if ( !$titleObj instanceof Title ) {
 88+ $titleObj = Title::newMainPage();
 89+ }
 90+ if ( $type == 'hard' ) {
 91+ $this->getOutput()->redirect( $titleObj->getFullURL() );
 92+ } else {
 93+ $this->getOutput()->addReturnTo( $titleObj );
 94+ }
 95+ }
 96+
 97+ protected function error( $msg ) {
 98+ $this->getOutput()->addHTML( Xml::element('p', array( 'class' => 'error' ), $msg ) );
 99+ }
 100+
 101+ protected function showForm() {
 102+ global $wgRequest;
 103+
 104+ $user = $this->getUser();
 105+
 106+ $oldEmailText = $user->getEmail()
 107+ ? $user->getEmail()
 108+ : wfMsg( 'changeemail-none' );
 109+
 110+ $this->getOutput()->addHTML(
 111+ Xml::fieldset( wfMsg( 'changeemail-header' ) ) .
 112+ Xml::openElement( 'form',
 113+ array(
 114+ 'method' => 'post',
 115+ 'action' => $this->getTitle()->getLocalUrl(),
 116+ 'id' => 'mw-changeemail-form' ) ) . "\n" .
 117+ Html::hidden( 'token', $user->editToken() ) . "\n" .
 118+ Html::hidden( 'returnto', $wgRequest->getVal( 'returnto' ) ) . "\n" .
 119+ wfMsgExt( 'changeemail-text', array( 'parse' ) ) . "\n" .
 120+ Xml::openElement( 'table', array( 'id' => 'mw-changeemail-table' ) ) . "\n" .
 121+ $this->pretty( array(
 122+ array( 'wpName', 'username', 'text', $user->getName() ),
 123+ array( 'wpOldEmail', 'changeemail-oldemail', 'text', $oldEmailText ),
 124+ array( 'wpNewEmail', 'changeemail-newemail', 'input', $this->mNewEmail ),
 125+ array( 'wpPassword', 'yourpassword', 'password', $this->mPassword ),
 126+ ) ) . "\n" .
 127+ "<tr>\n" .
 128+ "<td></td>\n" .
 129+ '<td class="mw-input">' .
 130+ Xml::submitButton( wfMsg( 'changeemail-submit' ) ) .
 131+ Xml::submitButton( wfMsg( 'changeemail-cancel' ), array( 'name' => 'wpCancel' ) ) .
 132+ "</td>\n" .
 133+ "</tr>\n" .
 134+ Xml::closeElement( 'table' ) .
 135+ Xml::closeElement( 'form' ) .
 136+ Xml::closeElement( 'fieldset' ) . "\n"
 137+ );
 138+ }
 139+
 140+ protected function pretty( $fields ) {
 141+ $out = '';
 142+ foreach ( $fields as $list ) {
 143+ list( $name, $label, $type, $value ) = $list;
 144+ if( $type == 'text' ) {
 145+ $field = htmlspecialchars( $value );
 146+ } else {
 147+ $attribs = array( 'id' => $name );
 148+ if ( $name == 'wpPassword' ) {
 149+ $attribs[] = 'autofocus';
 150+ }
 151+ $field = Html::input( $name, $value, $type, $attribs );
 152+ }
 153+ $out .= "<tr>\n";
 154+ $out .= "\t<td class='mw-label'>";
 155+ if ( $type != 'text' ) {
 156+ $out .= Xml::label( wfMsg( $label ), $name );
 157+ } else {
 158+ $out .= wfMsgHtml( $label );
 159+ }
 160+ $out .= "</td>\n";
 161+ $out .= "\t<td class='mw-input'>";
 162+ $out .= $field;
 163+ $out .= "</td>\n";
 164+ $out .= "</tr>";
 165+ }
 166+ return $out;
 167+ }
 168+
 169+ /**
 170+ * @return bool|string true or string on success, false on failure
 171+ */
 172+ protected function attemptChange( User $user, $pass, $newaddr ) {
 173+ if ( $newaddr != '' && !Sanitizer::validateEmail( $newaddr ) ) {
 174+ $this->error( wfMsgExt( 'invalidemailaddress', 'parseinline' ) );
 175+ return false;
 176+ }
 177+
 178+ $throttleCount = LoginForm::incLoginThrottle( $user->getName() );
 179+ if ( $throttleCount === true ) {
 180+ $this->error( wfMsgHtml( 'login-throttled' ) );
 181+ return false;
 182+ }
 183+
 184+ if ( !$user->checkPassword( $pass ) ) {
 185+ $this->error( wfMsgHtml( 'wrongpassword' ) );
 186+ return false;
 187+ }
 188+
 189+ if ( $throttleCount ) {
 190+ LoginForm::clearLoginThrottle( $user->getName() );
 191+ }
 192+
 193+ list( $status, $info ) = Preferences::trySetUserEmail( $user, $newaddr );
 194+ if ( $status !== true ) {
 195+ if ( $status instanceof Status ) {
 196+ $this->getOutput()->addHTML(
 197+ '<p class="error">' .
 198+ $this->getOutput()->parseInline( $status->getWikiText( $info ) ) .
 199+ '</p>' );
 200+ }
 201+ return false;
 202+ }
 203+
 204+ $user->saveSettings();
 205+ return $info ? $info : true;
 206+ }
 207+}
Property changes on: trunk/phase3/includes/specials/SpecialChangeEmail.php
___________________________________________________________________
Added: svn:eol-style
1208 + native
Index: trunk/phase3/includes/Preferences.php
@@ -353,13 +353,20 @@
354354 $helpMessages[] = 'prefs-help-email-others';
355355 }
356356
 357+ $link = $wgUser->getSkin()->link(
 358+ SpecialPage::getTitleFor( 'ChangeEmail' ),
 359+ wfMsgHtml( $user->getEmail() ? 'prefs-changeemail' : 'prefs-setemail' ),
 360+ array(),
 361+ array( 'returnto' => SpecialPage::getTitleFor( 'Preferences' ) ) );
 362+
357363 $defaultPreferences['emailaddress'] = array(
358 - 'type' => $wgAuth->allowPropChange( 'emailaddress' ) ? 'email' : 'info',
359 - 'default' => $user->getEmail(),
 364+ 'type' => 'info',
 365+ 'raw' => true,
 366+ 'default' => $user->getEmail()
 367+ ? htmlspecialchars( $user->getEmail() ) . " ($link)"
 368+ : $link,
 369+ 'label-message' => 'youremail',
360370 'section' => 'personal/email',
361 - 'label-message' => 'youremail',
362 - 'help-messages' => $helpMessages,
363 - 'validation-callback' => array( 'Preferences', 'validateEmail' ),
364371 );
365372
366373 global $wgEmailAuthentication;
@@ -1342,7 +1349,7 @@
13431350 * @return bool|Status|string
13441351 */
13451352 static function tryFormSubmit( $formData, $entryPoint = 'internal' ) {
1346 - global $wgUser, $wgEmailAuthentication, $wgEnableEmail;
 1353+ global $wgUser;
13471354
13481355 $result = true;
13491356
@@ -1360,34 +1367,6 @@
13611368 'emailaddress',
13621369 );
13631370
1364 - if ( $wgEnableEmail ) {
1365 - $newaddr = $formData['emailaddress'];
1366 - $oldaddr = $wgUser->getEmail();
1367 - if ( ( $newaddr != '' ) && ( $newaddr != $oldaddr ) ) {
1368 - # the user has supplied a new email address on the login page
1369 - # new behaviour: set this new emailaddr from login-page into user database record
1370 - $wgUser->setEmail( $newaddr );
1371 - # but flag as "dirty" = unauthenticated
1372 - $wgUser->invalidateEmail();
1373 - if ( $wgEmailAuthentication ) {
1374 - # Mail a temporary password to the dirty address.
1375 - # User can come back through the confirmation URL to re-enable email.
1376 - $type = $oldaddr != '' ? 'changed' : 'set';
1377 - $result = $wgUser->sendConfirmationMail( $type );
1378 - if ( !$result->isGood() ) {
1379 - return htmlspecialchars( $result->getWikiText( 'mailerror' ) );
1380 - } elseif ( $entryPoint == 'ui' ) {
1381 - $result = 'eauth';
1382 - }
1383 - }
1384 - } else {
1385 - $wgUser->setEmail( $newaddr );
1386 - }
1387 - if ( $oldaddr != $newaddr ) {
1388 - wfRunHooks( 'PrefsEmailAudit', array( $wgUser, $oldaddr, $newaddr ) );
1389 - }
1390 - }
1391 -
13921371 // Fortunately, the realname field is MUCH simpler
13931372 global $wgHiddenPrefs;
13941373 if ( !in_array( 'realname', $wgHiddenPrefs ) ) {
@@ -1447,6 +1426,46 @@
14481427 return Status::newGood();
14491428 }
14501429
 1430+ /*
 1431+ * Try to set a user's email address.
 1432+ * This does *not* try to validate the address.
 1433+ * @param $user User
 1434+ * @param $newaddr string New email address
 1435+ * @return Array (true on success or Status on failure, info string)
 1436+ */
 1437+ public static function trySetUserEmail( User $user, $newaddr ) {
 1438+ global $wgEnableEmail, $wgEmailAuthentication;
 1439+ $info = ''; // none
 1440+
 1441+ if ( $wgEnableEmail ) {
 1442+ $oldaddr = $user->getEmail();
 1443+ if ( ( $newaddr != '' ) && ( $newaddr != $oldaddr ) ) {
 1444+ # The user has supplied a new email address on the login page
 1445+ # new behaviour: set this new emailaddr from login-page into user database record
 1446+ $user->setEmail( $newaddr );
 1447+ # But flag as "dirty" = unauthenticated
 1448+ $user->invalidateEmail();
 1449+ if ( $wgEmailAuthentication ) {
 1450+ # Mail a temporary password to the dirty address.
 1451+ # User can come back through the confirmation URL to re-enable email.
 1452+ $type = $oldaddr != '' ? 'changed' : 'set';
 1453+ $result = $user->sendConfirmationMail( $type );
 1454+ if ( !$result->isGood() ) {
 1455+ return array( $result, 'mailerror' );
 1456+ }
 1457+ $info = 'eauth';
 1458+ }
 1459+ } else {
 1460+ $user->setEmail( $newaddr );
 1461+ }
 1462+ if ( $oldaddr != $newaddr ) {
 1463+ wfRunHooks( 'PrefsEmailAudit', array( $user, $oldaddr, $newaddr ) );
 1464+ }
 1465+ }
 1466+
 1467+ return array( true, $info );
 1468+ }
 1469+
14511470 /**
14521471 * @param $user User
14531472 * @return array
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -369,6 +369,7 @@
370370 'Booksources' => array( 'BookSources' ),
371371 'BrokenRedirects' => array( 'BrokenRedirects' ),
372372 'Categories' => array( 'Categories' ),
 373+ 'ChangeEmail' => array( 'ChangeEmail' ),
373374 'ChangePassword' => array( 'ChangePassword', 'ResetPass', 'ResetPassword' ),
374375 'ComparePages' => array( 'ComparePages' ),
375376 'Confirmemail' => array( 'ConfirmEmail' ),
@@ -1200,6 +1201,17 @@
12011202 Temporary password: $2',
12021203 'passwordreset-emailsent' => 'A reminder e-mail has been sent.',
12031204
 1205+# Special:ChangeEmail
 1206+'changeemail' => 'Change E-mail address',
 1207+'changeemail-header' => 'Change account e-mail address',
 1208+'changeemail-text' => 'Complete this form to change your e-mail address. You will need to enter your password to confirm this change.',
 1209+'changeemail-no-info' => 'You must be logged in to access this page directly.',
 1210+'changeemail-oldemail' => 'Current E-mail address:',
 1211+'changeemail-newemail' => 'New E-mail address:',
 1212+'changeemail-none' => '(none)',
 1213+'changeemail-submit' => 'Change E-mail',
 1214+'changeemail-cancel' => 'Cancel',
 1215+
12041216 # Edit page toolbar
12051217 'bold_sample' => 'Bold text',
12061218 'bold_tip' => 'Bold text',
@@ -1757,6 +1769,8 @@
17581770 'prefs-watchlist-token' => 'Watchlist token:',
17591771 'prefs-misc' => 'Misc',
17601772 'prefs-resetpass' => 'Change password',
 1773+'prefs-changeemail' => 'Change E-mail',
 1774+'prefs-setemail' => 'Set an E-mail address',
17611775 'prefs-email' => 'E-mail options',
17621776 'prefs-rendering' => 'Appearance',
17631777 'saveprefs' => 'Save',

Follow-up revisions

RevisionCommit summaryAuthorDate
r93004Follow-up r92924, set special page group for ChangeEmailaaron18:54, 24 July 2011
r98558re r92924 andFix Bug #31283 - "Change E-mail" option in Preference should be ...mah20:36, 30 September 2011
r98559(bug 31283) Made ChangeEmail check $wgAuth->allowPropChange( 'emailaddress' )...aaron20:43, 30 September 2011
r106516Follow-up r92924: move the CSS/JS for e-mail validation to Special:ChangeEmail....robin18:31, 17 December 2011
r113710* (bug 35152) Fix for r92924: help message for e-mail was removed from user p...ialex07:39, 13 March 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r86482(bug 13015, bug 18347, bug 18996, bug 20473, bug 23669, bug 28244) separate t...happy-melon15:27, 20 April 2011

Comments

#Comment by Nikerabbit (talk | contribs)   13:49, 23 July 2011
  • MessagesEn.php has tabs in the middle of the line :o
  • e-mail should be in lower case if no reason to capitalize.
#Comment by Aaron Schulz (talk | contribs)   18:56, 23 July 2011

I had it lower case, but then I noticed prefs had it upper case and stuck with that.

#Comment by Umherirrender (talk | contribs)   11:43, 24 July 2011

You have to set the section on Special:SpecialPages, compare r92078

#Comment by Wikinaut (talk | contribs)   07:05, 2 August 2011

Remark: I checked this against extension OpenID (trunk version, which I maintain), and did not find a negative side effect to that or from that extension.

Background info: E:OpenID has an option tab in preferences where users can opt-in that their e-mail address is updated from their OpenID sreg [1] record on every OpenID-login ("Update the following information from OpenID persona every time I log in: E-mail address").

[1] http://openid.net/specs/openid-simple-registration-extension-1_0.html#response_format

#Comment by Duplicatebug (talk | contribs)   16:57, 16 October 2011

You have removed the 'help-messages' from option 'emailaddress'. Please add this back or remove the corresponding messages from the MessageFile. Thanks.

#Comment by Umherirrender (talk | contribs)   13:47, 11 March 2012
#Comment by IAlex (talk | contribs)   07:40, 13 March 2012

Fixed in r113710.

#Comment by SPQRobin (talk | contribs)   18:35, 17 December 2011

The CSS/JS which gives an e-mail validation notice was still included on Special:Preferences. I moved it for Special:ChangeEmail in r106516.

Status & tagging log