From 4489c3475716e0c70d6ebacc3b82bae7563093ab Mon Sep 17 00:00:00 2001 From: Reo <112817228+reopt999@users.noreply.github.com> Date: Mon, 5 Dec 2022 00:48:21 +0700 Subject: [PATCH] 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 * fix(taglib): Add unit test and exclude scan for only unreadable file - #1576 Signed-off-by: reo * fix(taglib): Add unit test and exclude scan for only unreadable file - #1576 Signed-off-by: reo * fix(taglib): Add unit test and exclude scan for only unreadable file - #1576 Signed-off-by: reo * fix(taglib): Add unit test and exclude scan for only unreadable file - #1576 Signed-off-by: reo * fix(taglib): Add unit test and exclude scan for only unreadable file - #1576 Signed-off-by: reo * 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 Co-authored-by: Deluan --- scanner/metadata/taglib/taglib.go | 15 ++++++---- scanner/metadata/taglib/taglib_test.go | 33 ++++++++++++++++++++- scanner/metadata/taglib/taglib_wrapper.go | 23 +++++++------- scanner/tag_scanner_test.go | 3 +- scanner/walk_dir_tree_test.go | 2 +- tests/fixtures/test_no_read_permission.ogg | Bin 0 -> 5065 bytes 6 files changed, 58 insertions(+), 18 deletions(-) create mode 100644 tests/fixtures/test_no_read_permission.ogg diff --git a/scanner/metadata/taglib/taglib.go b/scanner/metadata/taglib/taglib.go index aa9bea998..6df5aa11e 100644 --- a/scanner/metadata/taglib/taglib.go +++ b/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 } diff --git a/scanner/metadata/taglib/taglib_test.go b/scanner/metadata/taglib/taglib_test.go index 9de0e3bac..4ce228e23 100644 --- a/scanner/metadata/taglib/taglib_test.go +++ b/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)) + }) + }) + }) diff --git a/scanner/metadata/taglib/taglib_wrapper.go b/scanner/metadata/taglib/taglib_wrapper.go index ebeb1e634..a00adca26 100644 --- a/scanner/metadata/taglib/taglib_wrapper.go +++ b/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 diff --git a/scanner/tag_scanner_test.go b/scanner/tag_scanner_test.go index 03cfb23e0..e5bb7be5c 100644 --- a/scanner/tag_scanner_test.go +++ b/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")) diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go index 4a226dc71..30352c32a 100644 --- a/scanner/walk_dir_tree_test.go +++ b/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"))) diff --git a/tests/fixtures/test_no_read_permission.ogg b/tests/fixtures/test_no_read_permission.ogg new file mode 100644 index 0000000000000000000000000000000000000000..be672812991e95e597a322eb3bb021f22143d087 GIT binary patch literal 5065 zcmeGrjaYVuKiQ(ghjHdIOr$!Mz->=;bFnrhnHq1!fVYqs`hnn(SZhE^ z2c)K3NK#r%6eDO!3`%b4sYc!D1=OJ+Wkf^IK$`~B(Od+<%jweH_U_W94_Mgi1*_7I zJ8FxQT?yPh@GUiD58h6f=1gdg08rrN8OUsFI z105JQS3md|j_A;$=bCnQOww>^z4@L}J0XfJ>buS0n{T=)+qpBP2#P$!gf#39X>=XZ z*~Q*)SLQnj1^63+aCs9rHrA0H?86K3;YpHs6<4b^f37y3t)6??xPp~=P8PZaL0TL5 z9`|-+vmBK1Q#iC9OO=Z5X3BAYGyyp&PS2Hq>7hx7wq$!n_YuPSu?9_}r zXaR_SpsKF9M1|1ZkcqBOSx> zB@o1vQ2^y^Ey+Mr0Y?fpfK*pP0d5+1-&+A<8p*{oLy}a6%MmiFs$Nq2tz0z#qvxRK zM3OL^53Z`0jc={oNiOalE}jYg$Av(ol(Ny-)?LG;A5bQQXiFIUst76NC1vM64;PxJ zzc&x)7FIS^8;%lHfj}2euD_4GlnxL@jY9n%o{zX8!}1kXRgKkAqvjM(si0vVY6;(< z(wYEdMDGPLIJOL;FxBGO?jdUC{DTuH@C|m5svM}XZJZ!jyA#LJgxd-^Y-&Y?XaIq^ z9F8wRoW@3AaWLB#HmX;mD(Z2O9XPQF0p-OK#M$zDoE4#Gplq^m&vjU;FkU-u1r;r3 zsLF__|Y zwfe^X29SW+63M^Lz9aq{gB(<7wGE;e3~g(JANWj!9~9{Q zh+;klTUW1BCvGCfT&e!)^OzQxBaHVL#H=>k&ZwKChvl4a$wo}o9bkMut>k(h4 zN|qZLCuSiCp_22!cew!vWCC7{z!8;VFVKjvR>F#65lA_s3`B|mUshT7z&AoHS_!Zs zARb@uMRhQ!^YC@Ti`j^iIKuL1R!22g#)l0HsXjEJ%F7Iq}>yq z>j6v6fC-giHtbkgC$*331#IScfQ01oKpJw{RItW|74h{vU#XX2!g&tht>k#Yc!N3w zX6ONIq_`fu0)u?U9Ye5DFXj|^H_RU?JT zKiXsL-0U5k$u>4rdz&3Qt?Ve4)a?}8omRW7tn3`@ciK>MyrPP7y!=8#hWW5U)M$yq zuIQ|W*(*A~5)Vg_%iB~4<^3`FY9qPA*}yFoS>xY6g!;`csC7>Ko@zDO*pyNqcK+RE zbSA&zmYe!2?1){$`P`T@UrMXm0xih{=ORagV$6SgVosu`pKy15Wz}}}q7xT$(q7Ws zuk!=cdBiCHM(SowR@>$3jQtlZAO6sAq^$UhH4nM745J(au&IUa^S@Sg7QrLFZ`Buy!}uuML_L`(dXvno%;g2RjX*Ksmzto;VN zV#UZ%LIyj*UqkmKk-RX4JGAG1^W`HyuQwp;=+2dVD-KIesJz}?lyL3khnXRz?Dogm zLr`Wp&P%7GfA1}GE)u-1V0mL*^+>>IK>WE;XXEfm)CTI^pt zC$1ZtGa5cx;P)*h1f$WxX2g8Uq1PlaKa`^vIITv zeoj_+g{^EDOe?wj^XIVr#RZE!7riY$=j84GlGNpP_i-rcB%<_Uf>9xgD}8YH-sI4n z=H~-z=f{$?dfv~ybMb|;`d77UdN-L~c>hQD=gmjoJ*&RkamBJW6U*QKOgz=4<=HQj zVeVbYFNq>+5kp{Xcw}B>#<7Jul(D*I9RouSW;_~NIGmNRtD*t>nS!wmeRV$fUJSo{ zaB}nX*foeRfL|+WE_@{Jmeetq-D9|7UW7)(YmwIlaQ3-&dCl*AqA&7t^E|RpVXAsgMOHHJEM8lVGWn@=R8qRpDk zWs-AL>HS-k+s3JVH|G8t2r776)Yg z?*v}toFvL`nc%O2mX>`|s81nTNp9Ptc=L<=i&ezCg`T{&@{enk>VWNl}UDNb_Iot~U<_ zx896Qq@JfGfBgKB_m!o$d4E@tYV-J*VGK{${(<<1cT4W5M^*TvedYOPaD33~{9{K= zqn;(&7=N(X>4(%2HY8%Px(eNCtX$FcV_H*5@|)=CMFRg%r>0IDI@_?tv4)PY-`9)z z?sqxDh)zT79|NS|!tgcSAv1!h#+GY_BVCfHFzo!+atobIdyjD^vgIRec?mg+HM{9a zA}7fX#Vc?3V{5$mT9fZyrE%-%u)F>z`Q}Z9b14mu!cwD;+af;ee|V|VqUy;X->2+yaC2s=!!u`$7&CXT zX_L}$Dben9ZCL4w6t_=@Zo9Esl~C~~(;8~-O4jo|YZo=b)&H6f>nl_ePH?lzCSn|I zGK`sT*1RDybCU*MpwOTIKd{HiV z^{doK{ZA{*XJReewbbKZrQ4E|IhId@qk`7r=NydoCPG48XTGl7$CNZi`fW}R;Ym+W zh#ZSpO*SH!-Rtap7+?D;^FvAB)yW9^Ulq{N#<*3@^OwasokdZWrZz=q4`V;s8T>v) zEGzczeSB^FObs-W60id+hKA!B`T}sm{)K|slFTj?P~?hIPJiab4$5+fM> zQexm(CwED^sY45Un|5Xye$(0XQmHMi*832m3IhL+HwfTR_p6F;RJ9*g6Z z#GRd){^{vQ{uW