From 3a938e18fa33971dad03cd9d7d232017f8bfaa42 Mon Sep 17 00:00:00 2001 From: Taloth Saldono Date: Wed, 21 Jan 2015 23:57:35 +0100 Subject: [PATCH] Fixed: Install Update UI should now report an error if the application folder is not writable instead of failing silently. --- .../Checks/DroneFactoryCheckFixture.cs | 12 +++---- .../HealthCheck/Checks/UpdateCheckFixture.cs | 12 +++---- .../UpdateTests/UpdateServiceFixture.cs | 35 +++++++++++++++++++ .../Download/DownloadClientBase.cs | 15 ++++---- .../HealthCheck/Checks/UpdateCheck.cs | 2 +- .../Update/InstallUpdateService.cs | 23 ++++++++++-- 6 files changed, 74 insertions(+), 25 deletions(-) diff --git a/src/NzbDrone.Core.Test/HealthCheck/Checks/DroneFactoryCheckFixture.cs b/src/NzbDrone.Core.Test/HealthCheck/Checks/DroneFactoryCheckFixture.cs index 3c363c658..06f01333f 100644 --- a/src/NzbDrone.Core.Test/HealthCheck/Checks/DroneFactoryCheckFixture.cs +++ b/src/NzbDrone.Core.Test/HealthCheck/Checks/DroneFactoryCheckFixture.cs @@ -13,7 +13,7 @@ namespace NzbDrone.Core.Test.HealthCheck.Checks { private const string DRONE_FACTORY_FOLDER = @"C:\Test\Unsorted"; - private void GivenDroneFactoryFolder(bool exists = false) + private void GivenDroneFactoryFolder(bool exists = false, bool writable = true) { Mocker.GetMock() .SetupGet(s => s.DownloadedEpisodesFolder) @@ -22,6 +22,10 @@ namespace NzbDrone.Core.Test.HealthCheck.Checks Mocker.GetMock() .Setup(s => s.FolderExists(DRONE_FACTORY_FOLDER)) .Returns(exists); + + Mocker.GetMock() + .Setup(s => s.FolderWritable(It.IsAny())) + .Returns(exists && writable); } [Test] @@ -35,11 +39,7 @@ namespace NzbDrone.Core.Test.HealthCheck.Checks [Test] public void should_return_error_when_unable_to_write_to_drone_factory_folder() { - GivenDroneFactoryFolder(true); - - Mocker.GetMock() - .Setup(s => s.WriteAllText(It.IsAny(), It.IsAny())) - .Throws(); + GivenDroneFactoryFolder(true, false); Subject.Check().ShouldBeError(); } diff --git a/src/NzbDrone.Core.Test/HealthCheck/Checks/UpdateCheckFixture.cs b/src/NzbDrone.Core.Test/HealthCheck/Checks/UpdateCheckFixture.cs index 68123676a..5d66d3220 100644 --- a/src/NzbDrone.Core.Test/HealthCheck/Checks/UpdateCheckFixture.cs +++ b/src/NzbDrone.Core.Test/HealthCheck/Checks/UpdateCheckFixture.cs @@ -21,9 +21,9 @@ namespace NzbDrone.Core.Test.HealthCheck.Checks .Setup(s => s.StartUpFolder) .Returns(@"C:\NzbDrone"); - Mocker.GetMock() - .Setup(s => s.WriteAllText(It.IsAny(), It.IsAny())) - .Throws(); + Mocker.GetMock() + .Setup(c => c.FolderWritable(Moq.It.IsAny())) + .Returns(false); Subject.Check().ShouldBeError(); } @@ -41,9 +41,9 @@ namespace NzbDrone.Core.Test.HealthCheck.Checks .Setup(s => s.StartUpFolder) .Returns(@"/opt/nzbdrone"); - Mocker.GetMock() - .Setup(s => s.WriteAllText(It.IsAny(), It.IsAny())) - .Throws(); + Mocker.GetMock() + .Setup(c => c.FolderWritable(Moq.It.IsAny())) + .Returns(false); Subject.Check().ShouldBeError(); } diff --git a/src/NzbDrone.Core.Test/UpdateTests/UpdateServiceFixture.cs b/src/NzbDrone.Core.Test/UpdateTests/UpdateServiceFixture.cs index 6fbd83684..a8175265d 100644 --- a/src/NzbDrone.Core.Test/UpdateTests/UpdateServiceFixture.cs +++ b/src/NzbDrone.Core.Test/UpdateTests/UpdateServiceFixture.cs @@ -11,6 +11,7 @@ using NzbDrone.Common.Http; using NzbDrone.Common.Model; using NzbDrone.Common.Processes; using NzbDrone.Core.Configuration; +using NzbDrone.Core.Exceptions; using NzbDrone.Core.Test.Framework; using NzbDrone.Core.Update; using NzbDrone.Core.Update.Commands; @@ -59,6 +60,10 @@ namespace NzbDrone.Core.Test.UpdateTests Mocker.GetMock().Setup(c => c.GetCurrentProcess()).Returns(new ProcessInfo { Id = 12 }); Mocker.GetMock().Setup(c => c.ExecutingApplication).Returns(@"C:\Test\NzbDrone.exe"); + Mocker.GetMock() + .Setup(c => c.FolderWritable(It.IsAny())) + .Returns(true); + _sandboxFolder = Mocker.GetMock().Object.GetUpdateSandboxFolder(); } @@ -259,6 +264,36 @@ namespace NzbDrone.Core.Test.UpdateTests ExceptionVerification.ExpectedErrors(1); } + [Test] + public void should_log_error_when_startup_folder_is_not_writable() + { + Mocker.GetMock() + .Setup(c => c.FolderWritable(It.IsAny())) + .Returns(false); + + var updateArchive = Path.Combine(_sandboxFolder, _updatePackage.FileName); + + Subject.Execute(new ApplicationUpdateCommand()); + + Mocker.GetMock().Verify(c => c.DownloadFile(_updatePackage.Url, updateArchive), Times.Never()); + ExceptionVerification.ExpectedErrors(1); + } + + [Test] + public void should_throw_when_install_cannot_be_started() + { + Mocker.GetMock() + .Setup(c => c.FolderWritable(It.IsAny())) + .Returns(false); + + var updateArchive = Path.Combine(_sandboxFolder, _updatePackage.FileName); + + Assert.Throws(() => Subject.Execute(new InstallUpdateCommand() { UpdatePackage = _updatePackage })); + + Mocker.GetMock().Verify(c => c.DownloadFile(_updatePackage.Url, updateArchive), Times.Never()); + ExceptionVerification.ExpectedErrors(1); + } + [TearDown] public void TearDown() { diff --git a/src/NzbDrone.Core/Download/DownloadClientBase.cs b/src/NzbDrone.Core/Download/DownloadClientBase.cs index 61de7769f..89c70c585 100644 --- a/src/NzbDrone.Core/Download/DownloadClientBase.cs +++ b/src/NzbDrone.Core/Download/DownloadClientBase.cs @@ -98,20 +98,17 @@ namespace NzbDrone.Core.Download { return new NzbDroneValidationFailure(propertyName, "Folder does not exist") { - DetailedDescription = "The folder you specified does not exist or is inaccessible. Please verify the folder permissions for the user account that is used to execute NzbDrone." + DetailedDescription = string.Format("The folder you specified does not exist or is inaccessible. Please verify the folder permissions for the user account '{0}', which is used to execute Sonarr.", Environment.UserName) }; } - if (mustBeWritable) + if (mustBeWritable && !_diskProvider.FolderWritable(folder)) { - if (!_diskProvider.FolderWritable(folder)) + _logger.Error("Folder '{0}' is not writable.", folder); + return new NzbDroneValidationFailure(propertyName, "Unable to write to folder") { - _logger.Error("Folder '{0}' is not writable.", folder); - return new NzbDroneValidationFailure(propertyName, "Unable to write to folder") - { - DetailedDescription = "The folder you specified is not writable. Please verify the folder permissions for the user account that is used to execute NzbDrone." - }; - } + DetailedDescription = string.Format("The folder you specified is not writable. Please verify the folder permissions for the user account '{0}', which is used to execute Sonarr.", Environment.UserName) + }; } return null; diff --git a/src/NzbDrone.Core/HealthCheck/Checks/UpdateCheck.cs b/src/NzbDrone.Core/HealthCheck/Checks/UpdateCheck.cs index c9c968a84..5dfd56841 100644 --- a/src/NzbDrone.Core/HealthCheck/Checks/UpdateCheck.cs +++ b/src/NzbDrone.Core/HealthCheck/Checks/UpdateCheck.cs @@ -31,7 +31,7 @@ namespace NzbDrone.Core.HealthCheck.Checks { if (!_diskProvider.FolderWritable(_appFolderInfo.StartUpFolder)) { - return new HealthCheck(GetType(), HealthCheckResult.Error, "Unable to update, running from write-protected folder"); + return new HealthCheck(GetType(), HealthCheckResult.Error, string.Format("Cannot install update because startup folder '{0}' is not writable by the user '{1}'.", _appFolderInfo.StartUpFolder, Environment.UserName)); } } diff --git a/src/NzbDrone.Core/Update/InstallUpdateService.cs b/src/NzbDrone.Core/Update/InstallUpdateService.cs index bdaedbb3a..af6ab6c6e 100644 --- a/src/NzbDrone.Core/Update/InstallUpdateService.cs +++ b/src/NzbDrone.Core/Update/InstallUpdateService.cs @@ -10,6 +10,7 @@ using NzbDrone.Common.Instrumentation.Extensions; using NzbDrone.Common.Processes; using NzbDrone.Core.Backup; using NzbDrone.Core.Configuration; +using NzbDrone.Core.Exceptions; using NzbDrone.Core.Messaging.Commands; using NzbDrone.Core.Update.Commands; @@ -61,12 +62,20 @@ namespace NzbDrone.Core.Update _logger = logger; } - private void InstallUpdate(UpdatePackage updatePackage) + private bool InstallUpdate(UpdatePackage updatePackage) { try { EnsureAppDataSafety(); + if (OsInfo.IsWindows || _configFileProvider.UpdateMechanism != UpdateMechanism.Script) + { + if (!_diskProvider.FolderWritable(_appFolderInfo.StartUpFolder)) + { + throw new ApplicationException(string.Format("Cannot install update because startup folder '{0}' is not writable by the user '{1}'.", _appFolderInfo.StartUpFolder, Environment.UserName)); + } + } + var updateSandboxFolder = _appFolderInfo.GetUpdateSandboxFolder(); var packageDestination = Path.Combine(updateSandboxFolder, updatePackage.FileName); @@ -100,7 +109,7 @@ namespace NzbDrone.Core.Update if (OsInfo.IsNotWindows && _configFileProvider.UpdateMechanism == UpdateMechanism.Script) { InstallUpdateWithScript(updateSandboxFolder); - return; + return true; } _logger.Info("Preparing client"); @@ -111,10 +120,13 @@ namespace NzbDrone.Core.Update _logger.ProgressInfo("NzbDrone will restart shortly."); _processProvider.Start(_appFolderInfo.GetUpdateClientExePath(), GetUpdaterArgs(updateSandboxFolder)); + + return true; } catch (Exception ex) { _logger.ErrorException("Update process failed", ex); + return false; } } @@ -170,7 +182,12 @@ namespace NzbDrone.Core.Update public void Execute(InstallUpdateCommand message) { - InstallUpdate(message.UpdatePackage); + var success = InstallUpdate(message.UpdatePackage); + + if (!success) + { + throw new NzbDroneClientException(System.Net.HttpStatusCode.Conflict, "Failed to install update"); + } } } }