fix(manual-import): authorize companion files against the root folder, not the book folder - #832
Open
m4bard wants to merge 2 commits into
Open
fix(manual-import): authorize companion files against the root folder, not the book folder#832m4bard wants to merge 2 commits into
m4bard wants to merge 2 commits into
Conversation
… the book folder ManualImportCompanionImporter passed destinationRoot to EnsureCreatedHierarchyAsync as the managed boundary. destinationRoot is DetermineScanPath over the batch's destination paths, so for a single-book import it is the book folder. LibraryDirectoryOwnershipBoundaryAuthorizer matches the boundary against configured root folders by equivalence rather than containment, so it refused every companion with 'The requested directory boundary is not a configured root folder'. The exception was caught and logged as a warning, so the import reported success and silently dropped the companions. Select the boundary the way the primary audio file's import already does, via LibraryDirectoryOwnershipPlanning.SelectMostSpecificBoundary over the configured roots, falling back to the destination resolution's boundary path. The authorizer is unchanged and keeps its existing strictness; the companion is now authorized against the same root as the audio file it sits beside.
…p store
The four existing companion tests all mock ILibraryDirectoryOwnershipStore with
It.IsAny<string>() for the boundary, so the real authorizer never runs and nothing in the
suite could tell the book folder from the root folder. All five pass unchanged either way,
which is why this shipped.
ImportAsync_ManagedBoundaryIsTheConfiguredRootFolderRatherThanTheBookFolder captures the
boundary argument instead of the outcome and asserts it is the configured root. It fails
with destinationRoot restored ("library/Author/Book" against the expected "library") and
passes with the fix. The four existing call sites gain the new rootFolders argument.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A manual import with
includeCompanionFiles: truecurrently drops every companion file and still reports success. The companion pass hands the ownership store the book folder as its managed boundary, and the authorizer requires the boundary to be a configured root folder rather than to be inside one, so every companion is refused. This selects the boundary the same way the primary audio file's own import already does.Full write-up, measurements and the alternative I did not take are in #831.
Changes
Fixed
ManualImportCompanionImporter.ImportAsyncselects the managed boundary withLibraryDirectoryOwnershipPlanning.SelectMostSpecificBoundaryover the configured root folders, falling back todestinationResolution.BoundaryPath, instead of passingdestinationRoot.destinationRootisDetermineScanPathover the batch's destination paths, so on a single-book import it is the book folder, which is never a root.ImportAsynctakes arootFoldersargument, supplied at its single call site inManualImportControllerwhererootFoldersis already in scope.The authorizer is untouched and keeps its current strictness. The companion is now authorized against the same root folder that the audio file it sits beside is already authorized against.
Testing
ManualImportCompanionImporterTestsgainsImportAsync_ManagedBoundaryIsTheConfiguredRootFolderRatherThanTheBookFolder, which captures the boundary handed toEnsureCreatedHierarchyAsyncand asserts it is the configured root. WithdestinationRootrestored it fails on the boundary (library/Author/Bookagainst the expectedlibrary); with the change it passes.It asserts the argument rather than the outcome deliberately. The four existing companion tests all mock
ILibraryDirectoryOwnershipStoreand match the boundary withIt.IsAny<string>(), so the real authorizer never runs and none of them can tell the book folder from the root. All four pass unchanged either way, which is a fair explanation for how this shipped. They gain the new argument and nothing else.Full suite on this branch: 3,010 passed, 0 failed, 125 skipped. The skips are environment-gated and sit in areas this does not touch.
Reproduced end to end before and after against
ghcr.io/listenarrs/listenarr:canary. On canary the audio arrives alone, with fourboundary is not a configured root folderrefusals andcompleted with 0 imported companion file(s). On a build of this branch the same run reports four imported, zero refusals, and the blacklisted decoy still filtered. The check is public, in the test-data repo linked from the issue.Notes
The alternative was to relax
LibraryDirectoryOwnershipBoundaryAuthorizerso a boundary inside a root is accepted. I left it out because it widens an authorization check, and because it would change the rename and download-import paths too, neither of which is failing. If you would rather converge that way, say so and I will redo it.One thing the reproduction ruled out, since it is the natural first guess: where the source folder sits makes no difference. The same import with the source outside every configured root folder and again inside one gives an identical result, before and after.