From 96ee9894a852c0ea5537dd97e940f23831d0758a Mon Sep 17 00:00:00 2001 From: Matthew Flaschen Date: Thu, 22 Sep 2016 21:20:55 -0400 Subject: [PATCH] SECURITY: Attribute Special:EnableFlow to initiating user It already does when the page doesn't exist. When it does, it uses the Converter Flow, which didn't fully support this. So refactor the Converter to allow this, and pass in the right user. Bug: T146425 Change-Id: I3636e637fe725f96fd80e57c55f6b0cbcea90744 --- includes/Import/Converter.php | 5 ++--- includes/Import/Importer.php | 21 +++++++++++++++++---- includes/Import/Wikitext/ConversionStrategy.php | 14 +++++++++++++- includes/Import/Wikitext/ImportSource.php | 13 ++++++++++--- includes/Specials/SpecialEnableFlow.php | 3 ++- .../convertLqtPageFromRemoteApiForTesting.php | 2 +- maintenance/convertNamespaceFromWikitext.php | 5 ++++- .../phpunit/Import/TalkpageImportOperationTest.php | 8 +++++++- .../Import/Wikitext/ConversionStrategyTest.php | 3 ++- tests/phpunit/Import/Wikitext/ImportSourceTest.php | 15 ++++++++++++--- 10 files changed, 70 insertions(+), 19 deletions(-) diff --git a/includes/Import/Converter.php b/includes/Import/Converter.php index cfe29c0..f4dc046 100644 --- a/includes/Import/Converter.php +++ b/includes/Import/Converter.php @@ -64,8 +64,7 @@ class Converter { * @param DatabaseBase $dbw Master wiki database to read from * @param Importer $importer * @param LoggerInterface $logger - * @param User $user Administrative user for moves and edits related - * to the conversion process. + * @param User $user User for moves and edits related to the conversion process * @param IConversionStrategy $strategy * @throws ImportException When $user does not have an Id */ @@ -184,7 +183,7 @@ class Converter { } $source = $this->strategy->createImportSource( $archiveTitle ); - if ( $this->importer->import( $source, $title, $this->strategy->getSourceStore() ) ) { + if ( $this->importer->import( $source, $title, $this->user, $this->strategy->getSourceStore() ) ) { $this->createArchiveCleanupRevision( $title, $archiveTitle ); $this->logger->info( "Completed import to $title from $archiveTitle" ); } else { diff --git a/includes/Import/Importer.php b/includes/Import/Importer.php index f320ac6..e1401a2 100644 --- a/includes/Import/Importer.php +++ b/includes/Import/Importer.php @@ -108,11 +108,14 @@ class Importer { * * @param IImportSource $source * @param Title $targetPage + * @param User User doing the conversion actions (e.g. initial description, + * wikitext archive edit). However, actions will be attributed to the original + * user when possible (e.g. the user who did the original LQT reply) * @param ImportSourceStore $sourceStore * @return bool True When the import completes with no failures */ - public function import( IImportSource $source, Title $targetPage, ImportSourceStore $sourceStore ) { - $operation = new TalkpageImportOperation( $source, $this->occupationController ); + public function import( IImportSource $source, Title $targetPage, User $user, ImportSourceStore $sourceStore ) { + $operation = new TalkpageImportOperation( $source, $user, $this->occupationController ); $pageImportState = new PageImportState( $this->workflowLoaderFactory ->createWorkflowLoader( $targetPage ) @@ -524,14 +527,24 @@ class TalkpageImportOperation { */ protected $importSource; + /** @var User User doing the conversion actions (e.g. initial description, wikitext + * archive edit). However, actions will be attributed to the original user when + * possible (e.g. the user who did the original LQT reply) + */ + protected $user; + /** @var OccupationController */ protected $occupationController; /** * @param IImportSource $source + * @param User $user The import user; this will only be used when there is no + * 'original' user + * @param OccupationController $occupationController */ - public function __construct( IImportSource $source, OccupationController $occupationController ) { + public function __construct( IImportSource $source, User $user, OccupationController $occupationController ) { $this->importSource = $source; + $this->user = $user; $this->occupationController = $occupationController; } @@ -550,7 +563,7 @@ class TalkpageImportOperation { // Explicitly allow creation of board $creationStatus = $this->occupationController->safeAllowCreation( $destinationTitle, - $this->occupationController->getTalkpageManager(), + $this->user, /* $mustNotExist = */ true ); if ( !$creationStatus->isGood() ) { diff --git a/includes/Import/Wikitext/ConversionStrategy.php b/includes/Import/Wikitext/ConversionStrategy.php index 4acdb87..ef9cf9d 100644 --- a/includes/Import/Wikitext/ConversionStrategy.php +++ b/includes/Import/Wikitext/ConversionStrategy.php @@ -12,6 +12,7 @@ use Parser; use Psr\Log\LoggerInterface; use StubObject; use Title; +use User; use WikitextContent; /** @@ -54,10 +55,19 @@ class ConversionStrategy implements IConversionStrategy { */ protected $headerSuffix; + /** @var User User doing the conversion actions (e.g. initial description, wikitext + * archive edit). However, actions will be attributed to the original user when + * possible (e.g. the user who did the original LQT reply) + * + */ + protected $user; + /** * @param Parser|StubObject $parser * @param ImportSourceStore $sourceStore * @param LoggerInterface $logger + * @param User $user User to take conversion actions are (applicable for actions + * where if there is no 'original' user) * @param Title[] $noConvertTemplates List of templates that flag pages that * shouldn't be converted (optional) * @param string $headerSuffix Wikitext to add to the end of the header (optional) @@ -66,12 +76,14 @@ class ConversionStrategy implements IConversionStrategy { $parser, ImportSourceStore $sourceStore, LoggerInterface $logger, + User $user, array $noConvertTemplates = array(), $headerSuffix = null ) { $this->parser = $parser; $this->sourceStore = $sourceStore; $this->logger = $logger; + $this->user = $user; $this->noConvertTemplates = $noConvertTemplates; $this->headerSuffix = $headerSuffix; @@ -123,7 +135,7 @@ class ConversionStrategy implements IConversionStrategy { * {@inheritDoc} */ public function createImportSource( Title $title ) { - return new ImportSource( $title, $this->parser, $this->headerSuffix ); + return new ImportSource( $title, $this->parser, $this->user, $this->headerSuffix ); } /** diff --git a/includes/Import/Wikitext/ImportSource.php b/includes/Import/Wikitext/ImportSource.php index 5f23199..3a0228d 100644 --- a/includes/Import/Wikitext/ImportSource.php +++ b/includes/Import/Wikitext/ImportSource.php @@ -15,6 +15,7 @@ use ParserOptions; use Revision; use StubObject; use Title; +use User; /** * Imports the header of a wikitext talk page. Does not attempt to @@ -22,19 +23,26 @@ use Title; * ConversionStrategy for more details. */ class ImportSource implements IImportSource { + /** @var User User doing the conversion actions (e.g. initial description, wikitext + * archive edit). However, actions will be attributed to the original user if + * applicable. + */ + protected $user; /** * @param Title $title * @param Parser|StubObject $parser * @param string $headerSuffix + * @param User $user User to take actions as * @throws ImportException When $title is an external title */ - public function __construct( Title $title, $parser, $headerSuffix = null ) { + public function __construct( Title $title, $parser, User $user, $headerSuffix = null ) { if ( $title->isExternal() ) { throw new ImportException( "Invalid non-local title: $title" ); } $this->title = $title; $this->parser = $parser; + $this->user = $user; $this->headerSuffix = $headerSuffix; } @@ -80,7 +88,7 @@ class ImportSource implements IImportSource { array( new ObjectRevision( $content, wfTimestampNow(), - FlowHooks::getOccupationController()->getTalkpageManager()->getName(), + $this->user->getName(), "wikitext-import:header-revision:{$revision->getId()}" ) ), "wikitext-import:header:{$this->title->getPrefixedText()}" @@ -94,4 +102,3 @@ class ImportSource implements IImportSource { return new ArrayIterator( array() ); } } - diff --git a/includes/Specials/SpecialEnableFlow.php b/includes/Specials/SpecialEnableFlow.php index 514b6f1..ca01eac 100644 --- a/includes/Specials/SpecialEnableFlow.php +++ b/includes/Specials/SpecialEnableFlow.php @@ -103,11 +103,12 @@ class SpecialEnableFlow extends FormSpecialPage { wfGetDB( DB_MASTER ), Container::get( 'importer' ), $logger, - $this->occupationController->getTalkpageManager(), + $this->getUser(), new EnableFlowWikitextConversionStrategy( Container::get( 'parser' ), new NullImportSourceStore(), $logger, + $this->getUser(), array(), $data['header'] ) diff --git a/maintenance/convertLqtPageFromRemoteApiForTesting.php b/maintenance/convertLqtPageFromRemoteApiForTesting.php index 3a63bbe..9e5536d 100644 --- a/maintenance/convertLqtPageFromRemoteApiForTesting.php +++ b/maintenance/convertLqtPageFromRemoteApiForTesting.php @@ -74,7 +74,7 @@ class ConvertLqtPageFromRemoteApiForTesting extends Maintenance { $logger->info( "Starting LQT conversion of page $srcPageName" ); - $importer->import( $source, $dstTitle, $sourceStore ); + $importer->import( $source, $dstTitle, $talkPageManagerUser, $sourceStore ); $logger->info( "Finished LQT conversion of page $srcPageName" ); } diff --git a/maintenance/convertNamespaceFromWikitext.php b/maintenance/convertNamespaceFromWikitext.php index 064cd5f..5f758a2 100644 --- a/maintenance/convertNamespaceFromWikitext.php +++ b/maintenance/convertNamespaceFromWikitext.php @@ -68,15 +68,18 @@ class ConvertNamespaceFromWikitext extends Maintenance { $logger = new MaintenanceDebugLogger( $this ); $dbw = wfGetDB( DB_MASTER ); + $talkpageManager = FlowHooks::getOccupationController()->getTalkpageManager(); $converter = new \Flow\Import\Converter( $dbw, Flow\Container::get( 'importer' ), $logger, - FlowHooks::getOccupationController()->getTalkpageManager(), + $talkpageManager, + new Flow\Import\Wikitext\ConversionStrategy( $wgParser, new Flow\Import\SourceStore\Null(), $logger, + $talkpageManager, $noConvertTemplates, $this->getOption( 'header-suffix', null ) ) diff --git a/tests/phpunit/Import/TalkpageImportOperationTest.php b/tests/phpunit/Import/TalkpageImportOperationTest.php index d9c33cd..f0bf942 100644 --- a/tests/phpunit/Import/TalkpageImportOperationTest.php +++ b/tests/phpunit/Import/TalkpageImportOperationTest.php @@ -113,7 +113,13 @@ class TalkpageImportOperationTest extends \MediaWikiTestCase { ) ); - $op = new TalkpageImportOperation( $source, Container::get( 'occupation_controller' ) ); + $occupationController = Container::get( 'occupation_controller' ); + $op = new TalkpageImportOperation( + $source, + $occupationController->getTalkpageManager(), + $occupationController + ); + $store = new NullImportSourceStore; $op->import( new PageImportState( $workflow, diff --git a/tests/phpunit/Import/Wikitext/ConversionStrategyTest.php b/tests/phpunit/Import/Wikitext/ConversionStrategyTest.php index a14d1bc..d00c64e 100644 --- a/tests/phpunit/Import/Wikitext/ConversionStrategyTest.php +++ b/tests/phpunit/Import/Wikitext/ConversionStrategyTest.php @@ -187,7 +187,8 @@ class ConversionStrategyTest extends \MediaWikiTestCase { return new ConversionStrategy( $parser ?: $wgParser, $sourceStore ?: new NullImportSourceStore, - Container::get( 'default_logger' ) + Container::get( 'default_logger' ), + Container::get( 'occupation_controller' )->getTalkpageManager() ); } } diff --git a/tests/phpunit/Import/Wikitext/ImportSourceTest.php b/tests/phpunit/Import/Wikitext/ImportSourceTest.php index da795f4..21877da 100644 --- a/tests/phpunit/Import/Wikitext/ImportSourceTest.php +++ b/tests/phpunit/Import/Wikitext/ImportSourceTest.php @@ -4,6 +4,7 @@ namespace Flow\Tests\Import\Wikitext; use DateTime; use DateTimeZone; +use Flow\Container; use Flow\Exception\WikitextException; use Flow\Import\Wikitext\ImportSource; use Flow\Conversion\Utils; @@ -34,7 +35,9 @@ class ImportSourceTest extends \MediaWikiTestCase { /** * @dataProvider getHeaderProvider */ - public function testGetHeader( $content, $expect ) { + public function testGetHeader( $content, $expectText ) { + $user = Container::get( 'occupation_controller' )->getTalkpageManager(); + // create a page with some content $status = WikiPage::factory( Title::newMainPage() ) ->doEditContent( @@ -45,7 +48,12 @@ class ImportSourceTest extends \MediaWikiTestCase { $this->fail( $status->getMessage()->plain() ); } - $source = new ImportSource( Title::newMainPage(), new Parser ); + $source = new ImportSource( + Title::newMainPage(), + new Parser, + $user + ); + $header = $source->getHeader(); $this->assertNotNull( $header ); $this->assertGreaterThan( 1, strlen( $header->getObjectKey() ) ); @@ -55,7 +63,8 @@ class ImportSourceTest extends \MediaWikiTestCase { $revision = reset( $revisions ); $this->assertInstanceOf( 'Flow\Import\IObjectRevision', $revision ); - $this->assertEquals( $expect, $revision->getText() ); + $this->assertEquals( $expectText, $revision->getText() ); + $this->assertEquals( $user->getName(), $revision->getAuthor() ); } public function getHeaderProvider() { -- 2.1.4