Merge pull request #3204 from mikiher/embed-permissions

Fix permission issues in embed/merge

server/managers/AbMergeManager.js

@@ -7,7 +7,7 @@ const { writeConcatFile } = require('../utils/ffmpegHelpers')
 const ffmpegHelpers = require('../utils/ffmpegHelpers')
 const Ffmpeg = require('../libs/fluentFfmpeg')
 const SocketAuthority = require('../SocketAuthority')
-const fileUtils = require('../utils/fileUtils')
+const { isWritable, copyToExisting } = require('../utils/fileUtils')
 const TrackProgressMonitor = require('../objects/TrackProgressMonitor')
 
 class AbMergeManager {
@@ -64,7 +64,7 @@ class AbMergeManager {
 
   async runAudiobookMerge(libraryItem, task, encodingOptions) {
     // Make sure the target directory is writable
-    if (!(await fileUtils.isWritable(libraryItem.path))) {
+    if (!(await isWritable(libraryItem.path))) {
       Logger.error(`[AbMergeManager] Target directory is not writable: ${libraryItem.path}`)
       task.setFailed('Target directory is not writable')
       this.removeTask(task, true)
@@ -139,18 +139,35 @@ class AbMergeManager {
     }
 
     // Move library item tracks to cache
-    for (const trackPath of task.data.originalTrackPaths) {
+    for (const [index, trackPath] of task.data.originalTrackPaths.entries()) {
       const trackFilename = Path.basename(trackPath)
       const moveToPath = Path.join(task.data.itemCachePath, trackFilename)
       Logger.debug(`[AbMergeManager] Backing up original track "${trackPath}" to ${moveToPath}`)
+      if (index === 0) {
+        // copy the first track to the cache directory
+        await fs.copy(trackPath, moveToPath).catch((err) => {
+          Logger.error(`[AbMergeManager] Failed to copy track "${trackPath}" to "${moveToPath}"`, err)
+        })
+      } else {
+        // move the rest of the tracks to the cache directory
         await fs.move(trackPath, moveToPath, { overwrite: true }).catch((err) => {
           Logger.error(`[AbMergeManager] Failed to move track "${trackPath}" to "${moveToPath}"`, err)
         })
       }
+    }
 
-    // Move m4b to target
+    // Move m4b to target, preserving the original track's permissions
     Logger.debug(`[AbMergeManager] Moving m4b from ${task.data.tempFilepath} to ${task.data.targetFilepath}`)
-    await fs.move(task.data.tempFilepath, task.data.targetFilepath)
+    try {
+      await copyToExisting(task.data.tempFilepath, task.data.originalTrackPaths[0])
+      await fs.rename(task.data.originalTrackPaths[0], task.data.targetFilepath)
+      await fs.remove(task.data.tempFilepath)
+    } catch (err) {
+      Logger.error(`[AbMergeManager] Failed to move m4b from ${task.data.tempFilepath} to ${task.data.targetFilepath}`, err)
+      task.setFailed('Failed to move m4b file')
+      this.removeTask(task, true)
+      return
+    }
 
     // Remove ffmetadata file
     await fs.remove(task.data.ffmetadataPath)

server/utils/ffmpegHelpers.js

@@ -4,7 +4,7 @@ const ffmpgegUtils = require('../libs/fluentFfmpeg/utils')
 const fs = require('../libs/fsExtra')
 const Path = require('path')
 const Logger = require('../Logger')
-const { filePathToPOSIX } = require('./fileUtils')
+const { filePathToPOSIX, copyToExisting } = require('./fileUtils')
 const LibraryItem = require('../objects/LibraryItem')
 
 function escapeSingleQuotes(path) {
@@ -250,11 +250,12 @@ module.exports.writeFFMetadataFile = writeFFMetadataFile
  * @param {string} metadataFilePath - Path to the ffmetadata file.
  * @param {number} track - The track number to embed in the audio file.
  * @param {string} mimeType - The MIME type of the audio file.
- * @param {Ffmpeg} ffmpeg - The Ffmpeg instance to use (optional). Used for dependency injection in tests.
  * @param {function(number): void|null} progressCB - A callback function to report progress.
+ * @param {Ffmpeg} ffmpeg - The Ffmpeg instance to use (optional). Used for dependency injection in tests.
+ * @param {function(string, string): Promise<void>} copyFunc - The function to use for copying files (optional). Used for dependency injection in tests.
  * @returns {Promise<void>} A promise that resolves if the operation is successful, rejects otherwise.
  */
-async function addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, progressCB = null, ffmpeg = Ffmpeg()) {
+async function addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, progressCB = null, ffmpeg = Ffmpeg(), copyFunc = copyToExisting) {
   const isMp4 = mimeType === 'audio/mp4'
   const isMp3 = mimeType === 'audio/mpeg'
 
@@ -316,7 +317,8 @@ async function addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataF
         Logger.debug('[ffmpegHelpers] ffmpeg stderr:', stderr)
         Logger.debug('[ffmpegHelpers] Moving temp file to audio file path:', `"${tempFilePath}"`, '->', `"${audioFilePath}"`)
         try {
-          await fs.move(tempFilePath, audioFilePath, { overwrite: true })
+          await copyFunc(tempFilePath, audioFilePath)
+          await fs.remove(tempFilePath)
           resolve()
         } catch (error) {
           Logger.error(`[ffmpegHelpers] Failed to move temp file to audio file path: "${tempFilePath}" -> "${audioFilePath}"`, error)

server/utils/fileUtils.js

@@ -464,3 +464,46 @@ module.exports.getDirectoriesInPath = async (dirPath, level) => {
     return []
   }
 }
+
+/**
+ * Copies a file from the source path to an existing destination path, preserving the destination's permissions.
+ *
+ * @param {string} srcPath - The path of the source file.
+ * @param {string} destPath - The path of the existing destination file.
+ * @returns {Promise<void>} A promise that resolves when the file has been successfully copied.
+ * @throws {Error} If there is an error reading the source file or writing the destination file.
+ */
+async function copyToExisting(srcPath, destPath) {
+  return new Promise((resolve, reject) => {
+    // Create a readable stream from the source file
+    const readStream = fs.createReadStream(srcPath)
+
+    // Create a writable stream to the destination file
+    const writeStream = fs.createWriteStream(destPath, { flags: 'w' })
+
+    // Pipe the read stream to the write stream
+    readStream.pipe(writeStream)
+
+    // Handle the end of the stream
+    writeStream.on('finish', () => {
+      Logger.debug(`[copyToExisting] Successfully copied file from ${srcPath} to ${destPath}`)
+      resolve()
+    })
+
+    // Handle errors
+    readStream.on('error', (error) => {
+      Logger.error(`[copyToExisting] Error reading from source file ${srcPath}: ${error.message}`)
+      readStream.close()
+      writeStream.close()
+      reject(error)
+    })
+
+    writeStream.on('error', (error) => {
+      Logger.error(`[copyToExisting] Error writing to destination file ${destPath}: ${error.message}`)
+      readStream.close()
+      writeStream.close()
+      reject(error)
+    })
+  })
+}
+module.exports.copyToExisting = copyToExisting

test/server/utils/ffmpegHelpers.test.js

@@ -1,9 +1,11 @@
 const { expect } = require('chai')
 const sinon = require('sinon')
-const { generateFFMetadata, addCoverAndMetadataToFile } = require('../../../server/utils/ffmpegHelpers')
+const fileUtils = require('../../../server/utils/fileUtils')
 const fs = require('../../../server/libs/fsExtra')
 const EventEmitter = require('events')
 
+const { generateFFMetadata, addCoverAndMetadataToFile } = require('../../../server/utils/ffmpegHelpers')
+
 global.isWin = process.platform === 'win32'
 
 describe('generateFFMetadata', () => {
@@ -81,9 +83,10 @@ describe('addCoverAndMetadataToFile', () => {
     ffmpegStub.run = sinon.stub().callsFake(() => {
       ffmpegStub.emit('end')
     })
-    const fsMove = sinon.stub(fs, 'move').resolves()
+    const copyStub = sinon.stub().resolves()
+    const fsRemoveStub = sinon.stub(fs, 'remove').resolves()
 
-    return { audioFilePath, coverFilePath, metadataFilePath, track, mimeType, ffmpegStub, fsMove }
+    return { audioFilePath, coverFilePath, metadataFilePath, track, mimeType, ffmpegStub, copyStub, fsRemoveStub }
   }
 
   let audioFilePath = null
@@ -92,7 +95,8 @@ describe('addCoverAndMetadataToFile', () => {
   let track = null
   let mimeType = null
   let ffmpegStub = null
-  let fsMove = null
+  let copyStub = null
+  let fsRemoveStub = null
   beforeEach(() => {
     const input = createTestSetup()
     audioFilePath = input.audioFilePath
@@ -101,12 +105,13 @@ describe('addCoverAndMetadataToFile', () => {
     track = input.track
     mimeType = input.mimeType
     ffmpegStub = input.ffmpegStub
-    fsMove = input.fsMove
+    copyStub = input.copyStub
+    fsRemoveStub = input.fsRemoveStub
   })
 
   it('should add cover image and metadata to audio file', async () => {
     // Act
-    await addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, null, ffmpegStub)
+    await addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, null, ffmpegStub, copyStub)
 
     // Assert
     expect(ffmpegStub.input.calledThrice).to.be.true
@@ -125,10 +130,11 @@ describe('addCoverAndMetadataToFile', () => {
 
     expect(ffmpegStub.run.calledOnce).to.be.true
 
-    expect(fsMove.calledOnce).to.be.true
+    expect(copyStub.calledOnce).to.be.true
-    expect(fsMove.firstCall.args[0]).to.equal('/path/to/audio/file.tmp.mp3')
+    expect(copyStub.firstCall.args[0]).to.equal('/path/to/audio/file.tmp.mp3')
-    expect(fsMove.firstCall.args[1]).to.equal('/path/to/audio/file.mp3')
+    expect(copyStub.firstCall.args[1]).to.equal('/path/to/audio/file.mp3')
-    expect(fsMove.firstCall.args[2]).to.deep.equal({ overwrite: true })
+    expect(fsRemoveStub.calledOnce).to.be.true
+    expect(fsRemoveStub.firstCall.args[0]).to.equal('/path/to/audio/file.tmp.mp3')
 
     // Restore the stub
     sinon.restore()
@@ -139,7 +145,7 @@ describe('addCoverAndMetadataToFile', () => {
     coverFilePath = null
 
     // Act
-    await addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, null, ffmpegStub)
+    await addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, null, ffmpegStub, copyStub)
 
     // Assert
     expect(ffmpegStub.input.calledTwice).to.be.true
@@ -157,10 +163,11 @@ describe('addCoverAndMetadataToFile', () => {
 
     expect(ffmpegStub.run.calledOnce).to.be.true
 
-    expect(fsMove.calledOnce).to.be.true
+    expect(copyStub.callCount).to.equal(1)
-    expect(fsMove.firstCall.args[0]).to.equal('/path/to/audio/file.tmp.mp3')
+    expect(copyStub.firstCall.args[0]).to.equal('/path/to/audio/file.tmp.mp3')
-    expect(fsMove.firstCall.args[1]).to.equal('/path/to/audio/file.mp3')
+    expect(copyStub.firstCall.args[1]).to.equal('/path/to/audio/file.mp3')
-    expect(fsMove.firstCall.args[2]).to.deep.equal({ overwrite: true })
+    expect(fsRemoveStub.calledOnce).to.be.true
+    expect(fsRemoveStub.firstCall.args[0]).to.equal('/path/to/audio/file.tmp.mp3')
 
     // Restore the stub
     sinon.restore()
@@ -174,7 +181,7 @@ describe('addCoverAndMetadataToFile', () => {
 
     // Act
     try {
-      await addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, null, ffmpegStub)
+      await addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, null, ffmpegStub, copyStub)
       expect.fail('Expected an error to be thrown')
     } catch (error) {
       // Assert
@@ -198,7 +205,8 @@ describe('addCoverAndMetadataToFile', () => {
 
     expect(ffmpegStub.run.calledOnce).to.be.true
 
-    expect(fsMove.called).to.be.false
+    expect(copyStub.called).to.be.false
+    expect(fsRemoveStub.called).to.be.false
 
     // Restore the stub
     sinon.restore()
@@ -210,7 +218,7 @@ describe('addCoverAndMetadataToFile', () => {
     audioFilePath = '/path/to/audio/file.m4b'
 
     // Act
-    await addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, null, ffmpegStub)
+    await addCoverAndMetadataToFile(audioFilePath, coverFilePath, metadataFilePath, track, mimeType, null, ffmpegStub, copyStub)
 
     // Assert
     expect(ffmpegStub.input.calledThrice).to.be.true
@@ -229,10 +237,11 @@ describe('addCoverAndMetadataToFile', () => {
 
     expect(ffmpegStub.run.calledOnce).to.be.true
 
-    expect(fsMove.calledOnce).to.be.true
+    expect(copyStub.calledOnce).to.be.true
-    expect(fsMove.firstCall.args[0]).to.equal('/path/to/audio/file.tmp.m4b')
+    expect(copyStub.firstCall.args[0]).to.equal('/path/to/audio/file.tmp.m4b')
-    expect(fsMove.firstCall.args[1]).to.equal('/path/to/audio/file.m4b')
+    expect(copyStub.firstCall.args[1]).to.equal('/path/to/audio/file.m4b')
-    expect(fsMove.firstCall.args[2]).to.deep.equal({ overwrite: true })
+    expect(fsRemoveStub.calledOnce).to.be.true
+    expect(fsRemoveStub.firstCall.args[0]).to.equal('/path/to/audio/file.tmp.m4b')
 
     // Restore the stub
     sinon.restore()