diff --git a/src/Chaptarr.Api.V1/ImportLists/ImportListController.cs b/src/Chaptarr.Api.V1/ImportLists/ImportListController.cs index 00b09f71..c8973d47 100644 --- a/src/Chaptarr.Api.V1/ImportLists/ImportListController.cs +++ b/src/Chaptarr.Api.V1/ImportLists/ImportListController.cs @@ -1,4 +1,6 @@ +using System; using Chaptarr.Http; +using FluentValidation; using NzbDrone.Core.ImportLists; using NzbDrone.Core.Validation; using NzbDrone.Core.Validation.Paths; @@ -11,11 +13,15 @@ public class ImportListController : ProviderControllerBase r?.Implementation != "HardcoverLibraryImportList" && r?.Implementation != "GoodreadsBookshelf" && @@ -31,6 +37,40 @@ public ImportListController(IImportListFactory importListFactory, SharedValidator.RuleFor(c => c.QualityProfileId).SetValidator(qualityProfileExistsValidator); SharedValidator.RuleFor(c => c.MetadataProfileId).SetValidator(metadataProfileExistsValidator); }); + + // MinRefreshInterval is a fixed, per-list-type constant (see each IImportList implementation) + // and is intentionally excluded from persistence (TableMapping ignores it). Prior to this + // check, a PUT changing it was silently accepted (202) with the change discarded - the + // response goes on to echo the provider's value, not the one that was requested. Reject a + // real change explicitly instead so the client gets a clear error. TimeSpan.Zero (the value + // of an omitted field, since MinRefreshInterval is a non-nullable TimeSpan) is always allowed + // through - no provider uses zero, so this can't mask an actual attempt to zero it out, and it + // keeps clients that only send the fields they mean to change from getting a spurious 400. + PutValidator.RuleFor(c => c.MinRefreshInterval) + .Must((resource, value) => value == default || IsUnchangedOrUnknownList(resource.Id, value)) + .WithMessage(resource => $"minRefreshInterval is fixed by the list type and cannot be changed (current value: {GetProviderMinRefreshInterval(resource.Id)})"); + } + + private bool IsUnchangedOrUnknownList(int id, TimeSpan value) + { + var providerValue = GetProviderMinRefreshInterval(id); + + // An unknown id is left for GetDefinition/_providerFactory.Get to reject with its usual 404 + // rather than surfacing as a validation error here. + return providerValue == null || providerValue == value; + } + + private TimeSpan? GetProviderMinRefreshInterval(int id) + { + var existing = _importListFactory.Find(id); + if (existing == null) + { + return null; + } + + _importListFactory.SetProviderCharacteristics(existing); + + return existing.MinRefreshInterval; } } } diff --git a/src/Chaptarr.Core.Test/Api/ImportListControllerMinRefreshIntervalFixture.cs b/src/Chaptarr.Core.Test/Api/ImportListControllerMinRefreshIntervalFixture.cs new file mode 100644 index 00000000..20d5455d --- /dev/null +++ b/src/Chaptarr.Core.Test/Api/ImportListControllerMinRefreshIntervalFixture.cs @@ -0,0 +1,150 @@ +using System; +using System.Collections.Generic; +using System.Reflection; +using Chaptarr.Api.V1.ImportLists; +using FluentValidation; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using NUnit.Framework; +using NzbDrone.Core.ImportLists; +using NzbDrone.Core.Profiles.Metadata; +using NzbDrone.Core.Profiles.Qualities; +using NzbDrone.Core.Validation; + +namespace Chaptarr.Core.Test.Api +{ + [TestFixture] + public class ImportListControllerMinRefreshIntervalFixture + { + private class ThrowingProxy : DispatchProxy where T : class + { + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + throw new NotImplementedException($"Test proxy does not implement {typeof(T).Name}.{targetMethod?.Name}"); + } + } + + private sealed class StubImportListFactory : IImportListFactory + { + public ImportListDefinition Existing { get; set; } + public TimeSpan ProviderMinRefreshInterval { get; set; } = TimeSpan.FromHours(12); + + public List All() => throw new NotImplementedException(); + public List GetAvailableProviders() => throw new NotImplementedException(); + public bool Exists(int id) => throw new NotImplementedException(); + public ImportListDefinition Find(int id) => Existing != null && Existing.Id == id ? Existing : null; + public ImportListDefinition Get(int id) => Existing != null && Existing.Id == id ? Existing : throw new NotImplementedException(); + public IEnumerable Get(IEnumerable ids) => throw new NotImplementedException(); + public ImportListDefinition Create(ImportListDefinition definition) => throw new NotImplementedException(); + public void Update(ImportListDefinition definition) => throw new NotImplementedException(); + public IEnumerable Update(IEnumerable definitions) => throw new NotImplementedException(); + public void Delete(int id) => throw new NotImplementedException(); + public void Delete(IEnumerable ids) => throw new NotImplementedException(); + public IEnumerable GetDefaultDefinitions() => throw new NotImplementedException(); + public IEnumerable GetPresetDefinitions(ImportListDefinition providerDefinition) => throw new NotImplementedException(); + public void SetProviderCharacteristics(ImportListDefinition definition) => definition.MinRefreshInterval = ProviderMinRefreshInterval; + public void SetProviderCharacteristics(IImportList provider, ImportListDefinition definition) => throw new NotImplementedException(); + public IImportList GetInstance(ImportListDefinition definition) => throw new NotImplementedException(); + public FluentValidation.Results.ValidationResult Test(ImportListDefinition definition) => throw new NotImplementedException(); + public object RequestAction(ImportListDefinition definition, string action, IDictionary query) => throw new NotImplementedException(); + public List AllForTag(int tagId) => throw new NotImplementedException(); + public List AutomaticAddEnabled(bool filterBlockedImportLists = true) => throw new NotImplementedException(); + } + + private static ImportListController BuildController(StubImportListFactory factory) + { + var controller = new ImportListController( + factory, + new QualityProfileExistsValidator(DispatchProxy.Create>()), + new MetadataProfileExistsValidator(DispatchProxy.Create>())); + + controller.ControllerContext = new ControllerContext + { + HttpContext = new DefaultHttpContext() + }; + controller.Request.Method = "PUT"; + + return controller; + } + + // ValidateResource is the same protected method OnActionExecuting calls on every real PUT + // (Chaptarr.Http/REST/RestController.cs). Invoking it directly here, with skipSharedValidate:true, + // isolates the PutValidator rule under test from the unrelated SharedValidator rules (name + // uniqueness, config contract, etc.) that would otherwise require a much heavier fixture. + private static void Validate(ImportListController controller, ImportListResource resource) + { + var method = typeof(ImportListController).GetMethod("ValidateResource", BindingFlags.NonPublic | BindingFlags.Instance); + try + { + method.Invoke(controller, new object[] { resource, false, true }); + } + catch (TargetInvocationException ex) when (ex.InnerException != null) + { + throw ex.InnerException; + } + } + + [Test] + public void should_reject_put_that_changes_min_refresh_interval() + { + var factory = new StubImportListFactory + { + Existing = new ImportListDefinition { Id = 7, MinRefreshInterval = TimeSpan.FromHours(12) }, + ProviderMinRefreshInterval = TimeSpan.FromHours(12) + }; + var controller = BuildController(factory); + + var resource = new ImportListResource { Id = 7, MinRefreshInterval = TimeSpan.FromMinutes(5) }; + + var ex = Assert.Throws(() => Validate(controller, resource)); + + Assert.That(ex.Message, Does.Contain("minRefreshInterval is fixed by the list type")); + Assert.That(ex.Message, Does.Contain("12:00:00")); + } + + [Test] + public void should_allow_put_that_leaves_min_refresh_interval_unchanged() + { + var factory = new StubImportListFactory + { + Existing = new ImportListDefinition { Id = 7, MinRefreshInterval = TimeSpan.FromHours(12) }, + ProviderMinRefreshInterval = TimeSpan.FromHours(12) + }; + var controller = BuildController(factory); + + var resource = new ImportListResource { Id = 7, MinRefreshInterval = TimeSpan.FromHours(12) }; + + Assert.DoesNotThrow(() => Validate(controller, resource)); + } + + [Test] + public void should_allow_put_that_omits_min_refresh_interval() + { + // A non-nullable TimeSpan field that's absent from the JSON body deserializes to + // TimeSpan.Zero, indistinguishable from a client explicitly sending "00:00:00". Since no + // provider ever uses zero, this can't mask a real attempt to change the value, and it stops + // clients that only send the fields they mean to change from getting a spurious 400. + var factory = new StubImportListFactory + { + Existing = new ImportListDefinition { Id = 7, MinRefreshInterval = TimeSpan.FromHours(12) }, + ProviderMinRefreshInterval = TimeSpan.FromHours(12) + }; + var controller = BuildController(factory); + + var resource = new ImportListResource { Id = 7, MinRefreshInterval = TimeSpan.Zero }; + + Assert.DoesNotThrow(() => Validate(controller, resource)); + } + + [Test] + public void should_defer_to_the_normal_not_found_handling_for_an_unknown_id() + { + var factory = new StubImportListFactory { Existing = null }; + var controller = BuildController(factory); + + var resource = new ImportListResource { Id = 999, MinRefreshInterval = TimeSpan.FromMinutes(5) }; + + Assert.DoesNotThrow(() => Validate(controller, resource)); + } + } +}