diff --git a/cmd/scan.go b/cmd/scan.go index 26eb7d7a2..d37ccd69f 100644 --- a/cmd/scan.go +++ b/cmd/scan.go @@ -6,8 +6,6 @@ import ( "os" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" - "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/persistence" @@ -70,7 +68,7 @@ func runScanner(ctx context.Context) { ds := persistence.New(sqlDB) pls := core.NewPlaylists(ds) - progress, err := scanner.CallScan(ctx, ds, artwork.NoopCacheWarmer(), pls, metrics.NewNoopInstance(), fullScan) + progress, err := scanner.CallScan(ctx, ds, pls, fullScan) if err != nil { log.Fatal(ctx, "Failed to scan", err) } diff --git a/conf/configuration.go b/conf/configuration.go index 67f43294d..c3a08bbfa 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -66,6 +66,7 @@ type configOptions struct { CoverArtPriority string CoverJpegQuality int ArtistArtPriority string + LyricsPriority string EnableGravatar bool EnableFavourites bool EnableStarRating bool @@ -86,25 +87,23 @@ type configOptions struct { PasswordEncryptionKey string ReverseProxyUserHeader string ReverseProxyWhitelist string - HTTPSecurityHeaders secureOptions - Prometheus prometheusOptions - Scanner scannerOptions - Jukebox jukeboxOptions - Backup backupOptions - PID pidOptions - Inspect inspectOptions - Subsonic subsonicOptions - LyricsPriority string - - Agents string - LastFM lastfmOptions - Spotify spotifyOptions - ListenBrainz listenBrainzOptions - Tags map[string]TagConf + HTTPSecurityHeaders secureOptions `json:",omitzero"` + Prometheus prometheusOptions `json:",omitzero"` + Scanner scannerOptions `json:",omitzero"` + Jukebox jukeboxOptions `json:",omitzero"` + Backup backupOptions `json:",omitzero"` + PID pidOptions `json:",omitzero"` + Inspect inspectOptions `json:",omitzero"` + Subsonic subsonicOptions `json:",omitzero"` + LastFM lastfmOptions `json:",omitzero"` + Spotify spotifyOptions `json:",omitzero"` + ListenBrainz listenBrainzOptions `json:",omitzero"` + Tags map[string]TagConf `json:",omitempty"` + Agents string // DevFlags. These are used to enable/disable debugging and incomplete features + DevLogLevels map[string]string `json:",omitempty"` DevLogSourceLine bool - DevLogLevels map[string]string DevEnableProfiler bool DevAutoCreateAdminPassword string DevAutoLoginUsername string @@ -112,6 +111,7 @@ type configOptions struct { DevActivityPanelUpdateRate time.Duration DevSidebarPlaylists bool DevShowArtistPage bool + DevUIShowConfig bool DevOffsetOptimize int DevArtworkMaxRequests int DevArtworkThrottleBacklogLimit int @@ -145,12 +145,12 @@ type subsonicOptions struct { } type TagConf struct { - Ignore bool `yaml:"ignore"` - Aliases []string `yaml:"aliases"` - Type string `yaml:"type"` - MaxLength int `yaml:"maxLength"` - Split []string `yaml:"split"` - Album bool `yaml:"album"` + Ignore bool `yaml:"ignore" json:",omitempty"` + Aliases []string `yaml:"aliases" json:",omitempty"` + Type string `yaml:"type" json:",omitempty"` + MaxLength int `yaml:"maxLength" json:",omitempty"` + Split []string `yaml:"split" json:",omitempty"` + Album bool `yaml:"album" json:",omitempty"` } type lastfmOptions struct { @@ -478,7 +478,7 @@ func setViperDefaults() { viper.SetDefault("ignoredarticles", "The El La Los Las Le Les Os As O A") viper.SetDefault("indexgroups", "A B C D E F G H I J K L M N O P Q R S T U V W X-Z(XYZ) [Unknown]([)") viper.SetDefault("ffmpegpath", "") - viper.SetDefault("mpvcmdtemplate", "mpv --audio-device=%d --no-audio-display --pause %f --input-ipc-server=%s") + viper.SetDefault("mpvcmdtemplate", "mpv --audio-device=%d --no-audio-display %f --input-ipc-server=%s") viper.SetDefault("coverartpriority", "cover.*, folder.*, front.*, embedded, external") viper.SetDefault("coverjpegquality", 75) viper.SetDefault("artistartpriority", "artist.*, album/artist.*, external") @@ -553,6 +553,7 @@ func setViperDefaults() { viper.SetDefault("devactivitypanelupdaterate", 300*time.Millisecond) viper.SetDefault("devsidebarplaylists", true) viper.SetDefault("devshowartistpage", true) + viper.SetDefault("devuishowconfig", true) viper.SetDefault("devoffsetoptimize", 50000) viper.SetDefault("devartworkmaxrequests", max(2, runtime.NumCPU()/3)) viper.SetDefault("devartworkthrottlebackloglimit", consts.RequestThrottleBacklogLimit) diff --git a/core/artwork/reader_artist.go b/core/artwork/reader_artist.go index 487346b4d..cb029a16e 100644 --- a/core/artwork/reader_artist.go +++ b/core/artwork/reader_artist.go @@ -20,6 +20,12 @@ import ( "github.com/navidrome/navidrome/utils/str" ) +const ( + // maxArtistFolderTraversalDepth defines how many directory levels to search + // when looking for artist images (artist folder + parent directories) + maxArtistFolderTraversalDepth = 3 +) + type artistReader struct { cacheKey a *artwork @@ -108,36 +114,52 @@ func (a *artistReader) fromArtistArtPriority(ctx context.Context, priority strin func fromArtistFolder(ctx context.Context, artistFolder string, pattern string) sourceFunc { return func() (io.ReadCloser, string, error) { - fsys := os.DirFS(artistFolder) - matches, err := fs.Glob(fsys, pattern) - if err != nil { - log.Warn(ctx, "Error matching artist image pattern", "pattern", pattern, "folder", artistFolder) - return nil, "", err - } - if len(matches) == 0 { - return nil, "", fmt.Errorf(`no matches for '%s' in '%s'`, pattern, artistFolder) - } - for _, m := range matches { - filePath := filepath.Join(artistFolder, m) - if !model.IsImageFile(m) { - continue + current := artistFolder + for i := 0; i < maxArtistFolderTraversalDepth; i++ { + if reader, path, err := findImageInFolder(ctx, current, pattern); err == nil { + return reader, path, nil } - f, err := os.Open(filePath) - if err != nil { - log.Warn(ctx, "Could not open cover art file", "file", filePath, err) - return nil, "", err + + parent := filepath.Dir(current) + if parent == current { + break } - return f, filePath, nil + current = parent } - return nil, "", nil + return nil, "", fmt.Errorf(`no matches for '%s' in '%s' or its parent directories`, pattern, artistFolder) } } +func findImageInFolder(ctx context.Context, folder, pattern string) (io.ReadCloser, string, error) { + log.Trace(ctx, "looking for artist image", "pattern", pattern, "folder", folder) + fsys := os.DirFS(folder) + matches, err := fs.Glob(fsys, pattern) + if err != nil { + log.Warn(ctx, "Error matching artist image pattern", "pattern", pattern, "folder", folder, err) + return nil, "", err + } + + for _, m := range matches { + if !model.IsImageFile(m) { + continue + } + filePath := filepath.Join(folder, m) + f, err := os.Open(filePath) + if err != nil { + log.Warn(ctx, "Could not open cover art file", "file", filePath, err) + continue + } + return f, filePath, nil + } + + return nil, "", fmt.Errorf(`no matches for '%s' in '%s'`, pattern, folder) +} + func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albums, paths []string) (string, time.Time, error) { if len(albums) == 0 { return "", time.Time{}, nil } - libID := albums[0].LibraryID // Just need one of the albums, as they should all be in the same Library + libID := albums[0].LibraryID // Just need one of the albums, as they should all be in the same Library - for now! TODO: Support multiple libraries folderPath := str.LongestCommonPrefix(paths) if !strings.HasSuffix(folderPath, string(filepath.Separator)) { diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go index 294a5db0b..527b0849f 100644 --- a/core/artwork/reader_artist_test.go +++ b/core/artwork/reader_artist_test.go @@ -3,6 +3,8 @@ package artwork import ( "context" "errors" + "io" + "os" "path/filepath" "time" @@ -108,6 +110,254 @@ var _ = Describe("artistArtworkReader", func() { }) }) }) + + var _ = Describe("fromArtistFolder", func() { + var ( + ctx context.Context + tempDir string + testFunc sourceFunc + ) + + BeforeEach(func() { + ctx = context.Background() + tempDir = GinkgoT().TempDir() + }) + + When("artist folder contains matching image", func() { + BeforeEach(func() { + // Create test structure: /temp/artist/artist.jpg + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + artistImagePath := filepath.Join(artistDir, "artist.jpg") + Expect(os.WriteFile(artistImagePath, []byte("fake image data"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("finds and returns the image", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("artist.jpg")) + + // Verify we can read the content + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("fake image data")) + reader.Close() + }) + }) + + When("artist folder is empty but parent contains image", func() { + BeforeEach(func() { + // Create test structure: /temp/parent/artist.jpg and /temp/parent/artist/album/ + parentDir := filepath.Join(tempDir, "parent") + artistDir := filepath.Join(parentDir, "artist") + albumDir := filepath.Join(artistDir, "album") + Expect(os.MkdirAll(albumDir, 0755)).To(Succeed()) + + // Put artist image in parent directory + artistImagePath := filepath.Join(parentDir, "artist.jpg") + Expect(os.WriteFile(artistImagePath, []byte("parent image"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("finds image in parent directory", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("parent" + string(filepath.Separator) + "artist.jpg")) + + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("parent image")) + reader.Close() + }) + }) + + When("image is two levels up", func() { + BeforeEach(func() { + // Create test structure: /temp/grandparent/artist.jpg and /temp/grandparent/parent/artist/ + grandparentDir := filepath.Join(tempDir, "grandparent") + parentDir := filepath.Join(grandparentDir, "parent") + artistDir := filepath.Join(parentDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Put artist image in grandparent directory + artistImagePath := filepath.Join(grandparentDir, "artist.jpg") + Expect(os.WriteFile(artistImagePath, []byte("grandparent image"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("finds image in grandparent directory", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("grandparent" + string(filepath.Separator) + "artist.jpg")) + + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("grandparent image")) + reader.Close() + }) + }) + + When("images exist at multiple levels", func() { + BeforeEach(func() { + // Create test structure with images at multiple levels + grandparentDir := filepath.Join(tempDir, "grandparent") + parentDir := filepath.Join(grandparentDir, "parent") + artistDir := filepath.Join(parentDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Put artist images at all levels + Expect(os.WriteFile(filepath.Join(artistDir, "artist.jpg"), []byte("artist level"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(parentDir, "artist.jpg"), []byte("parent level"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(grandparentDir, "artist.jpg"), []byte("grandparent level"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("prioritizes the closest (artist folder) image", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("artist" + string(filepath.Separator) + "artist.jpg")) + + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("artist level")) + reader.Close() + }) + }) + + When("pattern matches multiple files", func() { + BeforeEach(func() { + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Create multiple matching files + Expect(os.WriteFile(filepath.Join(artistDir, "artist.jpg"), []byte("jpg image"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(artistDir, "artist.png"), []byte("png image"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(artistDir, "artist.txt"), []byte("text file"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("returns the first valid image file", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + + // Should return an image file, not the text file + Expect(path).To(SatisfyAny( + ContainSubstring("artist.jpg"), + ContainSubstring("artist.png"), + )) + Expect(path).ToNot(ContainSubstring("artist.txt")) + reader.Close() + }) + }) + + When("no matching files exist anywhere", func() { + BeforeEach(func() { + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Create non-matching files + Expect(os.WriteFile(filepath.Join(artistDir, "cover.jpg"), []byte("cover image"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("returns an error", func() { + reader, path, err := testFunc() + Expect(err).To(HaveOccurred()) + Expect(reader).To(BeNil()) + Expect(path).To(BeEmpty()) + Expect(err.Error()).To(ContainSubstring("no matches for 'artist.*'")) + Expect(err.Error()).To(ContainSubstring("parent directories")) + }) + }) + + When("directory traversal reaches filesystem root", func() { + BeforeEach(func() { + // Start from a shallow directory to test root boundary + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("handles root boundary gracefully", func() { + reader, path, err := testFunc() + Expect(err).To(HaveOccurred()) + Expect(reader).To(BeNil()) + Expect(path).To(BeEmpty()) + // Should not panic or cause infinite loop + }) + }) + + When("file exists but cannot be opened", func() { + BeforeEach(func() { + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Create a file that cannot be opened (permission denied) + restrictedFile := filepath.Join(artistDir, "artist.jpg") + Expect(os.WriteFile(restrictedFile, []byte("restricted"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("logs warning and continues searching", func() { + // This test depends on the ability to restrict file permissions + // For now, we'll just ensure it doesn't panic and returns appropriate error + reader, _, err := testFunc() + // The file should be readable in test environment, so this will succeed + // In a real scenario with permission issues, it would continue searching + if err == nil { + Expect(reader).ToNot(BeNil()) + reader.Close() + } + }) + }) + + When("single album artist scenario (original issue)", func() { + BeforeEach(func() { + // Simulate the exact folder structure from the issue: + // /music/artist/album1/ (single album) + // /music/artist/artist.jpg (artist image that should be found) + artistDir := filepath.Join(tempDir, "music", "artist") + albumDir := filepath.Join(artistDir, "album1") + Expect(os.MkdirAll(albumDir, 0755)).To(Succeed()) + + // Create artist.jpg in the artist folder (this was not being found before) + artistImagePath := filepath.Join(artistDir, "artist.jpg") + Expect(os.WriteFile(artistImagePath, []byte("single album artist image"), 0600)).To(Succeed()) + + // The fromArtistFolder is called with the artist folder path + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("finds artist.jpg in artist folder for single album artist", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("artist.jpg")) + Expect(path).To(ContainSubstring("artist")) + + // Verify the content + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("single album artist image")) + reader.Close() + }) + }) + }) }) type fakeFolderRepo struct { diff --git a/core/playback/mpv/mpv.go b/core/playback/mpv/mpv.go index 495d27512..f356a1410 100644 --- a/core/playback/mpv/mpv.go +++ b/core/playback/mpv/mpv.go @@ -10,11 +10,15 @@ import ( "strings" "sync" + "github.com/kballard/go-shellquote" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" ) func start(ctx context.Context, args []string) (Executor, error) { + if len(args) == 0 { + return Executor{}, fmt.Errorf("no command arguments provided") + } log.Debug("Executing mpv command", "cmd", args) j := Executor{args: args} j.PipeReader, j.out = io.Pipe() @@ -71,28 +75,32 @@ func (j *Executor) wait() { // Path will always be an absolute path func createMPVCommand(deviceName string, filename string, socketName string) []string { - split := strings.Split(fixCmd(conf.Server.MPVCmdTemplate), " ") - for i, s := range split { - s = strings.ReplaceAll(s, "%d", deviceName) - s = strings.ReplaceAll(s, "%f", filename) - s = strings.ReplaceAll(s, "%s", socketName) - split[i] = s + // Parse the template structure using shell parsing to handle quoted arguments + templateArgs, err := shellquote.Split(conf.Server.MPVCmdTemplate) + if err != nil { + log.Error("Failed to parse MPV command template", "template", conf.Server.MPVCmdTemplate, err) + return nil } - return split -} -func fixCmd(cmd string) string { - split := strings.Split(cmd, " ") - var result []string - cmdPath, _ := mpvCommand() - for _, s := range split { - if s == "mpv" || s == "mpv.exe" { - result = append(result, cmdPath) - } else { - result = append(result, s) + // Replace placeholders in each parsed argument to preserve spaces in substituted values + for i, arg := range templateArgs { + arg = strings.ReplaceAll(arg, "%d", deviceName) + arg = strings.ReplaceAll(arg, "%f", filename) + arg = strings.ReplaceAll(arg, "%s", socketName) + templateArgs[i] = arg + } + + // Replace mpv executable references with the configured path + if len(templateArgs) > 0 { + cmdPath, err := mpvCommand() + if err == nil { + if templateArgs[0] == "mpv" || templateArgs[0] == "mpv.exe" { + templateArgs[0] = cmdPath + } } } - return strings.Join(result, " ") + + return templateArgs } // This is a 1:1 copy of the stuff in ffmpeg.go, need to be unified. diff --git a/core/playback/mpv/mpv_suite_test.go b/core/playback/mpv/mpv_suite_test.go new file mode 100644 index 000000000..f8f827620 --- /dev/null +++ b/core/playback/mpv/mpv_suite_test.go @@ -0,0 +1,17 @@ +package mpv + +import ( + "testing" + + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestMPV(t *testing.T) { + tests.Init(t, false) + log.SetLevel(log.LevelFatal) + RegisterFailHandler(Fail) + RunSpecs(t, "MPV Suite") +} diff --git a/core/playback/mpv/mpv_test.go b/core/playback/mpv/mpv_test.go new file mode 100644 index 000000000..08432bef3 --- /dev/null +++ b/core/playback/mpv/mpv_test.go @@ -0,0 +1,390 @@ +package mpv + +import ( + "context" + "fmt" + "io" + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "time" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("MPV", func() { + var ( + testScript string + tempDir string + ) + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + + // Reset MPV cache + mpvOnce = sync.Once{} + mpvPath = "" + mpvErr = nil + + // Create temporary directory for test files + var err error + tempDir, err = os.MkdirTemp("", "mpv_test_*") + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { os.RemoveAll(tempDir) }) + + // Create mock MPV script that outputs arguments to stdout + testScript = createMockMPVScript(tempDir) + + // Configure test MPV path + conf.Server.MPVPath = testScript + }) + + Describe("createMPVCommand", func() { + Context("with default template", func() { + BeforeEach(func() { + conf.Server.MPVCmdTemplate = "mpv --audio-device=%d --no-audio-display --pause %f --input-ipc-server=%s" + }) + + It("creates correct command with simple paths", func() { + args := createMPVCommand("auto", "/music/test.mp3", "/tmp/socket") + Expect(args).To(Equal([]string{ + testScript, + "--audio-device=auto", + "--no-audio-display", + "--pause", + "/music/test.mp3", + "--input-ipc-server=/tmp/socket", + })) + }) + + It("handles paths with spaces", func() { + args := createMPVCommand("auto", "/music/My Album/01 - Song.mp3", "/tmp/socket") + Expect(args).To(Equal([]string{ + testScript, + "--audio-device=auto", + "--no-audio-display", + "--pause", + "/music/My Album/01 - Song.mp3", + "--input-ipc-server=/tmp/socket", + })) + }) + + It("handles complex device names", func() { + deviceName := "coreaudio/AppleUSBAudioEngine:Cambridge Audio :Cambridge Audio USB Audio 1.0:0000:1" + args := createMPVCommand(deviceName, "/music/test.mp3", "/tmp/socket") + Expect(args).To(Equal([]string{ + testScript, + "--audio-device=" + deviceName, + "--no-audio-display", + "--pause", + "/music/test.mp3", + "--input-ipc-server=/tmp/socket", + })) + }) + }) + + Context("with snapcast template (issue #3619)", func() { + BeforeEach(func() { + // This is the template that fails with naive space splitting + conf.Server.MPVCmdTemplate = "mpv --no-audio-display --pause %f --input-ipc-server=%s --audio-channels=stereo --audio-samplerate=48000 --audio-format=s16 --ao=pcm --ao-pcm-file=/audio/snapcast_fifo" + }) + + It("creates correct command for snapcast integration", func() { + args := createMPVCommand("auto", "/music/test.mp3", "/tmp/socket") + Expect(args).To(Equal([]string{ + testScript, + "--no-audio-display", + "--pause", + "/music/test.mp3", + "--input-ipc-server=/tmp/socket", + "--audio-channels=stereo", + "--audio-samplerate=48000", + "--audio-format=s16", + "--ao=pcm", + "--ao-pcm-file=/audio/snapcast_fifo", + })) + }) + }) + + Context("with wrapper script template", func() { + BeforeEach(func() { + // Test case that would break with naive splitting due to quoted arguments + conf.Server.MPVCmdTemplate = `/tmp/mpv.sh --no-audio-display --pause %f --input-ipc-server=%s --audio-channels=stereo` + }) + + It("handles wrapper script paths", func() { + args := createMPVCommand("auto", "/music/test.mp3", "/tmp/socket") + Expect(args).To(Equal([]string{ + "/tmp/mpv.sh", + "--no-audio-display", + "--pause", + "/music/test.mp3", + "--input-ipc-server=/tmp/socket", + "--audio-channels=stereo", + })) + }) + }) + + Context("with extra spaces in template", func() { + BeforeEach(func() { + conf.Server.MPVCmdTemplate = "mpv --audio-device=%d --no-audio-display --pause %f --input-ipc-server=%s" + }) + + It("handles extra spaces correctly", func() { + args := createMPVCommand("auto", "/music/test.mp3", "/tmp/socket") + Expect(args).To(Equal([]string{ + testScript, + "--audio-device=auto", + "--no-audio-display", + "--pause", + "/music/test.mp3", + "--input-ipc-server=/tmp/socket", + })) + }) + }) + Context("with paths containing spaces in template arguments", func() { + BeforeEach(func() { + // Template with spaces in the path arguments themselves + conf.Server.MPVCmdTemplate = `mpv --no-audio-display --pause %f --ao-pcm-file="/audio/my folder/snapcast_fifo" --input-ipc-server=%s` + }) + + It("handles spaces in quoted template argument paths", func() { + args := createMPVCommand("auto", "/music/test.mp3", "/tmp/socket") + // This test reveals the limitation of strings.Fields() - it will split on all spaces + // Expected behavior would be to keep the path as one argument + Expect(args).To(Equal([]string{ + testScript, + "--no-audio-display", + "--pause", + "/music/test.mp3", + "--ao-pcm-file=/audio/my folder/snapcast_fifo", // This should be one argument + "--input-ipc-server=/tmp/socket", + })) + }) + }) + + Context("with malformed template", func() { + BeforeEach(func() { + // Template with unmatched quotes that will cause shell parsing to fail + conf.Server.MPVCmdTemplate = `mpv --no-audio-display --pause %f --input-ipc-server=%s --ao-pcm-file="/unclosed/quote` + }) + + It("returns nil when shell parsing fails", func() { + args := createMPVCommand("auto", "/music/test.mp3", "/tmp/socket") + Expect(args).To(BeNil()) + }) + }) + + Context("with empty template", func() { + BeforeEach(func() { + conf.Server.MPVCmdTemplate = "" + }) + + It("returns empty slice for empty template", func() { + args := createMPVCommand("auto", "/music/test.mp3", "/tmp/socket") + Expect(args).To(Equal([]string{})) + }) + }) + }) + + Describe("start", func() { + BeforeEach(func() { + conf.Server.MPVCmdTemplate = "mpv --audio-device=%d --no-audio-display --pause %f --input-ipc-server=%s" + }) + + It("executes MPV command and captures arguments correctly", func() { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + deviceName := "auto" + filename := "/music/test.mp3" + socketName := "/tmp/test_socket" + + args := createMPVCommand(deviceName, filename, socketName) + executor, err := start(ctx, args) + Expect(err).ToNot(HaveOccurred()) + + // Read all the output from stdout (this will block until the process finishes or is canceled) + output, err := io.ReadAll(executor) + Expect(err).ToNot(HaveOccurred()) + + // Parse the captured arguments + lines := strings.Split(strings.TrimSpace(string(output)), "\n") + Expect(lines).To(HaveLen(6)) + Expect(lines[0]).To(Equal(testScript)) + Expect(lines[1]).To(Equal("--audio-device=auto")) + Expect(lines[2]).To(Equal("--no-audio-display")) + Expect(lines[3]).To(Equal("--pause")) + Expect(lines[4]).To(Equal("/music/test.mp3")) + Expect(lines[5]).To(Equal("--input-ipc-server=/tmp/test_socket")) + }) + + It("handles file paths with spaces", func() { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + deviceName := "auto" + filename := "/music/My Album/01 - My Song.mp3" + socketName := "/tmp/test socket" + + args := createMPVCommand(deviceName, filename, socketName) + executor, err := start(ctx, args) + Expect(err).ToNot(HaveOccurred()) + + // Read all the output from stdout (this will block until the process finishes or is canceled) + output, err := io.ReadAll(executor) + Expect(err).ToNot(HaveOccurred()) + + // Parse the captured arguments + lines := strings.Split(strings.TrimSpace(string(output)), "\n") + Expect(lines).To(ContainElement("/music/My Album/01 - My Song.mp3")) + Expect(lines).To(ContainElement("--input-ipc-server=/tmp/test socket")) + }) + + Context("with complex snapcast configuration", func() { + BeforeEach(func() { + conf.Server.MPVCmdTemplate = "mpv --no-audio-display --pause %f --input-ipc-server=%s --audio-channels=stereo --audio-samplerate=48000 --audio-format=s16 --ao=pcm --ao-pcm-file=/audio/snapcast_fifo" + }) + + It("passes all snapcast arguments correctly", func() { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + deviceName := "auto" + filename := "/music/album/track.flac" + socketName := "/tmp/mpv-ctrl-test.socket" + + args := createMPVCommand(deviceName, filename, socketName) + executor, err := start(ctx, args) + Expect(err).ToNot(HaveOccurred()) + + // Read all the output from stdout (this will block until the process finishes or is canceled) + output, err := io.ReadAll(executor) + Expect(err).ToNot(HaveOccurred()) + + // Parse the captured arguments + lines := strings.Split(strings.TrimSpace(string(output)), "\n") + + // Verify all expected arguments are present + Expect(lines).To(ContainElement("--no-audio-display")) + Expect(lines).To(ContainElement("--pause")) + Expect(lines).To(ContainElement("/music/album/track.flac")) + Expect(lines).To(ContainElement("--input-ipc-server=/tmp/mpv-ctrl-test.socket")) + Expect(lines).To(ContainElement("--audio-channels=stereo")) + Expect(lines).To(ContainElement("--audio-samplerate=48000")) + Expect(lines).To(ContainElement("--audio-format=s16")) + Expect(lines).To(ContainElement("--ao=pcm")) + Expect(lines).To(ContainElement("--ao-pcm-file=/audio/snapcast_fifo")) + }) + }) + + Context("with nil args", func() { + It("returns error when args is nil", func() { + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + defer cancel() + + _, err := start(ctx, nil) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("no command arguments provided")) + }) + + It("returns error when args is empty", func() { + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + defer cancel() + + _, err := start(ctx, []string{}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("no command arguments provided")) + }) + }) + }) + + Describe("mpvCommand", func() { + BeforeEach(func() { + // Reset the mpv command cache + mpvOnce = sync.Once{} + mpvPath = "" + mpvErr = nil + }) + + It("finds the configured MPV path", func() { + conf.Server.MPVPath = testScript + path, err := mpvCommand() + Expect(err).ToNot(HaveOccurred()) + Expect(path).To(Equal(testScript)) + }) + }) + + Describe("NewTrack integration", func() { + var testMediaFile model.MediaFile + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.MPVPath = testScript + + // Create a test media file + testMediaFile = model.MediaFile{ + ID: "test-id", + Path: "/music/test.mp3", + } + }) + + Context("with malformed template", func() { + BeforeEach(func() { + // Template with unmatched quotes that will cause shell parsing to fail + conf.Server.MPVCmdTemplate = `mpv --no-audio-display --pause %f --input-ipc-server=%s --ao-pcm-file="/unclosed/quote` + }) + + It("returns error when createMPVCommand fails", func() { + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + defer cancel() + + playbackDone := make(chan bool, 1) + _, err := NewTrack(ctx, playbackDone, "auto", testMediaFile) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("no mpv command arguments provided")) + }) + }) + }) +}) + +// createMockMPVScript creates a mock script that outputs arguments to stdout +func createMockMPVScript(tempDir string) string { + var scriptContent string + var scriptExt string + + if runtime.GOOS == "windows" { + scriptExt = ".bat" + scriptContent = `@echo off +echo %0 +:loop +if "%~1"=="" goto end +echo %~1 +shift +goto loop +:end +` + } else { + scriptExt = ".sh" + scriptContent = `#!/bin/bash +echo "$0" +for arg in "$@"; do + echo "$arg" +done +` + } + + scriptPath := filepath.Join(tempDir, "mock_mpv"+scriptExt) + err := os.WriteFile(scriptPath, []byte(scriptContent), 0755) // nolint:gosec + if err != nil { + panic(fmt.Sprintf("Failed to create mock script: %v", err)) + } + + return scriptPath +} diff --git a/core/playback/mpv/track.go b/core/playback/mpv/track.go index b894ff3ad..14170efd4 100644 --- a/core/playback/mpv/track.go +++ b/core/playback/mpv/track.go @@ -34,7 +34,10 @@ func NewTrack(ctx context.Context, playbackDoneChannel chan bool, deviceName str tmpSocketName := socketName("mpv-ctrl-", ".socket") - args := createMPVCommand(deviceName, mf.Path, tmpSocketName) + args := createMPVCommand(deviceName, mf.AbsolutePath(), tmpSocketName) + if len(args) == 0 { + return nil, fmt.Errorf("no mpv command arguments provided") + } exe, err := start(ctx, args) if err != nil { log.Error("Error starting mpv process", err) diff --git a/go.mod b/go.mod index 5c09c0b17..513ebb4a4 100644 --- a/go.mod +++ b/go.mod @@ -34,6 +34,7 @@ require ( github.com/hashicorp/go-multierror v1.1.1 github.com/jellydator/ttlcache/v3 v3.3.0 github.com/kardianos/service v1.2.2 + github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 github.com/kr/pretty v0.3.1 github.com/lestrrat-go/jwx/v2 v2.1.6 github.com/matoous/go-nanoid/v2 v2.1.0 @@ -85,7 +86,6 @@ require ( github.com/gorilla/css v1.0.1 // indirect github.com/hashicorp/errwrap v1.1.0 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect - github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect github.com/klauspost/cpuid/v2 v2.2.10 // indirect github.com/kr/text v0.2.0 // indirect github.com/lann/builder v0.0.0-20180802200727-47ae307949d0 // indirect diff --git a/model/playlist.go b/model/playlist.go index c06fbeb9b..6380cfb4d 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -104,6 +104,7 @@ type PlaylistRepository interface { FindByPath(path string) (*Playlist, error) Delete(id string) error Tracks(playlistId string, refreshSmartPlaylist bool) PlaylistTrackRepository + GetPlaylists(mediaFileId string) (Playlists, error) } type PlaylistTrack struct { diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index ac3b60bb2..a279ef2ee 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -203,6 +203,25 @@ func (r *playlistRepository) GetAll(options ...model.QueryOptions) (model.Playli return playlists, err } +func (r *playlistRepository) GetPlaylists(mediaFileId string) (model.Playlists, error) { + sel := r.selectPlaylist(model.QueryOptions{Sort: "name"}). + Join("playlist_tracks on playlist.id = playlist_tracks.playlist_id"). + Where(And{Eq{"playlist_tracks.media_file_id": mediaFileId}, r.userFilter()}) + var res []dbPlaylist + err := r.queryAll(sel, &res) + if err != nil { + if errors.Is(err, model.ErrNotFound) { + return model.Playlists{}, nil + } + return nil, err + } + playlists := make(model.Playlists, len(res)) + for i, p := range res { + playlists[i] = p.Playlist + } + return playlists, nil +} + func (r *playlistRepository) selectPlaylist(options ...model.QueryOptions) SelectBuilder { query := r.newSelect(options...).Join("user on user.id = owner_id"). Columns(r.tableName+".*", "user.user_name as owner_name") diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index 9bfa52e3c..aac643cc4 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -152,6 +152,21 @@ var _ = Describe("PlaylistRepository", func() { }) }) + Describe("GetPlaylists", func() { + It("returns playlists for a track", func() { + pls, err := repo.GetPlaylists(songRadioactivity.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(pls).To(HaveLen(1)) + Expect(pls[0].ID).To(Equal(plsBest.ID)) + }) + + It("returns empty when none", func() { + pls, err := repo.GetPlaylists("9999") + Expect(err).ToNot(HaveOccurred()) + Expect(pls).To(HaveLen(0)) + }) + }) + Context("Smart Playlists", func() { var rules *criteria.Criteria BeforeEach(func() { diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index d33bd5113..80925aa88 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -99,10 +99,10 @@ func (r *playlistTrackRepository) Read(id string) (interface{}, error) { "playlist_tracks.*", ). Join("media_file f on f.id = media_file_id"). - Where(And{Eq{"playlist_id": r.playlistId}, Eq{"id": id}}) + Where(And{Eq{"playlist_id": r.playlistId}, Eq{"playlist_tracks.id": id}}) var trk dbPlaylistTrack err := r.queryOne(sel, &trk) - return trk.PlaylistTrack.MediaFile, err + return trk.PlaylistTrack, err } func (r *playlistTrackRepository) GetAll(options ...model.QueryOptions) (model.PlaylistTracks, error) { diff --git a/resources/i18n/es.json b/resources/i18n/es.json index 2fdbb8fda..b640ec115 100644 --- a/resources/i18n/es.json +++ b/resources/i18n/es.json @@ -94,7 +94,7 @@ "recentlyPlayed": "Recientes", "mostPlayed": "Más reproducidos", "starred": "Favoritos", - "topRated": "Los mejores calificados" + "topRated": "Mejor calificados" } }, "artist": { @@ -523,4 +523,4 @@ "current_song": "Canción actual" } } -} \ No newline at end of file +} diff --git a/resources/i18n/pt-br.json b/resources/i18n/pt-br.json index febdcf769..cfb3c8485 100644 --- a/resources/i18n/pt-br.json +++ b/resources/i18n/pt-br.json @@ -197,11 +197,17 @@ "export": "Exportar", "makePublic": "Pública", "makePrivate": "Pessoal", - "saveQueue": "Salvar fila em nova Playlist" + "saveQueue": "Salvar fila em nova Playlist", + "searchOrCreate": "Buscar playlists ou criar nova...", + "pressEnterToCreate": "Pressione Enter para criar nova playlist", + "removeFromSelection": "Remover da seleção", + "removeSymbol": "×" }, "message": { "duplicate_song": "Adicionar músicas duplicadas", - "song_exist": "Algumas destas músicas já existem na playlist. Você quer adicionar as duplicadas ou ignorá-las?" + "song_exist": "Algumas destas músicas já existem na playlist. Você quer adicionar as duplicadas ou ignorá-las?", + "noPlaylistsFound": "Nenhuma playlist encontrada", + "noPlaylists": "Nenhuma playlist disponível" } }, "radio": { @@ -496,6 +502,21 @@ "disabled": "Desligado", "waiting": "Aguardando" } + }, + "tabs": { + "about": "Sobre", + "config": "Configuração" + }, + "config": { + "configName": "Nome da Configuração", + "environmentVariable": "Variável de Ambiente", + "currentValue": "Valor Atual", + "configurationFile": "Arquivo de Configuração", + "exportToml": "Exportar Configuração (TOML)", + "exportSuccess": "Configuração exportada para o clipboard em formato TOML", + "exportFailed": "Falha ao copiar configuração", + "devFlagsHeader": "Flags de Desenvolvimento (sujeitas a mudança/remoção)", + "devFlagsComment": "Estas são configurações experimentais e podem ser removidas em versões futuras" } }, "activity": { @@ -523,4 +544,4 @@ "current_song": "Vai para música atual" } } -} \ No newline at end of file +} diff --git a/scanner/controller.go b/scanner/controller.go index 0b3e5d122..a6aa0ae8c 100644 --- a/scanner/controller.go +++ b/scanner/controller.go @@ -63,13 +63,12 @@ func (s *controller) getScanner() scanner { if conf.Server.DevExternalScanner { return &scannerExternal{} } - return &scannerImpl{ds: s.ds, cw: s.cw, pls: s.pls, metrics: s.metrics} + return &scannerImpl{ds: s.ds, cw: s.cw, pls: s.pls} } // CallScan starts an in-process scan of the music library. // This is meant to be called from the command line (see cmd/scan.go). -func CallScan(ctx context.Context, ds model.DataStore, cw artwork.CacheWarmer, pls core.Playlists, - metrics metrics.Metrics, fullScan bool) (<-chan *ProgressInfo, error) { +func CallScan(ctx context.Context, ds model.DataStore, pls core.Playlists, fullScan bool) (<-chan *ProgressInfo, error) { release, err := lockScan(ctx) if err != nil { return nil, err @@ -80,7 +79,7 @@ func CallScan(ctx context.Context, ds model.DataStore, cw artwork.CacheWarmer, p progress := make(chan *ProgressInfo, 100) go func() { defer close(progress) - scanner := &scannerImpl{ds: ds, cw: cw, pls: pls, metrics: metrics} + scanner := &scannerImpl{ds: ds, cw: artwork.NoopCacheWarmer(), pls: pls} scanner.scanAll(ctx, fullScan, progress) }() return progress, nil @@ -230,9 +229,11 @@ func (s *controller) ScanAll(requestCtx context.Context, fullScan bool) ([]strin } // Send the final scan status event, with totals if count, folderCount, err := s.getCounters(ctx); err != nil { + s.metrics.WriteAfterScanMetrics(ctx, false) return scanWarnings, err } else { scanType, elapsed, lastErr := s.getScanInfo(ctx) + s.metrics.WriteAfterScanMetrics(ctx, true) s.sendMessage(ctx, &events.ScanStatus{ Scanning: false, Count: count, diff --git a/scanner/scanner.go b/scanner/scanner.go index f08dec311..5edac5d65 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -11,7 +11,6 @@ import ( "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" - "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -19,10 +18,9 @@ import ( ) type scannerImpl struct { - ds model.DataStore - cw artwork.CacheWarmer - pls core.Playlists - metrics metrics.Metrics + ds model.DataStore + cw artwork.CacheWarmer + pls core.Playlists } // scanState holds the state of an in-progress scan, to be passed to the various phases @@ -111,7 +109,6 @@ func (s *scannerImpl) scanAll(ctx context.Context, fullScan bool, progress chan< log.Error(ctx, "Scanner: Finished with error", "duration", time.Since(startTime), err) _ = s.ds.Property(ctx).Put(consts.LastScanErrorKey, err.Error()) state.sendError(err) - s.metrics.WriteAfterScanMetrics(ctx, false) return } @@ -121,7 +118,6 @@ func (s *scannerImpl) scanAll(ctx context.Context, fullScan bool, progress chan< state.sendProgress(&ProgressInfo{ChangesDetected: true}) } - s.metrics.WriteAfterScanMetrics(ctx, err == nil) log.Info(ctx, "Scanner: Finished scanning all libraries", "duration", time.Since(startTime)) } diff --git a/server/auth.go b/server/auth.go index 5b35f72ed..86d5221ca 100644 --- a/server/auth.go +++ b/server/auth.go @@ -171,7 +171,7 @@ func validateLogin(userRepo model.UserRepository, userName, password string) (*m return u, nil } -func jwtVerifier(next http.Handler) http.Handler { +func JWTVerifier(next http.Handler) http.Handler { return jwtauth.Verify(auth.TokenAuth, tokenFromHeader, jwtauth.TokenFromCookie, jwtauth.TokenFromQuery)(next) } diff --git a/server/nativeapi/config.go b/server/nativeapi/config.go new file mode 100644 index 000000000..d708d72f9 --- /dev/null +++ b/server/nativeapi/config.go @@ -0,0 +1,138 @@ +package nativeapi + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "strings" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model/request" +) + +// sensitiveFieldsPartialMask contains configuration field names that should be redacted +// using partial masking (first and last character visible, middle replaced with *). +// For values with 7+ characters: "secretvalue123" becomes "s***********3" +// For values with <7 characters: "short" becomes "****" +// Add field paths using dot notation (e.g., "LastFM.ApiKey", "Spotify.Secret") +var sensitiveFieldsPartialMask = []string{ + "LastFM.ApiKey", + "LastFM.Secret", + "Prometheus.MetricsPath", + "Spotify.ID", + "Spotify.Secret", + "DevAutoLoginUsername", +} + +// sensitiveFieldsFullMask contains configuration field names that should always be +// completely masked with "****" regardless of their length. +// Add field paths using dot notation for any fields that should never show any content. +var sensitiveFieldsFullMask = []string{ + "DevAutoCreateAdminPassword", + "PasswordEncryptionKey", + "Prometheus.Password", +} + +type configResponse struct { + ID string `json:"id"` + ConfigFile string `json:"configFile"` + Config map[string]interface{} `json:"config"` +} + +func redactValue(key string, value string) string { + // Return empty values as-is + if len(value) == 0 { + return value + } + + // Check if this field should be fully masked + for _, field := range sensitiveFieldsFullMask { + if field == key { + return "****" + } + } + + // Check if this field should be partially masked + for _, field := range sensitiveFieldsPartialMask { + if field == key { + if len(value) < 7 { + return "****" + } + // Show first and last character with * in between + return string(value[0]) + strings.Repeat("*", len(value)-2) + string(value[len(value)-1]) + } + } + + // Return original value if not sensitive + return value +} + +// applySensitiveFieldMasking recursively applies masking to sensitive fields in the configuration map +func applySensitiveFieldMasking(ctx context.Context, config map[string]interface{}, prefix string) { + for key, value := range config { + fullKey := key + if prefix != "" { + fullKey = prefix + "." + key + } + + switch v := value.(type) { + case map[string]interface{}: + // Recursively process nested maps + applySensitiveFieldMasking(ctx, v, fullKey) + case string: + // Apply masking to string values + config[key] = redactValue(fullKey, v) + default: + // For other types (numbers, booleans, etc.), convert to string and check for masking + if str := fmt.Sprint(v); str != "" { + masked := redactValue(fullKey, str) + if masked != str { + // Only replace if masking was applied + config[key] = masked + } + } + } + } +} + +func getConfig(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + user, _ := request.UserFrom(ctx) + if !user.IsAdmin { + http.Error(w, "Config endpoint is only available to admin users", http.StatusUnauthorized) + return + } + + // Marshal the actual configuration struct to preserve original field names + configBytes, err := json.Marshal(*conf.Server) + if err != nil { + log.Error(ctx, "Error marshaling config", err) + http.Error(w, "Internal server error", http.StatusInternalServerError) + return + } + + // Unmarshal back to map to get the structure with proper field names + var configMap map[string]interface{} + err = json.Unmarshal(configBytes, &configMap) + if err != nil { + log.Error(ctx, "Error unmarshaling config to map", err) + http.Error(w, "Internal server error", http.StatusInternalServerError) + return + } + + // Apply sensitive field masking + applySensitiveFieldMasking(ctx, configMap, "") + + resp := configResponse{ + ID: "config", + ConfigFile: conf.Server.ConfigFile, + Config: configMap, + } + + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(resp); err != nil { + log.Error(ctx, "Error encoding config response", err) + } +} diff --git a/server/nativeapi/config_test.go b/server/nativeapi/config_test.go new file mode 100644 index 000000000..52baef83a --- /dev/null +++ b/server/nativeapi/config_test.go @@ -0,0 +1,147 @@ +package nativeapi + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("getConfig", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + + Context("when user is not admin", func() { + It("returns unauthorized", func() { + req := httptest.NewRequest("GET", "/config", nil) + w := httptest.NewRecorder() + ctx := request.WithUser(req.Context(), model.User{IsAdmin: false}) + + getConfig(w, req.WithContext(ctx)) + + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) + }) + + Context("when user is admin", func() { + It("returns config successfully", func() { + req := httptest.NewRequest("GET", "/config", nil) + w := httptest.NewRecorder() + ctx := request.WithUser(req.Context(), model.User{IsAdmin: true}) + + getConfig(w, req.WithContext(ctx)) + + Expect(w.Code).To(Equal(http.StatusOK)) + var resp configResponse + Expect(json.Unmarshal(w.Body.Bytes(), &resp)).To(Succeed()) + Expect(resp.ID).To(Equal("config")) + Expect(resp.ConfigFile).To(Equal(conf.Server.ConfigFile)) + Expect(resp.Config).ToNot(BeEmpty()) + }) + + It("redacts sensitive fields", func() { + conf.Server.LastFM.ApiKey = "secretapikey123" + conf.Server.Spotify.Secret = "spotifysecret456" + conf.Server.PasswordEncryptionKey = "encryptionkey789" + conf.Server.DevAutoCreateAdminPassword = "adminpassword123" + conf.Server.Prometheus.Password = "prometheuspass" + + req := httptest.NewRequest("GET", "/config", nil) + w := httptest.NewRecorder() + ctx := request.WithUser(req.Context(), model.User{IsAdmin: true}) + getConfig(w, req.WithContext(ctx)) + + Expect(w.Code).To(Equal(http.StatusOK)) + var resp configResponse + Expect(json.Unmarshal(w.Body.Bytes(), &resp)).To(Succeed()) + + // Check LastFM.ApiKey (partially masked) + lastfm, ok := resp.Config["LastFM"].(map[string]interface{}) + Expect(ok).To(BeTrue()) + Expect(lastfm["ApiKey"]).To(Equal("s*************3")) + + // Check Spotify.Secret (partially masked) + spotify, ok := resp.Config["Spotify"].(map[string]interface{}) + Expect(ok).To(BeTrue()) + Expect(spotify["Secret"]).To(Equal("s**************6")) + + // Check PasswordEncryptionKey (fully masked) + Expect(resp.Config["PasswordEncryptionKey"]).To(Equal("****")) + + // Check DevAutoCreateAdminPassword (fully masked) + Expect(resp.Config["DevAutoCreateAdminPassword"]).To(Equal("****")) + + // Check Prometheus.Password (fully masked) + prometheus, ok := resp.Config["Prometheus"].(map[string]interface{}) + Expect(ok).To(BeTrue()) + Expect(prometheus["Password"]).To(Equal("****")) + }) + + It("handles empty sensitive values", func() { + conf.Server.LastFM.ApiKey = "" + conf.Server.PasswordEncryptionKey = "" + + req := httptest.NewRequest("GET", "/config", nil) + w := httptest.NewRecorder() + ctx := request.WithUser(req.Context(), model.User{IsAdmin: true}) + getConfig(w, req.WithContext(ctx)) + + Expect(w.Code).To(Equal(http.StatusOK)) + var resp configResponse + Expect(json.Unmarshal(w.Body.Bytes(), &resp)).To(Succeed()) + + // Check LastFM.ApiKey - should be preserved because it's sensitive + lastfm, ok := resp.Config["LastFM"].(map[string]interface{}) + Expect(ok).To(BeTrue()) + Expect(lastfm["ApiKey"]).To(Equal("")) + + // Empty sensitive values should remain empty - should be preserved because it's sensitive + Expect(resp.Config["PasswordEncryptionKey"]).To(Equal("")) + }) + }) +}) + +var _ = Describe("redactValue function", func() { + It("partially masks long sensitive values", func() { + Expect(redactValue("LastFM.ApiKey", "ba46f0e84a")).To(Equal("b********a")) + Expect(redactValue("Spotify.Secret", "verylongsecret123")).To(Equal("v***************3")) + }) + + It("fully masks long sensitive values that should be completely hidden", func() { + Expect(redactValue("PasswordEncryptionKey", "1234567890")).To(Equal("****")) + Expect(redactValue("DevAutoCreateAdminPassword", "1234567890")).To(Equal("****")) + Expect(redactValue("Prometheus.Password", "1234567890")).To(Equal("****")) + }) + + It("fully masks short sensitive values", func() { + Expect(redactValue("LastFM.Secret", "short")).To(Equal("****")) + Expect(redactValue("Spotify.ID", "abc")).To(Equal("****")) + Expect(redactValue("PasswordEncryptionKey", "12345")).To(Equal("****")) + Expect(redactValue("DevAutoCreateAdminPassword", "short")).To(Equal("****")) + Expect(redactValue("Prometheus.Password", "short")).To(Equal("****")) + }) + + It("does not mask non-sensitive values", func() { + Expect(redactValue("MusicFolder", "/path/to/music")).To(Equal("/path/to/music")) + Expect(redactValue("Port", "4533")).To(Equal("4533")) + Expect(redactValue("SomeOtherField", "secretvalue")).To(Equal("secretvalue")) + }) + + It("handles empty values", func() { + Expect(redactValue("LastFM.ApiKey", "")).To(Equal("")) + Expect(redactValue("NonSensitive", "")).To(Equal("")) + }) + + It("handles edge case values", func() { + Expect(redactValue("LastFM.ApiKey", "a")).To(Equal("****")) + Expect(redactValue("LastFM.ApiKey", "ab")).To(Equal("****")) + Expect(redactValue("LastFM.ApiKey", "abcdefg")).To(Equal("a*****g")) + }) +}) diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index ddf5df1c3..3586a86a0 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -59,23 +59,12 @@ func (n *Router) routes() http.Handler { n.addPlaylistRoute(r) n.addPlaylistTrackRoute(r) + n.addSongPlaylistsRoute(r) n.addMissingFilesRoute(r) n.addInspectRoute(r) - - // Keepalive endpoint to be used to keep the session valid (ex: while playing songs) - r.Get("/keepalive/*", func(w http.ResponseWriter, r *http.Request) { - _, _ = w.Write([]byte(`{"response":"ok", "id":"keepalive"}`)) - }) - - // Insights status endpoint - r.Get("/insights/*", func(w http.ResponseWriter, r *http.Request) { - last, success := n.insights.LastRun(r.Context()) - if conf.Server.EnableInsightsCollector { - _, _ = w.Write([]byte(`{"id":"insights_status", "lastRun":"` + last.Format("2006-01-02 15:04:05") + `", "success":` + strconv.FormatBool(success) + `}`)) - } else { - _, _ = w.Write([]byte(`{"id":"insights_status", "lastRun":"disabled", "success":false}`)) - } - }) + n.addConfigRoute(r) + n.addKeepAliveRoute(r) + n.addInsightsRoute(r) }) return r @@ -144,6 +133,9 @@ func (n *Router) addPlaylistTrackRoute(r chi.Router) { }) r.Route("/{id}", func(r chi.Router) { r.Use(server.URLParamsMiddleware) + r.Get("/", func(w http.ResponseWriter, r *http.Request) { + getPlaylistTrack(n.ds)(w, r) + }) r.Put("/", func(w http.ResponseWriter, r *http.Request) { reorderItem(n.ds)(w, r) }) @@ -154,6 +146,12 @@ func (n *Router) addPlaylistTrackRoute(r chi.Router) { }) } +func (n *Router) addSongPlaylistsRoute(r chi.Router) { + r.With(server.URLParamsMiddleware).Get("/song/{id}/playlists", func(w http.ResponseWriter, r *http.Request) { + getSongPlaylists(n.ds)(w, r) + }) +} + func (n *Router) addMissingFilesRoute(r chi.Router) { r.Route("/missing", func(r chi.Router) { n.RX(r, "/", newMissingRepository(n.ds), false) @@ -196,3 +194,26 @@ func (n *Router) addInspectRoute(r chi.Router) { }) } } + +func (n *Router) addConfigRoute(r chi.Router) { + if conf.Server.DevUIShowConfig { + r.Get("/config/*", getConfig) + } +} + +func (n *Router) addKeepAliveRoute(r chi.Router) { + r.Get("/keepalive/*", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"response":"ok", "id":"keepalive"}`)) + }) +} + +func (n *Router) addInsightsRoute(r chi.Router) { + r.Get("/insights/*", func(w http.ResponseWriter, r *http.Request) { + last, success := n.insights.LastRun(r.Context()) + if conf.Server.EnableInsightsCollector { + _, _ = w.Write([]byte(`{"id":"insights_status", "lastRun":"` + last.Format("2006-01-02 15:04:05") + `", "success":` + strconv.FormatBool(success) + `}`)) + } else { + _, _ = w.Write([]byte(`{"id":"insights_status", "lastRun":"disabled", "success":false}`)) + } + }) +} diff --git a/server/nativeapi/native_api_song_test.go b/server/nativeapi/native_api_song_test.go new file mode 100644 index 000000000..0b183c1d9 --- /dev/null +++ b/server/nativeapi/native_api_song_test.go @@ -0,0 +1,464 @@ +package nativeapi + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "time" + + "github.com/deluan/rest" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/core/metrics" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/server" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// Simple mock implementations for missing types +type mockShare struct { + core.Share +} + +func (m *mockShare) NewRepository(ctx context.Context) rest.Repository { + return &tests.MockShareRepo{} +} + +type mockPlaylists struct { + core.Playlists +} + +func (m *mockPlaylists) ImportFile(ctx context.Context, folder *model.Folder, filename string) (*model.Playlist, error) { + return &model.Playlist{}, nil +} + +type mockInsights struct { + metrics.Insights +} + +func (m *mockInsights) LastRun(ctx context.Context) (time.Time, bool) { + return time.Now(), true +} + +var _ = Describe("Song Endpoints", func() { + var ( + router http.Handler + ds *tests.MockDataStore + mfRepo *tests.MockMediaFileRepo + userRepo *tests.MockedUserRepo + w *httptest.ResponseRecorder + testUser model.User + testSongs model.MediaFiles + ) + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.SessionTimeout = time.Minute + + // Setup mock repositories + mfRepo = tests.CreateMockMediaFileRepo() + userRepo = tests.CreateMockUserRepo() + + ds = &tests.MockDataStore{ + MockedMediaFile: mfRepo, + MockedUser: userRepo, + MockedProperty: &tests.MockedPropertyRepo{}, + } + + // Initialize auth system + auth.Init(ds) + + // Create test user + testUser = model.User{ + ID: "user-1", + UserName: "testuser", + Name: "Test User", + IsAdmin: false, + NewPassword: "testpass", + } + err := userRepo.Put(&testUser) + Expect(err).ToNot(HaveOccurred()) + + // Create test songs + testSongs = model.MediaFiles{ + { + ID: "song-1", + Title: "Test Song 1", + Artist: "Test Artist 1", + Album: "Test Album 1", + AlbumID: "album-1", + ArtistID: "artist-1", + Duration: 180.5, + BitRate: 320, + Path: "/music/song1.mp3", + Suffix: "mp3", + Size: 5242880, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + }, + { + ID: "song-2", + Title: "Test Song 2", + Artist: "Test Artist 2", + Album: "Test Album 2", + AlbumID: "album-2", + ArtistID: "artist-2", + Duration: 240.0, + BitRate: 256, + Path: "/music/song2.mp3", + Suffix: "mp3", + Size: 7340032, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + }, + } + mfRepo.SetData(testSongs) + + // Setup router with mocked dependencies + mockShareImpl := &mockShare{} + mockPlaylistsImpl := &mockPlaylists{} + mockInsightsImpl := &mockInsights{} + + // Create the native API router and wrap it with the JWTVerifier middleware + nativeRouter := New(ds, mockShareImpl, mockPlaylistsImpl, mockInsightsImpl) + router = server.JWTVerifier(nativeRouter) + w = httptest.NewRecorder() + }) + + // Helper function to create unauthenticated request + createUnauthenticatedRequest := func(method, path string, body []byte) *http.Request { + var req *http.Request + if body != nil { + req = httptest.NewRequest(method, path, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + } else { + req = httptest.NewRequest(method, path, nil) + } + return req + } + + // Helper function to create authenticated request with JWT token + createAuthenticatedRequest := func(method, path string, body []byte) *http.Request { + req := createUnauthenticatedRequest(method, path, body) + + // Create JWT token for the test user + token, err := auth.CreateToken(&testUser) + Expect(err).ToNot(HaveOccurred()) + + // Add JWT token to Authorization header + req.Header.Set(consts.UIAuthorizationHeader, "Bearer "+token) + + return req + } + + Describe("GET /song", func() { + Context("when user is authenticated", func() { + It("returns all songs", func() { + req := createAuthenticatedRequest("GET", "/song", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + + var response []model.MediaFile + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).ToNot(HaveOccurred()) + + Expect(response).To(HaveLen(2)) + Expect(response[0].ID).To(Equal("song-1")) + Expect(response[0].Title).To(Equal("Test Song 1")) + Expect(response[1].ID).To(Equal("song-2")) + Expect(response[1].Title).To(Equal("Test Song 2")) + }) + + It("handles repository errors gracefully", func() { + mfRepo.SetError(true) + + req := createAuthenticatedRequest("GET", "/song", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + }) + }) + + Context("when user is not authenticated", func() { + It("returns unauthorized", func() { + req := createUnauthenticatedRequest("GET", "/song", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) + }) + }) + + Describe("GET /song/{id}", func() { + Context("when user is authenticated", func() { + It("returns the specific song", func() { + req := createAuthenticatedRequest("GET", "/song/song-1", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + + var response model.MediaFile + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).ToNot(HaveOccurred()) + + Expect(response.ID).To(Equal("song-1")) + Expect(response.Title).To(Equal("Test Song 1")) + Expect(response.Artist).To(Equal("Test Artist 1")) + }) + + It("returns 404 for non-existent song", func() { + req := createAuthenticatedRequest("GET", "/song/non-existent", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusNotFound)) + }) + + It("handles repository errors gracefully", func() { + mfRepo.SetError(true) + + req := createAuthenticatedRequest("GET", "/song/song-1", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + }) + }) + + Context("when user is not authenticated", func() { + It("returns unauthorized", func() { + req := createUnauthenticatedRequest("GET", "/song/song-1", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) + }) + }) + + Describe("Song endpoints are read-only", func() { + Context("POST /song", func() { + It("should not be available (songs are not persistable)", func() { + newSong := model.MediaFile{ + Title: "New Song", + Artist: "New Artist", + Album: "New Album", + Duration: 200.0, + } + + body, _ := json.Marshal(newSong) + req := createAuthenticatedRequest("POST", "/song", body) + router.ServeHTTP(w, req) + + // Should return 405 Method Not Allowed or 404 Not Found + Expect(w.Code).To(Equal(http.StatusMethodNotAllowed)) + }) + }) + + Context("PUT /song/{id}", func() { + It("should not be available (songs are not persistable)", func() { + updatedSong := model.MediaFile{ + ID: "song-1", + Title: "Updated Song", + Artist: "Updated Artist", + Album: "Updated Album", + Duration: 250.0, + } + + body, _ := json.Marshal(updatedSong) + req := createAuthenticatedRequest("PUT", "/song/song-1", body) + router.ServeHTTP(w, req) + + // Should return 405 Method Not Allowed or 404 Not Found + Expect(w.Code).To(Equal(http.StatusMethodNotAllowed)) + }) + }) + + Context("DELETE /song/{id}", func() { + It("should not be available (songs are not persistable)", func() { + req := createAuthenticatedRequest("DELETE", "/song/song-1", nil) + router.ServeHTTP(w, req) + + // Should return 405 Method Not Allowed or 404 Not Found + Expect(w.Code).To(Equal(http.StatusMethodNotAllowed)) + }) + }) + }) + + Describe("Query parameters and filtering", func() { + Context("when using query parameters", func() { + It("handles pagination parameters", func() { + req := createAuthenticatedRequest("GET", "/song?_start=0&_end=1", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + + var response []model.MediaFile + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).ToNot(HaveOccurred()) + + // Should still return all songs since our mock doesn't implement pagination + // but the request should be processed successfully + Expect(len(response)).To(BeNumerically(">=", 1)) + }) + + It("handles sort parameters", func() { + req := createAuthenticatedRequest("GET", "/song?_sort=title&_order=ASC", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + + var response []model.MediaFile + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).ToNot(HaveOccurred()) + + Expect(response).To(HaveLen(2)) + }) + + It("handles filter parameters", func() { + // Properly encode the URL with query parameters + baseURL := "/song" + params := url.Values{} + params.Add("title", "Test Song 1") + fullURL := baseURL + "?" + params.Encode() + + req := createAuthenticatedRequest("GET", fullURL, nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + + var response []model.MediaFile + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).ToNot(HaveOccurred()) + + // Mock doesn't implement filtering, but request should be processed + Expect(len(response)).To(BeNumerically(">=", 1)) + }) + }) + }) + + Describe("Response headers and content type", func() { + It("sets correct content type for JSON responses", func() { + req := createAuthenticatedRequest("GET", "/song", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Type")).To(ContainSubstring("application/json")) + }) + + It("includes total count header when available", func() { + req := createAuthenticatedRequest("GET", "/song", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + // The X-Total-Count header might be set by the REST framework + // We just verify the request is processed successfully + }) + }) + + Describe("Edge cases and error handling", func() { + Context("when repository is unavailable", func() { + It("handles database connection errors", func() { + mfRepo.SetError(true) + + req := createAuthenticatedRequest("GET", "/song", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + }) + }) + + Context("when no songs exist", func() { + It("returns empty array when no songs are found", func() { + mfRepo.SetData(model.MediaFiles{}) // Empty dataset + + req := createAuthenticatedRequest("GET", "/song", nil) + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + + var response []model.MediaFile + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).ToNot(HaveOccurred()) + + Expect(response).To(HaveLen(0)) + }) + }) + }) + + Describe("Authentication middleware integration", func() { + Context("with different user types", func() { + It("works with admin users", func() { + adminUser := model.User{ + ID: "admin-1", + UserName: "admin", + Name: "Admin User", + IsAdmin: true, + NewPassword: "adminpass", + } + err := userRepo.Put(&adminUser) + Expect(err).ToNot(HaveOccurred()) + + // Create JWT token for admin user + token, err := auth.CreateToken(&adminUser) + Expect(err).ToNot(HaveOccurred()) + + req := createUnauthenticatedRequest("GET", "/song", nil) + req.Header.Set(consts.UIAuthorizationHeader, "Bearer "+token) + + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + }) + + It("works with regular users", func() { + regularUser := model.User{ + ID: "user-2", + UserName: "regular", + Name: "Regular User", + IsAdmin: false, + NewPassword: "userpass", + } + err := userRepo.Put(®ularUser) + Expect(err).ToNot(HaveOccurred()) + + // Create JWT token for regular user + token, err := auth.CreateToken(®ularUser) + Expect(err).ToNot(HaveOccurred()) + + req := createUnauthenticatedRequest("GET", "/song", nil) + req.Header.Set(consts.UIAuthorizationHeader, "Bearer "+token) + + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + }) + }) + + Context("with missing authentication context", func() { + It("rejects requests without user context", func() { + req := createUnauthenticatedRequest("GET", "/song", nil) + // No authentication header added + + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) + + It("rejects requests with invalid JWT tokens", func() { + req := createUnauthenticatedRequest("GET", "/song", nil) + req.Header.Set(consts.UIAuthorizationHeader, "Bearer invalid.token.here") + + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) + }) + }) +}) diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go index 1e8e961ca..17af19475 100644 --- a/server/nativeapi/playlists.go +++ b/server/nativeapi/playlists.go @@ -45,6 +45,23 @@ func getPlaylist(ds model.DataStore) http.HandlerFunc { } } +func getPlaylistTrack(ds model.DataStore) http.HandlerFunc { + // Add a middleware to capture the playlistId + wrapper := func(handler restHandler) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + constructor := func(ctx context.Context) rest.Repository { + plsRepo := ds.Playlist(ctx) + plsId := chi.URLParam(r, "playlistId") + return plsRepo.Tracks(plsId, true) + } + + handler(constructor).ServeHTTP(w, r) + } + } + + return wrapper(rest.Get) +} + func createPlaylistFromM3U(playlists core.Playlists) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -207,3 +224,21 @@ func reorderItem(ds model.DataStore) http.HandlerFunc { } } } + +func getSongPlaylists(ds model.DataStore) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + p := req.Params(r) + trackId, _ := p.String(":id") + playlists, err := ds.Playlist(r.Context()).GetPlaylists(trackId) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + data, err := json.Marshal(playlists) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + _, _ = w.Write(data) + } +} diff --git a/server/serve_index.go b/server/serve_index.go index 9a457ac20..1e55743f0 100644 --- a/server/serve_index.go +++ b/server/serve_index.go @@ -65,6 +65,7 @@ func serveIndex(ds model.DataStore, fs fs.FS, shareInfo *model.Share) http.Handl "devSidebarPlaylists": conf.Server.DevSidebarPlaylists, "lastFMEnabled": conf.Server.LastFM.Enabled, "devShowArtistPage": conf.Server.DevShowArtistPage, + "devUIShowConfig": conf.Server.DevUIShowConfig, "listenBrainzEnabled": conf.Server.ListenBrainz.Enabled, "enableExternalServices": conf.Server.EnableExternalServices, "enableReplayGain": conf.Server.EnableReplayGain, diff --git a/server/serve_index_test.go b/server/serve_index_test.go index 0f02153fd..fd0d42193 100644 --- a/server/serve_index_test.go +++ b/server/serve_index_test.go @@ -304,6 +304,17 @@ var _ = Describe("serveIndex", func() { Expect(config).To(HaveKeyWithValue("devShowArtistPage", true)) }) + It("sets the devUIShowConfig", func() { + conf.Server.DevUIShowConfig = true + r := httptest.NewRequest("GET", "/index.html", nil) + w := httptest.NewRecorder() + + serveIndex(ds, fs, nil)(w, r) + + config := extractAppConfig(w.Body.String()) + Expect(config).To(HaveKeyWithValue("devUIShowConfig", true)) + }) + It("sets the listenBrainzEnabled", func() { conf.Server.ListenBrainz.Enabled = true r := httptest.NewRequest("GET", "/index.html", nil) diff --git a/server/server.go b/server/server.go index 60350b6b4..49391e2b6 100644 --- a/server/server.go +++ b/server/server.go @@ -173,7 +173,7 @@ func (s *Server) initRoutes() { clientUniqueIDMiddleware, compressMiddleware(), loggerInjector, - jwtVerifier, + JWTVerifier, } // Mount the Native API /events endpoint with all default middlewares, adding the authentication middlewares diff --git a/server/subsonic/users.go b/server/subsonic/users.go index 0499b5ee0..39214eee2 100644 --- a/server/subsonic/users.go +++ b/server/subsonic/users.go @@ -4,26 +4,40 @@ import ( "net/http" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/subsonic/responses" ) +// buildUserResponse creates a User response object from a User model +func buildUserResponse(user model.User) responses.User { + userResponse := responses.User{ + Username: user.UserName, + AdminRole: user.IsAdmin, + Email: user.Email, + StreamRole: true, + ScrobblingEnabled: true, + DownloadRole: conf.Server.EnableDownloads, + ShareRole: conf.Server.EnableSharing, + } + + if conf.Server.Jukebox.Enabled { + userResponse.JukeboxRole = !conf.Server.Jukebox.AdminOnly || user.IsAdmin + } + + return userResponse +} + // TODO This is a placeholder. The real one has to read this info from a config file or the database func (api *Router) GetUser(r *http.Request) (*responses.Subsonic, error) { loggedUser, ok := request.UserFrom(r.Context()) if !ok { return nil, newError(responses.ErrorGeneric, "Internal error") } + response := newResponse() - response.User = &responses.User{} - response.User.Username = loggedUser.UserName - response.User.AdminRole = loggedUser.IsAdmin - response.User.Email = loggedUser.Email - response.User.StreamRole = true - response.User.ScrobblingEnabled = true - response.User.DownloadRole = conf.Server.EnableDownloads - response.User.ShareRole = conf.Server.EnableSharing - response.User.JukeboxRole = conf.Server.Jukebox.Enabled + user := buildUserResponse(loggedUser) + response.User = &user return response, nil } @@ -32,17 +46,8 @@ func (api *Router) GetUsers(r *http.Request) (*responses.Subsonic, error) { if !ok { return nil, newError(responses.ErrorGeneric, "Internal error") } - user := responses.User{} - user.Username = loggedUser.Name - user.AdminRole = loggedUser.IsAdmin - user.Email = loggedUser.Email - user.StreamRole = true - user.ScrobblingEnabled = true - user.DownloadRole = conf.Server.EnableDownloads - user.ShareRole = conf.Server.EnableSharing - if conf.Server.Jukebox.Enabled { - user.JukeboxRole = !conf.Server.Jukebox.AdminOnly || loggedUser.IsAdmin - } + + user := buildUserResponse(loggedUser) response := newResponse() response.Users = &responses.Users{User: []responses.User{user}} return response, nil diff --git a/server/subsonic/users_test.go b/server/subsonic/users_test.go new file mode 100644 index 000000000..d08462290 --- /dev/null +++ b/server/subsonic/users_test.go @@ -0,0 +1,96 @@ +package subsonic + +import ( + "context" + "net/http/httptest" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/server/subsonic/responses" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Users", func() { + var router *Router + var testUser model.User + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + router = &Router{} + + testUser = model.User{ + ID: "user123", + UserName: "testuser", + Name: "Test User", + Email: "test@example.com", + IsAdmin: false, + } + }) + + Describe("Happy path", func() { + It("should return consistent user data in both GetUser and GetUsers", func() { + conf.Server.EnableDownloads = true + conf.Server.EnableSharing = true + conf.Server.Jukebox.Enabled = false + + // Create request with user in context + req := httptest.NewRequest("GET", "/rest/getUser", nil) + ctx := request.WithUser(context.Background(), testUser) + req = req.WithContext(ctx) + + userResponse, err1 := router.GetUser(req) + usersResponse, err2 := router.GetUsers(req) + + Expect(err1).ToNot(HaveOccurred()) + Expect(err2).ToNot(HaveOccurred()) + + // Verify GetUser response structure + Expect(userResponse.Status).To(Equal(responses.StatusOK)) + Expect(userResponse.User).ToNot(BeNil()) + Expect(userResponse.User.Username).To(Equal("testuser")) + Expect(userResponse.User.Email).To(Equal("test@example.com")) + Expect(userResponse.User.AdminRole).To(BeFalse()) + Expect(userResponse.User.StreamRole).To(BeTrue()) + Expect(userResponse.User.ScrobblingEnabled).To(BeTrue()) + Expect(userResponse.User.DownloadRole).To(BeTrue()) + Expect(userResponse.User.ShareRole).To(BeTrue()) + + // Verify GetUsers response structure + Expect(usersResponse.Status).To(Equal(responses.StatusOK)) + Expect(usersResponse.Users).ToNot(BeNil()) + Expect(usersResponse.Users.User).To(HaveLen(1)) + + // Verify both methods return identical user data + singleUser := userResponse.User + userFromList := &usersResponse.Users.User[0] + + Expect(singleUser.Username).To(Equal(userFromList.Username)) + Expect(singleUser.Email).To(Equal(userFromList.Email)) + Expect(singleUser.AdminRole).To(Equal(userFromList.AdminRole)) + Expect(singleUser.StreamRole).To(Equal(userFromList.StreamRole)) + Expect(singleUser.ScrobblingEnabled).To(Equal(userFromList.ScrobblingEnabled)) + Expect(singleUser.DownloadRole).To(Equal(userFromList.DownloadRole)) + Expect(singleUser.ShareRole).To(Equal(userFromList.ShareRole)) + Expect(singleUser.JukeboxRole).To(Equal(userFromList.JukeboxRole)) + }) + }) + + DescribeTable("Jukebox role permissions", + func(jukeboxEnabled, adminOnly, isAdmin, expectedJukeboxRole bool) { + conf.Server.Jukebox.Enabled = jukeboxEnabled + conf.Server.Jukebox.AdminOnly = adminOnly + testUser.IsAdmin = isAdmin + + response := buildUserResponse(testUser) + Expect(response.JukeboxRole).To(Equal(expectedJukeboxRole)) + }, + Entry("jukebox disabled", false, false, false, false), + Entry("jukebox enabled, not admin-only, regular user", true, false, false, true), + Entry("jukebox enabled, not admin-only, admin user", true, false, true, true), + Entry("jukebox enabled, admin-only, regular user", true, true, false, false), + Entry("jukebox enabled, admin-only, admin user", true, true, true, true), + ) +}) diff --git a/tests/mock_data_store.go b/tests/mock_data_store.go index f380755e0..fb5bbd710 100644 --- a/tests/mock_data_store.go +++ b/tests/mock_data_store.go @@ -217,8 +217,33 @@ func (db *MockDataStore) WithTxImmediate(block func(tx model.DataStore) error, l return block(db) } -func (db *MockDataStore) Resource(context.Context, any) model.ResourceRepository { - return struct{ model.ResourceRepository }{} +func (db *MockDataStore) Resource(ctx context.Context, m any) model.ResourceRepository { + switch m.(type) { + case model.MediaFile, *model.MediaFile: + return db.MediaFile(ctx).(model.ResourceRepository) + case model.Album, *model.Album: + return db.Album(ctx).(model.ResourceRepository) + case model.Artist, *model.Artist: + return db.Artist(ctx).(model.ResourceRepository) + case model.User, *model.User: + return db.User(ctx).(model.ResourceRepository) + case model.Playlist, *model.Playlist: + return db.Playlist(ctx).(model.ResourceRepository) + case model.Radio, *model.Radio: + return db.Radio(ctx).(model.ResourceRepository) + case model.Share, *model.Share: + return db.Share(ctx).(model.ResourceRepository) + case model.Genre, *model.Genre: + return db.Genre(ctx).(model.ResourceRepository) + case model.Tag, *model.Tag: + return db.Tag(ctx).(model.ResourceRepository) + case model.Transcoding, *model.Transcoding: + return db.Transcoding(ctx).(model.ResourceRepository) + case model.Player, *model.Player: + return db.Player(ctx).(model.ResourceRepository) + default: + return struct{ model.ResourceRepository }{} + } } func (db *MockDataStore) GC(context.Context) error { diff --git a/tests/mock_mediafile_repo.go b/tests/mock_mediafile_repo.go index 85adb8a25..7bba8eda8 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -7,6 +7,7 @@ import ( "slices" "time" + "github.com/deluan/rest" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/utils/slice" @@ -76,9 +77,14 @@ func (m *MockMediaFileRepo) GetAll(...model.QueryOptions) (model.MediaFiles, err return nil, errors.New("error") } values := slices.Collect(maps.Values(m.Data)) - return slice.Map(values, func(p *model.MediaFile) model.MediaFile { + result := slice.Map(values, func(p *model.MediaFile) model.MediaFile { return *p - }), nil + }) + // Sort by ID to ensure deterministic ordering for tests + slices.SortFunc(result, func(a, b model.MediaFile) int { + return cmp.Compare(a.ID, b.ID) + }) + return result, nil } func (m *MockMediaFileRepo) Put(mf *model.MediaFile) error { @@ -196,4 +202,30 @@ func (m *MockMediaFileRepo) DeleteAllMissing() (int64, error) { return count, nil } +// ResourceRepository methods +func (m *MockMediaFileRepo) Count(...rest.QueryOptions) (int64, error) { + return m.CountAll() +} + +func (m *MockMediaFileRepo) Read(id string) (interface{}, error) { + mf, err := m.Get(id) + if errors.Is(err, model.ErrNotFound) { + return nil, rest.ErrNotFound + } + return mf, err +} + +func (m *MockMediaFileRepo) ReadAll(...rest.QueryOptions) (interface{}, error) { + return m.GetAll() +} + +func (m *MockMediaFileRepo) EntityName() string { + return "mediafile" +} + +func (m *MockMediaFileRepo) NewInstance() interface{} { + return &model.MediaFile{} +} + var _ model.MediaFileRepository = (*MockMediaFileRepo)(nil) +var _ model.ResourceRepository = (*MockMediaFileRepo)(nil) diff --git a/ui/src/App.jsx b/ui/src/App.jsx index 1b89f7b8c..4a38051b4 100644 --- a/ui/src/App.jsx +++ b/ui/src/App.jsx @@ -137,6 +137,9 @@ const Admin = (props) => { , , , + permissions === 'admin' && config.devUIShowConfig ? ( + + ) : null, , ]} diff --git a/ui/src/audioplayer/AudioTitle.jsx b/ui/src/audioplayer/AudioTitle.jsx index aebd37170..093bb53fb 100644 --- a/ui/src/audioplayer/AudioTitle.jsx +++ b/ui/src/audioplayer/AudioTitle.jsx @@ -38,16 +38,14 @@ const AudioTitle = React.memo(({ audioInfo, gainInfo, isMobile }) => { const subtitle = song.tags?.['subtitle'] const title = song.title + (subtitle ? ` (${subtitle})` : '') + const linkTo = audioInfo.isRadio + ? `/radio/${audioInfo.trackId}/show` + : song.playlistId + ? `/playlist/${song.playlistId}/show` + : `/album/${song.albumId}/show` + return ( - + {title} {isDesktop && ( diff --git a/ui/src/audioplayer/AudioTitle.test.jsx b/ui/src/audioplayer/AudioTitle.test.jsx new file mode 100644 index 000000000..c3f566f6b --- /dev/null +++ b/ui/src/audioplayer/AudioTitle.test.jsx @@ -0,0 +1,57 @@ +import React from 'react' +import { render, screen } from '@testing-library/react' +import { describe, it, expect, beforeEach, vi } from 'vitest' +import AudioTitle from './AudioTitle' + +vi.mock('@material-ui/core', async () => { + const actual = await import('@material-ui/core') + return { + ...actual, + useMediaQuery: vi.fn(), + } +}) + +vi.mock('react-router-dom', () => ({ + Link: ({ to, children, ...props }) => ( + + {children} + + ), +})) + +vi.mock('react-dnd', () => ({ + useDrag: vi.fn(() => [null, () => {}]), +})) + +describe('', () => { + const baseSong = { + id: 'song-1', + albumId: 'album-1', + playlistId: 'playlist-1', + title: 'Test Song', + artist: 'Artist', + album: 'Album', + year: '2020', + } + + beforeEach(() => { + vi.clearAllMocks() + }) + + it('links to playlist when playlistId is provided', () => { + const audioInfo = { trackId: 'track-1', song: baseSong } + render() + const link = screen.getByRole('link') + expect(link.getAttribute('href')).toBe('/playlist/playlist-1/show') + }) + + it('falls back to album link when no playlistId', () => { + const audioInfo = { + trackId: 'track-1', + song: { ...baseSong, playlistId: undefined }, + } + render() + const link = screen.getByRole('link') + expect(link.getAttribute('href')).toBe('/album/album-1/show') + }) +}) diff --git a/ui/src/audioplayer/PlayerToolbar.jsx b/ui/src/audioplayer/PlayerToolbar.jsx index 5230b30f2..4812141ab 100644 --- a/ui/src/audioplayer/PlayerToolbar.jsx +++ b/ui/src/audioplayer/PlayerToolbar.jsx @@ -57,7 +57,7 @@ const useStyles = makeStyles((theme) => ({ const PlayerToolbar = ({ id, isRadio }) => { const dispatch = useDispatch() - const { data, loading } = useGetOne('song', id, { enabled: !!id }) + const { data, loading } = useGetOne('song', id, { enabled: !!id && !isRadio }) const [toggleLove, toggling] = useToggleLove('song', data) const isDesktop = useMediaQuery('(min-width:810px)') const classes = useStyles() diff --git a/ui/src/common/RatingField.jsx b/ui/src/common/RatingField.jsx index 1b440c51e..b29c1eee8 100644 --- a/ui/src/common/RatingField.jsx +++ b/ui/src/common/RatingField.jsx @@ -38,15 +38,16 @@ export const RatingField = ({ const handleRating = useCallback( (e, val) => { - rate(val ?? 0, e.target.name) + const targetId = record.mediaFileId || record.id + rate(val ?? 0, targetId) }, - [rate], + [rate, record.mediaFileId, record.id], ) return ( stopPropagation(e)}> 0 ? ' ►' : ''), + action: (record, e) => { + setPlaylistAnchorEl(e.currentTarget) + }, + }, share: { enabled: config.enableSharing, label: translate('ra.action.share'), @@ -113,8 +132,8 @@ export const SongContextMenu = ({ if (permissions === 'admin' && !record.missing) { try { let id = record.mediaFileId ?? record.id - const data = await httpClient(`/api/inspect?id=${id}`) - fullRecord = { ...record, rawTags: data.json.rawTags } + const data = await dataProvider.inspect(id) + fullRecord = { ...record, rawTags: data.data.rawTags } } catch (error) { notify( translate('ra.notification.http_error') + ': ' + error.message, @@ -134,6 +153,21 @@ export const SongContextMenu = ({ const handleClick = (e) => { setAnchorEl(e.currentTarget) + if (!playlistsLoaded) { + const id = record.mediaFileId || record.id + dataProvider + .getPlaylists(id) + .then((res) => { + setPlaylists(res.data) + setPlaylistsLoaded(true) + }) + .catch((error) => { + // eslint-disable-next-line no-console + console.error('Failed to fetch playlists:', error) + setPlaylists([]) + setPlaylistsLoaded(true) + }) + } e.stopPropagation() } @@ -144,12 +178,39 @@ export const SongContextMenu = ({ const handleItemClick = (e) => { e.preventDefault() - setAnchorEl(null) const key = e.target.getAttribute('value') - options[key].action(record) + const action = options[key].action + + if (key === 'showInPlaylist') { + // For showInPlaylist, we keep the main menu open and show submenu + action(record, e) + } else { + // For other actions, close the main menu + setAnchorEl(null) + action(record) + } e.stopPropagation() } + const handlePlaylistClose = (e) => { + setPlaylistAnchorEl(null) + if (e) { + e.stopPropagation() + } + } + + const handleMainMenuClose = (e) => { + setAnchorEl(null) + setPlaylistAnchorEl(null) // Close both menus + e.stopPropagation() + } + + const handlePlaylistClick = (id, e) => { + e.stopPropagation() + redirect(`/playlist/${id}/show`) + handlePlaylistClose() + } + const open = Boolean(anchorEl) if (!record) { @@ -170,17 +231,41 @@ export const SongContextMenu = ({ id={'menu' + record.id} anchorEl={anchorEl} open={open} - onClose={handleClose} + onClose={handleMainMenuClose} > {Object.keys(options).map( (key) => options[key].enabled && ( - + {options[key].label} ), )} + + {playlists.map((p) => ( + handlePlaylistClick(p.id, e)}> + {p.name} + + ))} + ) } diff --git a/ui/src/common/SongContextMenu.test.jsx b/ui/src/common/SongContextMenu.test.jsx new file mode 100644 index 000000000..ee6a358d8 --- /dev/null +++ b/ui/src/common/SongContextMenu.test.jsx @@ -0,0 +1,82 @@ +import React from 'react' +import { render, fireEvent, screen, waitFor } from '@testing-library/react' +import { TestContext } from 'ra-test' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { SongContextMenu } from './SongContextMenu' + +vi.mock('../dataProvider', () => ({ + httpClient: vi.fn(), +})) + +vi.mock('react-redux', () => ({ useDispatch: () => vi.fn() })) + +vi.mock('react-admin', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useRedirect: () => (url) => { + window.location.hash = `#${url}` + }, + useDataProvider: () => ({ + getPlaylists: vi.fn().mockResolvedValue({ + data: [{ id: 'pl1', name: 'Pl 1' }], + }), + inspect: vi.fn().mockResolvedValue({ + data: { rawTags: {} }, + }), + }), + } +}) + +describe('SongContextMenu', () => { + beforeEach(() => { + vi.clearAllMocks() + window.location.hash = '' + }) + + it('navigates to playlist when selected', async () => { + render( + + + , + ) + fireEvent.click(screen.getAllByRole('button')[1]) + await waitFor(() => + screen.getByText(/resources\.song\.actions\.showInPlaylist/), + ) + fireEvent.click( + screen.getByText(/resources\.song\.actions\.showInPlaylist/), + ) + await waitFor(() => screen.getByText('Pl 1')) + fireEvent.click(screen.getByText('Pl 1')) + expect(window.location.hash).toBe('#/playlist/pl1/show') + }) + + it('stops event propagation when playlist submenu is closed', async () => { + const mockOnClick = vi.fn() + render( + +
+ +
+
, + ) + + // Open main menu + fireEvent.click(screen.getAllByRole('button')[1]) + await waitFor(() => + screen.getByText(/resources\.song\.actions\.showInPlaylist/), + ) + + // Open playlist submenu + fireEvent.click( + screen.getByText(/resources\.song\.actions\.showInPlaylist/), + ) + await waitFor(() => screen.getByText('Pl 1')) + + // Click outside the playlist submenu (should close it without triggering parent click) + fireEvent.click(document.body) + + expect(mockOnClick).not.toHaveBeenCalled() + }) +}) diff --git a/ui/src/common/SongInfo.jsx b/ui/src/common/SongInfo.jsx index 77e91b653..9b9ca18cd 100644 --- a/ui/src/common/SongInfo.jsx +++ b/ui/src/common/SongInfo.jsx @@ -138,7 +138,7 @@ export const SongInfo = (props) => { )}