From e6b4944dbfd46854589d2fb1d3f22268bec1b226 Mon Sep 17 00:00:00 2001 From: Max Semenik Date: Fri, 6 Jul 2018 12:21:22 -0700 Subject: [PATCH] Introduce preference filters This normalizes handling of transformations on the boundaries between preferences and generic form controls and removes the special case where email-blacklist is passed around as an array internally, leaking into the API. As a result of this normalization, meta=userinfo no longer returns an array of users, using the internal representation like action=options. Bug: T198935 Change-Id: Iff63da0d215585cfcf083e7f7ec8ed45d5b77301 --- RELEASE-NOTES-1.32 | 3 + includes/Preferences.php | 24 --- .../preferences/DefaultPreferencesFactory.php | 160 ++++++------------ includes/preferences/Filter.php | 39 +++++ includes/preferences/IntvalFilter.php | 38 +++++ includes/preferences/MultiUsernameFilter.php | 86 ++++++++++ includes/preferences/TimezoneFilter.php | 84 +++++++++ includes/specials/SpecialEmailuser.php | 4 +- includes/user/User.php | 26 --- tests/phpunit/includes/api/ApiOptionsTest.php | 4 + .../DefaultPreferencesFactoryTest.php | 3 +- .../includes/preferences/FiltersTest.php | 141 +++++++++++++++ .../specials/SpecialPreferencesTest.php | 4 + 13 files changed, 453 insertions(+), 163 deletions(-) create mode 100644 includes/preferences/Filter.php create mode 100644 includes/preferences/IntvalFilter.php create mode 100644 includes/preferences/MultiUsernameFilter.php create mode 100644 includes/preferences/TimezoneFilter.php create mode 100644 tests/phpunit/includes/preferences/FiltersTest.php diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32 index a3bc7d3798..1dbc787ff4 100644 --- a/RELEASE-NOTES-1.32 +++ b/RELEASE-NOTES-1.32 @@ -92,6 +92,9 @@ production. * Assertion failures from the 'assert' and 'assertuser' parameters will no longer use the action module's custom response format, for the few modules that use custom formatters that handle errors. +* (T198935) User list preferences such as `email-blacklist` and similar + extension preferences are no longer represented as arrays when returned by + action=query&meta=userinfo&uiprop=options. === Action API internal changes in 1.32 === * Added 'ApiParseMakeOutputPage' hook. diff --git a/includes/Preferences.php b/includes/Preferences.php index c458af0eab..a8a312c3bd 100644 --- a/includes/Preferences.php +++ b/includes/Preferences.php @@ -300,30 +300,6 @@ class Preferences { throw new Exception( __METHOD__ . '() is deprecated and does nothing' ); } - /** - * Handle the form submission if everything validated properly - * - * @deprecated since 1.31, use PreferencesFactory - * - * @param array $formData - * @param HTMLForm $form - * @return bool|Status|string - */ - public static function tryFormSubmit( $formData, $form ) { - $preferencesFactory = self::getDefaultPreferencesFactory(); - return $preferencesFactory->legacySaveFormData( $formData, $form ); - } - - /** - * @param array $formData - * @param HTMLForm $form - * @return Status - */ - public static function tryUISubmit( $formData, $form ) { - $preferencesFactory = self::getDefaultPreferencesFactory(); - return $preferencesFactory->legacySubmitForm( $formData, $form ); - } - /** * Get a list of all time zones * @param Language $language Language used for the localized names diff --git a/includes/preferences/DefaultPreferencesFactory.php b/includes/preferences/DefaultPreferencesFactory.php index 8c113f4e59..830da0653d 100644 --- a/includes/preferences/DefaultPreferencesFactory.php +++ b/includes/preferences/DefaultPreferencesFactory.php @@ -20,7 +20,6 @@ namespace MediaWiki\Preferences; -use CentralIdLookup; use Config; use DateTime; use DateTimeZone; @@ -53,6 +52,7 @@ use SpecialPage; use SpecialPreferences; use Status; use Title; +use UnexpectedValueException; use User; use UserGroupMembership; use Xml; @@ -94,22 +94,6 @@ class DefaultPreferencesFactory implements PreferencesFactory { $this->logger = new NullLogger(); } - /** - * @return callable[] - */ - protected function getSaveFilters() { - // Wrap intval() so that we can pass it multiple parameters and treat all filters the same. - $intvalFilter = function ( $value, $alldata ) { - return intval( $value ); - }; - return [ - 'timecorrection' => [ $this, 'filterTimezoneInput' ], - 'rclimit' => $intvalFilter, - 'wllimit' => $intvalFilter, - 'searchlimit' => $intvalFilter, - ]; - } - /** * @inheritDoc */ @@ -178,9 +162,11 @@ class DefaultPreferencesFactory implements PreferencesFactory { $disable = !$user->isAllowed( 'editmyoptions' ); $defaultOptions = User::getDefaultOptions(); + $userOptions = $user->getOptions(); + $this->applyFilters( $userOptions, $defaultPreferences, 'filterForForm' ); # # Prod in defaults from the user foreach ( $defaultPreferences as $name => &$info ) { - $prefFromUser = $this->getOptionFromUser( $name, $info, $user ); + $prefFromUser = $this->getOptionFromUser( $name, $info, $userOptions ); if ( $disable && !in_array( $name, $this->getSaveBlacklist() ) ) { $info['disabled'] = 'disabled'; } @@ -209,11 +195,11 @@ class DefaultPreferencesFactory implements PreferencesFactory { * * @param string $name * @param array $info - * @param User $user + * @param array $userOptions * @return array|string */ - protected function getOptionFromUser( $name, $info, User $user ) { - $val = $user->getOption( $name ); + protected function getOptionFromUser( $name, $info, array $userOptions ) { + $val = $userOptions[$name] ?? null; // Handling for multiselect preferences if ( ( isset( $info['type'] ) && $info['type'] == 'multiselect' ) || @@ -223,7 +209,7 @@ class DefaultPreferencesFactory implements PreferencesFactory { $val = []; foreach ( $options as $value ) { - if ( $user->getOption( "$prefix$value" ) ) { + if ( $userOptions["$prefix$value"] ?? false ) { $val[] = $value; } } @@ -239,7 +225,7 @@ class DefaultPreferencesFactory implements PreferencesFactory { foreach ( $columns as $column ) { foreach ( $rows as $row ) { - if ( $user->getOption( "$prefix$column-$row" ) ) { + if ( $userOptions["$prefix$column-$row"] ?? false ) { $val[] = "$column-$row"; } } @@ -653,16 +639,12 @@ class DefaultPreferencesFactory implements PreferencesFactory { ]; if ( $this->config->get( 'EnableUserEmailBlacklist' ) ) { - $lookup = CentralIdLookup::factory(); - $ids = $user->getOption( 'email-blacklist', [] ); - $names = $ids ? $lookup->namesFromCentralIds( $ids, $user ) : []; - $defaultPreferences['email-blacklist'] = [ 'type' => 'usersmultiselect', 'label-message' => 'email-blacklist-label', 'section' => 'personal/email', - 'default' => implode( "\n", $names ), 'disabled' => $disableEmailPrefs, + 'filter' => MultiUsernameFilter::class, ]; } } @@ -850,6 +832,7 @@ class DefaultPreferencesFactory implements PreferencesFactory { 'size' => 20, 'section' => 'rendering/timeoffset', 'id' => 'wpTimeCorrection', + 'filter' => TimezoneFilter::class, ]; } @@ -1010,6 +993,7 @@ class DefaultPreferencesFactory implements PreferencesFactory { 'label-message' => 'recentchangescount', 'help-message' => 'prefs-help-recentchangescount', 'section' => 'rc/displayrc', + 'filter' => IntvalFilter::class, ]; $defaultPreferences['usenewrc'] = [ 'type' => 'toggle', @@ -1153,6 +1137,7 @@ class DefaultPreferencesFactory implements PreferencesFactory { 'label-message' => 'prefs-watchlist-edits', 'help' => $context->msg( 'prefs-watchlist-edits-max' )->escaped(), 'section' => 'watchlist/displaywatchlist', + 'filter' => IntvalFilter::class, ]; $defaultPreferences['extendwatchlist'] = [ 'type' => 'toggle', @@ -1533,9 +1518,11 @@ class DefaultPreferencesFactory implements PreferencesFactory { # Used message keys: 'accesskey-preferences-save', 'tooltip-preferences-save' $htmlForm->setSubmitTooltip( 'preferences-save' ); $htmlForm->setSubmitID( 'prefcontrol' ); - $htmlForm->setSubmitCallback( function ( array $formData, HTMLForm $form ) { - return $this->submitForm( $formData, $form ); - } ); + $htmlForm->setSubmitCallback( + function ( array $formData, HTMLForm $form ) use ( $formDescriptor ) { + return $this->submitForm( $formData, $form, $formDescriptor ); + } + ); return $htmlForm; } @@ -1584,64 +1571,16 @@ class DefaultPreferencesFactory implements PreferencesFactory { return $opt; } - /** - * @param string $tz - * @param array $alldata - * @return string - */ - protected function filterTimezoneInput( $tz, array $alldata ) { - $data = explode( '|', $tz, 3 ); - switch ( $data[0] ) { - case 'ZoneInfo': - $valid = false; - - if ( count( $data ) === 3 ) { - // Make sure this timezone exists - try { - new DateTimeZone( $data[2] ); - // If the constructor didn't throw, we know it's valid - $valid = true; - } catch ( Exception $e ) { - // Not a valid timezone - } - } - - if ( !$valid ) { - // If the supplied timezone doesn't exist, fall back to the encoded offset - return 'Offset|' . intval( $tz[1] ); - } - return $tz; - case 'System': - return $tz; - default: - $data = explode( ':', $tz, 2 ); - if ( count( $data ) == 2 ) { - $data[0] = intval( $data[0] ); - $data[1] = intval( $data[1] ); - $minDiff = abs( $data[0] ) * 60 + $data[1]; - if ( $data[0] < 0 ) { - $minDiff = - $minDiff; - } - } else { - $minDiff = intval( $data[0] ) * 60; - } - - # Max is +14:00 and min is -12:00, see: - # https://en.wikipedia.org/wiki/Timezone - $minDiff = min( $minDiff, 840 ); # 14:00 - $minDiff = max( $minDiff, -720 ); # -12:00 - return 'Offset|' . $minDiff; - } - } - /** * Handle the form submission if everything validated properly * * @param array $formData * @param HTMLForm $form + * @param array[] $formDescriptor * @return bool|Status|string */ - protected function saveFormData( $formData, HTMLForm $form ) { + protected function saveFormData( $formData, HTMLForm $form, array $formDescriptor ) { + /** @var \User $user */ $user = $form->getModifiedUser(); $hiddenPrefs = $this->config->get( 'HiddenPrefs' ); $result = true; @@ -1651,12 +1590,7 @@ class DefaultPreferencesFactory implements PreferencesFactory { } // Filter input - foreach ( array_keys( $formData ) as $name ) { - $filters = $this->getSaveFilters(); - if ( isset( $filters[$name] ) ) { - $formData[$name] = call_user_func( $filters[$name], $formData[$name], $formData ); - } - } + $this->applyFilters( $formData, $formDescriptor, 'filterFromForm' ); // Fortunately, the realname field is MUCH simpler // (not really "private", but still shouldn't be edited without permission) @@ -1713,16 +1647,32 @@ class DefaultPreferencesFactory implements PreferencesFactory { } /** - * DO NOT USE. Temporary function to punch hole for the Preferences class. - * - * @deprecated since 1.31, its inception + * Applies filters to preferences either before or after form usage * - * @param array $formData - * @param HTMLForm $form - * @return bool|Status|string + * @param array &$preferences + * @param array $formDescriptor + * @param string $verb Name of the filter method to call, either 'filterFromForm' or + * 'filterForForm' */ - public function legacySaveFormData( $formData, HTMLForm $form ) { - return $this->saveFormData( $formData, $form ); + protected function applyFilters( array &$preferences, array $formDescriptor, $verb ) { + foreach ( $formDescriptor as $preference => $desc ) { + if ( !isset( $desc['filter'] ) || !isset( $preferences[$preference] ) ) { + continue; + } + $filterDesc = $desc['filter']; + if ( $filterDesc instanceof Filter ) { + $filter = $filterDesc; + } elseif ( class_exists( $filterDesc ) ) { + $filter = new $filterDesc(); + } elseif ( is_callable( $filterDesc ) ) { + $filter = $filterDesc(); + } else { + throw new UnexpectedValueException( + "Unrecognized filter type for preference '$preference'" + ); + } + $preferences[$preference] = $filter->$verb( $preferences[$preference] ); + } } /** @@ -1730,10 +1680,11 @@ class DefaultPreferencesFactory implements PreferencesFactory { * * @param array $formData * @param HTMLForm $form + * @param array $formDescriptor * @return Status */ - protected function submitForm( array $formData, HTMLForm $form ) { - $res = $this->saveFormData( $formData, $form ); + protected function submitForm( array $formData, HTMLForm $form, array $formDescriptor ) { + $res = $this->saveFormData( $formData, $form, $formDescriptor ); if ( $res === true ) { $context = $form->getContext(); @@ -1763,19 +1714,6 @@ class DefaultPreferencesFactory implements PreferencesFactory { return ( $res === true ? Status::newGood() : $res ); } - /** - * DO NOT USE. Temporary function to punch hole for the Preferences class. - * - * @deprecated since 1.31, its inception - * - * @param array $formData - * @param HTMLForm $form - * @return Status - */ - public function legacySubmitForm( array $formData, HTMLForm $form ) { - return $this->submitForm( $formData, $form ); - } - /** * Get a list of all time zones * @param Language $language Language used for the localized names diff --git a/includes/preferences/Filter.php b/includes/preferences/Filter.php new file mode 100644 index 0000000000..670dd5b046 --- /dev/null +++ b/includes/preferences/Filter.php @@ -0,0 +1,39 @@ +lookup = $lookup; + $this->userOrAudience = $userOrAudience; + } + + /** + * @inheritDoc + */ + public function filterFromForm( $names ) { + $names = trim( $names ); + if ( $names !== '' ) { + $names = preg_split( '/\n/', $names, -1, PREG_SPLIT_NO_EMPTY ); + $ids = $this->getLookup()->centralIdsFromNames( $names, $this->userOrAudience ); + if ( $ids ) { + return implode( "\n", $ids ); + } + } + // If the user list is empty, it should be null (don't save) rather than an empty string + return null; + } + + /** + * @inheritDoc + */ + public function filterForForm( $value ) { + $ids = self::splitIds( $value ); + $names = $ids ? $this->getLookup()->namesFromCentralIds( $ids, $this->userOrAudience ) : []; + return implode( "\n", $names ); + } + + /** + * Splits a newline separated list of user ids into a + * + * @param string $str + * @return int[] + */ + public static function splitIds( $str ) { + return array_map( 'intval', preg_split( '/\n/', $str, -1, PREG_SPLIT_NO_EMPTY ) ); + } + + /** + * @return CentralIdLookup + */ + private function getLookup() { + $this->lookup = $this->lookup ?? CentralIdLookup::factory(); + return $this->lookup; + } +} diff --git a/includes/preferences/TimezoneFilter.php b/includes/preferences/TimezoneFilter.php new file mode 100644 index 0000000000..53f12de7de --- /dev/null +++ b/includes/preferences/TimezoneFilter.php @@ -0,0 +1,84 @@ +getOption( 'email-blacklist', [] ); + $blacklist = $target->getOption( 'email-blacklist', '' ); if ( $blacklist ) { + $blacklist = MultiUsernameFilter::splitIds( $blacklist ); $lookup = CentralIdLookup::factory(); $senderId = $lookup->centralIdFromLocalUser( $sender ); if ( $senderId !== 0 && in_array( $senderId, $blacklist ) ) { diff --git a/includes/user/User.php b/includes/user/User.php index ea8cd57524..2ba01ff67d 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -5507,12 +5507,6 @@ class User implements IDBAccessObject, UserIdentity { } } - // Convert the email blacklist from a new line delimited string - // to an array of ids. - if ( isset( $data['email-blacklist'] ) && $data['email-blacklist'] ) { - $data['email-blacklist'] = array_map( 'intval', explode( "\n", $data['email-blacklist'] ) ); - } - foreach ( $data as $property => $value ) { $this->mOptionOverrides[$property] = $value; $this->mOptions[$property] = $value; @@ -5540,26 +5534,6 @@ class User implements IDBAccessObject, UserIdentity { // Not using getOptions(), to keep hidden preferences in database $saveOptions = $this->mOptions; - // Convert usernames to ids. - if ( isset( $this->mOptions['email-blacklist'] ) ) { - if ( $this->mOptions['email-blacklist'] ) { - $value = $this->mOptions['email-blacklist']; - // Email Blacklist may be an array of ids or a string of new line - // delimnated user names. - if ( is_array( $value ) ) { - $ids = array_filter( $value, 'is_numeric' ); - } else { - $lookup = CentralIdLookup::factory(); - $ids = $lookup->centralIdsFromNames( explode( "\n", $value ), $this ); - } - $this->mOptions['email-blacklist'] = $ids; - $saveOptions['email-blacklist'] = implode( "\n", $this->mOptions['email-blacklist'] ); - } else { - // If the blacklist is empty, set it to null rather than an empty string. - $this->mOptions['email-blacklist'] = null; - } - } - // Allow hooks to abort, for instance to save to a global profile. // Reset options to default state before saving. if ( !Hooks::run( 'UserSaveOptions', [ $this, &$saveOptions ] ) ) { diff --git a/tests/phpunit/includes/api/ApiOptionsTest.php b/tests/phpunit/includes/api/ApiOptionsTest.php index fbc1bedffa..29c7dae50d 100644 --- a/tests/phpunit/includes/api/ApiOptionsTest.php +++ b/tests/phpunit/includes/api/ApiOptionsTest.php @@ -40,6 +40,10 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->any() ) ->method( 'getInstanceForUpdate' )->will( $this->returnValue( $this->mUserMock ) ); + // Needs to return something + $this->mUserMock->method( 'getOptions' ) + ->willReturn( [] ); + // Create a new context $this->mContext = new DerivativeContext( new RequestContext() ); $this->mContext->getContext()->setTitle( Title::newFromText( 'Test' ) ); diff --git a/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php b/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php index 9a8608f475..296691d331 100644 --- a/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php +++ b/tests/phpunit/includes/preferences/DefaultPreferencesFactoryTest.php @@ -164,8 +164,9 @@ class DefaultPreferencesFactoryTest extends MediaWikiTestCase { } ); + /** @var DefaultPreferencesFactory $factory */ $factory = TestingAccessWrapper::newFromObject( $this->getPreferencesFactory() ); - $factory->saveFormData( $newOptions, $form ); + $factory->saveFormData( $newOptions, $form, [] ); } /** diff --git a/tests/phpunit/includes/preferences/FiltersTest.php b/tests/phpunit/includes/preferences/FiltersTest.php new file mode 100644 index 0000000000..42cbc2cc74 --- /dev/null +++ b/tests/phpunit/includes/preferences/FiltersTest.php @@ -0,0 +1,141 @@ +filterFromForm( '0' ) ); + self::assertSame( 3, $filter->filterFromForm( '3' ) ); + self::assertSame( '123', $filter->filterForForm( '123' ) ); + } + + /** + * @covers MediaWiki\Preferences\TimezoneFilter::filterFromForm() + * @dataProvider provideTimezoneFilter + * + * @param string $input + * @param string $expected + */ + public function testTimezoneFilter( $input, $expected ) { + $filter = new TimezoneFilter(); + $result = $filter->filterFromForm( $input ); + self::assertEquals( $expected, $result ); + } + + public function provideTimezoneFilter() { + return [ + [ 'ZoneInfo', 'Offset|0' ], + [ 'ZoneInfo|bogus', 'Offset|0' ], + [ 'System', 'System' ], + [ '2:30', 'Offset|150' ], + ]; + } + + /** + * @covers MediaWiki\Preferences\MultiUsernameFilter::filterFromForm() + * @dataProvider provideMultiUsernameFilterFrom + * + * @param string $input + * @param string|null $expected + */ + public function testMultiUsernameFilterFrom( $input, $expected ) { + $filter = $this->makeMultiUsernameFilter(); + $result = $filter->filterFromForm( $input ); + self::assertSame( $expected, $result ); + } + + public function provideMultiUsernameFilterFrom() { + return [ + [ '', null ], + [ "\n\n\n", null ], + [ 'Foo', '1' ], + [ "\n\n\nFoo\nBar\n", "1\n2" ], + [ "Baz\nInvalid\nFoo", "3\n1" ], + [ "Invalid", null ], + [ "Invalid\n\n\nInvalid\n", null ], + ]; + } + + /** + * @covers MediaWiki\Preferences\MultiUsernameFilter::filterForForm() + * @dataProvider provideMultiUsernameFilterFor + * + * @param string $input + * @param string $expected + */ + public function testMultiUsernameFilterFor( $input, $expected ) { + $filter = $this->makeMultiUsernameFilter(); + $result = $filter->filterForForm( $input ); + self::assertSame( $expected, $result ); + } + + public function provideMultiUsernameFilterFor() { + return [ + [ '', '' ], + [ "\n", '' ], + [ '1', 'Foo' ], + [ "\n1\n\n2\666\n", "Foo\nBar" ], + [ "666\n667", '' ], + ]; + } + + private function makeMultiUsernameFilter() { + $userMapping = [ + 'Foo' => 1, + 'Bar' => 2, + 'Baz' => 3, + ]; + $flipped = array_flip( $userMapping ); + $idLookup = self::getMockBuilder( CentralIdLookup::class ) + ->disableOriginalConstructor() + ->setMethods( [ 'centralIdsFromNames', 'namesFromCentralIds' ] ) + ->getMockForAbstractClass(); + + $idLookup->method( 'centralIdsFromNames' ) + ->will( self::returnCallback( function ( $names ) use ( $userMapping ) { + $ids = []; + foreach ( $names as $name ) { + $ids[] = $userMapping[$name] ?? null; + } + return array_filter( $ids, 'is_numeric' ); + } ) ); + $idLookup->method( 'namesFromCentralIds' ) + ->will( self::returnCallback( function ( $ids ) use ( $flipped ) { + $names = []; + foreach ( $ids as $id ) { + $names[] = $flipped[$id] ?? null; + } + return array_filter( $names, 'is_string' ); + } ) ); + + return new MultiUsernameFilter( $idLookup ); + } +} diff --git a/tests/phpunit/includes/specials/SpecialPreferencesTest.php b/tests/phpunit/includes/specials/SpecialPreferencesTest.php index bdfbb62e7e..cd6cd3be1f 100644 --- a/tests/phpunit/includes/specials/SpecialPreferencesTest.php +++ b/tests/phpunit/includes/specials/SpecialPreferencesTest.php @@ -43,6 +43,10 @@ class SpecialPreferencesTest extends MediaWikiTestCase { ] ) ); + # Needs to return something + $user->method( 'getOptions' ) + ->willReturn( [] ); + # Forge a request to call the special page $context = new RequestContext(); $context->setRequest( new FauxRequest() ); -- 2.20.1