r86146 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r86145‎ | r86146 | r86147 >
Date:22:49, 15 April 2011
Author:demon
Status:ok (Comments)
Tags:
Comment:
Revert r77555 and followups r77563, r77572, r78116 (merge DisableAccount to core).

Per CR at the time: this creates a nearly irreversable action that is not nearly well documented enough (even if disabled by default).

We already have the $wgBlockDisablesLogin kludge in place for this. If we're going to do more work on this idea, it should be well thought out, not another hack.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/SpecialPage.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialDisableAccount.php (deleted) (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
@@ -1149,7 +1149,6 @@
11501150 'right-override-export-depth',
11511151 'right-sendemail',
11521152 'right-revisionmove',
1153 - 'right-disableaccount',
11541153 ),
11551154 'rightslog' => array(
11561155 'rightslog',
@@ -3301,17 +3300,6 @@
33023301 'sqlite-has-fts',
33033302 'sqlite-no-fts',
33043303 ),
3305 - 'disableaccount' => array(
3306 - 'disableaccount',
3307 - 'disableaccount-user',
3308 - 'disableaccount-reason',
3309 - 'disableaccount-confirm',
3310 - 'disableaccount-mustconfirm',
3311 - 'disableaccount-confirm',
3312 - 'disableaccount-nosuchuser',
3313 - 'disableaccount-success',
3314 - 'disableaccount-logentry',
3315 - ),
33163304 );
33173305
33183306 /** Comments for each block */
@@ -3526,5 +3514,4 @@
35273515 'db-error-messages' => 'Database error messages',
35283516 'html-forms' => 'HTML forms',
35293517 'sqlite' => 'SQLite database support',
3530 - 'disableaccount' => 'Special:DisableAccount',
35313518 );
Index: trunk/phase3/includes/AutoLoader.php
@@ -683,7 +683,6 @@
684684 'SpecialBookSources' => 'includes/specials/SpecialBooksources.php',
685685 'SpecialCategories' => 'includes/specials/SpecialCategories.php',
686686 'SpecialComparePages' => 'includes/specials/SpecialComparePages.php',
687 - 'SpecialDisableAccount' => 'includes/specials/SpecialDisableAccount.php',
688687 'SpecialEditWatchlist' => 'includes/specials/SpecialEditWatchlist.php',
689688 'SpecialExport' => 'includes/specials/SpecialExport.php',
690689 'SpecialFilepath' => 'includes/specials/SpecialFilepath.php',
Index: trunk/phase3/includes/DefaultSettings.php
@@ -3370,11 +3370,6 @@
33713371 // For private suppression log access
33723372 #$wgGroupPermissions['suppress']['suppressionlog'] = true;
33733373
3374 -// Permission to disable user accounts
3375 -// Note that disabling an account is not reversible without a system administrator
3376 -// who has direct access to the database
3377 -#$wgGroupPermissions['bureaucrat']['disableaccount'] = true;
3378 -
33793374 /**
33803375 * The developer group is deprecated, but can be activated if need be
33813376 * to use the 'lockdb' and 'unlockdb' special pages. Those require
Index: trunk/phase3/includes/specials/SpecialDisableAccount.php
@@ -1,73 +0,0 @@
2 -<?php
3 -
4 -class SpecialDisableAccount extends SpecialPage {
5 - function __construct() {
6 - parent::__construct( 'DisableAccount', 'disableaccount',
7 - true, array( $this, 'show' ) );
8 - }
9 -
10 - public function show( $par ) {
11 - $formFields = array(
12 - 'account' => array(
13 - 'type' => 'text',
14 - 'validation-callback' => array( __CLASS__, 'validateUser' ),
15 - 'label-message' => 'disableaccount-user',
16 - ),
17 - 'comment' => array(
18 - 'type' => 'text',
19 - 'label-message' => 'disableaccount-reason',
20 - ),
21 - 'confirm' => array(
22 - 'type' => 'toggle',
23 - 'validation-callback' => array( __CLASS__, 'checkConfirmation' ),
24 - 'label-message' => 'disableaccount-confirm',
25 - ),
26 - );
27 -
28 - $htmlForm = new HTMLForm( $formFields, 'disableaccount' );
29 -
30 - $htmlForm->setSubmitCallback( array( __CLASS__, 'submit' ) );
31 - $htmlForm->setTitle( $this->getTitle() );
32 -
33 - $htmlForm->show();
34 - }
35 -
36 - static function validateUser( $field, $allFields ) {
37 - $u = User::newFromName( $field );
38 -
39 - if ( $u && $u->getID() != 0 ) {
40 - return true;
41 - } else {
42 - return wfMsgExt( 'disableaccount-nosuchuser', 'parseinline', array( $field ) );
43 - }
44 - }
45 -
46 - static function checkConfirmation( $field, $allFields ) {
47 - if ( $field ) {
48 - return true;
49 - } else {
50 - return wfMsgExt( 'disableaccount-mustconfirm', 'parseinline' );
51 - }
52 - }
53 -
54 - static function submit( $fields ) {
55 - $user = User::newFromName( $fields['account'] );
56 -
57 - $user->setPassword( null );
58 - $user->setEmail( null );
59 - $user->setToken();
60 - $user->addGroup( 'inactive' );
61 -
62 - $user->saveSettings();
63 - $user->invalidateCache();
64 -
65 - $logPage = new LogPage( 'rights' );
66 -
67 - $logPage->addEntry( 'disable', $user->getUserPage(), $fields['comment'] );
68 -
69 - global $wgOut;
70 - $wgOut->addWikiMsg( 'disableaccount-success', $user->getName() );
71 -
72 - return true;
73 - }
74 -}
\ No newline at end of file
Index: trunk/phase3/includes/SpecialPage.php
@@ -133,7 +133,6 @@
134134 'Listbots' => 'SpecialListBots',
135135 'Activeusers' => 'SpecialActiveUsers',
136136 'Userrights' => 'UserrightsPage',
137 - 'DisableAccount' => 'SpecialDisableAccount',
138137 'EditWatchlist' => 'SpecialEditWatchlist',
139138
140139 # Recent changes and logs
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -382,7 +382,6 @@
383383 'CreateAccount' => array( 'CreateAccount' ),
384384 'Deadendpages' => array( 'DeadendPages' ),
385385 'DeletedContributions' => array( 'DeletedContributions' ),
386 - 'DisableAccount' => array( 'DisableAccount' ),
387386 'Disambiguations' => array( 'Disambiguations' ),
388387 'DoubleRedirects' => array( 'DoubleRedirects' ),
389388 'EditWatchlist' => array( 'EditWatchlist' ),
@@ -1970,7 +1969,6 @@
19711970 'right-override-export-depth' => 'Export pages including linked pages up to a depth of 5',
19721971 'right-sendemail' => 'Send e-mail to other users',
19731972 'right-revisionmove' => 'Move revisions',
1974 -'right-disableaccount' => 'Disable accounts',
19751973
19761974 # User rights log
19771975 'rightslog' => 'User rights log',
@@ -4425,18 +4423,4 @@
44264424 # SQLite database support
44274425 'sqlite-has-fts' => '$1 with full-text search support',
44284426 'sqlite-no-fts' => '$1 without full-text search support',
4429 -
4430 -# Special:DisableAccount
4431 -'disableaccount' => 'Disable a user account',
4432 -'disableaccount-user' => 'Username:',
4433 -'disableaccount-reason' => 'Reason:',
4434 -'disableaccount-confirm' => "Disable this user account.
4435 -The user will not be able to log in, reset their password, or receive e-mail notifications.
4436 -If the user is currently logged in anywhere, they will be immediately logged out.
4437 -''Note that disabling an account is not reversible without system administrator intervention.''",
4438 -'disableaccount-mustconfirm' => 'You must confirm that you wish to disable this account.',
4439 -'disableaccount-nosuchuser' => 'The user account "$1" does not exist.',
4440 -'disableaccount-success' => 'The user account "$1" has been permanently disabled.',
4441 -'disableaccount-logentry' => 'permanently disabled the user account [[$1]]',
4442 -
44434427 );
Index: trunk/phase3/RELEASE-NOTES
@@ -67,8 +67,6 @@
6868 require_once "$IP/extensions/Math/Math.php";
6969
7070 === New features in 1.18 ===
71 -* Added a special page, disabled by default, that allows users with the
72 - 'disableaccount' privilege to permanently deactivate user accounts.
7371 * (bug 8130) Query pages should limit to content namespaces, not just main
7472 namespace
7573 * Search suggestions (other than in the Vector skin) will now use the HTML5

Follow-up revisions

RevisionCommit summaryAuthorDate
r86151Revert r77561, bring back DisableAccount...reedy23:07, 15 April 2011
r86199Rebuild all language files....siebrand11:27, 16 April 2011
r864641.17wmf1: MFT r85377, r85555, r85583, r86100, r86121, r86130, r86142, r86146,...catrope11:27, 20 April 2011
r864741.17: MFT r81731, r85377, r85547, r85555, r85583, r85803, r85881, r86100, r86...catrope13:22, 20 April 2011
r90856Remove disableaccount stuff from r77558. Left out from r86146 reverts.aaron21:50, 26 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r77555Merge DisableAccount extension into core, disabled by default because the rig...werdna04:17, 2 December 2010
r77563Follow-up r77555: Remove unused message (used for extensions only)...raymond08:03, 2 December 2010
r77572Follow-up r77555: Add a commented out line with the new disableaccount rightraymond10:47, 2 December 2010
r78116RELEASE-NOTES for r77555werdna04:08, 9 December 2010

Comments

#Comment by Werdna (talk | contribs)   07:32, 26 April 2011

I was assured that the correct people (Risker and Philippe) were consulted for this removal, but I'm starting to get complaints.

User:Jamesofur is saying that "block disables login" is NOT sufficient for their needs — it does not cover email notifications, for a start.

I'm perfectly happy for this code to be removed in favour of better solutions, it really is a five minute hack. But before you do that, these better solutions have to actually exist.

#Comment by P858snake (talk | contribs)   07:53, 26 April 2011
"User:Jamesofur is saying that "block disables login" is NOT sufficient for their needs — it does not cover email notifications, for a start."

And has anyone ever actually filed this as a bug?

#Comment by Philippe (WMF) (talk | contribs)   07:38, 26 April 2011

Agree with Werdna: we absolutely must have a way to secure the arbcom and checkuser wikis and kill the accounts dead. Until we get something more graceful, can we use this? I'm surprised this was backed out without notification to Arbcom.

#Comment by Risker (talk | contribs)   07:53, 26 April 2011

For the record, those of us managing the arbwiki were /not/ informed of this change. Please reinstate until an alternative that has the same features (i.e., no ability to change passwords, no ability to receive emails, no access in any form) has been written and committed. Extensions are not the preferred course.

#Comment by P858snake (talk | contribs)   07:56, 26 April 2011
"Extensions are not the preferred course."

In this case, Yes they are, Core is not a place for features which can't be reversed without sysadmin help, practically if this implemented in core or a ext has no difference to the frontend users which you are.

#Comment by 😂 (talk | contribs)   11:54, 26 April 2011

When I reverted this, I checked the configuration and it appeared to me that all the private wikis, sans arbcom wiki, were already using $wgBlockDisablesLogin. If that's not sufficient, somebody should file a bug for us to fix it and make it sufficient, which nobody using this feature ever bothered to do (or for that matter, even mentioned to me if filing a bug was too difficult).

I remain convinced that irreversible actions are a Bad Idea.

#Comment by Werdna (talk | contribs)   08:51, 26 April 2011

Reactivated as an extension

#Comment by P858snake (talk | contribs)   08:33, 31 May 2011

Does this still need to be marked as FIXME? its been moved back to a extension which is active on the wmf cluster.

#Comment by 😂 (talk | contribs)   01:49, 4 June 2011

No you're right; setting back to new. It's a straight revert so somebody can probably ok this.

Status & tagging log