diff --git a/app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Delete.php b/app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Delete.php index 1e9d194653c9c..1b7fdc2881073 100644 --- a/app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Delete.php +++ b/app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Delete.php @@ -9,9 +9,7 @@ use Magento\Backend\App\Action; use Magento\Framework\App\Action\HttpPostActionInterface; -use Magento\Framework\Controller\ResultFactory; use Magento\Framework\Exception\FileSystemException; -use Magento\Framework\Exception\LocalizedException; use Magento\Framework\App\Filesystem\DirectoryList; use Magento\ImportExport\Controller\Adminhtml\Export as ExportController; use Magento\Framework\Filesystem; @@ -56,29 +54,33 @@ public function __construct( /** * Controller basic method implementation. * - * @return \Magento\Framework\App\ResponseInterface|\Magento\Framework\Controller\ResultInterface - * @throws LocalizedException + * @return \Magento\Framework\Controller\ResultInterface */ public function execute() { + /** @var \Magento\Framework\Controller\Result\Redirect $resultRedirect */ + $resultRedirect = $this->resultRedirectFactory->create(); + $resultRedirect->setPath('adminhtml/export/index'); + $fileName = $this->getRequest()->getParam('filename'); + if (empty($fileName) || preg_match('/\.\.(\\\|\/)/', $fileName) !== 0) { + $this->messageManager->addErrorMessage(__('Please provide valid export file name')); + + return $resultRedirect; + } try { - if (empty($fileName = $this->getRequest()->getParam('filename'))) { - throw new LocalizedException(__('Please provide export file name')); - } $directory = $this->filesystem->getDirectoryRead(DirectoryList::VAR_DIR); $path = $directory->getAbsolutePath() . 'export/' . $fileName; - if (!$directory->isFile($path)) { - throw new LocalizedException(__('Sorry, but the data is invalid or the file is not uploaded.')); + if ($directory->isFile($path)) { + $this->file->deleteFile($path); + $this->messageManager->addSuccessMessage(__('File %1 deleted', $fileName)); + } else { + $this->messageManager->addErrorMessage(__('%1 is not a valid file', $fileName)); } - - $this->file->deleteFile($path); - /** @var \Magento\Backend\Model\View\Result\Redirect $resultRedirect */ - $resultRedirect = $this->resultFactory->create(ResultFactory::TYPE_REDIRECT); - $resultRedirect->setPath('adminhtml/export/index'); - return $resultRedirect; } catch (FileSystemException $exception) { - throw new LocalizedException(__('There are no export file with such name %1', $fileName)); + $this->messageManager->addErrorMessage($exception->getMessage()); } + + return $resultRedirect; } } diff --git a/app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Download.php b/app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Download.php index 8dbd9a0ae44ba..4107e19860328 100644 --- a/app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Download.php +++ b/app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Download.php @@ -10,7 +10,6 @@ use Magento\Backend\App\Action; use Magento\Framework\App\Action\HttpGetActionInterface; use Magento\Framework\App\Response\Http\FileFactory; -use Magento\Framework\Exception\LocalizedException; use Magento\Framework\App\Filesystem\DirectoryList; use Magento\ImportExport\Controller\Adminhtml\Export as ExportController; use Magento\Framework\Filesystem; @@ -55,12 +54,17 @@ public function __construct( * Controller basic method implementation. * * @return \Magento\Framework\App\ResponseInterface - * @throws LocalizedException */ public function execute() { - if (empty($fileName = $this->getRequest()->getParam('filename'))) { - throw new LocalizedException(__('Please provide export file name')); + /** @var \Magento\Framework\Controller\Result\Redirect $resultRedirect */ + $resultRedirect = $this->resultRedirectFactory->create(); + $resultRedirect->setPath('adminhtml/export/index'); + $fileName = $this->getRequest()->getParam('filename'); + if (empty($fileName) || preg_match('/\.\.(\\\|\/)/', $fileName) !== 0) { + $this->messageManager->addErrorMessage(__('Please provide valid export file name')); + + return $resultRedirect; } try { $path = 'export/' . $fileName; @@ -72,8 +76,11 @@ public function execute() DirectoryList::VAR_DIR ); } - } catch (LocalizedException | \Exception $exception) { - throw new LocalizedException(__('There are no export file with such name %1', $fileName)); + $this->messageManager->addErrorMessage(__('%1 is not a valid file', $fileName)); + } catch (\Exception $exception) { + $this->messageManager->addErrorMessage($exception->getMessage()); } + + return $resultRedirect; } } diff --git a/app/code/Magento/ImportExport/Test/Unit/Controller/Adminhtml/Export/File/DeleteTest.php b/app/code/Magento/ImportExport/Test/Unit/Controller/Adminhtml/Export/File/DeleteTest.php new file mode 100644 index 0000000000000..ef40651f95be9 --- /dev/null +++ b/app/code/Magento/ImportExport/Test/Unit/Controller/Adminhtml/Export/File/DeleteTest.php @@ -0,0 +1,198 @@ +requestMock = $this->getMockBuilder(Http::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->fileSystemMock = $this->getMockBuilder(Filesystem::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->directoryMock = $this->getMockBuilder(ReadInterface::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->fileMock = $this->getMockBuilder(DriverInterface::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->messageManagerMock = $this->getMockBuilder(ManagerInterface::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->contextMock = $this->createPartialMock( + Context::class, + ['getRequest', 'getResultRedirectFactory', 'getMessageManager'] + ); + + $this->redirectMock = $this->createPartialMock(Redirect::class, ['setPath']); + + $this->resultRedirectFactoryMock = $this->createPartialMock( + RedirectFactory::class, + ['create'] + ); + $this->resultRedirectFactoryMock->expects($this->any())->method('create')->willReturn($this->redirectMock); + $this->contextMock->expects($this->any())->method('getRequest')->willReturn($this->requestMock); + $this->contextMock->expects($this->any()) + ->method('getResultRedirectFactory') + ->willReturn($this->resultRedirectFactoryMock); + + $this->contextMock->expects($this->any()) + ->method('getMessageManager') + ->willReturn($this->messageManagerMock); + + $this->objectManagerHelper = new ObjectManagerHelper($this); + $this->deleteControllerMock = $this->objectManagerHelper->getObject( + Delete::class, + [ + 'context' => $this->contextMock, + 'filesystem' => $this->fileSystemMock, + 'file' => $this->fileMock + ] + ); + } + + /** + * Tests download controller with different file names in request. + */ + public function testExecuteSuccess() + { + $this->requestMock->method('getParam') + ->with('filename') + ->willReturn('sampleFile'); + + $this->fileSystemMock->expects($this->once()) + ->method('getDirectoryRead') + ->will($this->returnValue($this->directoryMock)); + $this->directoryMock->expects($this->once())->method('isFile')->willReturn(true); + $this->fileMock->expects($this->once())->method('deleteFile')->willReturn(true); + $this->messageManagerMock->expects($this->once())->method('addSuccessMessage'); + + $this->deleteControllerMock->execute(); + } + + /** + * Tests download controller with different file names in request. + */ + public function testExecuteFileDoesntExists() + { + $this->requestMock->method('getParam') + ->with('filename') + ->willReturn('sampleFile'); + + $this->fileSystemMock->expects($this->once()) + ->method('getDirectoryRead') + ->will($this->returnValue($this->directoryMock)); + $this->directoryMock->expects($this->once())->method('isFile')->willReturn(false); + $this->messageManagerMock->expects($this->once())->method('addErrorMessage'); + + $this->deleteControllerMock->execute(); + } + + /** + * Test execute() with invalid file name + * @param string $requestFilename + * @dataProvider invalidFileDataProvider + */ + public function testExecuteInvalidFileName($requestFilename) + { + $this->requestMock->method('getParam')->with('filename')->willReturn($requestFilename); + $this->messageManagerMock->expects($this->once())->method('addErrorMessage'); + + $this->deleteControllerMock->execute(); + } + + /** + * Data provider to test possible invalid filenames + * @return array + */ + public function invalidFileDataProvider() + { + return [ + 'Relative file name' => ['../.htaccess'], + 'Empty file name' => [''], + 'Null file name' => [null], + ]; + } +} diff --git a/app/code/Magento/ImportExport/Test/Unit/Controller/Adminhtml/Export/File/DownloadTest.php b/app/code/Magento/ImportExport/Test/Unit/Controller/Adminhtml/Export/File/DownloadTest.php new file mode 100644 index 0000000000000..4512aa6365ca5 --- /dev/null +++ b/app/code/Magento/ImportExport/Test/Unit/Controller/Adminhtml/Export/File/DownloadTest.php @@ -0,0 +1,206 @@ +requestMock = $this->getMockBuilder(Http::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->fileSystemMock = $this->getMockBuilder(Filesystem::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->directoryMock = $this->getMockBuilder(ReadInterface::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->fileFactoryMock = $this->getMockBuilder(FileFactory::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->messageManagerMock = $this->getMockBuilder(ManagerInterface::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->contextMock = $this->createPartialMock( + Context::class, + ['getRequest', 'getResultRedirectFactory', 'getMessageManager'] + ); + + $this->redirectMock = $this->createPartialMock( + Redirect::class, + ['setPath'] + ); + + $this->resultRedirectFactoryMock = $this->createPartialMock( + RedirectFactory::class, + ['create'] + ); + $this->resultRedirectFactoryMock->expects($this->any()) + ->method('create') + ->willReturn($this->redirectMock); + + $this->contextMock->expects($this->any()) + ->method('getRequest') + ->willReturn($this->requestMock); + + $this->contextMock->expects($this->any()) + ->method('getResultRedirectFactory') + ->willReturn($this->resultRedirectFactoryMock); + + $this->contextMock->expects($this->any()) + ->method('getMessageManager') + ->willReturn($this->messageManagerMock); + + $this->objectManagerHelper = new ObjectManagerHelper($this); + $this->downloadControllerMock = $this->objectManagerHelper->getObject( + Download::class, + [ + 'context' => $this->contextMock, + 'filesystem' => $this->fileSystemMock, + 'fileFactory' => $this->fileFactoryMock + ] + ); + } + + /** + * Tests download controller with successful file downloads + */ + public function testExecuteSuccess() + { + $this->requestMock->method('getParam') + ->with('filename') + ->willReturn('sampleFile.csv'); + + $this->fileSystemMock->expects($this->once()) + ->method('getDirectoryRead') + ->will($this->returnValue($this->directoryMock)); + $this->directoryMock->expects($this->once())->method('isFile')->willReturn(true); + $this->fileFactoryMock->expects($this->once())->method('create'); + + $this->downloadControllerMock->execute(); + } + + /** + * Tests download controller with file that doesn't exist + */ + public function testExecuteFileDoesntExists() + { + $this->requestMock->method('getParam') + ->with('filename') + ->willReturn('sampleFile'); + + $this->fileSystemMock->expects($this->once()) + ->method('getDirectoryRead') + ->will($this->returnValue($this->directoryMock)); + $this->directoryMock->expects($this->once())->method('isFile')->willReturn(false); + $this->messageManagerMock->expects($this->once())->method('addErrorMessage'); + + $this->downloadControllerMock->execute(); + } + + /** + * Test execute() with invalid file name + * @param ?string $requestFilename + * @dataProvider invalidFileDataProvider + */ + public function testExecuteInvalidFileName($requestFilename) + { + $this->requestMock->method('getParam')->with('filename')->willReturn($requestFilename); + $this->messageManagerMock->expects($this->once())->method('addErrorMessage'); + + $this->downloadControllerMock->execute(); + } + + /** + * Data provider to test possible invalid filenames + * @return array + */ + public function invalidFileDataProvider() + { + return [ + 'Relative file name' => ['../.htaccess'], + 'Empty file name' => [''], + 'Null file name' => [null], + ]; + } +} diff --git a/app/code/Magento/ImportExport/i18n/en_US.csv b/app/code/Magento/ImportExport/i18n/en_US.csv index fae93c78baa09..a4943fe72826f 100644 --- a/app/code/Magento/ImportExport/i18n/en_US.csv +++ b/app/code/Magento/ImportExport/i18n/en_US.csv @@ -124,3 +124,6 @@ Summary,Summary "Message is added to queue, wait to get your file soon. Make sure your cron job is running to export the file","Message is added to queue, wait to get your file soon. Make sure your cron job is running to export the file" "Invalid data","Invalid data" "Invalid response","Invalid response" +"File %1 deleted","File %1 deleted" +"Please provide valid export file name","Please provide valid export file name" +"%1 is not a valid file","%1 is not a valid file"