Fix Misleading Error Message on unreadable Media due to Permission (#1873)

* fix(taglib): Fix misleading error message on unreadable media - #1576

Signed-off-by: reo <reo_999@proton.me>

* fix(taglib): Add unit test and exclude scan for only unreadable file - #1576

Signed-off-by: reo <reo_999@proton.me>

* fix(taglib): Add unit test and exclude scan for only unreadable file - #1576

Signed-off-by: reo <reo_999@proton.me>

* fix(taglib): Add unit test and exclude scan for only unreadable file - #1576

Signed-off-by: reo <reo_999@proton.me>

* fix(taglib): Add unit test and exclude scan for only unreadable file - #1576

Signed-off-by: reo <reo_999@proton.me>

* fix(taglib): Add unit test and exclude scan for only unreadable file - #1576

Signed-off-by: reo <reo_999@proton.me>

* Fix test and simplify code a bit

We don't need to expose the type of error: `taglib.Parse()` always return nil

* Fix comment

Signed-off-by: reo <reo_999@proton.me>
Co-authored-by: Deluan <deluan@navidrome.org>

scanner/metadata/taglib/taglib.go

@@ -1,6 +1,8 @@
 package taglib
 
 import (
+	"errors"
+	"os"
 	"strconv"
 
 	"github.com/navidrome/navidrome/log"
@@ -13,16 +15,19 @@ type parsedTags = map[string][]string
 func (e *Parser) Parse(paths ...string) (map[string]parsedTags, error) {
 	fileTags := map[string]parsedTags{}
 	for _, path := range paths {
-		fileTags[path] = e.extractMetadata(path)
+		tags, err := e.extractMetadata(path)
+		if !errors.Is(err, os.ErrPermission) {
+			fileTags[path] = tags
+		}
 	}
 	return fileTags, nil
 }
 
-func (e *Parser) extractMetadata(filePath string) parsedTags {
+func (e *Parser) extractMetadata(filePath string) (parsedTags, error) {
 	tags, err := Read(filePath)
 	if err != nil {
-		log.Warn("Error reading metadata from file. Skipping", "filePath", filePath, err)
-		return nil
+		log.Warn("TagLib: Error reading metadata from file. Skipping", "filePath", filePath, err)
+		return nil, err
 	}
 
 	alternativeTags := map[string][]string{
@@ -46,5 +51,5 @@ func (e *Parser) extractMetadata(filePath string) parsedTags {
 			}
 		}
 	}
-	return tags
+	return tags, nil
 }

scanner/metadata/taglib/taglib_test.go

@@ -1,20 +1,38 @@
 package taglib
 
 import (
+	"io/fs"
+	"os"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
 var _ = Describe("Parser", func() {
 	var e *Parser
+	// This file will have 0222 (no read) permission during these tests
+	var accessForbiddenFile = "tests/fixtures/test_no_read_permission.ogg"
+
 	BeforeEach(func() {
 		e = &Parser{}
+
+		err := os.Chmod(accessForbiddenFile, 0222)
+		Expect(err).ToNot(HaveOccurred())
+		DeferCleanup(func() {
+			err = os.Chmod(accessForbiddenFile, 0644)
+			Expect(err).ToNot(HaveOccurred())
+		})
 	})
 	Context("Parse", func() {
 		It("correctly parses metadata from all files in folder", func() {
-			mds, err := e.Parse("tests/fixtures/test.mp3", "tests/fixtures/test.ogg")
+			mds, err := e.Parse(
+				"tests/fixtures/test.mp3",
+				"tests/fixtures/test.ogg",
+				accessForbiddenFile,
+			)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(mds).To(HaveLen(2))
+			Expect(mds).ToNot(HaveKey(accessForbiddenFile))
 
 			m := mds["tests/fixtures/test.mp3"]
 			Expect(m).To(HaveKeyWithValue("title", []string{"Song", "Song"}))
@@ -47,4 +65,17 @@ var _ = Describe("Parser", func() {
 			Expect(m["bitrate"][0]).To(BeElementOf("18", "39"))
 		})
 	})
+
+	Context("Error Checking", func() {
+		It("correctly handle unreadable file due to insufficient read permission", func() {
+			_, err := e.extractMetadata(accessForbiddenFile)
+			Expect(err).To(MatchError(os.ErrPermission))
+		})
+		It("returns a generic ErrPath if file does not exist", func() {
+			testFilePath := "tests/fixtures/NON_EXISTENT.ogg"
+			_, err := e.extractMetadata(testFilePath)
+			Expect(err).To(MatchError(fs.ErrNotExist))
+		})
+	})
+
 })

scanner/metadata/taglib/taglib_wrapper.go

@@ -12,6 +12,7 @@ package taglib
 import "C"
 import (
 	"fmt"
+	"os"
 	"runtime/debug"
 	"strconv"
 	"strings"
@@ -37,17 +38,19 @@ func Read(filename string) (tags map[string][]string, err error) {
 	defer deleteMap(id)
 
 	res := C.taglib_read(fp, C.ulong(id))
-	if log.CurrentLevel() >= log.LevelDebug {
-		switch res {
-		case C.TAGLIB_ERR_PARSE:
-			log.Warn("TagLib: cannot parse file", "filename", filename)
-		case C.TAGLIB_ERR_AUDIO_PROPS:
-			log.Warn("TagLib: can't get audio properties", "filename", filename)
-		}
-	}
+	switch res {
+	case C.TAGLIB_ERR_PARSE:
+		// Check additional case whether the file is unreadable due to permission
+		file, fileErr := os.OpenFile(filename, os.O_RDONLY, 0600)
+		defer file.Close()
 
-	if res != 0 {
-		return nil, fmt.Errorf("cannot process %s", filename)
+		if os.IsPermission(fileErr) {
+			return nil, fmt.Errorf("navidrome does not have permission: %w", fileErr)
+		} else {
+			return nil, fmt.Errorf("cannot parse file media file: %w", fileErr)
+		}
+	case C.TAGLIB_ERR_AUDIO_PROPS:
+		return nil, fmt.Errorf("can't get audio properties from file")
 	}
 	log.Trace("TagLib: read tags", "tags", m, "filename", filename, "id", id)
 	return m, nil

scanner/tag_scanner_test.go

@@ -10,9 +10,10 @@ var _ = Describe("TagScanner", func() {
 		It("return all audio files from the folder", func() {
 			files, err := loadAllAudioFiles("tests/fixtures")
 			Expect(err).ToNot(HaveOccurred())
-			Expect(files).To(HaveLen(3))
+			Expect(files).To(HaveLen(4))
 			Expect(files).To(HaveKey("tests/fixtures/test.ogg"))
 			Expect(files).To(HaveKey("tests/fixtures/test.mp3"))
+			Expect(files).To(HaveKey("tests/fixtures/test_no_read_permission.ogg"))
 			Expect(files).To(HaveKey("tests/fixtures/01 Invisible (RED) Edit Version.mp3"))
 			Expect(files).ToNot(HaveKey("tests/fixtures/._02 Invisible.mp3"))
 			Expect(files).ToNot(HaveKey("tests/fixtures/playlist.m3u"))

scanner/walk_dir_tree_test.go

@@ -36,7 +36,7 @@ var _ = Describe("walk_dir_tree", func() {
 			Expect(collected[baseDir]).To(MatchFields(IgnoreExtras, Fields{
 				"HasImages":       BeTrue(),
 				"HasPlaylist":     BeFalse(),
-				"AudioFilesCount": BeNumerically("==", 4),
+				"AudioFilesCount": BeNumerically("==", 5),
 			}))
 			Expect(collected[filepath.Join(baseDir, "playlists")].HasPlaylist).To(BeTrue())
 			Expect(collected).To(HaveKey(filepath.Join(baseDir, "symlink2dir")))