New: Removed chown and simplified chmod options for linux/osx

frontend/src/Settings/MediaManagement/MediaManagement.js

@@ -370,7 +370,7 @@ class MediaManagement extends Component {
                         <FormInputGroup
                           type={inputTypes.CHECK}
                           name="setPermissionsLinux"
-                          helpText="Should chmod/chown be run when files are imported/renamed?"
+                          helpText="Should chmod be run when files are imported/renamed?"
                           helpTextWarning="If you're unsure what these settings do, do not alter them."
                           onChange={onInputChange}
                           {...settings.setPermissionsLinux}
@@ -386,59 +386,14 @@ class MediaManagement extends Component {
                         <FormInputGroup
                           type={inputTypes.TEXT}
                           name="fileChmod"
-                          helpText="Octal, applied to media files when imported/renamed by Lidarr"
+                          helpTexts={[
+                            'Octal, applied to media files when imported/renamed by Lidarr',
+                            'The same mode is applied to movie/sub folders with the execute bit added, e.g., 0644 becomes 0755'
+                          ]}
                           onChange={onInputChange}
                           {...settings.fileChmod}
                         />
                       </FormGroup>
-
-                      <FormGroup
-                        advancedSettings={advancedSettings}
-                        isAdvanced={true}
-                      >
-                        <FormLabel>Folder chmod mode</FormLabel>
-
-                        <FormInputGroup
-                          type={inputTypes.TEXT}
-                          name="folderChmod"
-                          helpText="Octal, applied to artist/album folders created by Lidarr"
-                          values={fileDateOptions}
-                          onChange={onInputChange}
-                          {...settings.folderChmod}
-                        />
-                      </FormGroup>
-
-                      <FormGroup
-                        advancedSettings={advancedSettings}
-                        isAdvanced={true}
-                      >
-                        <FormLabel>chown User</FormLabel>
-
-                        <FormInputGroup
-                          type={inputTypes.TEXT}
-                          name="chownUser"
-                          helpText="Username or uid. Use uid for remote file systems."
-                          values={fileDateOptions}
-                          onChange={onInputChange}
-                          {...settings.chownUser}
-                        />
-                      </FormGroup>
-
-                      <FormGroup
-                        advancedSettings={advancedSettings}
-                        isAdvanced={true}
-                      >
-                        <FormLabel>chown Group</FormLabel>
-
-                        <FormInputGroup
-                          type={inputTypes.TEXT}
-                          name="chownGroup"
-                          helpText="Group name or gid. Use gid for remote file systems."
-                          values={fileDateOptions}
-                          onChange={onInputChange}
-                          {...settings.chownGroup}
-                        />
-                      </FormGroup>
                     </FieldSet>
                 }
               </Form>

src/Lidarr.Api.V1/Config/MediaManagementConfigModule.cs

@@ -1,17 +1,18 @@
-using FluentValidation;
+using FluentValidation;
+using NzbDrone.Common.EnvironmentInfo;
 using NzbDrone.Core.Configuration;
+using NzbDrone.Core.Validation;
 using NzbDrone.Core.Validation.Paths;
 
 namespace Lidarr.Api.V1.Config
 {
     public class MediaManagementConfigModule : LidarrConfigModule<MediaManagementConfigResource>
     {
-        public MediaManagementConfigModule(IConfigService configService, PathExistsValidator pathExistsValidator)
+        public MediaManagementConfigModule(IConfigService configService, PathExistsValidator pathExistsValidator, FileChmodValidator fileChmodValidator)
             : base(configService)
         {
             SharedValidator.RuleFor(c => c.RecycleBinCleanupDays).GreaterThanOrEqualTo(0);
-            SharedValidator.RuleFor(c => c.FileChmod).NotEmpty();
+            SharedValidator.RuleFor(c => c.FileChmod).SetValidator(fileChmodValidator).When(c => !string.IsNullOrEmpty(c.FileChmod) && (OsInfo.IsLinux || OsInfo.IsOsx));
-            SharedValidator.RuleFor(c => c.FolderChmod).NotEmpty();
             SharedValidator.RuleFor(c => c.RecycleBin).IsValidPath().SetValidator(pathExistsValidator).When(c => !string.IsNullOrWhiteSpace(c.RecycleBin));
             SharedValidator.RuleFor(c => c.MinimumFreeSpaceWhenImporting).GreaterThanOrEqualTo(100);
         }

src/Lidarr.Api.V1/Config/MediaManagementConfigResource.cs

@@ -20,9 +20,6 @@ namespace Lidarr.Api.V1.Config
 
         public bool SetPermissionsLinux { get; set; }
         public string FileChmod { get; set; }
-        public string FolderChmod { get; set; }
-        public string ChownUser { get; set; }
-        public string ChownGroup { get; set; }
 
         public bool SkipFreeSpaceCheckWhenImporting { get; set; }
         public int MinimumFreeSpaceWhenImporting { get; set; }
@@ -50,9 +47,6 @@ namespace Lidarr.Api.V1.Config
 
                 SetPermissionsLinux = model.SetPermissionsLinux,
                 FileChmod = model.FileChmod,
-                FolderChmod = model.FolderChmod,
-                ChownUser = model.ChownUser,
-                ChownGroup = model.ChownGroup,
 
                 SkipFreeSpaceCheckWhenImporting = model.SkipFreeSpaceCheckWhenImporting,
                 MinimumFreeSpaceWhenImporting = model.MinimumFreeSpaceWhenImporting,

src/NzbDrone.Common.Test/DiskTests/DiskTransferServiceFixture.cs

@@ -1048,7 +1048,7 @@ namespace NzbDrone.Common.Test.DiskTests
                 .Returns(new List<IFileInfo>());
 
             Mocker.GetMock<IDiskProvider>()
-                .Setup(v => v.CopyPermissions(It.IsAny<string>(), It.IsAny<string>(), false));
+                .Setup(v => v.CopyPermissions(It.IsAny<string>(), It.IsAny<string>()));
         }
 
         private void WithRealDiskProvider()
@@ -1112,7 +1112,7 @@ namespace NzbDrone.Common.Test.DiskTests
                 .Returns<string>(s => new FileStream(s, FileMode.Open, FileAccess.Read));
 
             Mocker.GetMock<IDiskProvider>()
-                .Setup(v => v.CopyPermissions(It.IsAny<string>(), It.IsAny<string>(), false));
+                .Setup(v => v.CopyPermissions(It.IsAny<string>(), It.IsAny<string>()));
         }
 
         private void WithMockMount(string root)

src/NzbDrone.Common/Disk/DiskProviderBase.cs

@@ -38,8 +38,8 @@ namespace NzbDrone.Common.Disk
 
         public abstract long? GetAvailableSpace(string path);
         public abstract void InheritFolderPermissions(string filename);
-        public abstract void SetPermissions(string path, string mask, string user, string group);
+        public abstract void SetPermissions(string path, string mask);
-        public abstract void CopyPermissions(string sourcePath, string targetPath, bool includeOwner);
+        public abstract void CopyPermissions(string sourcePath, string targetPath);
         public abstract long? GetTotalSize(string path);
 
         public DateTime FolderGetCreationTime(string path)
@@ -543,5 +543,10 @@ namespace NzbDrone.Common.Disk
                 stream.CopyTo(fileStream);
             }
         }
+
+        public virtual bool IsValidFilePermissionMask(string mask)
+        {
+            throw new NotSupportedException();
+        }
     }
 }

src/NzbDrone.Common/Disk/IDiskProvider.cs

@@ -11,8 +11,8 @@ namespace NzbDrone.Common.Disk
     {
         long? GetAvailableSpace(string path);
         void InheritFolderPermissions(string filename);
-        void SetPermissions(string path, string mask, string user, string group);
+        void SetPermissions(string path, string mask);
-        void CopyPermissions(string sourcePath, string targetPath, bool includeOwner = false);
+        void CopyPermissions(string sourcePath, string targetPath);
         long? GetTotalSize(string path);
         DateTime FolderGetCreationTime(string path);
         DateTime FolderGetLastWrite(string path);
@@ -55,5 +55,6 @@ namespace NzbDrone.Common.Disk
         List<IFileInfo> GetFileInfos(string path, SearchOption searchOption = SearchOption.TopDirectoryOnly);
         void RemoveEmptySubfolders(string path);
         void SaveStream(Stream stream, string path);
+        bool IsValidFilePermissionMask(string mask);
     }
 }

src/NzbDrone.Core/Configuration/ConfigService.cs

@@ -262,27 +262,6 @@ namespace NzbDrone.Core.Configuration
             set { SetValue("FileChmod", value); }
         }
 
-        public string FolderChmod
-        {
-            get { return GetValue("FolderChmod", "0755"); }
-
-            set { SetValue("FolderChmod", value); }
-        }
-
-        public string ChownUser
-        {
-            get { return GetValue("ChownUser", ""); }
-
-            set { SetValue("ChownUser", value); }
-        }
-
-        public string ChownGroup
-        {
-            get { return GetValue("ChownGroup", ""); }
-
-            set { SetValue("ChownGroup", value); }
-        }
-
         public string MetadataSource
         {
             get { return GetValue("MetadataSource", ""); }

src/NzbDrone.Core/Configuration/IConfigService.cs

@@ -43,9 +43,6 @@ namespace NzbDrone.Core.Configuration
         //Permissions (Media Management)
         bool SetPermissionsLinux { get; set; }
         string FileChmod { get; set; }
-        string FolderChmod { get; set; }
-        string ChownUser { get; set; }
-        string ChownGroup { get; set; }
 
         //Indexers
         int Retention { get; set; }

src/NzbDrone.Core/Datastore/Migration/045_remove_chown_and_folderchmod_config.cs

@@ -0,0 +1,14 @@
+using FluentMigrator;
+using NzbDrone.Core.Datastore.Migration.Framework;
+
+namespace NzbDrone.Core.Datastore.Migration
+{
+    [Migration(045)]
+    public class remove_chown_and_folderchmod_config : NzbDroneMigrationBase
+    {
+        protected override void MainDbUpgrade()
+        {
+            Execute.Sql("DELETE FROM config WHERE Key IN ('folderchmod', 'chownuser')");
+        }
+    }
+}

src/NzbDrone.Core/MediaFiles/DiskScanService.cs

@@ -10,6 +10,7 @@ using NzbDrone.Common;
 using NzbDrone.Common.Disk;
 using NzbDrone.Common.Extensions;
 using NzbDrone.Common.Instrumentation.Extensions;
+using NzbDrone.Core.Configuration;
 using NzbDrone.Core.MediaFiles.Commands;
 using NzbDrone.Core.MediaFiles.Events;
 using NzbDrone.Core.MediaFiles.TrackImport;
@@ -36,6 +37,7 @@ namespace NzbDrone.Core.MediaFiles
         public static readonly Regex ExcludedSubFoldersRegex = new Regex(@"(?:\\|\/|^)(?:extras|@eadir|\.@__thumb|extrafanart|plex versions|\.[^\\/]+)(?:\\|\/)", RegexOptions.Compiled | RegexOptions.IgnoreCase);
         public static readonly Regex ExcludedFilesRegex = new Regex(@"^\._|^Thumbs\.db$|^\.DS_store$|\.partial~$", RegexOptions.Compiled | RegexOptions.IgnoreCase);
 
+        private readonly IConfigService _configService;
         private readonly IDiskProvider _diskProvider;
         private readonly IMediaFileService _mediaFileService;
         private readonly IMakeImportDecision _importDecisionMaker;
@@ -46,7 +48,8 @@ namespace NzbDrone.Core.MediaFiles
         private readonly IEventAggregator _eventAggregator;
         private readonly Logger _logger;
 
-        public DiskScanService(IDiskProvider diskProvider,
+        public DiskScanService(IConfigService configService,
+                               IDiskProvider diskProvider,
                                IMediaFileService mediaFileService,
                                IMakeImportDecision importDecisionMaker,
                                IImportApprovedTracks importApprovedTracks,
@@ -56,6 +59,7 @@ namespace NzbDrone.Core.MediaFiles
                                IEventAggregator eventAggregator,
                                Logger logger)
         {
+            _configService = configService;
             _diskProvider = diskProvider;
             _mediaFileService = mediaFileService;
             _importDecisionMaker = importDecisionMaker;

src/NzbDrone.Core/MediaFiles/MediaFileAttributeService.cs

@@ -62,7 +62,7 @@ namespace NzbDrone.Core.MediaFiles
         {
             if (OsInfo.IsNotWindows)
             {
-                SetMonoPermissions(path, _configService.FolderChmod);
+                SetMonoPermissions(path, _configService.FileChmod);
             }
         }
 
@@ -84,7 +84,7 @@ namespace NzbDrone.Core.MediaFiles
 
             try
             {
-                _diskProvider.SetPermissions(path, permissions, _configService.ChownUser, _configService.ChownGroup);
+                _diskProvider.SetPermissions(path, permissions);
             }
             catch (Exception ex)
             {

src/NzbDrone.Core/Update/InstallUpdateService.cs

@@ -149,7 +149,7 @@ namespace NzbDrone.Core.Update
             // Set executable flag on update app
             if (OsInfo.IsOsx || (OsInfo.IsLinux && PlatformInfo.IsNetCore))
             {
-                _diskProvider.SetPermissions(_appFolderInfo.GetUpdateClientExePath(updatePackage.Runtime), "0755", null, null);
+                _diskProvider.SetPermissions(_appFolderInfo.GetUpdateClientExePath(updatePackage.Runtime), "0755");
             }
 
             _logger.Info("Starting update client {0}", _appFolderInfo.GetUpdateClientExePath(updatePackage.Runtime));

src/NzbDrone.Core/Validation/FileChmodValidator.cs

@@ -0,0 +1,26 @@
+using FluentValidation.Validators;
+using NzbDrone.Common.Disk;
+
+namespace NzbDrone.Core.Validation
+{
+    public class FileChmodValidator : PropertyValidator
+    {
+        private readonly IDiskProvider _diskProvider;
+
+        public FileChmodValidator(IDiskProvider diskProvider)
+            : base("Must contain a valid Unix permissions octal")
+        {
+            _diskProvider = diskProvider;
+        }
+
+        protected override bool IsValid(PropertyValidatorContext context)
+        {
+            if (context.PropertyValue == null)
+            {
+                return false;
+            }
+
+            return _diskProvider.IsValidFilePermissionMask(context.PropertyValue.ToString());
+        }
+    }
+}

src/NzbDrone.Mono.Test/DiskProviderTests/DiskProviderFixture.cs

@@ -151,11 +151,100 @@ namespace NzbDrone.Mono.Test.DiskProviderTests
             Syscall.stat(dst, out var dstStat);
             dstStat.st_mode.Should().Be(origStat.st_mode);
 
-            Subject.CopyPermissions(src, dst, false);
+            Subject.CopyPermissions(src, dst);
 
             // Verify CopyPermissions
             Syscall.stat(dst, out dstStat);
             dstStat.st_mode.Should().Be(srcStat.st_mode);
         }
+
+        [Test]
+        public void should_set_file_permissions()
+        {
+            var tempFile = GetTempFilePath();
+
+            File.WriteAllText(tempFile, "File1");
+            SetWritePermissions(tempFile, false);
+
+            // Verify test setup
+            Syscall.stat(tempFile, out var fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0444");
+
+            Subject.SetPermissions(tempFile, "644");
+            Syscall.stat(tempFile, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0644");
+
+            Subject.SetPermissions(tempFile, "0644");
+            Syscall.stat(tempFile, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0644");
+
+            Subject.SetPermissions(tempFile, "1664");
+            Syscall.stat(tempFile, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("1664");
+        }
+
+        [Test]
+        public void should_set_folder_permissions()
+        {
+            var tempPath = GetTempFilePath();
+
+            Directory.CreateDirectory(tempPath);
+            SetWritePermissions(tempPath, false);
+
+            // Verify test setup
+            Syscall.stat(tempPath, out var fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0555");
+
+            Subject.SetPermissions(tempPath, "644");
+            Syscall.stat(tempPath, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0755");
+
+            Subject.SetPermissions(tempPath, "0644");
+            Syscall.stat(tempPath, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0755");
+
+            Subject.SetPermissions(tempPath, "1664");
+            Syscall.stat(tempPath, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("1775");
+
+            Subject.SetPermissions(tempPath, "775");
+            Syscall.stat(tempPath, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0775");
+
+            Subject.SetPermissions(tempPath, "640");
+            Syscall.stat(tempPath, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0750");
+
+            Subject.SetPermissions(tempPath, "0041");
+            Syscall.stat(tempPath, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0051");
+
+            // reinstate sane permissions so fokder can be cleaned up
+            Subject.SetPermissions(tempPath, "775");
+            Syscall.stat(tempPath, out fileStat);
+            NativeConvert.ToOctalPermissionString(fileStat.st_mode).Should().Be("0775");
+        }
+
+        [Test]
+        public void IsValidFilePermissionMask_should_return_correct()
+        {
+            // Files may not be executable
+            Subject.IsValidFilePermissionMask("0777").Should().BeFalse();
+            Subject.IsValidFilePermissionMask("0544").Should().BeFalse();
+            Subject.IsValidFilePermissionMask("0454").Should().BeFalse();
+            Subject.IsValidFilePermissionMask("0445").Should().BeFalse();
+
+            // No special bits should be set
+            Subject.IsValidFilePermissionMask("1644").Should().BeFalse();
+            Subject.IsValidFilePermissionMask("2644").Should().BeFalse();
+            Subject.IsValidFilePermissionMask("4644").Should().BeFalse();
+            Subject.IsValidFilePermissionMask("7644").Should().BeFalse();
+
+            // Files should be readable and writeable by owner
+            Subject.IsValidFilePermissionMask("0400").Should().BeFalse();
+            Subject.IsValidFilePermissionMask("0000").Should().BeFalse();
+            Subject.IsValidFilePermissionMask("0200").Should().BeFalse();
+            Subject.IsValidFilePermissionMask("0600").Should().BeTrue();
+        }
     }
 }

src/NzbDrone.Mono/Disk/DiskProvider.cs

@@ -83,13 +83,62 @@ namespace NzbDrone.Mono.Disk
             }
         }
 
-        public override void SetPermissions(string path, string mask, string user, string group)
+        public override void SetPermissions(string path, string mask)
         {
-            SetPermissions(path, mask);
+            _logger.Debug("Setting permissions: {0} on {1}", mask, path);
-            SetOwner(path, user, group);
+
+            var permissions = NativeConvert.FromOctalPermissionString(mask);
+
+            if (Directory.Exists(path))
+            {
+                permissions = GetFolderPermissions(permissions);
+            }
+
+            if (Syscall.chmod(path, permissions) < 0)
+            {
+                var error = Stdlib.GetLastError();
+
+                throw new LinuxPermissionsException("Error setting permissions: " + error);
+            }
         }
 
-        public override void CopyPermissions(string sourcePath, string targetPath, bool includeOwner)
+        private static FilePermissions GetFolderPermissions(FilePermissions permissions)
+        {
+            permissions |= (FilePermissions)((int)(permissions & (FilePermissions.S_IRUSR | FilePermissions.S_IRGRP | FilePermissions.S_IROTH)) >> 2);
+
+            return permissions;
+        }
+
+        public override bool IsValidFilePermissionMask(string mask)
+        {
+            try
+            {
+                var permissions = NativeConvert.FromOctalPermissionString(mask);
+
+                if ((permissions & (FilePermissions.S_ISUID | FilePermissions.S_ISGID | FilePermissions.S_ISVTX)) != 0)
+                {
+                    return false;
+                }
+
+                if ((permissions & (FilePermissions.S_IXUSR | FilePermissions.S_IXGRP | FilePermissions.S_IXOTH)) != 0)
+                {
+                    return false;
+                }
+
+                if ((permissions & (FilePermissions.S_IRUSR | FilePermissions.S_IWUSR)) != (FilePermissions.S_IRUSR | FilePermissions.S_IWUSR))
+                {
+                    return false;
+                }
+
+                return true;
+            }
+            catch (FormatException)
+            {
+                return false;
+            }
+        }
+
+        public override void CopyPermissions(string sourcePath, string targetPath)
         {
             try
             {
@@ -100,11 +149,6 @@ namespace NzbDrone.Mono.Disk
                 {
                     Syscall.chmod(targetPath, srcStat.st_mode);
                 }
-
-                if (includeOwner && (srcStat.st_uid != tgtStat.st_uid || srcStat.st_gid != tgtStat.st_gid))
-                {
-                    Syscall.chown(targetPath, srcStat.st_uid, srcStat.st_gid);
-                }
             }
             catch (Exception ex)
             {
@@ -385,39 +429,6 @@ namespace NzbDrone.Mono.Disk
             }
         }
 
-        private void SetPermissions(string path, string mask)
-        {
-            _logger.Debug("Setting permissions: {0} on {1}", mask, path);
-
-            var filePermissions = NativeConvert.FromOctalPermissionString(mask);
-
-            if (Syscall.chmod(path, filePermissions) < 0)
-            {
-                var error = Stdlib.GetLastError();
-
-                throw new LinuxPermissionsException("Error setting file permissions: " + error);
-            }
-        }
-
-        private void SetOwner(string path, string user, string group)
-        {
-            if (string.IsNullOrWhiteSpace(user) && string.IsNullOrWhiteSpace(group))
-            {
-                _logger.Debug("User and Group for chown not configured, skipping chown.");
-                return;
-            }
-
-            var userId = GetUserId(user);
-            var groupId = GetGroupId(group);
-
-            if (Syscall.chown(path, userId, groupId) < 0)
-            {
-                var error = Stdlib.GetLastError();
-
-                throw new LinuxPermissionsException("Error setting file owner and/or group: " + error);
-            }
-        }
-
         private uint GetUserId(string user)
         {
             if (user.IsNullOrWhiteSpace())

src/NzbDrone.Update/UpdateEngine/InstallUpdateService.cs

@@ -122,7 +122,7 @@ namespace NzbDrone.Update.UpdateEngine
                     // Set executable flag on Lidarr app
                     if (OsInfo.IsOsx || (OsInfo.IsLinux && PlatformInfo.IsNetCore))
                     {
-                        _diskProvider.SetPermissions(Path.Combine(installationFolder, "Lidarr"), "0755", null, null);
+                        _diskProvider.SetPermissions(Path.Combine(installationFolder, "Lidarr"), "0755");
                     }
                 }
                 catch (Exception e)

src/NzbDrone.Windows/Disk/DiskProvider.cs

@@ -58,11 +58,11 @@ namespace NzbDrone.Windows.Disk
             file.SetAccessControl(fs);
         }
 
-        public override void SetPermissions(string path, string mask, string user, string group)
+        public override void SetPermissions(string path, string mask)
         {
         }
 
-        public override void CopyPermissions(string sourcePath, string targetPath, bool includeOwner)
+        public override void CopyPermissions(string sourcePath, string targetPath)
         {
         }