diff --git a/adapters/gotaglib/end_to_end_test.go b/adapters/gotaglib/end_to_end_test.go index e7dd18ac1..0f9a90d94 100644 --- a/adapters/gotaglib/end_to_end_test.go +++ b/adapters/gotaglib/end_to_end_test.go @@ -90,7 +90,7 @@ var _ = Describe("Extractor", func() { info.FileInfo = testFileInfo{FileInfo: fileInfo} metadata := metadata.New(path, info) - return new(metadata.ToMediaFile(1, "folderID")) + return new(metadata.ToMediaFile(model.Library{ID: 1}, "folderID")) } BeforeEach(func() { diff --git a/cmd/inspect.go b/cmd/inspect.go index 5e88793cc..05f569f3e 100644 --- a/cmd/inspect.go +++ b/cmd/inspect.go @@ -1,13 +1,17 @@ package cmd import ( + "context" "encoding/json" "fmt" + "path/filepath" "strings" "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/persistence" "github.com/pelletier/go-toml/v2" "github.com/spf13/cobra" "gopkg.in/yaml.v3" @@ -28,7 +32,7 @@ var inspectCmd = &cobra.Command{ Long: "Show file tags as seen by Navidrome", Args: cobra.MinimumNArgs(1), Run: func(cmd *cobra.Command, args []string) { - runInspector(args) + runInspector(cmd.Context(), args) }, } @@ -55,18 +59,24 @@ func prettyMarshal(v any) ([]byte, error) { return []byte(res.String()), nil } -func runInspector(args []string) { +func runInspector(ctx context.Context, args []string) { marshal := marshalers[format] if marshal == nil { log.Fatal("Invalid format", "format", format) } + libs := loadLibraries(ctx) + matcher := model.NewLibraryMatcher(libs) var out []core.InspectOutput for _, filePath := range args { if !model.IsAudioFile(filePath) { log.Warn("Not an audio file", "file", filePath) continue } - output, err := core.Inspect(filePath, 1, "") + lib, ok := libraryForFile(matcher, filePath) + if !ok && len(libs) > 0 { + log.Warn("File is not in any library, using the global PID config", "file", filePath) + } + output, err := core.Inspect(filePath, lib, "") if err != nil { log.Warn("Unable to process file", "file", filePath, "error", err) continue @@ -77,3 +87,33 @@ func runInspector(args []string) { data, _ := marshal(out) fmt.Println(string(data)) } + +// loadLibraries reads the libraries, so each file gets its library's PID config. It never creates a DB. +func loadLibraries(ctx context.Context) model.Libraries { + if dbFile, ok := existingDBFile(); !ok { + log.Warn(ctx, "No database found, using the global PID config", "path", dbFile) + return nil + } + defer db.Init(ctx)() + libs, err := persistence.New(db.Db()).Library().GetAll(ctx) + if err != nil { + log.Warn(ctx, "Could not load libraries, using the global PID config", err) + return nil + } + for i := range libs { + if absPath, err := filepath.Abs(libs[i].Path); err == nil { + libs[i].Path = absPath + } + } + return libs +} + +// libraryForFile falls back to the default library with no overrides, which uses the global PID config. +func libraryForFile(matcher *model.LibraryMatcher, filePath string) (model.Library, bool) { + if absPath, err := filepath.Abs(filePath); err == nil { + if lib, ok := matcher.FindLibrary(absPath); ok { + return lib, true + } + } + return model.Library{ID: model.DefaultLibraryID}, false +} diff --git a/cmd/inspect_test.go b/cmd/inspect_test.go new file mode 100644 index 000000000..728dc8770 --- /dev/null +++ b/cmd/inspect_test.go @@ -0,0 +1,61 @@ +package cmd + +import ( + "os" + "path/filepath" + + "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("inspect", func() { + Describe("libraryForFile", func() { + var matcher *model.LibraryMatcher + var root string + + BeforeEach(func() { + root = GinkgoT().TempDir() + cwd, err := os.Getwd() + Expect(err).ToNot(HaveOccurred()) + matcher = model.NewLibraryMatcher(model.Libraries{ + {ID: 1, Path: filepath.Join(root, "music")}, + {ID: 2, Path: filepath.Join(cwd, "loose"), PIDAlbum: "folder"}, + }) + }) + + It("returns the library that contains an absolute path", func() { + lib, ok := libraryForFile(matcher, filepath.Join(root, "music", "album", "track.mp3")) + Expect(ok).To(BeTrue()) + Expect(lib.ID).To(Equal(1)) + }) + + It("resolves a relative path against the working directory", func() { + lib, ok := libraryForFile(matcher, filepath.Join("loose", "track.mp3")) + Expect(ok).To(BeTrue()) + Expect(lib.PIDAlbum).To(Equal("folder")) + }) + + It("falls back to the default library without overrides", func() { + lib, ok := libraryForFile(matcher, filepath.Join(root, "elsewhere", "track.mp3")) + Expect(ok).To(BeFalse()) + Expect(lib).To(Equal(model.Library{ID: model.DefaultLibraryID})) + }) + }) + + Describe("loadLibraries", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + + It("does not create a database when there is none", func() { + dbFile := filepath.Join(GinkgoT().TempDir(), "navidrome.db") + conf.Server.DbPath = dbFile + "?_journal_mode=WAL" + + Expect(loadLibraries(GinkgoT().Context())).To(BeNil()) + Expect(dbFile).ToNot(BeAnExistingFile()) + }) + }) +}) diff --git a/cmd/root.go b/cmd/root.go index edfcbe69c..e39c55365 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -44,7 +44,9 @@ Complete documentation is available at https://www.navidrome.org/docs`, preRun() }, Run: func(cmd *cobra.Command, args []string) { - runNavidrome(cmd.Context()) + if err := runNavidrome(cmd.Context()); err != nil { + log.Fatal("Fatal error in Navidrome. Aborting", err) + } }, PostRun: func(cmd *cobra.Command, args []string) { postRun() @@ -76,12 +78,12 @@ func postRun() { } // runNavidrome is the main entry point for the Navidrome server. It starts all the services and blocks. -// If any of the services returns an error, it will log it and exit. If the process receives a signal to exit, -// it will cancel the context and exit gracefully. -func runNavidrome(ctx context.Context) { - defer db.Init(ctx)() +// If any of the services returns an error, it stops the others and returns that error, so the caller can +// exit with a non-zero code. If the context is cancelled (a signal or a service stop), it returns nil. +func runNavidrome(parentCtx context.Context) error { + defer db.Init(parentCtx)() - g, ctx := errgroup.WithContext(ctx) + g, ctx := errgroup.WithContext(parentCtx) g.Go(startServer(ctx)) g.Go(startSignaller(ctx)) g.Go(startScheduler(ctx)) @@ -102,9 +104,11 @@ func runNavidrome(ctx context.Context) { log.Warn(ctx, "Automatic Scanning is DISABLED") } - if err := g.Wait(); err != nil { - log.Error("Fatal error in Navidrome. Aborting", err) + // Errors caused by a normal shutdown are not failures + if err := g.Wait(); err != nil && parentCtx.Err() == nil { + return err } + return nil } // mainContext returns a context that is cancelled when the process receives a signal to exit. @@ -186,16 +190,20 @@ func schedulePeriodicScan(ctx context.Context) func() error { } } -func pidHashChanged(ds model.DataStore) (bool, error) { - pidAlbum, err := ds.Property().DefaultGet(context.Background(), consts.PIDAlbumKey, "") +// librariesWithChangedPID returns the names of the libraries whose effective PID config differs from +// the one used by their last finished scan +func librariesWithChangedPID(ctx context.Context, ds model.DataStore) ([]string, error) { + libs, err := ds.Library().GetAll(ctx) if err != nil { - return false, err + return nil, err } - pidTrack, err := ds.Property().DefaultGet(context.Background(), consts.PIDTrackKey, "") - if err != nil { - return false, err + var names []string + for _, lib := range libs { + if lib.PIDChanged() { + names = append(names, lib.Name) + } } - return !strings.EqualFold(pidAlbum, conf.Server.PID.Album) || !strings.EqualFold(pidTrack, conf.Server.PID.Track), nil + return names, nil } // runInitialScan runs an initial scan of the music library if needed. @@ -210,12 +218,12 @@ func runInitialScan(ctx context.Context) func() error { if err != nil { return err } - pidHasChanged, err := pidHashChanged(ds) + pidChangedLibs, err := librariesWithChangedPID(ctx, ds) if err != nil { return err } scanOnStartup := conf.Server.Scanner.Enabled && conf.Server.Scanner.ScanOnStartup - scanNeeded := scanOnStartup || inProgress || fullScanRequired == "1" || pidHasChanged + scanNeeded := scanOnStartup || inProgress || fullScanRequired == "1" || len(pidChangedLibs) > 0 time.Sleep(2 * time.Second) // Wait 2 seconds before the initial scan if scanNeeded { s := CreateScanner(ctx) @@ -223,9 +231,9 @@ func runInitialScan(ctx context.Context) func() error { case fullScanRequired == "1": log.Warn(ctx, "Full scan required after migration") _ = ds.Property().Delete(ctx, consts.FullScanAfterMigrationFlagKey) - case pidHasChanged: - log.Warn(ctx, "PID config changed, performing full scan") - fullScanRequired = "1" + case len(pidChangedLibs) > 0: + // Includes never-scanned libraries. The scanner rescans in full only the ones that need it + log.Warn(ctx, "Libraries with a new or changed PID config, scanning", "libraries", pidChangedLibs) case inProgress: log.Warn(ctx, "Resuming interrupted scan") default: diff --git a/cmd/root_test.go b/cmd/root_test.go index af8d44e7e..423cd2a8f 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -1,6 +1,7 @@ package cmd import ( + "errors" "net/http" "net/http/httptest" "path" @@ -9,6 +10,8 @@ import ( "github.com/go-chi/chi/v5" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -44,3 +47,30 @@ var _ = Describe("profilerHandler", func() { Entry("with a trailing-slash BasePath", "/music/"), ) }) + +var _ = Describe("librariesWithChangedPID", func() { + var ds *tests.MockDataStore + var libs *tests.MockLibraryRepo + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + libs = &tests.MockLibraryRepo{} + ds = &tests.MockDataStore{MockedLibrary: libs} + }) + + It("returns only the libraries whose PID config changed", func() { + pid := model.Library{}.EffectivePID() + libs.SetData(model.Libraries{ + {ID: 1, Name: "Same", ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, + {ID: 2, Name: "Changed", PIDAlbum: "folder", ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, + {ID: 3, Name: "Never scanned"}, + }) + Expect(librariesWithChangedPID(GinkgoT().Context(), ds)).To(ConsistOf("Changed", "Never scanned")) + }) + + It("returns the error from the repository", func() { + libs.Err = errors.New("db down") + _, err := librariesWithChangedPID(GinkgoT().Context(), ds) + Expect(err).To(MatchError("db down")) + }) +}) diff --git a/cmd/svc.go b/cmd/svc.go index c71f5ef2b..4e8b1fd85 100644 --- a/cmd/svc.go +++ b/cmd/svc.go @@ -53,8 +53,13 @@ func (p *svcControl) Start(service.Service) error { p.done = make(chan struct{}) p.ctx, p.cancel = context.WithCancel(context.Background()) go func() { - runNavidrome(p.ctx) + err := runNavidrome(p.ctx) close(p.done) + // service.Run() only returns when it gets a stop request, so exit here to let the + // service manager see the failure and restart the service + if err != nil { + log.Fatal("Fatal error in Navidrome. Aborting", err) + } }() return nil } @@ -74,7 +79,7 @@ func (p *svcControl) Stop(service.Service) error { var svcInstance = sync.OnceValue(func() service.Service { options := make(service.KeyValue) options["Restart"] = "on-failure" - options["SuccessExitStatus"] = "1 2 8 SIGKILL" + options["SuccessExitStatus"] = "SIGKILL" options["UserService"] = false options["LogDirectory"] = conf.Server.DataFolder.String() options["SystemdScript"] = systemdScript diff --git a/cmd/utils.go b/cmd/utils.go index 72ec67f90..f35a31fb1 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -18,11 +18,16 @@ import ( "github.com/navidrome/navidrome/persistence" ) -// requireExistingDB aborts the command when the database file (DbPath minus DSN -// params) does not exist. -func requireExistingDB() { +// existingDBFile returns the database file (DbPath minus DSN params), and whether it exists. +func existingDBFile() (string, bool) { path, _, _ := strings.Cut(conf.Server.DbPath, "?") - if _, err := os.Stat(path); os.IsNotExist(err) { + _, err := os.Stat(path) + return path, err == nil +} + +// requireExistingDB aborts the command when the database file does not exist. +func requireExistingDB() { + if path, ok := existingDBFile(); !ok { log.Fatal("No existing database", "path", path) } } diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 19f92d9d5..2a396689d 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -95,8 +95,9 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg) transcodingCache := stream.GetTranscodingCache() mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) + transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg) share := core.NewShare(dataStore) - archiver := core.NewArchiver(mediaStreamer, dataStore, share, artworkArtwork) + archiver := core.NewArchiver(mediaStreamer, transcodeDecider, dataStore, share, artworkArtwork) players := core.NewPlayers(dataStore) broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) @@ -110,7 +111,6 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager) playbackServer := playback.GetInstance(dataStore) lyricsLyrics := lyrics.NewLyrics(dataStore, manager) - transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg) sonicSonic := sonic.New(dataStore, manager, matcherMatcher) router := subsonic.New(dataStore, artworkArtwork, mediaStreamer, archiver, players, provider, modelScanner, broker, playlistsPlaylists, playTracker, share, playbackServer, metricsMetrics, lyricsLyrics, transcodeDecider, sonicSonic) return router @@ -159,9 +159,10 @@ func CreatePublicRouter() *public.Router { artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg) transcodingCache := stream.GetTranscodingCache() mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) + transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg) share := core.NewShare(dataStore) - archiver := core.NewArchiver(mediaStreamer, dataStore, share, artworkArtwork) - router := public.New(dataStore, artworkArtwork, mediaStreamer, share, archiver) + archiver := core.NewArchiver(mediaStreamer, transcodeDecider, dataStore, share, artworkArtwork) + router := public.New(dataStore, artworkArtwork, mediaStreamer, transcodeDecider, share, archiver) return router } diff --git a/consts/consts.go b/consts/consts.go index 7228299c6..42e9ec42f 100644 --- a/consts/consts.go +++ b/consts/consts.go @@ -49,6 +49,7 @@ const ( DefaultEncryptionKey = "just for obfuscation" PasswordsEncryptedKey = "PasswordsEncryptedKey" PasswordAutogenPrefix = "__NAVIDROME_AUTOGEN__" //nolint:gosec + APIKeyPrefix = "nds_" DevInitialUserName = "admin" DevInitialName = "Dev Admin" @@ -155,8 +156,6 @@ const ( //DefaultAlbumPID = "album_legacy" DefaultAlbumPID = "musicbrainz_albumid|albumartistid,album,albumversion,releasedate" DefaultTrackPID = "musicbrainz_trackid|albumid,discnumber,tracknumber,title" - PIDAlbumKey = "PIDAlbum" - PIDTrackKey = "PIDTrack" ) const ( diff --git a/core/archiver.go b/core/archiver.go index 6f362322a..60eb44858 100644 --- a/core/archiver.go +++ b/core/archiver.go @@ -35,13 +35,14 @@ type Archiver interface { ZipPlaylist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error } -func NewArchiver(ms stream.MediaStreamer, ds model.DataStore, shares Share, artwork artwork.Artwork) Archiver { - return &archiver{ds: ds, ms: ms, shares: shares, artwork: artwork} +func NewArchiver(ms stream.MediaStreamer, decider stream.TranscodeDecider, ds model.DataStore, shares Share, artwork artwork.Artwork) Archiver { + return &archiver{ds: ds, ms: ms, decider: decider, shares: shares, artwork: artwork} } type archiver struct { ds model.DataStore ms stream.MediaStreamer + decider stream.TranscodeDecider shares Share artwork artwork.Artwork } @@ -78,8 +79,9 @@ func (a *archiver) zipAlbums(ctx context.Context, id string, format string, bitr log.Debug(ctx, "Zipping album", "name", album[0].Album, "artist", album[0].AlbumArtist, "folder", folder, "format", format, "bitrate", bitrate, "isMultiDisc", isMultiDisc, "numTracks", len(album)) for _, mf := range album { - file := a.albumFilename(mf, format, isMultiDisc, folder) - if addErr := a.addFileToZip(ctx, z, mf, format, bitrate, file); errors.Is(addErr, stream.ErrTooManyTranscodes) { + req := a.resolveRequest(ctx, &mf, format, bitrate) + file := a.albumFilename(mf, req.Format, isMultiDisc, folder) + if addErr := a.addFileToZip(ctx, z, mf, req, file); errors.Is(addErr, stream.ErrTooManyTranscodes) { // Stop iterating: continuing would just rack up more // rejections from the limiter. Close finalises whatever // tracks were already written; the rejected one is not @@ -204,8 +206,9 @@ func (a *archiver) zipMediaFiles(ctx context.Context, id, name string, format st zippedMfs := make(model.MediaFiles, len(mfs)) for idx, mf := range mfs { - file := a.playlistFilename(mf, format, idx) - if addErr := a.addFileToZip(ctx, z, mf, format, bitrate, file); errors.Is(addErr, stream.ErrTooManyTranscodes) { + req := a.resolveRequest(ctx, &mf, format, bitrate) + file := a.playlistFilename(mf, req.Format, idx) + if addErr := a.addFileToZip(ctx, z, mf, req, file); errors.Is(addErr, stream.ErrTooManyTranscodes) { // Abort the whole archive: continuing would silently emit // empty zip entries since the headers are already written. _ = z.Close() @@ -251,7 +254,14 @@ func (a *archiver) playlistFilename(mf model.MediaFile, format string, idx int) return fmt.Sprintf("%02d - %s - %s.%s", idx+1, str.SanitizeFilename(mf.Artist), str.SanitizeFilename(mf.Title), ext) } -func (a *archiver) addFileToZip(ctx context.Context, z *zip.Writer, mf model.MediaFile, format string, bitrate int, filename string) error { +func (a *archiver) resolveRequest(ctx context.Context, mf *model.MediaFile, format string, bitrate int) stream.Request { + if format == "" || format == "raw" { + return stream.Request{Format: "raw"} + } + return a.decider.ResolveRequest(ctx, mf, format, bitrate, 0) +} + +func (a *archiver) addFileToZip(ctx context.Context, z *zip.Writer, mf model.MediaFile, req stream.Request, filename string) error { path := mf.AbsolutePath() // Open the source before writing the zip entry header so a rejection @@ -259,13 +269,13 @@ func (a *archiver) addFileToZip(ctx context.Context, z *zip.Writer, mf model.Med // archive. var r io.ReadCloser var err error - if format != "raw" && format != "" { - r, err = a.ms.NewStream(ctx, &mf, stream.Request{Format: format, BitRate: bitrate}) + if req.Format != "raw" { + r, err = a.ms.NewStream(ctx, &mf, req) } else { r, err = os.Open(path) } if err != nil { - log.Error(ctx, "Error opening file for zipping", "file", path, "format", format, err) + log.Error(ctx, "Error opening file for zipping", "file", path, "format", req.Format, err) return err } defer func() { diff --git a/core/archiver_test.go b/core/archiver_test.go index 4e00ce78c..178d1b6b9 100644 --- a/core/archiver_test.go +++ b/core/archiver_test.go @@ -26,6 +26,7 @@ var _ = Describe("Archiver", func() { var ( arch core.Archiver ms *mockMediaStreamer + dc *fakeDecider ds *mockDataStore sh *mockShare ca *mockCoverArt @@ -33,10 +34,11 @@ var _ = Describe("Archiver", func() { BeforeEach(func() { ms = &mockMediaStreamer{} + dc = &fakeDecider{} sh = &mockShare{} ds = &mockDataStore{} ca = &mockCoverArt{images: map[string][]byte{}} - arch = core.NewArchiver(ms, ds, sh, ca) + arch = core.NewArchiver(ms, dc, ds, sh, ca) }) Context("ZipAlbum", func() { @@ -66,6 +68,23 @@ var _ = Describe("Archiver", func() { Expect(zr.File[0].Name).To(Equal("Album_Promo/01 - track1.mp3")) Expect(zr.File[1].Name).To(Equal("Album_Promo/02 - track2.mp3")) }) + + It("streams the request resolved by the transcode decider and names the entry after its format", func() { + mfRepo := &mockMediaFileRepository{} + mfRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{{Path: "test_data/01 - track1.flac", Suffix: "flac", AlbumID: "1"}}, nil) + ds.On("MediaFile").Return(mfRepo) + resolved := stream.Request{Format: "opus", BitRate: 128, SampleRate: 48000, Channels: 2} + dc.resolved = &resolved + ms.On("NewStream", mock.Anything, mock.Anything, resolved).Return(io.NopCloser(strings.NewReader("test")), nil).Once() + + out := new(bytes.Buffer) + Expect(arch.ZipAlbum(GinkgoT().Context(), "1", "mp3", 128, out)).To(Succeed()) + ms.AssertExpectations(GinkgoT()) + + zr, err := zip.NewReader(bytes.NewReader(out.Bytes()), int64(out.Len())) + Expect(err).ToNot(HaveOccurred()) + Expect(zr.File[0].Name).To(HaveSuffix("01 - track1.opus")) + }) }) Context("ZipArtist", func() { @@ -296,6 +315,30 @@ var _ = Describe("Archiver", func() { }) Context("ZipPlaylist", func() { + It("names the entries and the M3U lines after the resolved format", func() { + pls := &model.Playlist{ID: "1", Name: "Test Playlist", Tracks: []model.PlaylistTrack{ + {MediaFile: model.MediaFile{Path: "test_data/01 - track1.flac", Suffix: "flac", Artist: "Artist 1", Title: "track1"}}, + }} + plRepo := &mockPlaylistRepository{} + plRepo.On("GetWithTracks", "1", true, false).Return(pls, nil) + ds.On("Playlist").Return(plRepo) + dc.resolved = &stream.Request{Format: "opus", BitRate: 128} + ms.On("NewStream", mock.Anything, mock.Anything, *dc.resolved).Return(io.NopCloser(strings.NewReader("test")), nil) + + out := new(bytes.Buffer) + Expect(arch.ZipPlaylist(GinkgoT().Context(), "1", "mp3", 128, out)).To(Succeed()) + + zr, err := zip.NewReader(bytes.NewReader(out.Bytes()), int64(out.Len())) + Expect(err).ToNot(HaveOccurred()) + Expect(zr.File[0].Name).To(Equal("01 - Artist 1 - track1.opus")) + m3u, err := zr.File[1].Open() + Expect(err).ToNot(HaveOccurred()) + defer m3u.Close() + content, err := io.ReadAll(m3u) + Expect(err).ToNot(HaveOccurred()) + Expect(string(content)).To(ContainSubstring("01 - Artist 1 - track1.opus")) + }) + It("zips a playlist correctly", func() { tracks := []model.PlaylistTrack{ {MediaFile: model.MediaFile{Path: "test_data/01 - track1.mp3", Suffix: "mp3", AlbumID: "1", Album: "Album 1", DiscNumber: 1, Artist: "AC/DC", Title: "track1"}}, @@ -571,6 +614,19 @@ func (m *mockMediaStreamer) NewStream(ctx context.Context, mf *model.MediaFile, return &stream.Stream{ReadCloser: args.Get(0).(io.ReadCloser)}, nil } +// fakeDecider echoes the legacy format/bitrate unless a resolved request is set. +type fakeDecider struct { + stream.TranscodeDecider + resolved *stream.Request +} + +func (f *fakeDecider) ResolveRequest(_ context.Context, _ *model.MediaFile, format string, bitRate int, offset int) stream.Request { + if f.resolved != nil { + return *f.resolved + } + return stream.Request{Format: format, BitRate: bitRate, Offset: offset} +} + type mockShare struct { mock.Mock core.Share diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index 40baa2495..fb07332fe 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -374,8 +374,11 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso } } - albumIDs, err := r.ds.Playlist().Tracks(ctx, pl.ID, false). - GetAlbumIDs(ctx, model.QueryOptions{Max: PlaylistGridSamples, Sort: "random()"}) + tracks := r.ds.Playlist().Tracks(ctx, pl.ID, false) + if tracks == nil { + return resolution{}, fmt.Errorf("resolvePlaylist: could not load tracks for playlist %s", pl.ID) + } + albumIDs, err := tracks.GetAlbumIDs(ctx, model.QueryOptions{Max: PlaylistGridSamples, Sort: "random()"}) if err != nil { return resolution{}, err } diff --git a/core/artwork/resolve_test.go b/core/artwork/resolve_test.go index da144d8e2..2a36531bb 100644 --- a/core/artwork/resolve_test.go +++ b/core/artwork/resolve_test.go @@ -707,6 +707,16 @@ var _ = Describe("resolveItem", func() { Expect(err).To(HaveOccurred()) Expect(res).To(Equal(resolution{})) }) + + It("returns an error when the playlist tracks cannot be loaded", func() { + plRepo := tests.CreateMockPlaylistRepo() + plRepo.SetData(model.Playlists{{ID: "pl4", Name: "Playlist"}}) + ds.MockedPlaylist = plRepo + + res, err := newResolver(ds, ag, ffm, nil).resolve(ctx, model.ArtworkQueueItem{ItemKind: "pl", ItemID: "pl4"}) + Expect(err).To(HaveOccurred()) + Expect(res).To(Equal(resolution{})) + }) }) }) diff --git a/core/artwork/worker.go b/core/artwork/worker.go index 28e51958c..4be99f92e 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -4,9 +4,11 @@ import ( "bytes" "cmp" "context" + "fmt" "io" "math" "math/rand/v2" + "runtime/debug" "sync" "time" @@ -244,7 +246,7 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc item.ImageType = cmp.Or(item.ImageType, model.ImageTypePrimary) trace := &ChainTrace{} ctx = withTrace(ctx, trace) - out, got, retryIn := w.proc.acquire(ctx, item) + out, got, retryIn := w.safeAcquire(ctx, item) queue := w.proc.ds.ArtworkQueue() switch out { @@ -286,6 +288,20 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc return out, got } +// safeAcquire turns a panic into a failed attempt: the drain runs on a bare goroutine, so an +// unrecovered panic would crash the server, and the still-queued row would crash it again on restart. +func (w *Worker) safeAcquire(ctx context.Context, item model.ArtworkQueueItem) (out outcome, got *acquired, retryIn time.Duration) { + defer func() { + if r := recover(); r != nil { + log.Error(ctx, "Artwork: Panic while processing item", "kind", item.ItemKind, "id", item.ItemID, + "imageType", item.ImageType, "attempts", item.Attempts, "panic", r, "stack", string(debug.Stack())) + traceStage(ctx, "panic", fmt.Errorf("%v", r)) + out, got, retryIn = outcomeFailed, nil, 0 + } + }() + return w.proc.acquire(ctx, item) +} + // recordGiveUp keeps the last failure on the state row after the queue row is deleted. An item // that never resolved has no row to update, and creating one would settle it absent. func (w *Worker) recordGiveUp(ctx context.Context, item model.ArtworkQueueItem, trace string) { diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index a6c07b763..80ca68bc3 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -142,6 +142,18 @@ func (v *visibilityPlaylistRepo) Get(ctx context.Context, id string) (*model.Pla return v.MockPlaylistRepo.Get(ctx, id) } +type panickingAlbumRepo struct { + *tests.MockAlbumRepo + panicID string +} + +func (r *panickingAlbumRepo) Get(ctx context.Context, id string) (*model.Album, error) { + if id == r.panicID { + panic("boom") + } + return r.MockAlbumRepo.Get(ctx, id) +} + func adminUserRepo() *tests.MockedUserRepo { repo := tests.CreateMockUserRepo() Expect(repo.Put(GinkgoT().Context(), &model.User{ID: "admin", UserName: "admin", IsAdmin: true})).To(Succeed()) @@ -278,6 +290,36 @@ var _ = Describe("Worker", func() { Expect(err).To(MatchError(model.ErrNotFound), "a timeout must never settle on absent") }) + It("fails an item that panics, without stopping the rest of the batch", func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg"}, + }} + albums := tests.CreateMockAlbumRepo() + albums.SetData(model.Albums{ + {ID: "alboom", Name: "Album", FolderIDs: []string{"f1"}}, + {ID: "alok", Name: "Album", FolderIDs: []string{"f1"}}, + }) + ds.MockedAlbum = &panickingAlbumRepo{MockAlbumRepo: albums, panicID: "alboom"} + Expect(queueRepo.Enqueue(ctx, model.ArtworkQueueItem{ItemKind: "al", ItemID: "alboom"})).To(Succeed()) + Expect(queueRepo.Enqueue(ctx, model.ArtworkQueueItem{ItemKind: "al", ItemID: "alok"})).To(Succeed()) + + n, err := w.drain(ctx, 1) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(Equal(2)) + + it := findQueued(queueRepo, "al", "alboom") + Expect(it).ToNot(BeNil(), "a panicking item must be rescheduled, not dropped") + Expect(it.Attempts).To(Equal(1)) + Expect(it.RetryAt).To(BeTemporally(">", time.Now())) + Expect(it.Trace).To(ContainSubstring("boom")) + + Expect(findQueued(queueRepo, "al", "alok")).To(BeNil()) + ia, err := artRepo.GetItemArtwork(ctx, model.KindAlbumArtwork, "alok", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(ia.Source).To(Equal("folder")) + }) + It("reschedules past the provider's requested delay when it exceeds the backoff", func() { conf.Server.CoverArtPriority = "external" ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "al9", Name: "Album"}}) diff --git a/core/inspect.go b/core/inspect.go index 01ec33760..c60459b88 100644 --- a/core/inspect.go +++ b/core/inspect.go @@ -15,7 +15,7 @@ type InspectOutput struct { MappedTags *model.MediaFile `json:"mappedTags,omitempty"` } -func Inspect(filePath string, libraryId int, folderId string) (*InspectOutput, error) { +func Inspect(filePath string, lib model.Library, folderId string) (*InspectOutput, error) { path, file := filepath.Split(filePath) s, err := storage.For(path) @@ -39,12 +39,22 @@ func Inspect(filePath string, libraryId int, folderId string) (*InspectOutput, e return nil, model.ErrNotFound } - md := metadata.New(path, tag) + md := metadata.New(scannerPath(lib, filePath), tag) result := &InspectOutput{ File: filePath, RawTags: tags[file].Tags, - MappedTags: new(md.ToMediaFile(libraryId, folderId)), + MappedTags: new(md.ToMediaFile(lib, folderId)), } return result, nil } + +// scannerPath returns the path the scanner uses for the file (relative to its library), so +// folder-based PIDs match the DB. Files outside the library keep their absolute path. +func scannerPath(lib model.Library, filePath string) string { + absPath, err := filepath.Abs(filePath) + if err != nil || lib.Path == "" { + return filePath + } + return model.LibraryRelativePath(lib.Path, absPath) +} diff --git a/core/inspect_test.go b/core/inspect_test.go new file mode 100644 index 000000000..0ac90990c --- /dev/null +++ b/core/inspect_test.go @@ -0,0 +1,45 @@ +package core_test + +import ( + "path/filepath" + + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Inspect", func() { + var fixtures string + + BeforeEach(func() { + var err error + fixtures, err = filepath.Abs(filepath.Join("tests", "fixtures")) + Expect(err).ToNot(HaveOccurred()) + }) + + It("maps the file with the library-relative path the scanner uses", func() { + lib := model.Library{ID: 2, Path: filepath.Dir(fixtures), PIDAlbum: "folder"} + out, err := core.Inspect(filepath.Join(fixtures, "test.mp3"), lib, "") + Expect(err).ToNot(HaveOccurred()) + Expect(out.MappedTags.Path).To(Equal("fixtures/test.mp3")) + Expect(out.MappedTags.LibraryID).To(Equal(2)) + }) + + It("gives the same IDs for relative and absolute paths", func() { + lib := model.Library{ID: 2, Path: filepath.Dir(fixtures), PIDAlbum: "folder"} + abs, err := core.Inspect(filepath.Join(fixtures, "test.mp3"), lib, "") + Expect(err).ToNot(HaveOccurred()) + rel, err := core.Inspect(filepath.Join("tests", "fixtures", "test.mp3"), lib, "") + Expect(err).ToNot(HaveOccurred()) + Expect(rel.MappedTags.AlbumID).To(Equal(abs.MappedTags.AlbumID)) + Expect(rel.MappedTags.PID).To(Equal(abs.MappedTags.PID)) + }) + + It("keeps the given path for a file outside the library", func() { + filePath := filepath.Join(fixtures, "test.mp3") + out, err := core.Inspect(filePath, model.Library{ID: model.DefaultLibraryID}, "") + Expect(err).ToNot(HaveOccurred()) + Expect(out.MappedTags.Path).To(Equal(filePath)) + }) +}) diff --git a/core/library.go b/core/library.go index 628ee4b7b..f1153da26 100644 --- a/core/library.go +++ b/core/library.go @@ -2,10 +2,12 @@ package core import ( "context" + "errors" "fmt" "io/fs" "os" "path/filepath" + "slices" "strconv" "strings" "time" @@ -15,6 +17,7 @@ import ( "github.com/navidrome/navidrome/core/storage" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/metadata" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/utils/slice" @@ -200,23 +203,22 @@ func (r *libraryRepositoryWrapper) Update(ctx context.Context, id string, entity } pathChanged := originalLib.Path != lib.Path + pidChanged := (updatesColumn(cols, "pidAlbum") && originalLib.PIDAlbum != lib.PIDAlbum) || + (updatesColumn(cols, "pidTrack") && originalLib.PIDTrack != lib.PIDTrack) err = r.LibraryRepository.Put(ctx, lib, cols...) if err != nil { return r.mapError(err) } - // Restart watcher and trigger scan if path was updated - if pathChanged { - if r.watcher != nil { - if err := r.watcher.Watch(ctx, lib); err != nil { - log.Warn(ctx, "Failed to restart watcher for updated library", "libraryID", lib.ID, "name", lib.Name, "path", lib.Path, err) - } + if pathChanged && r.watcher != nil { + if err := r.watcher.Watch(ctx, lib); err != nil { + log.Warn(ctx, "Failed to restart watcher for updated library", "libraryID", lib.ID, "name", lib.Name, "path", lib.Path, err) } + } - if r.scanner != nil { - go r.triggerScan(ctx, lib, "updated") - } + if (pathChanged || pidChanged) && r.scanner != nil { + go r.triggerScan(ctx, lib, "updated") } // Send library refresh event to all clients @@ -325,6 +327,15 @@ func (r *libraryRepositoryWrapper) validateLibrary(ctx context.Context, library } } + library.PIDAlbum = strings.TrimSpace(library.PIDAlbum) + library.PIDTrack = strings.TrimSpace(library.PIDTrack) + if err := metadata.ValidatePIDSpec(library.PIDAlbum, true); err != nil { + validationErrors["pidAlbum"] = err.Error() + } + if err := metadata.ValidatePIDSpec(library.PIDTrack, false); err != nil { + validationErrors["pidTrack"] = err.Error() + } + if len(validationErrors) > 0 { return &rest.ValidationError{Errors: validationErrors} } @@ -332,6 +343,11 @@ func (r *libraryRepositoryWrapper) validateLibrary(ctx context.Context, library return nil } +// updatesColumn reports whether an update with these columns writes col. No columns means all of them. +func updatesColumn(cols []string, col string) bool { + return len(cols) == 0 || slices.Contains(cols, col) +} + func (r *libraryRepositoryWrapper) validateLibraryPath(ctx context.Context, library *model.Library) error { // Validate path format if !filepath.IsAbs(library.Path) { @@ -399,11 +415,27 @@ func (s *libraryService) validateLibraryIDs(ctx context.Context, libraryIDs []in return nil } +var scanWaitInterval = time.Second + func (r *libraryRepositoryWrapper) triggerScan(ctx context.Context, lib *model.Library, action string) { + // Runs in its own goroutine and outlives the HTTP request + ctx = context.WithoutCancel(ctx) + + // A running scan loaded the libraries before this change, and would reject a new request + for { + status, err := r.scanner.Status(ctx) + if err != nil || !status.Scanning { + break + } + time.Sleep(scanWaitInterval) + } + log.Info(ctx, fmt.Sprintf("Triggering scan for %s library", action), "libraryID", lib.ID, "name", lib.Name, "path", lib.Path) start := time.Now() - warnings, err := r.scanner.ScanAll(ctx, false) // Quick scan for new library - if err != nil { + warnings, err := r.scanner.ScanAll(ctx, false) // Quick scan: the scanner rescans libraries with a changed PID config in full + if errors.Is(err, model.ErrAlreadyScanning) { + log.Debug(ctx, "Scan already running, it covers this change", "libraryID", lib.ID, "name", lib.Name) + } else if err != nil { log.Error(ctx, fmt.Sprintf("Error scanning %s library", action), "libraryID", lib.ID, "name", lib.Name, err) } else { log.Info(ctx, fmt.Sprintf("Scan completed for %s library", action), "libraryID", lib.ID, "name", lib.Name, "warnings", len(warnings), "elapsed", time.Since(start)) diff --git a/core/library_test.go b/core/library_test.go index 5402eac22..e6ebb1974 100644 --- a/core/library_test.go +++ b/core/library_test.go @@ -322,6 +322,37 @@ var _ = Describe("Library Service", func() { }) }) + Describe("PID validation", func() { + pidError := func(err error, field string) string { + var validationErr *rest.ValidationError + Expect(errors.As(err, &validationErr)).To(BeTrue()) + return validationErr.Errors[field] + } + + It("rejects an unknown attribute in the album PID", func() { + _, err := repo.Save(ctx, &model.Library{Name: "Lib", Path: tempDir, PIDAlbum: "albmversion"}) + Expect(pidError(err, "pidAlbum")).To(ContainSubstring(`unknown attribute "albmversion"`)) + }) + + It("rejects albumid in the album PID", func() { + _, err := repo.Save(ctx, &model.Library{Name: "Lib", Path: tempDir, PIDAlbum: "albumid"}) + Expect(pidError(err, "pidAlbum")).To(ContainSubstring("albumid")) + }) + + It("rejects an unknown attribute in the track PID", func() { + _, err := repo.Save(ctx, &model.Library{Name: "Lib", Path: tempDir, PIDTrack: "nosuchtag"}) + Expect(pidError(err, "pidTrack")).To(ContainSubstring(`unknown attribute "nosuchtag"`)) + }) + + It("trims spaces", func() { + library := &model.Library{Name: "Lib", Path: tempDir, PIDAlbum: " folder ", PIDTrack: " "} + _, err := repo.Save(ctx, library) + Expect(err).ToNot(HaveOccurred()) + Expect(library.PIDAlbum).To(Equal("folder")) + Expect(library.PIDTrack).To(BeEmpty()) + }) + }) + Describe("Path Validation", func() { Context("Create operation", func() { It("fails when path is not absolute", func() { @@ -679,6 +710,48 @@ var _ = Describe("Library Service", func() { }, "100ms", "10ms").Should(Equal(0)) }) + It("triggers scan when updating the library PID config", func() { + libraryRepo.SetData(model.Libraries{{ID: 1, Name: "Library", Path: tempDir}}) + + library := model.Library{ID: 1, Name: "Library", Path: tempDir, PIDAlbum: "folder"} + Expect(repo.Update(ctx, "1", library)).To(Succeed()) + + Eventually(func() int { + return scanner.GetScanAllCallCount() + }, "1s", "10ms").Should(Equal(1)) + // A quick scan: the scanner itself rescans this library in full + Expect(scanner.GetScanAllCalls()[0].FullScan).To(BeFalse()) + }) + + It("does not trigger scan when the PID fields were not sent", func() { + libraryRepo.SetData(model.Libraries{{ID: 1, Name: "Library", Path: tempDir, PIDAlbum: "folder"}}) + + // The REST layer decodes a missing pidAlbum as "". Only the sent fields count. + library := model.Library{ID: 1, Name: "Renamed", Path: tempDir} + Expect(repo.Update(ctx, "1", library, "name", "path")).To(Succeed()) + + Consistently(func() int { + return scanner.GetScanAllCallCount() + }, "100ms", "10ms").Should(Equal(0)) + }) + + It("waits for a running scan before triggering a new one", func() { + libraryRepo.SetData(model.Libraries{{ID: 1, Name: "Library", Path: tempDir}}) + scanner.SetScanning(true) + + library := model.Library{ID: 1, Name: "Library", Path: tempDir, PIDAlbum: "folder"} + Expect(repo.Update(ctx, "1", library)).To(Succeed()) + + Consistently(func() int { + return scanner.GetScanAllCallCount() + }, "200ms", "20ms").Should(Equal(0)) + + scanner.SetScanning(false) + Eventually(func() int { + return scanner.GetScanAllCallCount() + }, "3s", "20ms").Should(Equal(1)) + }) + It("does not trigger scan when library creation fails", func() { // Try to create library with invalid data (empty name) library := &model.Library{Path: tempDir} diff --git a/core/metrics/insights.go b/core/metrics/insights.go index 4a78a7f3f..1d2ff5df4 100644 --- a/core/metrics/insights.go +++ b/core/metrics/insights.go @@ -10,6 +10,7 @@ import ( "path/filepath" "runtime" "runtime/debug" + "slices" "strings" "sync" "sync/atomic" @@ -269,9 +270,13 @@ func (c *insightsCollector) collect(ctx context.Context) []byte { if err != nil { log.Trace(ctx, "Error reading radios count", err) } - data.Library.Libraries, err = c.ds.Library().CountAll(ctx) + libs, err := c.ds.Library().GetAll(ctx) if err != nil { - log.Trace(ctx, "Error reading libraries count", err) + log.Trace(ctx, "Error reading libraries", err) + } + data.Library.Libraries = int64(len(libs)) + if slices.ContainsFunc(libs, func(lib model.Library) bool { return lib.PIDAlbum != "" || lib.PIDTrack != "" }) { + data.Config.HasCustomPID = true } data.Library.ActiveUsers, err = c.ds.User().CountAll(ctx, model.QueryOptions{ Filters: squirrel.Gt{"last_access_at": time.Now().Add(-7 * 24 * time.Hour)}, diff --git a/core/players.go b/core/players.go index e03d8caa2..6fe86fd70 100644 --- a/core/players.go +++ b/core/players.go @@ -17,6 +17,7 @@ import ( type Players interface { Get(ctx context.Context, playerId string) (*model.Player, error) Register(ctx context.Context, id, client, userAgent, ip string) (*model.Player, *model.Transcoding, error) + Touch(ctx context.Context, plr model.Player, client, userAgent, ip string) (*model.Player, *model.Transcoding, error) } func NewPlayers(ds model.DataStore) Players { @@ -33,7 +34,6 @@ type players struct { func (p *players) Register(ctx context.Context, playerID, client, userAgent, ip string) (*model.Player, *model.Transcoding, error) { var plr *model.Player - var trc *model.Transcoding var err error user, _ := request.UserFrom(ctx) if playerID != "" { @@ -58,7 +58,21 @@ func (p *players) Register(ctx context.Context, playerID, client, userAgent, ip log.Info(ctx, "Registering new player", "id", plr.ID, "client", client, "username", username, "type", userAgent) } } - plr.Name = fmt.Sprintf("%s [%s]", client, userAgent) + if !plr.HasAPIKey { + plr.Name = fmt.Sprintf("%s [%s]", client, userAgent) + } + return p.refresh(ctx, plr, userAgent, ip) +} + +// Touch refreshes a player that the request already identified (by API key), without guessing or renaming it. +func (p *players) Touch(ctx context.Context, plr model.Player, client, userAgent, ip string) (*model.Player, *model.Transcoding, error) { + if plr.Client == "" { + plr.Client = client + } + return p.refresh(ctx, &plr, userAgent, ip) +} + +func (p *players) refresh(ctx context.Context, plr *model.Player, userAgent, ip string) (*model.Player, *model.Transcoding, error) { plr.UserAgent = userAgent plr.IP = ip plr.LastSeen = time.Now() @@ -66,14 +80,14 @@ func (p *players) Register(ctx context.Context, playerID, client, userAgent, ip ctx, cancel := context.WithTimeout(ctx, time.Second) defer cancel() - err = p.ds.Player().Put(ctx, plr) - if err != nil { - log.Warn(ctx, "Could not save player", "id", plr.ID, "client", client, "username", username, "type", userAgent, err) + if err := p.ds.Player().Put(ctx, plr); err != nil { + log.Warn(ctx, "Could not save player", "id", plr.ID, "client", plr.Client, "username", userName(ctx), "type", plr.UserAgent, err) } }) - if plr.TranscodingId != "" { - trc, err = p.ds.Transcoding().Get(ctx, plr.TranscodingId) + if plr.TranscodingId == "" { + return plr, nil, nil } + trc, err := p.ds.Transcoding().Get(ctx, plr.TranscodingId) return plr, trc, err } diff --git a/core/players_test.go b/core/players_test.go index 302d63157..e452c52ba 100644 --- a/core/players_test.go +++ b/core/players_test.go @@ -114,6 +114,15 @@ var _ = Describe("Players", func() { Expect(trc.ID).To(Equal("1")) }) + It("does not rename a player that has an API key", func() { + plr := &model.Player{ID: "123", Name: "My Phone", Client: "client", UserId: "userid", HasAPIKey: true} + repo.add(plr) + p, _, err := players.Register(ctx, "123", "client", "chrome", "1.2.3.4") + Expect(err).ToNot(HaveOccurred()) + Expect(p.ID).To(Equal("123")) + Expect(p.Name).To(Equal("My Phone")) + }) + Context("bad username casing", func() { ctx := log.NewContext(context.TODO()) ctx = request.WithUser(ctx, model.User{ID: "userid", UserName: "Johndoe"}) @@ -130,6 +139,34 @@ var _ = Describe("Players", func() { }) }) }) + + Describe("Touch", func() { + It("records usage but keeps the name and client", func() { + plr := model.Player{ID: "123", Name: "My Phone", Client: "Symfonium", UserId: "userid", HasAPIKey: true} + p, trc, err := players.Touch(ctx, plr, "OtherClient", "android", "1.2.3.4") + Expect(err).ToNot(HaveOccurred()) + Expect(p.Name).To(Equal("My Phone")) + Expect(p.Client).To(Equal("Symfonium")) + Expect(p.UserAgent).To(Equal("android")) + Expect(p.IP).To(Equal("1.2.3.4")) + Expect(p.LastSeen).To(BeTemporally(">=", beforeRegister)) + Expect(repo.lastSaved).To(Equal(p)) + Expect(trc).To(BeNil()) + }) + + It("fills in the client on first use", func() { + p, _, err := players.Touch(ctx, model.Player{ID: "123", Name: "Manual", UserId: "userid"}, "Symfonium", "android", "1.2.3.4") + Expect(err).ToNot(HaveOccurred()) + Expect(p.Client).To(Equal("Symfonium")) + }) + + It("returns the player's transcoding", func() { + p, trc, err := players.Touch(ctx, model.Player{ID: "123", UserId: "userid", TranscodingId: "1"}, "c", "ua", "1.2.3.4") + Expect(err).ToNot(HaveOccurred()) + Expect(p.ID).To(Equal("123")) + Expect(trc.ID).To(Equal("1")) + }) + }) }) type mockPlayerRepository struct { diff --git a/core/playlists/import.go b/core/playlists/import.go index 658bd92dc..b5991b095 100644 --- a/core/playlists/import.go +++ b/core/playlists/import.go @@ -78,8 +78,8 @@ func (s *playlists) resolveFolder(ctx context.Context, dir string) (*model.Folde if err != nil { return nil, err } - matcher := newLibraryMatcher(libs) - lib, ok := matcher.findLibrary(dir) + matcher := model.NewLibraryMatcher(libs) + lib, ok := matcher.FindLibrary(dir) if !ok { return nil, fmt.Errorf("%w: %s", errNotInLibrary, dir) } diff --git a/core/playlists/parse_m3u.go b/core/playlists/parse_m3u.go index 9610e9dbb..ab95b8850 100644 --- a/core/playlists/parse_m3u.go +++ b/core/playlists/parse_m3u.go @@ -1,13 +1,11 @@ package playlists import ( - "cmp" "context" "fmt" "io" "net/url" "path/filepath" - "slices" "strings" "time" @@ -156,61 +154,9 @@ func (r pathResolution) ToQualifiedString() (string, error) { return fmt.Sprintf("%d:%s", r.libraryID, filepath.ToSlash(relativePath)), nil } -// libraryMatcher holds sorted libraries with cleaned paths for efficient path matching. -type libraryMatcher struct { - libraries model.Libraries - cleanedPaths []string -} - -// findLibraryForPath finds which library contains the given absolute path. -// Returns library ID and path, or 0 and empty string if not found. -func (lm *libraryMatcher) findLibraryForPath(absolutePath string) (int, string) { - lib, ok := lm.findLibrary(absolutePath) - if !ok { - return 0, "" - } - return lib.ID, filepath.Clean(lib.Path) -} - -// findLibrary checks if the absolute path is under any of the library paths. -func (lm *libraryMatcher) findLibrary(absolutePath string) (model.Library, bool) { - // Check sorted libraries (longest path first) to find the best match - for i, cleanLibPath := range lm.cleanedPaths { - // Check if absolutePath is under this library path - if strings.HasPrefix(absolutePath, cleanLibPath) { - // Ensure it's a proper path boundary (not just a prefix) - if len(absolutePath) == len(cleanLibPath) || absolutePath[len(cleanLibPath)] == filepath.Separator { - return lm.libraries[i], true - } - } - } - return model.Library{}, false -} - -// newLibraryMatcher creates a libraryMatcher with libraries sorted by path length (longest first). -// This ensures correct matching when library paths are prefixes of each other. -// Example: /music-classical must be checked before /music -// Otherwise, /music-classical/track.mp3 would match /music instead of /music-classical -func newLibraryMatcher(libs model.Libraries) *libraryMatcher { - // Sort libraries by path length (descending) to ensure longest paths match first. - slices.SortFunc(libs, func(i, j model.Library) int { - return cmp.Compare(len(j.Path), len(i.Path)) // Reverse order for descending - }) - - // Pre-clean all library paths once for efficient matching - cleanedPaths := make([]string, len(libs)) - for i, lib := range libs { - cleanedPaths[i] = filepath.Clean(lib.Path) - } - return &libraryMatcher{ - libraries: libs, - cleanedPaths: cleanedPaths, - } -} - // pathResolver handles path resolution logic for playlist imports. type pathResolver struct { - matcher *libraryMatcher + matcher *model.LibraryMatcher } // newPathResolver creates a pathResolver with libraries loaded from the datastore. @@ -219,7 +165,7 @@ func newPathResolver(ctx context.Context, ds model.DataStore) (*pathResolver, er if err != nil { return nil, err } - matcher := newLibraryMatcher(libs) + matcher := model.NewLibraryMatcher(libs) return &pathResolver{matcher: matcher}, nil } @@ -246,14 +192,14 @@ func (r *pathResolver) resolvePath(line string, folder *model.Folder) pathResolu // a pathResolution with the library information. Returns an invalid resolution if // the path is not found in any library. func (r *pathResolver) findInLibraries(absolutePath string) pathResolution { - libID, libPath := r.matcher.findLibraryForPath(absolutePath) - if libID == 0 { + lib, ok := r.matcher.FindLibrary(absolutePath) + if !ok { return pathResolution{valid: false} } return pathResolution{ absolutePath: absolutePath, - libraryPath: libPath, - libraryID: libID, + libraryPath: filepath.Clean(lib.Path), + libraryID: lib.ID, valid: true, } } @@ -288,7 +234,7 @@ func (r *pathResolver) resolvePaths(ctx context.Context, folder *model.Folder, l // HTTP(S) URLs are stored as-is (gated by EnableM3UExternalAlbumArt). // Local paths (file://, absolute, or relative) are resolved to an absolute path // and validated against known library boundaries via matcher. -func resolveImageURL(value string, folder *model.Folder, matcher *libraryMatcher, owner model.User) string { +func resolveImageURL(value string, folder *model.Folder, matcher *model.LibraryMatcher, owner model.User) string { value = strings.TrimSpace(value) if value == "" { return "" @@ -308,7 +254,7 @@ func resolveImageURL(value string, folder *model.Folder, matcher *libraryMatcher return "" } - lib, ok := matcher.findLibrary(localPath) + lib, ok := matcher.FindLibrary(localPath) // A playlist without a folder (API upload, or CLI import from outside all libraries) may only use the owner's libraries. if !ok || (folder == nil && !owner.HasLibraryAccess(lib.ID)) { return "" diff --git a/core/playlists/parse_m3u_test.go b/core/playlists/parse_m3u_test.go index b6a3a96f9..ced6c2b16 100644 --- a/core/playlists/parse_m3u_test.go +++ b/core/playlists/parse_m3u_test.go @@ -9,187 +9,6 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("libraryMatcher", func() { - var ds *tests.MockDataStore - var mockLibRepo *tests.MockLibraryRepo - ctx := context.Background() - - BeforeEach(func() { - tests.SkipOnWindows("path separator bug (#TBD-path-sep-playlists)") - mockLibRepo = &tests.MockLibraryRepo{} - ds = &tests.MockDataStore{ - MockedLibrary: mockLibRepo, - } - }) - - // Helper function to create a libraryMatcher from the mock datastore - createMatcher := func(ds model.DataStore) *libraryMatcher { - libs, err := ds.Library().GetAll(ctx) - Expect(err).ToNot(HaveOccurred()) - return newLibraryMatcher(libs) - } - - Describe("Longest library path matching", func() { - It("matches the longest library path when multiple libraries share a prefix", func() { - // Setup libraries with prefix conflicts - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/music"}, - {ID: 2, Path: "/music-classical"}, - {ID: 3, Path: "/music-classical/opera"}, - }) - - matcher := createMatcher(ds) - - // Test that longest path matches first and returns correct library ID - testCases := []struct { - path string - expectedLibID int - expectedLibPath string - }{ - {"/music-classical/opera/track.mp3", 3, "/music-classical/opera"}, - {"/music-classical/track.mp3", 2, "/music-classical"}, - {"/music/track.mp3", 1, "/music"}, - {"/music-classical/opera/subdir/file.mp3", 3, "/music-classical/opera"}, - } - - for _, tc := range testCases { - libID, libPath := matcher.findLibraryForPath(tc.path) - Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d, but got %d", tc.path, tc.expectedLibID, libID) - Expect(libPath).To(Equal(tc.expectedLibPath), "Path %s should match library path %s, but got %s", tc.path, tc.expectedLibPath, libPath) - } - }) - - It("handles libraries with similar prefixes but different structures", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/home/user/music"}, - {ID: 2, Path: "/home/user/music-backup"}, - }) - - matcher := createMatcher(ds) - - // Test that music-backup library is matched correctly - libID, libPath := matcher.findLibraryForPath("/home/user/music-backup/track.mp3") - Expect(libID).To(Equal(2)) - Expect(libPath).To(Equal("/home/user/music-backup")) - - // Test that music library is still matched correctly - libID, libPath = matcher.findLibraryForPath("/home/user/music/track.mp3") - Expect(libID).To(Equal(1)) - Expect(libPath).To(Equal("/home/user/music")) - }) - - It("matches path that is exactly the library root", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/music"}, - {ID: 2, Path: "/music-classical"}, - }) - - matcher := createMatcher(ds) - - // Exact library path should match - libID, libPath := matcher.findLibraryForPath("/music-classical") - Expect(libID).To(Equal(2)) - Expect(libPath).To(Equal("/music-classical")) - }) - - It("handles complex nested library structures", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/media"}, - {ID: 2, Path: "/media/audio"}, - {ID: 3, Path: "/media/audio/classical"}, - {ID: 4, Path: "/media/audio/classical/baroque"}, - }) - - matcher := createMatcher(ds) - - testCases := []struct { - path string - expectedLibID int - expectedLibPath string - }{ - {"/media/audio/classical/baroque/bach/track.mp3", 4, "/media/audio/classical/baroque"}, - {"/media/audio/classical/mozart/track.mp3", 3, "/media/audio/classical"}, - {"/media/audio/rock/track.mp3", 2, "/media/audio"}, - {"/media/video/movie.mp4", 1, "/media"}, - } - - for _, tc := range testCases { - libID, libPath := matcher.findLibraryForPath(tc.path) - Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d", tc.path, tc.expectedLibID) - Expect(libPath).To(Equal(tc.expectedLibPath), "Path %s should match library path %s", tc.path, tc.expectedLibPath) - } - }) - }) - - Describe("Edge cases", func() { - It("handles empty library list", func() { - mockLibRepo.SetData([]model.Library{}) - - matcher := createMatcher(ds) - Expect(matcher).ToNot(BeNil()) - - // Should not match anything - libID, libPath := matcher.findLibraryForPath("/music/track.mp3") - Expect(libID).To(Equal(0)) - Expect(libPath).To(BeEmpty()) - }) - - It("handles single library", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/music"}, - }) - - matcher := createMatcher(ds) - - libID, libPath := matcher.findLibraryForPath("/music/track.mp3") - Expect(libID).To(Equal(1)) - Expect(libPath).To(Equal("/music")) - }) - - It("handles libraries with special characters in paths", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/music[test]"}, - {ID: 2, Path: "/music(backup)"}, - }) - - matcher := createMatcher(ds) - Expect(matcher).ToNot(BeNil()) - - // Special characters should match literally - libID, libPath := matcher.findLibraryForPath("/music[test]/track.mp3") - Expect(libID).To(Equal(1)) - Expect(libPath).To(Equal("/music[test]")) - }) - }) - - Describe("Path matching order", func() { - It("ensures longest paths match first", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/a"}, - {ID: 2, Path: "/ab"}, - {ID: 3, Path: "/abc"}, - }) - - matcher := createMatcher(ds) - - // Verify that longer paths match correctly (not cut off by shorter prefix) - testCases := []struct { - path string - expectedLibID int - }{ - {"/abc/file.mp3", 3}, - {"/ab/file.mp3", 2}, - {"/a/file.mp3", 1}, - } - - for _, tc := range testCases { - libID, _ := matcher.findLibraryForPath(tc.path) - Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d", tc.path, tc.expectedLibID) - } - }) - }) -}) - var _ = Describe("pathResolver", func() { var ds *tests.MockDataStore var mockLibRepo *tests.MockLibraryRepo diff --git a/db/migrations/20260924010054_add_player_api_key_hash.sql b/db/migrations/20260924010054_add_player_api_key_hash.sql new file mode 100644 index 000000000..bbc4cf4d9 --- /dev/null +++ b/db/migrations/20260924010054_add_player_api_key_hash.sql @@ -0,0 +1,8 @@ +-- +goose Up +-- +goose StatementBegin +alter table player add column api_key_hash varchar default null; +create unique index if not exists player_api_key_hash on player(api_key_hash); +-- +goose StatementEnd + +-- +goose Down +SELECT 1; diff --git a/db/migrations/20260929221042_add_library_pid_columns.sql b/db/migrations/20260929221042_add_library_pid_columns.sql new file mode 100644 index 000000000..487512287 --- /dev/null +++ b/db/migrations/20260929221042_add_library_pid_columns.sql @@ -0,0 +1,18 @@ +-- +goose Up +-- +goose StatementBegin +alter table library add column pid_album varchar default '' not null; +alter table library add column pid_track varchar default '' not null; +alter table library add column scanned_pid_album varchar default '' not null; +alter table library add column scanned_pid_track varchar default '' not null; + +-- Every library was scanned with the global PID config, so seed it as their scanned config. +-- This way the upgrade does not trigger a full rescan. +update library set + scanned_pid_album = coalesce((select value from property where id = 'PIDAlbum'), ''), + scanned_pid_track = coalesce((select value from property where id = 'PIDTrack'), ''); + +delete from property where id in ('PIDAlbum', 'PIDTrack'); +-- +goose StatementEnd + +-- +goose Down +SELECT 1; diff --git a/log/log.go b/log/log.go index de2f171b2..da1d7622e 100644 --- a/log/log.go +++ b/log/log.go @@ -27,14 +27,16 @@ var redacted = &Hook{ AcceptedLevels: logrus.AllLevels, RedactionList: []string{ // Keys from the config - "(ApiKey:\")[\\w]*", - "(Secret:\")[\\w]*", + "(ApiKey:[\\s]*\")[\\w]*", + "(Secret:[\\s]*\")[\\w]*", "(PasswordEncryptionKey:[\\s]*\")[^\"]*", "(UserHeader:[\\s]*\")[^\"]*", "(TrustedSources:[\\s]*\")[^\"]*", "(MetricsPath:[\\s]*\")[^\"]*", "(DevAutoCreateAdminPassword:[\\s]*\")[^\"]*", "(DevAutoLoginUsername:[\\s]*\")[^\"]*", + // Prometheus.Password. Any character is allowed, so skip escaped quotes in the value + `(Password:[\s]*")(?:[^"\\]|\\.)*`, // UI appConfig "(subsonicToken:)[\\w]+(\\s)", diff --git a/log/log_test.go b/log/log_test.go index 82207c672..184ff57db 100644 --- a/log/log_test.go +++ b/log/log_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/kr/pretty" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/sirupsen/logrus" @@ -94,7 +95,7 @@ var _ = Describe("Logger", func() { SetLogSourceLine(true) Error("A crash happened") // NOTE: This assertion breaks if the line number above changes - Expect(hook.LastEntry().Data[" source"]).To(ContainSubstring("/log/log_test.go:95")) + Expect(hook.LastEntry().Data[" source"]).To(ContainSubstring("/log/log_test.go:96")) Expect(hook.LastEntry().Message).To(Equal("A crash happened")) }) @@ -291,5 +292,69 @@ var _ = Describe("Logger", func() { Expect(got).ToNot(ContainSubstring("secret")) Expect(got).To(ContainSubstring(`"User-Agent":["Finamp/1.0"]`)) }) + + // https://github.com/navidrome/navidrome/discussions/6232 + DescribeTable("redacts config keys in the startup Configuration dump", + func(line, expected string) { + Expect(Redact(line)).To(Equal(expected)) + }, + Entry("unpadded ApiKey", `ApiKey:"0123456789abcdef0123456789abcdef"`, `ApiKey:"[REDACTED]"`), + Entry("unpadded Secret", `Secret:"fedcba9876543210fedcba9876543210"`, `Secret:"[REDACTED]"`), + Entry("padded ApiKey", ` ApiKey: "0123456789abcdef0123456789abcdef",`, + ` ApiKey: "[REDACTED]",`), + Entry("padded Secret", ` Secret: "fedcba9876543210fedcba9876543210",`, + ` Secret: "[REDACTED]",`), + Entry("unpadded Prometheus Password", `Password:"p@ss w0rd!"`, `Password:"[REDACTED]"`), + Entry("padded Prometheus Password", ` Password: "p@ss w0rd!",`, ` Password: "[REDACTED]",`), + Entry("Prometheus Password with escaped quotes", ` Password: "a\"b\\\"c",`, + ` Password: "[REDACTED]",`), + ) + + It("redacts secrets in a pretty-printed config struct", func() { + // Mirrors conf.lastfmOptions and conf.prometheusOptions (conf imports log, so it can't be + // used here). pretty only breaks a struct into padded lines when it is long enough, so + // keep all the fields. + type lastfmOptions struct { + Enabled bool + ApiKey string + Secret string + Language string + ScrobbleFirstArtistOnly bool + Languages []string + } + type prometheusOptions struct { + Enabled bool + MetricsPath string + Password string + } + type configOptions struct { + Address string + LastFM lastfmOptions + Prometheus prometheusOptions + } + cfg := configOptions{ + Address: "0.0.0.0", + LastFM: lastfmOptions{ //nolint:gosec + Enabled: true, + ApiKey: "0123456789abcdef0123456789abcdef", + Secret: "fedcba9876543210fedcba9876543210", + Language: "en", + Languages: []string{"en"}, + }, + Prometheus: prometheusOptions{ //nolint:gosec + Enabled: true, + MetricsPath: "/metrics", + Password: `prom"pass-tail`, + }, + } + dump := pretty.Sprintf("Configuration: %# v", cfg) + Expect(dump).To(MatchRegexp(`ApiKey:\s{2,}"`), "the dump must use the padded layout") + + got := Redact(dump) + Expect(got).ToNot(ContainSubstring(cfg.LastFM.ApiKey)) + Expect(got).ToNot(ContainSubstring(cfg.LastFM.Secret)) + Expect(got).ToNot(ContainSubstring("pass-tail")) + Expect(got).To(ContainSubstring(`"en"`)) + }) }) }) diff --git a/model/library.go b/model/library.go index 1e33222ac..e80d22c89 100644 --- a/model/library.go +++ b/model/library.go @@ -1,10 +1,13 @@ package model import ( + "cmp" "context" + "strings" "time" "github.com/deluan/rest" + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/utils/slice" ) @@ -27,6 +30,38 @@ type Library struct { TotalSize int64 `json:"totalSize" db:"total_size"` TotalDuration float64 `json:"totalDuration" db:"total_duration"` DefaultNewUsers bool `json:"defaultNewUsers" db:"default_new_users"` + PIDAlbum string `json:"pidAlbum" db:"pid_album"` + PIDTrack string `json:"pidTrack" db:"pid_track"` + ScannedPIDAlbum string `json:"-" db:"scanned_pid_album"` + ScannedPIDTrack string `json:"-" db:"scanned_pid_track"` +} + +// PIDConfig holds the persistent ID specs used to compute track and album IDs. +type PIDConfig struct { + Track string + Album string +} + +// EffectivePID returns the PID specs in effect for this library: its own overrides, falling back to +// the global config. +func (l Library) EffectivePID() PIDConfig { + return PIDConfig{ + Track: cmp.Or(l.PIDTrack, conf.Server.PID.Track), + Album: cmp.Or(l.PIDAlbum, conf.Server.PID.Album), + } +} + +// PIDChanged reports whether the effective PID specs differ from the ones used by the last finished +// scan of this library. A library that was never scanned counts as changed. +func (l Library) PIDChanged() bool { + pid := l.EffectivePID() + return !strings.EqualFold(l.ScannedPIDAlbum, pid.Album) || !strings.EqualFold(l.ScannedPIDTrack, pid.Track) +} + +// NeedsPIDRescan reports whether the library has content imported with an old PID config, so it must be +// rescanned in full. A library that never finished a scan has nothing to regroup. +func (l Library) NeedsPIDRescan() bool { + return !l.LastScanAt.IsZero() && l.PIDChanged() } const ( @@ -59,6 +94,8 @@ type LibraryRepository interface { // TODO These methods should be moved to a core service ScanBegin(ctx context.Context, id int, fullScan bool) error ScanEnd(ctx context.Context, id int) error + // SetScannedPID records the PID specs used by the last finished scan of the library + SetScannedPID(ctx context.Context, id int, pid PIDConfig) error ScanInProgress(ctx context.Context) (bool, error) RefreshStats(ctx context.Context, id int) error } diff --git a/model/library_matcher.go b/model/library_matcher.go new file mode 100644 index 000000000..83af96f9f --- /dev/null +++ b/model/library_matcher.go @@ -0,0 +1,57 @@ +package model + +import ( + "cmp" + "path/filepath" + "slices" + "strings" +) + +// LibraryMatcher finds the library that contains an absolute path. +type LibraryMatcher struct { + libraries Libraries + cleanedPaths []string +} + +// NewLibraryMatcher sorts the libraries longest path first, so /music-classical is checked before /music. +func NewLibraryMatcher(libs Libraries) *LibraryMatcher { + libs = slices.Clone(libs) + slices.SortFunc(libs, func(i, j Library) int { + return cmp.Compare(len(j.Path), len(i.Path)) + }) + cleanedPaths := make([]string, len(libs)) + for i, lib := range libs { + cleanedPaths[i] = filepath.Clean(lib.Path) + } + return &LibraryMatcher{libraries: libs, cleanedPaths: cleanedPaths} +} + +// FindLibrary returns the library whose path contains absolutePath. +func (lm *LibraryMatcher) FindLibrary(absolutePath string) (Library, bool) { + for i, libPath := range lm.cleanedPaths { + // A cleaned path only ends with a separator when it is a filesystem root + if strings.HasPrefix(absolutePath, libPath) && (len(absolutePath) == len(libPath) || + absolutePath[len(libPath)] == filepath.Separator || strings.HasSuffix(libPath, string(filepath.Separator))) { + return lm.libraries[i], true + } + } + return Library{}, false +} + +// LibraryRelativePath rebases an absolute path onto the library root, as the scanner's io/fs sees it +// (forward slashes). Relative paths, and absolute paths outside the library root, are returned unchanged. +func LibraryRelativePath(libPath, path string) string { + if !filepath.IsAbs(path) { + return path + } + // The library root may be relative (e.g. the default "./music"); it resolves against the same cwd + absLib, err := filepath.Abs(libPath) + if err != nil { + return path + } + rel, err := filepath.Rel(absLib, path) + if err != nil || !filepath.IsLocal(rel) { + return path + } + return filepath.ToSlash(rel) +} diff --git a/model/library_matcher_test.go b/model/library_matcher_test.go new file mode 100644 index 000000000..09e6f7e33 --- /dev/null +++ b/model/library_matcher_test.go @@ -0,0 +1,91 @@ +package model_test + +import ( + "os" + "path/filepath" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("LibraryMatcher", func() { + // Paths are written Unix-style and converted, so they use the OS separator, as filepath.Abs output does + find := func(libs model.Libraries, path string) int { + for i := range libs { + libs[i].Path = filepath.FromSlash(libs[i].Path) + } + lib, ok := model.NewLibraryMatcher(libs).FindLibrary(filepath.FromSlash(path)) + if !ok { + return 0 + } + return lib.ID + } + + DescribeTable("matches the longest library path", + func(libs model.Libraries, path string, expectedID int) { + Expect(find(libs, path)).To(Equal(expectedID)) + }, + Entry("nested library", model.Libraries{{ID: 1, Path: "/music"}, {ID: 2, Path: "/music-classical"}, {ID: 3, Path: "/music-classical/opera"}}, "/music-classical/opera/subdir/track.mp3", 3), + Entry("sibling with a shared prefix", model.Libraries{{ID: 1, Path: "/music"}, {ID: 2, Path: "/music-classical"}}, "/music-classical/track.mp3", 2), + Entry("shorter library", model.Libraries{{ID: 1, Path: "/music"}, {ID: 2, Path: "/music-classical"}}, "/music/track.mp3", 1), + Entry("exact library root", model.Libraries{{ID: 1, Path: "/music"}, {ID: 2, Path: "/music-classical"}}, "/music-classical", 2), + Entry("deeply nested libraries", model.Libraries{{ID: 1, Path: "/media"}, {ID: 2, Path: "/media/audio"}, {ID: 3, Path: "/media/audio/classical"}, {ID: 4, Path: "/media/audio/classical/baroque"}}, "/media/audio/classical/mozart/track.mp3", 3), + Entry("prefix that is not a path boundary", model.Libraries{{ID: 1, Path: "/a"}, {ID: 2, Path: "/ab"}, {ID: 3, Path: "/abc"}}, "/ab/file.mp3", 2), + Entry("special characters match literally", model.Libraries{{ID: 1, Path: "/music[test]"}, {ID: 2, Path: "/music(backup)"}}, "/music[test]/track.mp3", 1), + Entry("library path with a trailing slash", model.Libraries{{ID: 1, Path: "/music/"}}, "/music/track.mp3", 1), + Entry("library at the filesystem root", model.Libraries{{ID: 1, Path: "/"}}, "/music/track.mp3", 1), + Entry("nested library under a root library", model.Libraries{{ID: 1, Path: "/"}, {ID: 2, Path: "/music"}}, "/music/track.mp3", 2), + ) + + It("does not match a path outside every library", func() { + Expect(find(model.Libraries{{ID: 1, Path: "/music"}}, "/music-backup/track.mp3")).To(BeZero()) + }) + + It("does not match anything without libraries", func() { + Expect(find(nil, "/music/track.mp3")).To(BeZero()) + }) + + It("does not reorder the caller's libraries", func() { + libs := model.Libraries{{ID: 1, Path: "/a"}, {ID: 2, Path: "/abc"}} + model.NewLibraryMatcher(libs) + Expect(libs.IDs()).To(Equal([]int{1, 2})) + }) +}) + +var _ = Describe("LibraryRelativePath", func() { + // Paths are built with filepath so the "absolute" cases stay absolute on every OS + // (a Unix-style "/foo" is not absolute on Windows). + libRoot, _ := filepath.Abs(filepath.Join("jukebox", "collection")) + outside, _ := filepath.Abs(filepath.Join("somewhere", "else")) + + It("returns a relative path unchanged", func() { + Expect(model.LibraryRelativePath(libRoot, "_Collection")).To(Equal("_Collection")) + }) + + It("rebases an absolute target when the library root is relative", func() { + cwd, err := os.Getwd() + Expect(err).ToNot(HaveOccurred()) + Expect(model.LibraryRelativePath(filepath.Join("music", "library"), filepath.Join(cwd, "music", "library", "rock"))).To(Equal("rock")) + }) + + It("rebases an absolute path that equals the library root to '.'", func() { + Expect(model.LibraryRelativePath(libRoot, libRoot)).To(Equal(".")) + }) + + It("rebases an absolute path under the library root", func() { + Expect(model.LibraryRelativePath(libRoot, filepath.Join(libRoot, "_Collection"))).To(Equal("_Collection")) + }) + + It("handles a trailing slash on the library path", func() { + Expect(model.LibraryRelativePath(libRoot+string(filepath.Separator), filepath.Join(libRoot, "_Collection"))).To(Equal("_Collection")) + }) + + It("leaves an absolute path outside the library root unchanged", func() { + Expect(model.LibraryRelativePath(libRoot, outside)).To(Equal(outside)) + }) + + It("returns an empty path unchanged", func() { + Expect(model.LibraryRelativePath(libRoot, "")).To(Equal("")) + }) +}) diff --git a/model/library_test.go b/model/library_test.go new file mode 100644 index 000000000..4e799e13f --- /dev/null +++ b/model/library_test.go @@ -0,0 +1,73 @@ +package model_test + +import ( + "encoding/json" + "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("Library PID config", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.PID.Album = "global_album" + conf.Server.PID.Track = "global_track" + }) + + Describe("EffectivePID", func() { + It("falls back to the global config", func() { + Expect(model.Library{}.EffectivePID()).To(Equal(model.PIDConfig{Track: "global_track", Album: "global_album"})) + }) + It("uses the library overrides", func() { + lib := model.Library{PIDAlbum: "folder", PIDTrack: "title"} + Expect(lib.EffectivePID()).To(Equal(model.PIDConfig{Track: "title", Album: "folder"})) + }) + }) + + Describe("PIDChanged", func() { + It("is false when the scanned specs match, ignoring case", func() { + lib := model.Library{ScannedPIDAlbum: "GLOBAL_ALBUM", ScannedPIDTrack: "global_track"} + Expect(lib.PIDChanged()).To(BeFalse()) + }) + It("is true when the album override differs from the scanned spec", func() { + lib := model.Library{PIDAlbum: "folder", ScannedPIDAlbum: "global_album", ScannedPIDTrack: "global_track"} + Expect(lib.PIDChanged()).To(BeTrue()) + }) + It("is true when only the track spec changed", func() { + lib := model.Library{PIDTrack: "title", ScannedPIDAlbum: "global_album", ScannedPIDTrack: "global_track"} + Expect(lib.PIDChanged()).To(BeTrue()) + }) + It("is true when the global config changed for a library without overrides", func() { + lib := model.Library{ScannedPIDAlbum: "old_album", ScannedPIDTrack: "global_track"} + Expect(lib.PIDChanged()).To(BeTrue()) + }) + It("is true for a library that was never scanned", func() { + Expect(model.Library{}.PIDChanged()).To(BeTrue()) + }) + }) + + Describe("NeedsPIDRescan", func() { + It("is false for a library that never finished a scan", func() { + Expect(model.Library{PIDAlbum: "folder"}.NeedsPIDRescan()).To(BeFalse()) + }) + It("is true for a scanned library whose PID config changed", func() { + lib := model.Library{PIDAlbum: "folder", ScannedPIDAlbum: "global_album", ScannedPIDTrack: "global_track", LastScanAt: time.Now()} + Expect(lib.NeedsPIDRescan()).To(BeTrue()) + }) + It("is false for a scanned library whose PID config did not change", func() { + lib := model.Library{ScannedPIDAlbum: "global_album", ScannedPIDTrack: "global_track", LastScanAt: time.Now()} + Expect(lib.NeedsPIDRescan()).To(BeFalse()) + }) + }) + + It("does not expose the scanned specs in JSON", func() { + data, err := json.Marshal(model.Library{PIDAlbum: "folder", ScannedPIDAlbum: "secret_album", ScannedPIDTrack: "secret_track"}) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(ContainSubstring(`"pidAlbum":"folder"`)) + Expect(string(data)).ToNot(ContainSubstring("secret_")) + }) +}) diff --git a/model/metadata/map_mediafile.go b/model/metadata/map_mediafile.go index 6d12feba9..2135824d2 100644 --- a/model/metadata/map_mediafile.go +++ b/model/metadata/map_mediafile.go @@ -8,15 +8,14 @@ import ( "math" "strconv" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/str" ) -func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile { +func (md Metadata) ToMediaFile(lib model.Library, folderID string) model.MediaFile { mf := model.MediaFile{ - LibraryID: libID, + LibraryID: lib.ID, FolderID: folderID, Tags: maps.Clone(md.tags), } @@ -84,8 +83,9 @@ func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile { mf.AlbumArtist = md.mapDisplayAlbumArtist(mf) // Persistent IDs - mf.PID = md.trackPID(mf) - mf.AlbumID = md.albumID(mf, conf.Server.PID.Album) + pid := lib.EffectivePID() + mf.PID = md.trackPID(mf, pid) + mf.AlbumID = md.albumID(mf, pid.Album) // BFR These IDs will go away once the UI handle multiple participants. // BFR For Legacy Subsonic compatibility, we will set them in the API handlers diff --git a/model/metadata/map_mediafile_test.go b/model/metadata/map_mediafile_test.go index baaf8fab5..c19398841 100644 --- a/model/metadata/map_mediafile_test.go +++ b/model/metadata/map_mediafile_test.go @@ -30,9 +30,23 @@ var _ = Describe("ToMediaFile", func() { var toMediaFile = func(tags model.RawTags) model.MediaFile { props.Tags = tags md = metadata.New("filepath", props) - return md.ToMediaFile(1, "folderID") + return md.ToMediaFile(model.Library{ID: 1}, "folderID") } + Describe("Persistent IDs", func() { + It("uses the library PID config for the album ID and for albumid in the track spec", func() { + props.Tags = model.RawTags{"ALBUM": {"Kind of Blue"}, "TITLE": {"So What"}} + md = metadata.New("Jazz/Loose/01.mp3", props) + + byTags := md.ToMediaFile(model.Library{ID: 1, PIDAlbum: "album", PIDTrack: "albumid,title"}, "folderID") + byFolder := md.ToMediaFile(model.Library{ID: 1, PIDAlbum: "folder", PIDTrack: "albumid,title"}, "folderID") + + Expect(byFolder.AlbumID).ToNot(Equal(byTags.AlbumID)) + Expect(byFolder.AlbumID).To(Equal(md.AlbumID(byFolder, "folder"))) + Expect(byFolder.PID).ToNot(Equal(byTags.PID)) + }) + }) + Describe("Dates", func() { It("should parse properly tagged dates ", func() { mf = toMediaFile(model.RawTags{ diff --git a/model/metadata/map_participants_test.go b/model/metadata/map_participants_test.go index ec66e12b9..db652fb8b 100644 --- a/model/metadata/map_participants_test.go +++ b/model/metadata/map_participants_test.go @@ -38,7 +38,7 @@ var _ = Describe("Participants", func() { var toMediaFile = func(tags model.RawTags) model.MediaFile { props.Tags = tags md = metadata.New("filepath", props) - return md.ToMediaFile(1, "folderID") + return md.ToMediaFile(model.Library{ID: 1}, "folderID") } Describe("ARTIST(S) tags", func() { diff --git a/model/metadata/metadata_test.go b/model/metadata/metadata_test.go index c84d93981..a1a675006 100644 --- a/model/metadata/metadata_test.go +++ b/model/metadata/metadata_test.go @@ -323,7 +323,7 @@ var _ = Describe("Metadata", func() { tag: {tagValue}, } md = metadata.New(filePath, props) - return md.ToMediaFile(0, "0") + return md.ToMediaFile(model.Library{}, "0") } DescribeTable("Gain", diff --git a/model/metadata/persistent_ids.go b/model/metadata/persistent_ids.go index db315dc6b..b66ce824a 100644 --- a/model/metadata/persistent_ids.go +++ b/model/metadata/persistent_ids.go @@ -2,11 +2,11 @@ package metadata import ( "cmp" + "errors" "fmt" "path/filepath" "strings" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -22,12 +22,13 @@ type hashFunc = func(...string) string // attributes. Attributes can be either tags or processed values like folder, // albumid, albumartistid, etc. For each field, it gets all its attribute values // and concatenates them, then hashes the result. If a field is empty, it is -// skipped and the function looks for the next field. +// skipped and the function looks for the next field. albumSpec is the album PID +// spec used to resolve the `albumid` attribute. // // Taking hash as a parameter (instead of closing over it in a factory) keeps // mf on the stack: closing over mf would force the whole ~1KB MediaFile to the // heap on every call. -func computePID(mf model.MediaFile, md Metadata, spec string, prependLibId bool, hash hashFunc) string { +func computePID(mf model.MediaFile, md Metadata, spec, albumSpec string, prependLibId bool, hash hashFunc) string { switch spec { case "track_legacy": return legacyTrackID(mf, prependLibId) @@ -41,7 +42,7 @@ func computePID(mf model.MediaFile, md Metadata, spec string, prependLibId bool, values := make([]string, len(attributes)) hasValue := false for i, attr := range attributes { - v := getPIDAttr(mf, md, attr, prependLibId, spec, hash) + v := getPIDAttr(mf, md, attr, prependLibId, spec, albumSpec, hash) if v != "" { hasValue = true } @@ -58,15 +59,15 @@ func computePID(mf model.MediaFile, md Metadata, spec string, prependLibId bool, return hash(pid) } -func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, spec string, hash hashFunc) string { +func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, spec, albumSpec string, hash hashFunc) string { attr = strings.TrimSpace(strings.ToLower(attr)) switch attr { case "albumid": - if spec == conf.Server.PID.Album { + if spec == albumSpec { log.Error("Recursive PID definition detected, ignoring `albumid`", "spec", spec) return "" } - return computePID(mf, md, conf.Server.PID.Album, prependLibId, hash) + return computePID(mf, md, albumSpec, albumSpec, prependLibId, hash) case "folder": return filepath.Dir(mf.Path) case "albumartistid": @@ -79,18 +80,50 @@ func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, return md.String(model.TagName(attr)) } -func (md Metadata) trackPID(mf model.MediaFile) string { - return computePID(mf, md, conf.Server.PID.Track, true, id.NewHash) +// ValidatePIDSpec checks a PID override before it is stored; empty means "use the global config". +// Aliases resolve to empty at scan time: accepted only in track specs, because the default one uses them. +func ValidatePIDSpec(spec string, isAlbum bool) error { + switch { + case spec == "", isAlbum && spec == "album_legacy", !isAlbum && spec == "track_legacy": + return nil + } + for field := range strings.SplitSeq(spec, "|") { + for attr := range strings.SplitSeq(field, ",") { + attr = strings.TrimSpace(strings.ToLower(attr)) + switch attr { + case "": + return fmt.Errorf("empty attribute in %q", spec) + case "albumid": + if isAlbum { + return errors.New("albumid cannot be used in an album PID") + } + case "folder", "albumartistid": + default: + name, ok := model.CanonicalTagName(attr) + if !ok { + return fmt.Errorf("unknown attribute %q", attr) + } + if isAlbum && string(name) != attr { + return fmt.Errorf("use the tag name %q instead of its alias %q", name, attr) + } + } + } + } + return nil +} + +func (md Metadata) trackPID(mf model.MediaFile, pid model.PIDConfig) string { + return computePID(mf, md, pid.Track, pid.Album, true, id.NewHash) } func (md Metadata) albumID(mf model.MediaFile, pidConf string) string { - return computePID(mf, md, pidConf, true, id.NewHash) + return computePID(mf, md, pidConf, pidConf, true, id.NewHash) } // BFR Must be configurable? func (md Metadata) artistID(name string) string { mf := model.MediaFile{AlbumArtist: name} - return computePID(mf, md, "albumartistid", false, id.NewHash) + return computePID(mf, md, "albumartistid", "", false, id.NewHash) } func (md Metadata) mapTrackTitle() string { diff --git a/model/metadata/persistent_ids_test.go b/model/metadata/persistent_ids_test.go index 8e38bbd42..9f6eaf1f4 100644 --- a/model/metadata/persistent_ids_test.go +++ b/model/metadata/persistent_ids_test.go @@ -3,8 +3,7 @@ package metadata import ( "strings" - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" @@ -13,16 +12,18 @@ import ( var _ = Describe("getPID", func() { var ( - md Metadata - mf model.MediaFile - sum hashFunc + md Metadata + mf model.MediaFile + sum hashFunc + albumSpec string ) getPID := func(mf model.MediaFile, md Metadata, spec string, prependLibId bool) string { - return computePID(mf, md, spec, prependLibId, sum) + return computePID(mf, md, spec, albumSpec, prependLibId, sum) } BeforeEach(func() { sum = func(s ...string) string { return "(" + strings.Join(s, ",") + ")" } + albumSpec = consts.DefaultAlbumPID }) Context("attributes are tags", func() { @@ -66,8 +67,7 @@ var _ = Describe("getPID", func() { Context("calculated attributes", func() { BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.PID.Album = "musicbrainz_albumid|albumartistid,album,albumversion,releasedate" + albumSpec = "musicbrainz_albumid|albumartistid,album,albumversion,releasedate" }) When("field is title", func() { It("should return the pid", func() { @@ -121,8 +121,8 @@ var _ = Describe("getPID", func() { When("albumid configuration refers to albumid recursively", func() { It("should avoid infinite recursion", func() { // Reproduce the issue from #4920 - conf.Server.PID.Album = "albumid,album,albumversion,releasedate" - spec := conf.Server.PID.Album + albumSpec = "albumid,album,albumversion,releasedate" + spec := albumSpec md.tags = map[model.TagName][]string{ "album": {"Album Name"}, "albumversion": {"Version"}, @@ -205,8 +205,7 @@ var _ = Describe("getPID", func() { }) When("prependLibId is true with nested albumid", func() { It("should handle nested albumid calls correctly", func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.PID.Album = "album" + albumSpec = "album" spec := "albumid" md.tags = map[model.TagName][]string{"album": {"Test Album"}} mf.AlbumArtist = "Test Artist" @@ -306,3 +305,34 @@ var _ = Describe("getPID", func() { }) }) }) + +var _ = Describe("ValidatePIDSpec", func() { + DescribeTable("accepts valid specs", + func(spec string, isAlbum bool) { + Expect(ValidatePIDSpec(spec, isAlbum)).To(Succeed()) + }, + Entry("empty, meaning the global config", "", true), + Entry("default album spec", consts.DefaultAlbumPID, true), + Entry("default track spec, which uses tag aliases", consts.DefaultTrackPID, false), + Entry("folder", "folder", true), + Entry("album legacy", "album_legacy", true), + Entry("track legacy", "track_legacy", false), + Entry("computed attributes", "albumartistid,album|title", true), + Entry("albumid in a track spec", "albumid,title", false), + Entry("spaces and mixed case", "MusicBrainz_AlbumID | Folder", true), + ) + + DescribeTable("rejects invalid specs", + func(spec string, isAlbum bool, msg string) { + Expect(ValidatePIDSpec(spec, isAlbum)).To(MatchError(ContainSubstring(msg))) + }, + Entry("unknown tag", "albmversion", true, `unknown attribute "albmversion"`), + Entry("empty field", "album||title", true, "empty attribute"), + Entry("empty attribute", "album,,title", true, "empty attribute"), + Entry("trailing separator", "album|", true, "empty attribute"), + Entry("albumid in an album spec", "albumid,album", true, "albumid"), + Entry("tag alias in an album spec", "talb", true, `use the tag name "album" instead of its alias "talb"`), + Entry("track legacy in an album spec", "track_legacy", true, `unknown attribute "track_legacy"`), + Entry("album legacy in a track spec", "album_legacy", false, `unknown attribute "album_legacy"`), + ) +}) diff --git a/model/player.go b/model/player.go index 2e4484a10..c03058419 100644 --- a/model/player.go +++ b/model/player.go @@ -21,6 +21,8 @@ type Player struct { MaxBitRate int `structs:"max_bit_rate" json:"maxBitRate"` ReportRealPath bool `structs:"report_real_path" json:"reportRealPath"` ScrobbleEnabled bool `structs:"scrobble_enabled" json:"scrobbleEnabled"` + HasAPIKey bool `structs:"-" db:"has_api_key" json:"hasApiKey"` + APIKey *string `structs:"-" json:"apiKey,omitempty"` } type Players []Player @@ -33,4 +35,6 @@ type PlayerRepository interface { Put(ctx context.Context, p *Player) error CountAll(ctx context.Context, options ...QueryOptions) (int64, error) CountByClient(ctx context.Context, options ...QueryOptions) (map[string]int64, error) + FindByAPIKey(ctx context.Context, key string) (*Player, error) + SetAPIKey(ctx context.Context, playerID, key string) error } diff --git a/model/scanner.go b/model/scanner.go index 36c9007fb..d22c3d0d6 100644 --- a/model/scanner.go +++ b/model/scanner.go @@ -2,12 +2,15 @@ package model import ( "context" + "errors" "fmt" "strconv" "strings" "time" ) +var ErrAlreadyScanning = errors.New("already scanning") + // ScanTarget represents a specific folder within a library to be scanned. // NOTE: This struct is used as a map key, so it should only contain comparable types. type ScanTarget struct { diff --git a/model/tag_mappings.go b/model/tag_mappings.go index ce7d2f37b..5a8168754 100644 --- a/model/tag_mappings.go +++ b/model/tag_mappings.go @@ -195,6 +195,28 @@ func TagMappings() map[TagName]TagConf { return mappings } +// CanonicalTagName returns the mapped tag that name is, or is an alias of. Tags are stored under this name. +func CanonicalTagName(name string) (TagName, bool) { + tagName, ok := tagNameIndex()[TagName(name).ToLower()] + return tagName, ok +} + +// tagNameIndex maps every tag name and alias to its tag name. Names are added last, so they win over aliases +// (musicbrainz_trackid is a tag and also an alias of musicbrainz_recordingid). +var tagNameIndex = sync.OnceValue(func() map[TagName]TagName { + mappings := TagMappings() + index := make(map[TagName]TagName, len(mappings)) + for name, tag := range mappings { + for _, alias := range tag.Aliases { + index[TagName(alias)] = name + } + } + for name := range mappings { + index[name] = name + } + return index +}) + func TagRolesConf() TagConf { _, cfg := parseMappings() return cfg.Roles diff --git a/model/tag_mappings_test.go b/model/tag_mappings_test.go index e582c3f2f..91e54e5d4 100644 --- a/model/tag_mappings_test.go +++ b/model/tag_mappings_test.go @@ -192,3 +192,22 @@ var _ = Describe("TagConf", func() { }) }) }) + +var _ = Describe("CanonicalTagName", func() { + DescribeTable("resolves tag names and aliases", + func(name string, expected TagName) { + tagName, ok := CanonicalTagName(name) + Expect(ok).To(BeTrue()) + Expect(tagName).To(Equal(expected)) + }, + Entry("tag name", "album", TagAlbum), + Entry("alias", "talb", TagAlbum), + Entry("mixed case alias", "TALB", TagAlbum), + Entry("tag name that is also an alias of another tag", "musicbrainz_trackid", TagMusicBrainzTrackID), + ) + + It("does not resolve an unknown name", func() { + _, ok := CanonicalTagName("nosuchtag") + Expect(ok).To(BeFalse()) + }) +}) diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 8e7662b8f..6a940a320 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -131,6 +131,7 @@ var albumFilters = sync.OnceValue(func() map[string]filterFunc { "recently_played": recentlyPlayedFilter, "starred": annotationBoolFilter("starred"), "has_rating": annotationBoolFilter("rating"), + "played": annotationBoolFilter("play_count"), "missing": booleanFilter, "genre_id": genreFilter(AlbumGenres), "role_total_id": allRolesFilter, diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 7652e703d..0235805c0 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -417,6 +417,80 @@ var _ = Describe("AlbumRepository", func() { } }) }) + + Describe("played", func() { + var playedAlbum model.Album + + BeforeEach(func() { + playedAlbum = model.Album{ID: "played-album", Name: "Played Album", LibraryID: 1, SongCount: 1} + Expect(albumRepo.Put(ctx, &playedAlbum)).To(Succeed()) + Expect(albumRepo.IncPlayCount(ctx, playedAlbum.ID, time.Now())).To(Succeed()) + }) + + AfterEach(func() { + _, _ = albumRepo.executeSQL(ctx, squirrel.Delete("annotation").Where(squirrel.Eq{"item_id": playedAlbum.ID})) + _, _ = albumRepo.executeSQL(ctx, squirrel.Delete("album").Where(squirrel.Eq{"id": playedAlbum.ID})) + }) + + It("false includes items without annotations", func() { + res, err := albumRepo.ReadAll(ctx, rest.QueryOptions{ + Filters: map[string]any{"played": "false"}, + }) + Expect(err).ToNot(HaveOccurred()) + albums := res + + var found bool + for _, a := range albums { + if a.ID == albumWithoutAnnotation.ID { + found = true + break + } + } + Expect(found).To(BeTrue(), "Album without annotation should be included in played=false filter") + }) + + It("true excludes items without annotations", func() { + res, err := albumRepo.ReadAll(ctx, rest.QueryOptions{ + Filters: map[string]any{"played": "true"}, + }) + Expect(err).ToNot(HaveOccurred()) + albums := res + + for _, a := range albums { + Expect(a.ID).ToNot(Equal(albumWithoutAnnotation.ID)) + } + }) + + It("true includes items with play count", func() { + res, err := albumRepo.ReadAll(ctx, rest.QueryOptions{ + Filters: map[string]any{"played": "true"}, + }) + Expect(err).ToNot(HaveOccurred()) + albums := res + + var found bool + for _, a := range albums { + if a.ID == playedAlbum.ID { + found = true + Expect(a.PlayCount).To(BeNumerically(">", 0)) + break + } + } + Expect(found).To(BeTrue(), "Album with play count should be included in played=true filter") + }) + + It("false excludes items with play count", func() { + res, err := albumRepo.ReadAll(ctx, rest.QueryOptions{ + Filters: map[string]any{"played": "false"}, + }) + Expect(err).ToNot(HaveOccurred()) + albums := res + + for _, a := range albums { + Expect(a.ID).ToNot(Equal(playedAlbum.ID)) + } + }) + }) }) Describe("Album.PlayCount", func() { diff --git a/persistence/library_repository.go b/persistence/library_repository.go index bf6b8995e..85da65cbc 100644 --- a/persistence/library_repository.go +++ b/persistence/library_repository.go @@ -93,6 +93,8 @@ func (r *libraryRepository) Put(ctx context.Context, l *model.Library, colsToUpd "path": l.Path, "remote_path": l.RemotePath, "default_new_users": l.DefaultNewUsers, + "pid_album": l.PIDAlbum, + "pid_track": l.PIDTrack, }, colsToUpdate...) cols["updated_at"] = l.UpdatedAt sq := Update(r.tableName).SetMap(cols).Where(Eq{"id": l.ID}) @@ -176,6 +178,15 @@ func (r *libraryRepository) ScanEnd(ctx context.Context, id int) error { return err } +func (r *libraryRepository) SetScannedPID(ctx context.Context, id int, pid model.PIDConfig) error { + sq := Update(r.tableName). + Set("scanned_pid_album", pid.Album). + Set("scanned_pid_track", pid.Track). + Where(Eq{"id": id}) + _, err := r.executeSQL(ctx, sq) + return err +} + func (r *libraryRepository) ScanInProgress(ctx context.Context) (bool, error) { query := r.newSelect(ctx).Where(NotEq{"last_scan_started_at": time.Time{}}) count, err := r.count(ctx, query) diff --git a/persistence/library_repository_test.go b/persistence/library_repository_test.go index 0ff470861..bf485a06f 100644 --- a/persistence/library_repository_test.go +++ b/persistence/library_repository_test.go @@ -270,6 +270,38 @@ var _ = Describe("LibraryRepository", func() { }) }) + Describe("PID config", func() { + It("stores the overrides, and Put never touches the scanned specs", func() { + lib := &model.Library{Name: "PID Library", Path: "/music/pid", PIDAlbum: "folder", PIDTrack: "title"} + Expect(repo.Put(ctx, lib)).To(Succeed()) + Expect(repo.SetScannedPID(ctx, lib.ID, model.PIDConfig{Album: "folder", Track: "title"})).To(Succeed()) + + // An update coming from the REST API has no scanned specs. It must not clear them + update := &model.Library{ID: lib.ID, Name: "PID Library", Path: "/music/pid", PIDTrack: "title"} + Expect(repo.Put(ctx, update)).To(Succeed()) + + saved, err := repo.Get(ctx, lib.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(saved.PIDAlbum).To(BeEmpty()) + Expect(saved.PIDTrack).To(Equal("title")) + Expect(saved.ScannedPIDAlbum).To(Equal("folder")) + Expect(saved.ScannedPIDTrack).To(Equal("title")) + }) + + It("keeps the overrides when a partial update does not send them", func() { + lib := &model.Library{Name: "Partial", Path: "/music/partial", PIDAlbum: "folder", PIDTrack: "title"} + Expect(repo.Put(ctx, lib)).To(Succeed()) + + Expect(repo.Put(ctx, &model.Library{ID: lib.ID, Name: "Renamed"}, "name")).To(Succeed()) + + saved, err := repo.Get(ctx, lib.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(saved.Name).To(Equal("Renamed")) + Expect(saved.PIDAlbum).To(Equal("folder")) + Expect(saved.PIDTrack).To(Equal("title")) + }) + }) + Describe("Delete", func() { var adminRepo model.LibraryRepository var artistRepo model.ArtistRepository diff --git a/persistence/player_repository.go b/persistence/player_repository.go index e46a8d82d..ba5d26794 100644 --- a/persistence/player_repository.go +++ b/persistence/player_repository.go @@ -2,10 +2,16 @@ package persistence import ( "context" + "crypto/sha256" + "encoding/hex" + "regexp" + "strings" . "github.com/Masterminds/squirrel" "github.com/deluan/rest" + "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/id" "github.com/pocketbase/dbx" ) @@ -17,7 +23,8 @@ func NewPlayerRepository(db dbx.Builder) model.PlayerRepository { r := &playerRepository{} r.db = db r.registerModel(&model.Player{}, map[string]filterFunc{ - "name": containsFilter("player.name"), + "name": containsFilter("player.name"), + "hasapikey": hasAPIKeyFilter, }) r.setSortMappings(map[string]string{ "user_name": "username", //TODO rename all user_name and userName to username @@ -25,6 +32,13 @@ func NewPlayerRepository(db dbx.Builder) model.PlayerRepository { return r } +func hasAPIKeyFilter(_ string, value any) Sqlizer { + if v, _ := value.(string); strings.EqualFold(v, "true") { + return NotEq{"player.api_key_hash": nil} + } + return Eq{"player.api_key_hash": nil} +} + func (r *playerRepository) Put(ctx context.Context, p *model.Player) error { _, err := r.put(ctx, p.ID, p) return err @@ -32,7 +46,7 @@ func (r *playerRepository) Put(ctx context.Context, p *model.Player) error { func (r *playerRepository) selectPlayer(ctx context.Context, options ...model.QueryOptions) SelectBuilder { return r.newSelect(ctx, options...). - Columns("player.*"). + Columns("player.*", "player.api_key_hash is not null as has_api_key"). Join("user ON player.user_id = user.id"). Columns("user.user_name username") } @@ -103,32 +117,115 @@ func (r *playerRepository) ReadAll(ctx context.Context, options ...rest.QueryOpt return res, err } -// isPermitted authorizes creating a new record, based on the owner declared in the request body. -// This is only safe for inserts: there is no stored row yet, and a non-admin may only create a -// player they own. Updates must not use this (the body owner is attacker-controlled); they go -// through updateOwned, which authorizes against the persisted user_id in the WHERE clause. -func (r *playerRepository) isPermitted(ctx context.Context, p *model.Player) bool { - u := loggedUser(ctx) - return u.IsAdmin || p.UserId == u.ID +var apiKeyFormat = regexp.MustCompile(`^` + consts.APIKeyPrefix + `[0-9A-Za-z]{22}$`) + +func apiKeyValidationError(msg string) error { + return &rest.ValidationError{Errors: map[string]string{"apiKey": msg}} +} + +func validateAPIKey(key string) error { + if !apiKeyFormat.MatchString(key) { + return apiKeyValidationError("resources.player.validation.apiKeyFormat") + } + return nil } func (r *playerRepository) Save(ctx context.Context, t *model.Player) (string, error) { - if !r.isPermitted(ctx, t) { + u := loggedUser(ctx) + if t.UserId == "" && u.ID != invalidUserId { + t.UserId = u.ID + } + if t.UserId != u.ID { return "", rest.ErrPermissionDenied } - return r.put(ctx, "", t) // Save only creates; edits go through the owner-scoped Update + // Hand-made players are only reachable through a key, so one is required + if t.APIKey == nil || *t.APIKey == "" { + return "", apiKeyValidationError("ra.validation.required") + } + if err := validateAPIKey(*t.APIKey); err != nil { + return "", err + } + values, err := toSQLArgs(t) + if err != nil { + return "", err + } + // Save only creates, so the key hash goes in the same INSERT and the unique index settles races + values["id"] = id.NewRandom() + values["api_key_hash"] = hashAPIKey(*t.APIKey) + _, err = r.executeSQL(ctx, Insert(r.tableName).SetMap(values)) + if isUniqueViolation(err) { + return "", apiKeyValidationError("ra.validation.unique") + } + if err != nil { + return "", err + } + return values["id"].(string), nil } func (r *playerRepository) Update(ctx context.Context, id string, entity model.Player, cols ...string) error { t := &entity t.ID = id - return r.updateOwned(ctx, id, t, cols...) + if t.APIKey == nil { + return r.updateOwned(ctx, id, t, cols...) + } + // The key and the other columns are two writes; commit both or neither + return r.inTx(func(tx *playerRepository) error { + if err := tx.SetAPIKey(ctx, id, *t.APIKey); err != nil { + return err + } + return tx.updateOwned(ctx, id, t, cols...) + }) +} + +func (r *playerRepository) inTx(block func(tx *playerRepository) error) error { + conn, ok := r.db.(*dbx.DB) + if !ok { + return block(r) // already inside a transaction + } + return conn.Transactional(func(tx *dbx.Tx) error { + return block(NewPlayerRepository(tx).(*playerRepository)) + }) } func (r *playerRepository) Delete(ctx context.Context, ids ...string) error { return r.deleteOwnedAll(ctx, ids...) } +// Keys are long random strings, not user-chosen passwords, so a fast unsalted hash is enough and keeps lookups indexed. +func hashAPIKey(key string) string { + sum := sha256.Sum256([]byte(key)) + return hex.EncodeToString(sum[:]) +} + +func (r *playerRepository) FindByAPIKey(ctx context.Context, key string) (*model.Player, error) { + sel := r.selectPlayer(ctx).Where(Eq{"player.api_key_hash": hashAPIKey(key)}) + var res model.Player + if err := r.queryOne(ctx, sel, &res); err != nil { + return nil, err + } + return &res, nil +} + +// SetAPIKey stores the key's hash, or revokes it when key is empty. Setting is owner-only, even for +// admins, so nobody can mint a login for someone else. +func (r *playerRepository) SetAPIKey(ctx context.Context, playerID, key string) error { + if key == "" { + return r.updateOwnedRow(ctx, playerID, ownerOrAdmin, map[string]any{"api_key_hash": nil}) + } + if err := validateAPIKey(key); err != nil { + return err + } + err := r.updateOwnedRow(ctx, playerID, ownerOnly, map[string]any{"api_key_hash": hashAPIKey(key)}) + if isUniqueViolation(err) { + return apiKeyValidationError("ra.validation.unique") + } + return err +} + +func isUniqueViolation(err error) bool { + return err != nil && strings.Contains(err.Error(), "UNIQUE constraint failed") +} + var _ model.PlayerRepository = (*playerRepository)(nil) var _ rest.Repository[model.Player] = (*playerRepository)(nil) var _ rest.Persistable[model.Player] = (*playerRepository)(nil) diff --git a/persistence/player_repository_test.go b/persistence/player_repository_test.go index f12f3e74e..69afa9556 100644 --- a/persistence/player_repository_test.go +++ b/persistence/player_repository_test.go @@ -2,6 +2,7 @@ package persistence import ( "context" + "errors" "github.com/deluan/rest" "github.com/navidrome/navidrome/log" @@ -12,6 +13,14 @@ import ( "github.com/pocketbase/dbx" ) +const testAPIKey = "nds_0123456789abcdefghijkl" + +func expectAPIKeyError(err error, msg string) { + var verr *rest.ValidationError + ExpectWithOffset(1, errors.As(err, &verr)).To(BeTrue()) + ExpectWithOffset(1, verr.Errors).To(HaveKeyWithValue("apiKey", msg)) +} + var _ = Describe("PlayerRepository", func() { var adminRepo *playerRepository var database *dbx.DB @@ -178,11 +187,12 @@ var _ = Describe("PlayerRepository", func() { clone := player clone.ID = "" clone.IP = "192.168.1.1" + clone.APIKey = new(testAPIKey) id, err := repo.Save(repoCtx, &clone) if clone.UserId == "" { Expect(err).To(HaveOccurred()) - } else if !admin && player.Username == adminPlayer1.Username { + } else if player.UserId != userPlayer.UserId { Expect(err).To(Equal(rest.ErrPermissionDenied)) clone.UserId = "" } else { @@ -202,12 +212,13 @@ var _ = Describe("PlayerRepository", func() { } else { Expect(count).To(Equal(baseCount + 1)) Expect(err).To(BeNil()) + clone.APIKey = nil + clone.HasAPIKey = true Expect(*newItem).To(Equal(clone)) } }, Entry("same user", userPlayer), Entry("other item", otherPlayer), - Entry("fake item", model.Player{}), ) }) @@ -251,6 +262,259 @@ var _ = Describe("PlayerRepository", func() { Entry("regular context", false, model.Players{regularPlayer}, regularPlayer, adminPlayer1), ) + Describe("API keys", func() { + const key = testAPIKey + const otherKey = "nds_ABCDEFGHIJKLMNOPQRSTUV" + var ownerCtx, otherCtx context.Context + + BeforeEach(func() { + ownerCtx = request.WithUser(log.NewContext(GinkgoT().Context()), regularUser) + otherCtx = request.WithUser(log.NewContext(GinkgoT().Context()), thirdUser) + }) + + storedHash := func(id string) string { + var row struct { + Hash string `db:"api_key_hash"` + } + Expect(database.NewQuery("select coalesce(api_key_hash, '') as api_key_hash from player where id = {:id}"). + Bind(dbx.Params{"id": id}).One(&row)).To(Succeed()) + return row.Hash + } + + Describe("SetAPIKey", func() { + It("stores only the hash and finds the player by the key", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + + Expect(storedHash(regularPlayer.ID)).To(Equal(hashAPIKey(key))) + plr, err := adminRepo.FindByAPIKey(ctx, key) + Expect(err).ToNot(HaveOccurred()) + Expect(plr.ID).To(Equal(regularPlayer.ID)) + Expect(plr.HasAPIKey).To(BeTrue()) + Expect(plr.APIKey).To(BeNil()) + }) + + It("replaces the previous key", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, otherKey)).To(Succeed()) + + _, err := adminRepo.FindByAPIKey(ctx, key) + Expect(err).To(MatchError(model.ErrNotFound)) + _, err = adminRepo.FindByAPIKey(ctx, otherKey) + Expect(err).ToNot(HaveOccurred()) + }) + + DescribeTable("rejects malformed keys", + func(bad string) { + err := adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, bad) + expectAPIKeyError(err, "resources.player.validation.apiKeyFormat") + Expect(storedHash(regularPlayer.ID)).To(BeEmpty()) + }, + Entry("no prefix", "0123456789abcdefghijklmn"), + Entry("too short", "nds_short"), + Entry("too long", key+"x"), + Entry("bad chars", "nds_0123456789abcdefghij-!"), + ) + + It("revokes with an empty key, by the owner or an admin", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, "")).To(Succeed()) + Expect(storedHash(regularPlayer.ID)).To(BeEmpty()) + + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + Expect(adminRepo.SetAPIKey(ctx, regularPlayer.ID, "")).To(Succeed()) + Expect(storedHash(regularPlayer.ID)).To(BeEmpty()) + }) + + It("accepts revoking a player that has no key", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, "")).To(Succeed()) + }) + + It("does not let an admin set a key on another user's player", func() { + Expect(adminRepo.SetAPIKey(ctx, regularPlayer.ID, key)).To(MatchError(rest.ErrPermissionDenied)) + Expect(storedHash(regularPlayer.ID)).To(BeEmpty()) + }) + + It("does not let another user set or revoke", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + Expect(adminRepo.SetAPIKey(otherCtx, regularPlayer.ID, otherKey)).To(MatchError(rest.ErrPermissionDenied)) + Expect(adminRepo.SetAPIKey(otherCtx, regularPlayer.ID, "")).To(MatchError(rest.ErrPermissionDenied)) + Expect(storedHash(regularPlayer.ID)).To(Equal(hashAPIKey(key))) + }) + + It("returns not found for a missing player", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, "missing", key)).To(MatchError(rest.ErrNotFound)) + Expect(adminRepo.SetAPIKey(ownerCtx, "missing", "")).To(MatchError(rest.ErrNotFound)) + }) + + It("does not find unknown or empty keys", func() { + _, err := adminRepo.FindByAPIKey(ctx, otherKey) + Expect(err).To(MatchError(model.ErrNotFound)) + _, err = adminRepo.FindByAPIKey(ctx, "") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("drops the key with the player", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + Expect(adminRepo.Delete(ownerCtx, regularPlayer.ID)).To(Succeed()) + _, err := adminRepo.FindByAPIKey(ctx, key) + Expect(err).To(MatchError(model.ErrNotFound)) + }) + }) + + Describe("hasApiKey filter", func() { + BeforeEach(func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + }) + + filtered := func(value string) []string { + res, err := adminRepo.ReadAll(ctx, rest.QueryOptions{Filters: map[string]any{"hasApiKey": value}}) + Expect(err).ToNot(HaveOccurred()) + var ids []string + for _, p := range res { + ids = append(ids, p.ID) + } + return ids + } + + It("lists only players with a key", func() { + Expect(filtered("true")).To(ConsistOf(regularPlayer.ID)) + count, err := adminRepo.Count(ctx, rest.QueryOptions{Filters: map[string]any{"hasApiKey": "true"}}) + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(1))) + }) + + It("lists only players without a key", func() { + Expect(filtered("false")).To(ConsistOf(adminPlayer1.ID, adminPlayer2.ID)) + }) + }) + + Describe("Save (create)", func() { + It("creates the player with the key, owned by the logged-in user", func() { + id, err := adminRepo.Save(ownerCtx, &model.Player{Name: "Manual player", APIKey: new(key)}) + Expect(err).ToNot(HaveOccurred()) + + plr, err := adminRepo.FindByAPIKey(ctx, key) + Expect(err).ToNot(HaveOccurred()) + Expect(plr.ID).To(Equal(id)) + Expect(plr.UserId).To(Equal(regularUser.ID)) + }) + + It("requires a key", func() { + count, _ := adminRepo.CountAll(ctx) + _, err := adminRepo.Save(ownerCtx, &model.Player{Name: "No key"}) + expectAPIKeyError(err, "ra.validation.required") + + _, err = adminRepo.Save(ownerCtx, &model.Player{Name: "Empty key", APIKey: new("")}) + expectAPIKeyError(err, "ra.validation.required") + Expect(adminRepo.CountAll(ctx)).To(Equal(count)) + }) + + It("rejects a malformed key without creating the player", func() { + count, _ := adminRepo.CountAll(ctx) + _, err := adminRepo.Save(ownerCtx, &model.Player{Name: "Bad", APIKey: new("nds_bad")}) + expectAPIKeyError(err, "resources.player.validation.apiKeyFormat") + Expect(adminRepo.CountAll(ctx)).To(Equal(count)) + }) + + It("does not let an admin create a keyed player for another user", func() { + count, _ := adminRepo.CountAll(ctx) + _, err := adminRepo.Save(ctx, &model.Player{Name: "For someone", UserId: regularUser.ID, APIKey: new(key)}) + Expect(err).To(MatchError(rest.ErrPermissionDenied)) + _, err = adminRepo.Save(ctx, &model.Player{Name: "For someone", UserId: regularUser.ID}) + Expect(err).To(MatchError(rest.ErrPermissionDenied)) + Expect(adminRepo.CountAll(ctx)).To(Equal(count)) + }) + + It("rejects a key already used by another player without creating the player", func() { + Expect(adminRepo.SetAPIKey(ctx, adminPlayer1.ID, key)).To(Succeed()) + count, _ := adminRepo.CountAll(ctx) + _, err := adminRepo.Save(ownerCtx, &model.Player{Name: "Duplicate", APIKey: new(key)}) + expectAPIKeyError(err, "ra.validation.unique") + Expect(adminRepo.CountAll(ctx)).To(Equal(count)) + }) + }) + + Describe("Update (edit)", func() { + It("rolls back the key change when the rest of the edit fails", func() { + _, err := database.NewQuery(`create trigger fail_player_rename before update of name on player + when new.name = 'boom' begin select raise(abort, 'boom'); end`).Execute() + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + _, _ = database.NewQuery("drop trigger if exists fail_player_rename").Execute() + }) + + plr := regularPlayer + plr.Name = "boom" + plr.APIKey = new(key) + Expect(adminRepo.Update(ownerCtx, plr.ID, plr, "name", "apiKey")).ToNot(Succeed()) + Expect(storedHash(regularPlayer.ID)).To(BeEmpty()) + }) + + It("keeps the key when apiKey is absent (a normal edit)", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + + plr := regularPlayer + plr.Name = "Renamed" + Expect(adminRepo.Update(ownerCtx, plr.ID, plr, "name", "hasApiKey")).To(Succeed()) + Expect(adminRepo.Update(ownerCtx, plr.ID, plr)).To(Succeed()) + + found, err := adminRepo.FindByAPIKey(ctx, key) + Expect(err).ToNot(HaveOccurred()) + Expect(found.Name).To(Equal("Renamed")) + }) + + It("sets a new key when apiKey has a value", func() { + plr := regularPlayer + plr.APIKey = new(key) + Expect(adminRepo.Update(ownerCtx, plr.ID, plr, "name", "apiKey")).To(Succeed()) + Expect(storedHash(regularPlayer.ID)).To(Equal(hashAPIKey(key))) + }) + + It("revokes the key when apiKey is empty", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + plr := regularPlayer + plr.APIKey = new("") + Expect(adminRepo.Update(ownerCtx, plr.ID, plr, "apiKey")).To(Succeed()) + Expect(storedHash(regularPlayer.ID)).To(BeEmpty()) + }) + + It("lets an admin edit another user's keyed player without touching the key", func() { + Expect(adminRepo.SetAPIKey(ownerCtx, regularPlayer.ID, key)).To(Succeed()) + plr := regularPlayer + plr.MaxBitRate = 192 + Expect(adminRepo.Update(ctx, plr.ID, plr, "maxBitRate", "hasApiKey")).To(Succeed()) + Expect(storedHash(regularPlayer.ID)).To(Equal(hashAPIKey(key))) + }) + + It("refuses an admin setting a key on another user's player and leaves other columns alone", func() { + plr := regularPlayer + plr.Name = "Hijacked" + plr.APIKey = new(key) + Expect(adminRepo.Update(ctx, plr.ID, plr, "name", "apiKey")).To(MatchError(rest.ErrPermissionDenied)) + + got, err := adminRepo.Get(ctx, regularPlayer.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(got.Name).To(Equal(regularPlayer.Name)) + Expect(storedHash(regularPlayer.ID)).To(BeEmpty()) + }) + + It("refuses a key already used by another player and leaves other columns alone", func() { + Expect(adminRepo.SetAPIKey(ctx, adminPlayer1.ID, key)).To(Succeed()) + plr := regularPlayer + plr.Name = "Renamed" + plr.APIKey = new(key) + err := adminRepo.Update(ownerCtx, plr.ID, plr, "name", "apiKey") + expectAPIKeyError(err, "ra.validation.unique") + + got, err := adminRepo.Get(ctx, regularPlayer.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(got.Name).To(Equal(regularPlayer.Name)) + Expect(storedHash(regularPlayer.ID)).To(BeEmpty()) + Expect(storedHash(adminPlayer1.ID)).To(Equal(hashAPIKey(key))) + }) + }) + }) + Describe("Ownership enforcement (cross-tenant write protection)", func() { var regularRepo *playerRepository var regularCtx context.Context @@ -287,6 +551,7 @@ var _ = Describe("PlayerRepository", func() { Name: "HIJACKED", UserId: regularUser.ID, ReportRealPath: true, + APIKey: new(testAPIKey), } id, err := regularRepo.Save(regularCtx, &spoofed) diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index 341aab8af..392446cef 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -234,7 +234,9 @@ func (r *playlistTrackRepository) AddAlbums(ctx context.Context, albumIds []stri } func (r *playlistTrackRepository) AddArtists(ctx context.Context, artistIds []string) (int, error) { - return r.addMediaFileIds(ctx, Eq{"album_artist_id": artistIds}) + // Match by album-artist participation, not the deprecated album_artist_id + // column, which only holds the first album artist. + return r.addMediaFileIds(ctx, ParticipantIDFilter("media_file", artistIds, model.RoleAlbumArtist)) } func (r *playlistTrackRepository) AddDiscs(ctx context.Context, discs []model.DiscID) (int, error) { diff --git a/persistence/playlist_track_repository_test.go b/persistence/playlist_track_repository_test.go index 3c532c405..119c57116 100644 --- a/persistence/playlist_track_repository_test.go +++ b/persistence/playlist_track_repository_test.go @@ -219,6 +219,45 @@ var _ = Describe("PlaylistTrackRepository", func() { }) }) + Describe("AddArtists", func() { + var tracks model.PlaylistTrackRepository + var joint model.MediaFile + + BeforeEach(func() { + mfRepo := NewMediaFileRepository(GetDBXBuilder()) + joint = mf(model.MediaFile{ID: "pls-coartist-track", Title: "Joint Track", ArtistID: artistPunctuation.ID, + Artist: artistPunctuation.Name, AlbumID: "pls-coartist-album", Album: "Joint Album", + AlbumArtistID: artistKraftwerk.ID, AlbumArtist: artistKraftwerk.Name, Path: p("joint/track.mp3")}) + joint.Participants[model.RoleAlbumArtist] = model.ParticipantList{ + {Artist: artistKraftwerk}, + {Artist: artistBeatles}, + } + Expect(mfRepo.Put(ctx, &joint)).To(Succeed()) + DeferCleanup(func() { _ = mfRepo.Delete(ctx, joint.ID) }) + + plsRepo := NewPlaylistRepository(GetDBXBuilder()) + pls := model.Playlist{Name: "Co-album-artist", OwnerID: adminUser.ID, OwnerName: adminUser.UserName} + Expect(plsRepo.Put(ctx, &pls)).To(Succeed()) + DeferCleanup(func() { _ = plsRepo.Delete(ctx, pls.ID) }) + tracks = plsRepo.Tracks(ctx, pls.ID, false) + }) + + It("adds tracks where the artist is the first album artist", func() { + Expect(tracks.AddArtists(ctx, []string{artistKraftwerk.ID})).To(Equal(1)) + Expect(tracks.GetMediaFileIDs(ctx)).To(ConsistOf(joint.ID)) + }) + + It("adds tracks where the artist is not the first album artist", func() { + Expect(tracks.AddArtists(ctx, []string{artistBeatles.ID})).To(Equal(1)) + Expect(tracks.GetMediaFileIDs(ctx)).To(ConsistOf(joint.ID)) + }) + + It("does not add tracks where the artist is only the track artist", func() { + Expect(tracks.AddArtists(ctx, []string{artistPunctuation.ID})).To(Equal(0)) + Expect(tracks.GetMediaFileIDs(ctx)).To(BeEmpty()) + }) + }) + Describe("library access", func() { var otherLib model.Library var restrictedUser model.User diff --git a/persistence/sql_annotations_test.go b/persistence/sql_annotations_test.go index ec28c610a..6761331d9 100644 --- a/persistence/sql_annotations_test.go +++ b/persistence/sql_annotations_test.go @@ -91,6 +91,8 @@ var _ = Describe("Annotation Filters", func() { Entry("starred=false", "starred", "false", "COALESCE(starred, 0) = 0", []any(nil)), Entry("starred=True (case insensitive)", "starred", "True", "COALESCE(starred, 0) > 0", []any(nil)), Entry("rating=true", "rating", "true", "COALESCE(rating, 0) > 0", []any(nil)), + Entry("play_count=true", "play_count", "true", "COALESCE(play_count, 0) > 0", []any(nil)), + Entry("play_count=false", "play_count", "false", "COALESCE(play_count, 0) = 0", []any(nil)), ) It("returns nil if value is not a string", func() { diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index be88156d8..03cc6a01b 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -85,6 +85,22 @@ func (r sqlRepository) addRestriction(ctx context.Context, sql ...Sqlizer) Sqliz return s } +// writeAccess says who may change a row in a table with a user_id column. +type writeAccess int + +const ( + ownerOrAdmin writeAccess = iota // admins may write any row + ownerOnly // even admins may only write their own rows +) + +// ownedRow matches the row rowID only if the logged-in user may write it under access. +func (r sqlRepository) ownedRow(ctx context.Context, rowID string, access writeAccess) Sqlizer { + if access == ownerOnly { + return And{Eq{"id": rowID}, Eq{"user_id": loggedUser(ctx).ID}} + } + return r.addRestriction(ctx, Eq{"id": rowID}) +} + func (r *sqlRepository) registerModel(instance any, filters map[string]filterFunc) { if r.tableName == "" { r.tableName = strings.TrimPrefix(reflect.TypeOf(instance).String(), "*model.") @@ -494,15 +510,12 @@ func (r sqlRepository) updateOwned(ctx context.Context, id string, m any, colsTo } updateValues := filterUpdateValues(values, id, colsToUpdate...) delete(updateValues, "user_id") // ownership is immutable on update - update := Update(r.tableName).Where(r.addRestriction(ctx, Eq{"id": id})).SetMap(updateValues) - count, err := r.executeSQL(ctx, update) - if err != nil { - return err - } - if count == 0 { - return r.classifyOwnedWriteMiss(ctx, id) - } - return nil + return r.updateOwnedRow(ctx, id, ownerOrAdmin, updateValues) +} + +// updateOwnedRow sets values on the row rowID if the logged-in user may write it under access. +func (r sqlRepository) updateOwnedRow(ctx context.Context, rowID string, access writeAccess, values map[string]any) error { + return r.runRowWrite(ctx, rowID, Update(r.tableName).SetMap(values).Where(r.ownedRow(ctx, rowID, access))) } // deleteOwned performs an atomic, ownership-restricted delete of the row identified by id, for @@ -511,12 +524,17 @@ func (r sqlRepository) updateOwned(ctx context.Context, id string, m any, colsTo // does not match and is left untouched. The failure path mirrors updateOwned (see // classifyOwnedWriteMiss), so there is no TOCTOU on the delete. func (r sqlRepository) deleteOwned(ctx context.Context, id string) error { - count, err := r.executeSQL(ctx, Delete(r.tableName).Where(r.addRestriction(ctx, Eq{"id": id}))) + return r.runRowWrite(ctx, id, Delete(r.tableName).Where(r.ownedRow(ctx, id, ownerOrAdmin))) +} + +// runRowWrite executes q, a write already filtered by ownedRow(rowID, …), and classifies a miss. +func (r sqlRepository) runRowWrite(ctx context.Context, rowID string, q Sqlizer) error { + count, err := r.executeSQL(ctx, q) if err != nil { return err } if count == 0 { - return r.classifyOwnedWriteMiss(ctx, id) + return r.classifyOwnedWriteMiss(ctx, rowID) } return nil } diff --git a/persistence/sql_base_repository_test.go b/persistence/sql_base_repository_test.go index 33a8140f8..4b42e7f1d 100644 --- a/persistence/sql_base_repository_test.go +++ b/persistence/sql_base_repository_test.go @@ -19,6 +19,26 @@ var _ = Describe("sqlRepository", func() { r.tableName = "table" }) + Describe("ownedRow", func() { + DescribeTable("matches the row, limited to what the logged-in user may write", + func(user model.User, access writeAccess, expectedSQL string, expectedArgs ...any) { + userCtx := request.WithUser(GinkgoT().Context(), user) + sql, args, err := r.ownedRow(userCtx, "row-1", access).ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(Equal(expectedSQL)) + Expect(args).To(Equal(expectedArgs)) + }, + Entry("admin, ownerOrAdmin: any row", model.User{ID: "admin", IsAdmin: true}, ownerOrAdmin, + "(id = ?)", "row-1"), + Entry("regular, ownerOrAdmin: own rows", model.User{ID: "user"}, ownerOrAdmin, + "(id = ? AND user_id = ?)", "row-1", "user"), + Entry("admin, ownerOnly: own rows", model.User{ID: "admin", IsAdmin: true}, ownerOnly, + "(id = ? AND user_id = ?)", "row-1", "admin"), + Entry("regular, ownerOnly: own rows", model.User{ID: "user"}, ownerOnly, + "(id = ? AND user_id = ?)", "row-1", "user"), + ) + }) + Describe("applyOptions", func() { var sq squirrel.SelectBuilder BeforeEach(func() { diff --git a/resources/i18n/pt-br.json b/resources/i18n/pt-br.json index 6fcde56ca..238919259 100644 --- a/resources/i18n/pt-br.json +++ b/resources/i18n/pt-br.json @@ -182,6 +182,7 @@ }, "player": { "name": "Tocador |||| Tocadores", + "menuName": "Tocadores e chaves de API", "fields": { "name": "Nome", "transcodingId": "Conversão", @@ -190,7 +191,29 @@ "userName": "Usuário", "lastSeen": "Últ. acesso", "reportRealPath": "Use paths reais", - "scrobbleEnabled": "Enviar scrobbles para serviços externos" + "scrobbleEnabled": "Enviar scrobbles para serviços externos", + "hasApiKey": "Chave de API" + }, + "actions": { + "generateApiKey": "Gerar chave de API", + "regenerateApiKey": "Gerar nova", + "revokeApiKey": "Revogar", + "copyApiKey": "Copiar" + }, + "message": { + "apiKeyActive": "Este tocador tem uma chave de API. Use-a no seu app como chave de API, ou como senha se o app não usar autenticação por token.", + "apiKeyNone": "Sem chave de API. Gere uma para conectar um app a este tocador.", + "apiKeyNoneOther": "Sem chave de API.", + "apiKeyPending": "Copie esta chave agora. Ela será salva quando você clicar em Salvar e não será exibida novamente.", + "apiKeyRevokePending": "A chave de API será removida quando você salvar.", + "deleteWithKeyTitle": "Excluir tocador", + "deleteWithKeyContent": "Este tocador tem uma chave de API. Os apps que a usam vão parar de funcionar." + }, + "notifications": { + "apiKeyCopied": "Chave de API copiada para o clipboard" + }, + "validation": { + "apiKeyFormat": "Formato de chave de API inválido" } }, "transcoding": { @@ -305,11 +328,22 @@ "totalDuration": "Duração", "defaultNewUsers": "Padrão para Novos Usuários", "createdAt": "Data de Criação", - "updatedAt": "Últ. Atualização" + "updatedAt": "Últ. Atualização", + "pidAlbum": "Agrupamento de álbuns", + "pidTrack": "Identificação das faixas" }, "sections": { "basic": "Informações Básicas", - "statistics": "Estatísticas" + "statistics": "Estatísticas", + "pid": "IDs Persistentes" + }, + "pid": { + "global": "Usar configuração global (%{value})", + "folder": "Pasta (um álbum por pasta)", + "custom": "Personalizado", + "spec": "Especificação do PID", + "help": "Tags e atributos que identificam um item. Consulte a sintaxe na documentação:", + "docs": "IDs Persistentes" }, "actions": { "scan": "Scanear Biblioteca", @@ -339,7 +373,9 @@ "messages": { "deleteConfirm": "Tem certeza que deseja excluir esta biblioteca? Isso removerá todos os dados associados.", "scanInProgress": "Scan em progresso...", - "noLibrariesAssigned": "Nenhuma biblioteca atribuída a este usuário" + "noLibrariesAssigned": "Nenhuma biblioteca atribuída a este usuário", + "pidChangeTitle": "Alterar os IDs persistentes?", + "pidChangeConfirm": "Ao salvar, os álbuns desta biblioteca serão reagrupados e as faixas serão identificadas novamente. Um scan completo da biblioteca começará imediatamente. As marcações como favoritas, as classificações e as contagens de reprodução das faixas serão mantidas. Os favoritos e as classificações dos álbuns serão transferidos para os novos álbuns quando um álbum antigo corresponder a um novo." } }, "plugin": { diff --git a/scanner/controller.go b/scanner/controller.go index 5eed6c58d..1b13c1846 100644 --- a/scanner/controller.go +++ b/scanner/controller.go @@ -25,7 +25,7 @@ import ( ) var ( - ErrAlreadyScanning = errors.New("already scanning") + ErrAlreadyScanning = model.ErrAlreadyScanning ) func New(rootCtx context.Context, ds model.DataStore, broker events.Broker, @@ -304,14 +304,14 @@ func LockForMaintenance() (func(), bool) { return scanMaintenanceMux.Unlock, true } -// EffectiveFullScan reports whether a scan was requested as full or will resume an interrupted -// full scan in one of the included libraries. +// EffectiveFullScan reports whether a scan was requested as full, will resume an interrupted full scan, +// or will rescan a library in full because its PID config changed, in one of the included libraries. func EffectiveFullScan(ctx context.Context, ds model.DataStore, fullScan bool, targets []model.ScanTarget) bool { if fullScan { return true } return anyIncludedLibrary(ctx, ds, targets, func(library model.Library) bool { - return library.FullScanInProgress + return library.FullScanInProgress || library.NeedsPIDRescan() }) } diff --git a/scanner/controller_test.go b/scanner/controller_test.go index bdcb99eda..974540e32 100644 --- a/scanner/controller_test.go +++ b/scanner/controller_test.go @@ -2,6 +2,7 @@ package scanner_test import ( "context" + "time" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" @@ -70,14 +71,21 @@ var _ = Describe("EffectiveFullScan", func() { var ds *tests.MockDataStore BeforeEach(func() { + pid := model.Library{}.EffectivePID() libraries := &tests.MockLibraryRepo{} libraries.SetData(model.Libraries{ - {ID: 1, FullScanInProgress: true}, - {ID: 2}, + {ID: 1, FullScanInProgress: true, ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, + {ID: 2, ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, + {ID: 3, LastScanAt: time.Now(), PIDAlbum: "folder", ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, }) ds = &tests.MockDataStore{MockedLibrary: libraries} }) + It("detects a library that needs a full rescan for a PID change", func() { + targets := []model.ScanTarget{{LibraryID: 3, FolderPath: "."}} + Expect(scanner.EffectiveFullScan(GinkgoT().Context(), ds, false, targets)).To(BeTrue()) + }) + It("detects an interrupted full scan in a targeted library", func() { targets := []model.ScanTarget{{LibraryID: 1, FolderPath: "."}} Expect(scanner.EffectiveFullScan(context.Background(), ds, false, targets)).To(BeTrue()) diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 7b6a6b097..4edaecacd 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -40,6 +40,7 @@ func createPhaseFolders(ctx context.Context, state *scanState, ds model.DataStor if err != nil { log.Error(ctx, "Scanner: Error creating scan context", "lib", lib.Name, err) state.sendError(err) + state.markFailed(lib.ID) continue } jobs = append(jobs, job) @@ -51,12 +52,13 @@ func createPhaseFolders(ctx context.Context, state *scanState, ds model.DataStor } type scanJob struct { - lib model.Library - fs storage.MusicFS - lastUpdates map[string]model.FolderUpdateInfo // Holds last update info for all (DB) folders in this library - targetFolders []string // Specific folders to scan (including all descendants) - lock sync.Mutex - numFolders atomic.Int64 + lib model.Library + fs storage.MusicFS + lastUpdates map[string]model.FolderUpdateInfo // Holds last update info for all (DB) folders in this library + targetFolders []string // Specific folders to scan (including all descendants) + prevAlbumPIDConf string // Album PID spec of the last finished scan, only when it differs from the current one + lock sync.Mutex + numFolders atomic.Int64 } func newScanJob(ctx context.Context, ds model.DataStore, lib model.Library, fullScan bool, targetFolders []string) (*scanJob, error) { @@ -77,16 +79,32 @@ func newScanJob(ctx context.Context, ds model.DataStore, lib model.Library, full return nil, fmt.Errorf("getting fs for library: %w", err) } + pid := lib.EffectivePID() + if lib.NeedsPIDRescan() { + msg := "Scanner: PID config changed, rescanning library in full" + if len(targetFolders) > 0 { + msg = "Scanner: PID config changed, rescanning target folders in full" + } + log.Info(ctx, msg, "lib", lib.Name, "targetFolders", targetFolders, + "album", pid.Album, "track", pid.Track, "scannedAlbum", lib.ScannedPIDAlbum, "scannedTrack", lib.ScannedPIDTrack) + fullScan = true + } + var prevAlbumPIDConf string + if lib.ScannedPIDAlbum != pid.Album { + prevAlbumPIDConf = lib.ScannedPIDAlbum + } + // Ensure FullScanInProgress reflects the current scan request. // This is important when resuming an interrupted quick scan as a full scan: // the DB may have FullScanInProgress=false, but we need it true for isOutdated() to work correctly. lib.FullScanInProgress = lib.FullScanInProgress || fullScan return &scanJob{ - lib: lib, - fs: fsys, - lastUpdates: lastUpdates, - targetFolders: targetFolders, + lib: lib, + fs: fsys, + lastUpdates: lastUpdates, + targetFolders: targetFolders, + prevAlbumPIDConf: prevAlbumPIDConf, }, nil } @@ -122,14 +140,13 @@ func (j *scanJob) createFolderEntry(path string) *folderEntry { // The phaseFolders struct implements the phase interface, providing methods to produce // folder entries, process folders, persist changes to the database, and log the results. type phaseFolders struct { - jobs []*scanJob - ds model.DataStore - ctx context.Context //nolint:containedctx // phase runs under a single scan ctx - walkCtx context.Context //nolint:containedctx // cancelled when a folder fails to persist, so the walk stops early - stopWalk context.CancelCauseFunc - state *scanState - prevAlbumPIDConf string - imageChanges *imageChangeCollector + jobs []*scanJob + ds model.DataStore + ctx context.Context //nolint:containedctx // phase runs under a single scan ctx + walkCtx context.Context //nolint:containedctx // cancelled when a folder fails to persist, so the walk stops early + stopWalk context.CancelCauseFunc + state *scanState + imageChanges *imageChangeCollector } func (p *phaseFolders) description() string { @@ -138,12 +155,6 @@ func (p *phaseFolders) description() string { func (p *phaseFolders) producer() ppl.Producer[*folderEntry] { return ppl.NewProducer(func(put func(entry *folderEntry)) error { - var err error - p.prevAlbumPIDConf, err = p.ds.Property().DefaultGet(p.ctx, consts.PIDAlbumKey, "") - if err != nil { - return fmt.Errorf("getting album PID conf: %w", err) - } - // TODO Parallelize multiple job when we have multiple libraries var total int64 var totalChanged int64 @@ -173,7 +184,7 @@ func (p *phaseFolders) producer() ppl.Producer[*folderEntry] { // Check if folder is outdated if folder.isOutdated() { - if !p.state.fullScan { + if !folder.job.lib.FullScanInProgress { // Ancestor folders need a row even with no files of their own: artwork // resolution climbs them, and an image added later needs a state to diff. if folder.isEmpty() && folder.isNew() { @@ -239,7 +250,7 @@ func (p *phaseFolders) processFolder(entry *folderEntry) (*folderEntry, error) { for afPath, af := range entry.audioFiles { fullPath := path.Join(entry.path, afPath) dbTrack, foundInDB := dbTracks[fullPath] - if !foundInDB || p.state.fullScan { + if !foundInDB || entry.job.lib.FullScanInProgress { filesToImport[fullPath] = dbTrack } else { info, err := af.Info() @@ -289,18 +300,18 @@ func (p *phaseFolders) loadTagsFromFiles(entry *folderEntry, toImport map[string } for filePath, info := range allInfo { md := metadata.New(filePath, info) - track := md.ToMediaFile(entry.job.lib.ID, entry.id) + track := md.ToMediaFile(entry.job.lib, entry.id) tracks = append(tracks, track) for _, t := range track.Tags.FlattenAll() { uniqueTags[t.ID] = t } // Keep track of any album ID changes, to reassign annotations later - prevAlbumID := "" + prevAlbumID := track.AlbumID if prev := toImport[filePath]; prev != nil { prevAlbumID = prev.AlbumID - } else { - prevAlbumID = md.AlbumID(track, p.prevAlbumPIDConf) + } else if entry.job.prevAlbumPIDConf != "" { + prevAlbumID = md.AlbumID(track, entry.job.prevAlbumPIDConf) } _, ok := entry.albumIDMap[track.AlbumID] if prevAlbumID != track.AlbumID && !ok { @@ -453,7 +464,7 @@ func (p *phaseFolders) persistFolder(ctx context.Context, tx model.DataStore, en if len(queueItems) > 0 { queue := tx.ArtworkQueue() enqueue := queue.Enqueue - if p.state.fullScan { + if entry.job.lib.FullScanInProgress { enqueue = queue.EnqueueIfMissing } if err := enqueue(ctx, queueItems...); err != nil { diff --git a/scanner/scanner.go b/scanner/scanner.go index cd2fe3c8d..305c443f4 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -4,13 +4,11 @@ import ( "context" "fmt" "maps" - "path/filepath" "slices" "sync/atomic" "time" ppl "github.com/google/go-pipeline/pkg/pipeline" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" @@ -32,6 +30,7 @@ type scanState struct { libraries model.Libraries // Store libraries list for consistency across phases targets map[int][]string // Optional: map[libraryID][]folderPaths for selective scans totalLibraryCount int // Total number of libraries (unfiltered), for cross-library move detection + failedLibs map[int]bool // Libraries that could not be scanned in this run } func (s *scanState) sendProgress(info *ProgressInfo) { @@ -48,29 +47,15 @@ func (s *scanState) sendWarning(msg string) { s.sendProgress(&ProgressInfo{Warning: msg}) } -func (s *scanState) sendError(err error) { - s.sendProgress(&ProgressInfo{Error: err.Error()}) +func (s *scanState) markFailed(libID int) { + if s.failedLibs == nil { + s.failedLibs = map[int]bool{} + } + s.failedLibs[libID] = true } -// libraryRelativePath rebases an absolute scan target path onto the library root, since the -// scanner's fs.FS only accepts paths relative to it. Relative paths, and absolute paths outside -// the library root, are returned unchanged. -func libraryRelativePath(libPath, folderPath string) string { - if !filepath.IsAbs(folderPath) { - return folderPath - } - // The library root may be relative (e.g. the default "./music"); it must be made absolute - // to match against an absolute target, and it resolves against the same cwd as the scanner's fs. - absLib, err := filepath.Abs(libPath) - if err != nil { - return folderPath - } - rel, err := filepath.Rel(absLib, folderPath) - if err != nil || !filepath.IsLocal(rel) { - return folderPath - } - // The scanner's fs.FS is an io/fs, which always uses forward slashes. - return filepath.ToSlash(rel) +func (s *scanState) sendError(err error) { + s.sendProgress(&ProgressInfo{Error: err.Error()}) } func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets []model.ScanTarget, progress chan<- *ProgressInfo) { @@ -104,7 +89,7 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] }) for _, target := range targets { - folderPath := libraryRelativePath(libPaths[target.LibraryID], target.FolderPath) + folderPath := model.LibraryRelativePath(libPaths[target.LibraryID], target.FolderPath) if folderPath == "" { folderPath = "." } @@ -137,6 +122,10 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] // if there was a full scan in progress, force a full scan if !state.fullScan { for _, lib := range state.libraries { + // A pending PID rescan already restarts in full through its own job + if lib.NeedsPIDRescan() { + continue + } if lib.FullScanInProgress { log.Info(ctx, "Scanner: Interrupted full scan detected", "lib", lib.Name) state.fullScan = true @@ -215,10 +204,13 @@ func (s *scannerImpl) prepareLibrariesForScan(ctx context.Context, state *scanSt var successfulLibs []model.Library for _, lib := range state.libraries { - if lib.LastScanStartedAt.IsZero() { + // A library with a changed PID config restarts its scan: resuming would skip the folders that + // the interrupted scan already processed with the old config + pidRescan := lib.NeedsPIDRescan() + if lib.LastScanStartedAt.IsZero() || pidRescan { // This is a new scan - mark it as started err := s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { - return tx.Library().ScanBegin(ctx, lib.ID, state.fullScan) + return tx.Library().ScanBegin(ctx, lib.ID, state.fullScan || pidRescan) }, "scanner: begin library scan") if err != nil { log.Error(ctx, "Scanner: Error marking scan start", "lib", lib.Name, err) @@ -340,11 +332,12 @@ func (s *scannerImpl) runUpdateLibraries(ctx context.Context, state *scanState) if err := tx.Library().ScanEnd(ctx, lib.ID); err != nil { return fmt.Errorf("updating last scan completed for %s: %w", lib.Name, err) } - if err := tx.Property().Put(ctx, consts.PIDTrackKey, conf.Server.PID.Track); err != nil { - return fmt.Errorf("updating track PID conf: %w", err) - } - if err := tx.Property().Put(ctx, consts.PIDAlbumKey, conf.Server.PID.Album); err != nil { - return fmt.Errorf("updating album PID conf: %w", err) + // A selective scan covers only part of the library, so the rest may still use the old PID + // config. A library that could not be scanned did not apply it either. + if !state.isSelectiveScan() && !state.failedLibs[lib.ID] { + if err := tx.Library().SetScannedPID(ctx, lib.ID, lib.EffectivePID()); err != nil { + return fmt.Errorf("updating PID conf for %s: %w", lib.Name, err) + } } if state.changesDetected.Load() { log.Debug(ctx, "Scanner: Refreshing library stats", "lib", lib.Name) diff --git a/scanner/scanner_internal_test.go b/scanner/scanner_internal_test.go index 0778bd6ec..e8abb7c7d 100644 --- a/scanner/scanner_internal_test.go +++ b/scanner/scanner_internal_test.go @@ -4,8 +4,6 @@ package scanner import ( "context" "errors" - "os" - "path/filepath" "sync/atomic" ppl "github.com/google/go-pipeline/pkg/pipeline" @@ -13,43 +11,6 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("libraryRelativePath", func() { - // Paths are built with filepath so the "absolute" cases stay absolute on every OS - // (a Unix-style "/foo" is not absolute on Windows). - libRoot, _ := filepath.Abs(filepath.Join("jukebox", "collection")) - outside, _ := filepath.Abs(filepath.Join("somewhere", "else")) - - It("returns a relative path unchanged", func() { - Expect(libraryRelativePath(libRoot, "_Collection")).To(Equal("_Collection")) - }) - - It("rebases an absolute target when the library root is relative", func() { - cwd, err := os.Getwd() - Expect(err).ToNot(HaveOccurred()) - Expect(libraryRelativePath(filepath.Join("music", "library"), filepath.Join(cwd, "music", "library", "rock"))).To(Equal("rock")) - }) - - It("rebases an absolute path that equals the library root to '.'", func() { - Expect(libraryRelativePath(libRoot, libRoot)).To(Equal(".")) - }) - - It("rebases an absolute path under the library root", func() { - Expect(libraryRelativePath(libRoot, filepath.Join(libRoot, "_Collection"))).To(Equal("_Collection")) - }) - - It("handles a trailing slash on the library path", func() { - Expect(libraryRelativePath(libRoot+string(filepath.Separator), filepath.Join(libRoot, "_Collection"))).To(Equal("_Collection")) - }) - - It("leaves an absolute path outside the library root unchanged", func() { - Expect(libraryRelativePath(libRoot, outside)).To(Equal(outside)) - }) - - It("returns an empty path unchanged", func() { - Expect(libraryRelativePath(libRoot, "")).To(Equal("")) - }) -}) - type mockPhase struct { num int produceFunc func() ppl.Producer[int] diff --git a/scanner/scanner_multilibrary_test.go b/scanner/scanner_multilibrary_test.go index 546baf756..f6634c875 100644 --- a/scanner/scanner_multilibrary_test.go +++ b/scanner/scanner_multilibrary_test.go @@ -835,4 +835,170 @@ var _ = Describe("Scanner - Multi-Library", Ordered, func() { Expect(lastError).To(BeEmpty()) }) }) + + Context("Per-library PID config", func() { + albumsOf := func(libID int) model.Albums { + // The mock datastore's GC is a no-op, so run the real one to purge the albums left + // empty by a regroup, as the scanner does in production + Expect(ds.RealDS.GC(ctx)).To(Succeed()) + albums, err := ds.Album().GetAll(ctx, model.QueryOptions{ + Filters: squirrel.Eq{"library_id": libID, "missing": false}, + Sort: "name", + }) + Expect(err).ToNot(HaveOccurred()) + return albums + } + trackByTitle := func(libID int, title string) model.MediaFile { + mfs, err := ds.MediaFile().GetAll(ctx, model.QueryOptions{ + Filters: squirrel.Eq{"library_id": libID, "title": title}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(1)) + return mfs[0] + } + rockTitles := func() []string { + mfs, err := ds.MediaFile().GetAll(ctx, model.QueryOptions{Filters: squirrel.Eq{"library_id": lib1.ID}}) + Expect(err).ToNot(HaveOccurred()) + return slice.Map(mfs, func(mf model.MediaFile) string { return mf.Title }) + } + // changeRockInDB edits the rock track in the DB only. A full rescan of the rock library would + // restore the title from the file tags, a quick scan leaves it alone. + changeRockInDB := func() { + _, err := db.Db().ExecContext(ctx, "update media_file set title = 'Changed In DB' where library_id = ?", lib1.ID) + Expect(err).ToNot(HaveOccurred()) + } + // changeBlueTrainInDB does the same for one jazz track + changeBlueTrainInDB := func() { + _, err := db.Db().ExecContext(ctx, "update media_file set title = 'Blue Train In DB' where library_id = ? and title = 'Blue Train'", lib2.ID) + Expect(err).ToNot(HaveOccurred()) + } + + BeforeEach(func() { + beatles := template(_t{"albumartist": "The Beatles", "album": "Abbey Road", "year": 1969}) + _ = createFS("rock", fstest.MapFS{ + "The Beatles/Abbey Road/01 - Come Together.mp3": beatles(track(1, "Come Together")), + }) + + miles := template(_t{"albumartist": "Miles Davis", "album": "Kind of Blue", "year": 1959}) + coltrane := template(_t{"albumartist": "John Coltrane", "album": "Giant Steps", "year": 1960}) + blueTrain := template(_t{"albumartist": "John Coltrane", "album": "Blue Train", "year": 1957}) + _ = createFS("jazz", fstest.MapFS{ + "Loose/01 - So What.mp3": miles(track(1, "So What")), + "Loose/02 - Giant Steps.mp3": coltrane(track(1, "Giant Steps")), + "Coltrane/Blue Train/01 - Blue Train.mp3": blueTrain(track(1, "Blue Train")), + }) + }) + + It("regroups only the library whose PID config changed, keeping annotations", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + Expect(albumsOf(lib2.ID)).To(HaveLen(3)) + + // Star Blue Train, to check the star follows the album to its new ID + oldBlueTrain := trackByTitle(lib2.ID, "Blue Train") + Expect(ds.Album().SetStar(ctx, true, oldBlueTrain.AlbumID)).To(Succeed()) + changeRockInDB() + + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + Expect(runScanner(ctx, false)).To(Succeed()) + + // Jazz is grouped by folder now: "Loose" is one album + Expect(albumsOf(lib2.ID)).To(HaveLen(2)) + Expect(trackByTitle(lib2.ID, "So What").AlbumID).To(Equal(trackByTitle(lib2.ID, "Giant Steps").AlbumID)) + + newBlueTrain := trackByTitle(lib2.ID, "Blue Train") + Expect(newBlueTrain.AlbumID).ToNot(Equal(oldBlueTrain.AlbumID)) + album, err := ds.Album().Get(ctx, newBlueTrain.AlbumID) + Expect(err).ToNot(HaveOccurred()) + Expect(album.Starred).To(BeTrue()) + + // Rock only got a quick scan + Expect(rockTitles()).To(ConsistOf("Changed In DB")) + + jazz, err := ds.Library().Get(ctx, lib2.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(jazz.ScannedPIDAlbum).To(Equal("folder")) + Expect(jazz.PIDChanged()).To(BeFalse()) + rock, err := ds.Library().Get(ctx, lib1.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(rock.PIDChanged()).To(BeFalse()) + }) + + It("rescans only libraries that follow the global config", func() { + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + Expect(runScanner(ctx, true)).To(Succeed()) + changeRockInDB() + changeBlueTrainInDB() + + conf.Server.PID.Album = "album" + Expect(runScanner(ctx, false)).To(Succeed()) + + // Rock follows the global config, so it was rescanned in full and its title restored + Expect(rockTitles()).To(ConsistOf("Come Together")) + // Jazz has its own override, so it only got a quick scan + trackByTitle(lib2.ID, "Blue Train In DB") + jazz, err := ds.Library().Get(ctx, lib2.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(jazz.ScannedPIDAlbum).To(Equal("folder")) + }) + + It("restarts an interrupted scan when the PID config changed meanwhile", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + + // Simulate a quick scan of jazz that was interrupted after it had processed every folder: + // the folders were updated after the (old) scan start time + _, err := db.Db().ExecContext(ctx, "update library set last_scan_started_at = ?, full_scan_in_progress = false where id = ?", + time.Now().Add(-time.Hour), lib2.ID) + Expect(err).ToNot(HaveOccurred()) + + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + Expect(runScanner(ctx, false)).To(Succeed()) + + // Every folder was revisited with the new config + Expect(albumsOf(lib2.ID)).To(HaveLen(2)) + }) + + It("does not turn an interrupted PID rescan into a full scan of every library", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + changeRockInDB() + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + + // Simulate a PID full scan of jazz that was interrupted + Expect(ds.Library().ScanBegin(ctx, lib2.ID, true)).To(Succeed()) + Expect(runScanner(ctx, false)).To(Succeed()) + + // Rock only got a quick scan, jazz was rescanned with the new config + Expect(rockTitles()).To(ConsistOf("Changed In DB")) + Expect(albumsOf(lib2.ID)).To(HaveLen(2)) + }) + + It("does not record the PID config for a library that could not be scanned", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + broken := model.Library{Name: "Broken", Path: "unregistered:///music", PIDAlbum: "folder"} + Expect(ds.Library().Put(ctx, &broken)).To(Succeed()) + + // The scan reports an error for the broken library, and still finishes the others + _ = runScanner(ctx, false) + + reloaded, err := ds.Library().Get(ctx, broken.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(reloaded.PIDChanged()).To(BeTrue()) + }) + + It("does not record the PID config after a selective scan", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + + _, err := s.ScanFolders(ctx, false, []model.ScanTarget{{LibraryID: lib2.ID, FolderPath: "Loose"}}) + Expect(err).ToNot(HaveOccurred()) + + jazz, err := ds.Library().Get(ctx, lib2.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(jazz.PIDChanged()).To(BeTrue()) + }) + }) }) diff --git a/server/nativeapi/inspect.go b/server/nativeapi/inspect.go index f1e6c4539..61013dd5a 100644 --- a/server/nativeapi/inspect.go +++ b/server/nativeapi/inspect.go @@ -22,7 +22,12 @@ func doInspect(ctx context.Context, ds model.DataStore, id string) (*core.Inspec return nil, model.ErrNotFound } - return core.Inspect(file.AbsolutePath(), file.LibraryID, file.FolderID) + lib, err := ds.Library().Get(ctx, file.LibraryID) + if err != nil { + return nil, err + } + + return core.Inspect(file.AbsolutePath(), *lib, file.FolderID) } func inspect(ds model.DataStore) http.HandlerFunc { diff --git a/server/public/handle_streams.go b/server/public/handle_streams.go index 37ae56c2b..46c7ca210 100644 --- a/server/public/handle_streams.go +++ b/server/public/handle_streams.go @@ -60,9 +60,8 @@ func (pub *Router) handleStream(w http.ResponseWriter, r *http.Request) { return } - stream, err := pub.streamer.NewStream(ctx, mf, streampkg.Request{ - Format: info.format, BitRate: info.bitrate, - }) + streamReq := pub.decider.ResolveRequest(ctx, mf, info.format, info.bitrate, 0) + stream, err := pub.streamer.NewStream(ctx, mf, streamReq) if err != nil { if errors.Is(err, streampkg.ErrTooManyTranscodes) { w.Header().Set("Retry-After", strconv.Itoa(streampkg.RetryAfterSeconds)) diff --git a/server/public/handle_streams_test.go b/server/public/handle_streams_test.go index 4b4a3545b..965bc7e05 100644 --- a/server/public/handle_streams_test.go +++ b/server/public/handle_streams_test.go @@ -116,11 +116,11 @@ var _ = Describe("handleStream", func() { BeforeEach(func() { ctx = GinkgoT().Context() auth.PublicTokenAuth = jwtauth.New("HS256", []byte("test-secret"), nil) - ds = &tests.MockDataStore{} + ds = &tests.MockDataStore{MockedTranscoding: &tests.MockTranscodingRepo{}} shareRepo = &tests.MockShareRepo{} ds.MockedShare = shareRepo streamer = &mockStreamer{} - pub = &Router{ds: ds, streamer: streamer} + pub = &Router{ds: ds, streamer: streamer, decider: stream.NewTranscodeDecider(ds, tests.NewMockFFmpeg(""))} }) makeRequest := func(token string) *httptest.ResponseRecorder { @@ -152,8 +152,17 @@ var _ = Describe("handleStream", func() { makeRequest(token) Expect(streamer.called).To(BeTrue()) - Expect(streamer.req.Format).To(Equal("mp3")) - Expect(streamer.req.BitRate).To(Equal(192)) + }) + + It("resolves the full stream request like the Subsonic endpoint, so transcodes share the cache", func() { + mf := model.MediaFile{ID: "mf-123", Suffix: "flac", BitRate: 1500, SampleRate: 44100, BitDepth: new(24), Channels: 2} + shareOwnedBy(model.User{ID: "owner1", UserName: "owner1", IsAdmin: true}, mf) + + claims := auth.Claims{ID: "mf-123", Format: "opus", BitRate: 128, ShareID: "share123"} + token, _ := auth.CreateExpiringPublicToken(time.Now().Add(time.Hour), claims) + makeRequest(token) + + Expect(streamer.req).To(Equal(stream.Request{Format: "opus", BitRate: 128, SampleRate: 48000, Channels: 2})) }) It("returns 404 when the track is outside the share owner's libraries", func() { diff --git a/server/public/public.go b/server/public/public.go index 142c474bd..8239ef927 100644 --- a/server/public/public.go +++ b/server/public/public.go @@ -21,14 +21,15 @@ type Router struct { http.Handler artwork artwork.Artwork streamer stream.MediaStreamer + decider stream.TranscodeDecider archiver core.Archiver share core.Share assetsHandler http.Handler ds model.DataStore } -func New(ds model.DataStore, artwork artwork.Artwork, streamer stream.MediaStreamer, share core.Share, archiver core.Archiver) *Router { - p := &Router{ds: ds, artwork: artwork, streamer: streamer, share: share, archiver: archiver} +func New(ds model.DataStore, artwork artwork.Artwork, streamer stream.MediaStreamer, decider stream.TranscodeDecider, share core.Share, archiver core.Archiver) *Router { + p := &Router{ds: ds, artwork: artwork, streamer: streamer, decider: decider, share: share, archiver: archiver} shareRoot := path.Join(conf.Server.BasePath, consts.URLPathPublic) p.assetsHandler = http.StripPrefix(shareRoot, http.FileServer(http.FS(ui.BuildAssets()))) p.Handler = p.routes() diff --git a/server/serve_index.go b/server/serve_index.go index 4b093b953..167197403 100644 --- a/server/serve_index.go +++ b/server/serve_index.go @@ -58,6 +58,8 @@ func serveIndex(ds model.DataStore, fs fs.FS, shareInfo *model.Share) http.Handl "uiSearchDebounceMs": conf.Server.UISearchDebounceMs, "uiCoverArtSize": conf.Server.UICoverArtSize, "enableCoverAnimation": conf.Server.EnableCoverAnimation, + "pidAlbum": conf.Server.PID.Album, + "pidTrack": conf.Server.PID.Track, "enableNowPlaying": conf.Server.EnableNowPlaying, "playbackReportIntervalMs": conf.Server.UIPlaybackReportInterval.Milliseconds(), "gaTrackingId": conf.Server.GATrackingID, diff --git a/server/serve_index_test.go b/server/serve_index_test.go index e2df55c4b..277513768 100644 --- a/server/serve_index_test.go +++ b/server/serve_index_test.go @@ -89,6 +89,8 @@ var _ = Describe("serveIndex", func() { Entry("uiSearchDebounceMs", func() { conf.Server.UISearchDebounceMs = 500 }, "uiSearchDebounceMs", float64(500)), Entry("uiCoverArtSize", func() { conf.Server.UICoverArtSize = 300 }, "uiCoverArtSize", float64(300)), Entry("enableCoverAnimation", func() { conf.Server.EnableCoverAnimation = true }, "enableCoverAnimation", true), + Entry("pidAlbum", func() { conf.Server.PID.Album = "folder" }, "pidAlbum", "folder"), + Entry("pidTrack", func() { conf.Server.PID.Track = "title" }, "pidTrack", "title"), Entry("enableNowPlaying", func() { conf.Server.EnableNowPlaying = true }, "enableNowPlaying", true), Entry("gaTrackingId", func() { conf.Server.GATrackingID = "UA-12345" }, "gaTrackingId", "UA-12345"), Entry("defaultDownloadableShare", func() { conf.Server.DefaultDownloadableShare = true }, "defaultDownloadableShare", true), diff --git a/server/subsonic/api.go b/server/subsonic/api.go index deedc46c7..fe724741c 100644 --- a/server/subsonic/api.go +++ b/server/subsonic/api.go @@ -107,6 +107,7 @@ func (api *Router) routes() http.Handler { r.Use(getPlayer(api.players)) h(r, "ping", api.Ping) h(r, "getLicense", api.GetLicense) + h(r, "tokenInfo", api.TokenInfo) }) r.Group(func(r chi.Router) { r.Use(getPlayer(api.players)) diff --git a/server/subsonic/e2e/subsonic_apikey_test.go b/server/subsonic/e2e/subsonic_apikey_test.go new file mode 100644 index 000000000..bb263c15d --- /dev/null +++ b/server/subsonic/e2e/subsonic_apikey_test.go @@ -0,0 +1,54 @@ +package e2e + +import ( + "net/http/httptest" + "net/url" + + "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("API key authentication", func() { + var key string + + BeforeEach(func() { + setupTestDB() + userCtx := request.WithUser(ctx, regularUser) + player := &model.Player{ID: "apikey-player", Name: "Phone", UserId: regularUser.ID, Client: "test-client"} + Expect(ds.Player().Put(userCtx, player)).To(Succeed()) + key = "nds_0123456789abcdefghijkl" + Expect(ds.Player().SetAPIKey(userCtx, player.ID, key)).To(Succeed()) + }) + + doKeyReq := func(endpoint, apiKey string) *responses.Subsonic { + q := url.Values{"apiKey": {apiKey}, "v": {"1.16.1"}, "c": {"test-client"}, "f": {"json"}} + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("GET", "/"+endpoint+"?"+q.Encode(), nil)) + return parseJSONResponse(w) + } + + It("authenticates ping with only the key", func() { + resp := doKeyReq("ping", key) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + }) + + It("reports the key owner in tokenInfo", func() { + resp := doKeyReq("tokenInfo", key) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.TokenInfo).ToNot(BeNil()) + Expect(resp.TokenInfo.Username).To(Equal(regularUser.UserName)) + }) + + It("rejects an unknown key with error 44", func() { + resp := doKeyReq("ping", "nds_unknown") + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + Expect(resp.Error.Code).To(Equal(int32(44))) + }) +}) diff --git a/server/subsonic/e2e/subsonic_artwork_test.go b/server/subsonic/e2e/subsonic_artwork_test.go index 9394c8830..530588759 100644 --- a/server/subsonic/e2e/subsonic_artwork_test.go +++ b/server/subsonic/e2e/subsonic_artwork_test.go @@ -137,7 +137,7 @@ var _ = Describe("Artwork Serving", Ordered, func() { artRouter = buildArtworkRouter(artSvc) router = artRouter // so the shared doReq/doRawReq helpers hit the artwork-wired router - pubRouter = public.New(ds, artSvc, streamerSpy, core.NewShare(ds), noopArchiver{}) + pubRouter = public.New(ds, artSvc, streamerSpy, stream.NewTranscodeDecider(ds, ffm), core.NewShare(ds), noopArchiver{}) }) It("emits a bare optimistic coverArt id before the queue is drained", func() { diff --git a/server/subsonic/middlewares.go b/server/subsonic/middlewares.go index 6617661a9..fdb4af28d 100644 --- a/server/subsonic/middlewares.go +++ b/server/subsonic/middlewares.go @@ -65,14 +65,15 @@ func checkRequiredParameters(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var requiredParameters []string + p := req.Params(r) username, _ := fromInternalOrProxyAuth(r) - if username != "" { + apiKey, _ := p.String("apiKey") + if username != "" || apiKey != "" { requiredParameters = []string{"v", "c"} } else { requiredParameters = []string{"u", "v", "c"} } - p := req.Params(r) for _, param := range requiredParameters { if _, err := p.String(param); err != nil { log.Warn(r, err) @@ -104,10 +105,14 @@ func authenticate(ds model.DataStore) func(next http.Handler) http.Handler { ctx := r.Context() var usr *model.User + var keyPlayer *model.Player var err error + p := req.Params(r) + apiKey, _ := p.String("apiKey") username, isInternalAuth := fromInternalOrProxyAuth(r) - if username != "" { + switch { + case username != "": authType := If(isInternalAuth, "internal", "reverse-proxy") usr, err = ds.User().FindByUsername(ctx, username) if errors.Is(err, context.Canceled) { @@ -119,8 +124,16 @@ func authenticate(ds model.DataStore) func(next http.Handler) http.Handler { } else if err != nil { log.Error(ctx, "API: Error authenticating username", "auth", authType, "username", username, "remoteAddr", r.RemoteAddr, err) } - } else { - p := req.Params(r) + case apiKey != "": + usr, keyPlayer, err = authenticateAPIKey(ctx, ds, limiter, r, apiKey) + if err != nil { + if ctx.Err() == nil { + sendError(w, r, err) + } + return + } + ctx = request.WithUsername(ctx, usr.UserName) + default: username, _ := p.String("u") pass, _ := p.String("p") token, _ := p.String("t") @@ -142,6 +155,9 @@ func authenticate(ds model.DataStore) func(next http.Handler) http.Handler { usr, err = ds.User().FindByUsernameWithPassword(ctx, username) if err == nil { err = validateCredentials(usr, pass, token, salt, jwt) + if errors.Is(err, model.ErrInvalidAuth) && pass != "" && jwt == "" { + keyPlayer, err = playerFromPasswordKey(ctx, ds, usr, pass) + } } invalidLogin := errors.Is(err, model.ErrNotFound) || errors.Is(err, model.ErrInvalidAuth) slot.release(invalidLogin) @@ -162,11 +178,77 @@ func authenticate(ds model.DataStore) func(next http.Handler) http.Handler { } ctx = request.WithUser(ctx, *usr) + if keyPlayer != nil { + ctx = request.WithPlayer(ctx, *keyPlayer) + } next.ServeHTTP(w, r.WithContext(ctx)) }) } } +var apiKeyConflicts = []string{"u", "p", "t", "s", "jwt"} + +func authenticateAPIKey(ctx context.Context, ds model.DataStore, limiter *authLimiter, r *http.Request, key string) (*model.User, *model.Player, error) { + query := r.URL.Query() + for _, param := range apiKeyConflicts { + if query.Has(param) { + log.Warn(ctx, "API: apiKey sent with other credentials", "auth", "apikey", "param", param, "remoteAddr", r.RemoteAddr) + return nil, nil, newError(responses.ErrorMultipleAuthMechanismsProvided) + } + } + + // Per key, so a stale key on one device cannot lock out valid keys sharing the IP + slot, allowed := limiter.acquire(ctx, "apikey\x00"+server.ClientIP(r)+"\x00"+key) + if !allowed { + if err := ctx.Err(); err != nil { + return nil, nil, err + } + log.Warn(ctx, "API: Too many failed API key attempts", "auth", "apikey", "remoteAddr", r.RemoteAddr) + return nil, nil, newError(responses.ErrorInvalidAPIKey) + } + + player, err := ds.Player().FindByAPIKey(ctx, key) + var usr *model.User + if err == nil { + usr, err = ds.User().Get(ctx, player.UserId) + } + slot.release(errors.Is(err, model.ErrNotFound)) + switch { + case errors.Is(err, context.Canceled): + return nil, nil, err + case errors.Is(err, model.ErrNotFound): + log.Warn(ctx, "API: Invalid API key", "auth", "apikey", "remoteAddr", r.RemoteAddr) + return nil, nil, newError(responses.ErrorInvalidAPIKey) + case err != nil: + log.Error(ctx, "API: Error authenticating API key", "auth", "apikey", "remoteAddr", r.RemoteAddr, err) + return nil, nil, newError(responses.ErrorAuthenticationFail) + } + return usr, player, nil +} + +// playerFromPasswordKey lets clients that only have a password field log in with an API key. +// It returns ErrInvalidAuth when pass is not a key of usr, so only real failures skip the limiter count. +func playerFromPasswordKey(ctx context.Context, ds model.DataStore, usr *model.User, pass string) (*model.Player, error) { + key := decodePassword(pass) + if !strings.HasPrefix(key, consts.APIKeyPrefix) { + return nil, model.ErrInvalidAuth + } + plr, err := ds.Player().FindByAPIKey(ctx, key) + if errors.Is(err, model.ErrNotFound) || (err == nil && plr.UserId != usr.ID) { + return nil, model.ErrInvalidAuth + } + return plr, err +} + +func decodePassword(pass string) string { + if strings.HasPrefix(pass, "enc:") { + if dec, err := hex.DecodeString(pass[4:]); err == nil { + return string(dec) + } + } + return pass +} + func adminOnly(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { loggedUser, ok := request.UserFrom(r.Context()) @@ -194,12 +276,7 @@ func validateCredentials(user *model.User, pass, token, salt, jwt string) error claims.Subject == user.UserName && auth.CheckClaims(claims, *user, auth.AudienceSubsonic) == nil case pass != "": - if strings.HasPrefix(pass, "enc:") { - if dec, err := hex.DecodeString(pass[4:]); err == nil { - pass = string(dec) - } - } - valid = pass == user.Password + valid = decodePassword(pass) == user.Password case token != "": t := fmt.Sprintf("%x", md5.Sum([]byte(user.Password+salt))) valid = t == token @@ -217,12 +294,20 @@ func getPlayer(players core.Players) func(next http.Handler) http.Handler { ctx := r.Context() userName, _ := request.UsernameFrom(ctx) client, _ := request.ClientFrom(ctx) - playerId := playerIDFromCookie(r, userName) ip, _, _ := net.SplitHostPort(r.RemoteAddr) userAgent := canonicalUserAgent(r) - player, trc, err := players.Register(ctx, playerId, client, userAgent, ip) + + var player *model.Player + var trc *model.Transcoding + var err error + keyPlayer, boundByKey := request.PlayerFrom(ctx) + if boundByKey { + player, trc, err = players.Touch(ctx, keyPlayer, client, userAgent, ip) + } else { + player, trc, err = players.Register(ctx, playerIDFromCookie(r, userName), client, userAgent, ip) + } if err != nil { - log.Error(ctx, "Could not register player", "username", userName, "client", client, err) + log.Error(ctx, "Could not resolve player", "username", userName, "client", client, err) } else { ctx = request.WithPlayer(ctx, *player) if trc != nil { @@ -230,6 +315,11 @@ func getPlayer(players core.Players) func(next http.Handler) http.Handler { } r = r.WithContext(ctx) + // A key already identifies the player, so the cookie would only add a second, weaker signal + if boundByKey { + next.ServeHTTP(w, r) + return + } cookie := &http.Cookie{ //nolint:gosec // Secure omitted: Navidrome may run over plain HTTP Name: playerIDCookieName(userName), Value: player.ID, diff --git a/server/subsonic/middlewares_test.go b/server/subsonic/middlewares_test.go index 0879ee540..b3ad972b7 100644 --- a/server/subsonic/middlewares_test.go +++ b/server/subsonic/middlewares_test.go @@ -3,6 +3,7 @@ package subsonic import ( "context" "crypto/md5" + "encoding/hex" "errors" "fmt" "net/http" @@ -119,6 +120,14 @@ var _ = Describe("Middlewares", func() { Expect(next.called).To(BeTrue()) }) + It("does not require u when apiKey is present", func() { + r := newGetRequest("apiKey=nds_abc", "v=1.15", "c=test") + cp := checkRequiredParameters(next) + cp.ServeHTTP(w, r) + + Expect(next.called).To(BeTrue()) + }) + It("fails when user is missing", func() { r := newGetRequest("v=1.15", "c=test") cp := checkRequiredParameters(next) @@ -311,6 +320,101 @@ var _ = Describe("Middlewares", func() { }) }) + When("using API key authentication", func() { + var key string + serve := func(params ...string) { + authenticate(ds)(next).ServeHTTP(w, newGetRequest(params...)) + } + + BeforeEach(func() { + usr, err := ds.User().FindByUsername(ctx, "admin") + Expect(err).ToNot(HaveOccurred()) + Expect(ds.Player().Put(ctx, &model.Player{ID: "player-1", Name: "My Phone", UserId: usr.ID, Client: "Symfonium"})).To(Succeed()) + key = "nds_0123456789abcdefghijkl" + Expect(ds.Player().SetAPIKey(ctx, "player-1", key)).To(Succeed()) + }) + + It("authenticates the owner and binds the key's player", func() { + serve("apiKey=" + key) + + Expect(next.called).To(BeTrue()) + user, _ := request.UserFrom(next.req.Context()) + Expect(user.UserName).To(Equal("admin")) + username, _ := request.UsernameFrom(next.req.Context()) + Expect(username).To(Equal("admin")) + player, ok := request.PlayerFrom(next.req.Context()) + Expect(ok).To(BeTrue()) + Expect(player.ID).To(Equal("player-1")) + }) + + It("accepts the key in a POST form body", func() { + r := newPostRequest("", "apiKey="+key) + cp := postFormToQueryParams(authenticate(ds)(next)) + cp.ServeHTTP(w, r) + + Expect(next.called).To(BeTrue()) + player, _ := request.PlayerFrom(next.req.Context()) + Expect(player.ID).To(Equal("player-1")) + }) + + It("rejects an unknown key with error 44", func() { + serve("apiKey=nds_unknown") + + Expect(w.Body.String()).To(ContainSubstring(`code="44"`)) + Expect(next.called).To(BeFalse()) + }) + + DescribeTable("rejects apiKey mixed with other credentials with error 43", + func(extra string) { + serve("apiKey="+key, extra) + + Expect(w.Body.String()).To(ContainSubstring(`code="43"`)) + Expect(next.called).To(BeFalse()) + }, + Entry("u", "u=admin"), + Entry("p", "p=wordpass"), + Entry("t", "t=abc"), + Entry("s", "s=abc"), + Entry("jwt", "jwt=abc"), + Entry("empty u", "u="), + Entry("empty p", "p="), + ) + + Context("key sent as the password", func() { + It("authenticates and binds the key's player", func() { + serve("u=admin", "p="+key) + + Expect(next.called).To(BeTrue()) + player, ok := request.PlayerFrom(next.req.Context()) + Expect(ok).To(BeTrue()) + Expect(player.ID).To(Equal("player-1")) + }) + + It("accepts the hex-encoded form", func() { + serve("u=admin", "p=enc:"+hex.EncodeToString([]byte(key))) + + Expect(next.called).To(BeTrue()) + }) + + It("still accepts a real password that starts with the key prefix", func() { + Expect(ds.User().Put(ctx, &model.User{UserName: "prefixed", NewPassword: "nds_secret"})).To(Succeed()) + serve("u=prefixed", "p=nds_secret") + + Expect(next.called).To(BeTrue()) + _, ok := request.PlayerFrom(next.req.Context()) + Expect(ok).To(BeFalse()) + }) + + It("rejects another user's key with error 40", func() { + Expect(ds.User().Put(ctx, &model.User{UserName: "other", NewPassword: "pw"})).To(Succeed()) + serve("u=other", "p="+key) + + Expect(w.Body.String()).To(ContainSubstring(`code="40"`)) + Expect(next.called).To(BeFalse()) + }) + }) + }) + When("failed attempts reach AuthRequestLimit", func() { var cp http.Handler @@ -376,6 +480,21 @@ var _ = Describe("Middlewares", func() { Expect(next.called).To(BeTrue()) }) + It("does not count server errors when a key is sent as the password", func() { + usr, _ := ds.User().FindByUsername(ctx, "admin") + playerRepo := ds.Player().(*tests.MockPlayerRepo) + Expect(playerRepo.Put(ctx, &model.Player{ID: "player-1", UserId: usr.ID})).To(Succeed()) + key := "nds_0123456789abcdefghijkl" + Expect(playerRepo.SetAPIKey(ctx, "player-1", key)).To(Succeed()) + + playerRepo.Error = errors.New("db down") + failTimes(5, "u=admin", "p="+key) + playerRepo.Error = nil + + serve(newGetRequest("u=admin", "p="+key)) + Expect(next.called).To(BeTrue()) + }) + It("does not block other usernames from the same IP", func() { _ = ds.User().Put(ctx, &model.User{UserName: "other", NewPassword: "otherpass"}) failTimes(3, "u=admin", "p=WRONG") @@ -405,6 +524,25 @@ var _ = Describe("Middlewares", func() { Expect(next.called).To(BeTrue()) }) + It("throttles a repeated bad key without locking out valid keys from the same IP", func() { + usr, _ := ds.User().FindByUsername(ctx, "admin") + playerRepo := ds.Player().(*tests.MockPlayerRepo) + Expect(playerRepo.Put(ctx, &model.Player{ID: "player-1", UserId: usr.ID})).To(Succeed()) + key := "nds_0123456789abcdefghijkl" + Expect(playerRepo.SetAPIKey(ctx, "player-1", key)).To(Succeed()) + + for range 3 { + Expect(serve(newGetRequest("apiKey=nds_bad")).Body.String()).To(ContainSubstring(`code="44"`)) + } + playerRepo.APIKeys["nds_bad"] = "player-1" + rec := serve(newGetRequest("apiKey=nds_bad")) + Expect(next.called).To(BeFalse()) + Expect(rec.Body.String()).To(ContainSubstring(`code="44"`)) + + serve(newGetRequest("apiKey=" + key)) + Expect(next.called).To(BeTrue()) + }) + It("is disabled when AuthRequestLimit is 0", func() { conf.Server.AuthRequestLimit = 0 cp = authenticate(ds)(next) @@ -532,6 +670,24 @@ var _ = Describe("Middlewares", func() { Expect(cookieStr).To(BeEmpty()) }) + Context("player bound by an API key", func() { + BeforeEach(func() { + r = r.WithContext(request.WithPlayer(r.Context(), model.Player{ID: "keyed"})) + gp := getPlayer(mockedPlayers)(next) + gp.ServeHTTP(w, r) + }) + + It("uses the key's player", func() { + Expect(mockedPlayers.touched).To(BeTrue()) + player, _ := request.PlayerFrom(next.req.Context()) + Expect(player.ID).To(Equal("keyed")) + }) + + It("does not set the player cookie", func() { + Expect(w.Header().Get("Set-Cookie")).To(BeEmpty()) + }) + }) + Context("PlayerId specified in Cookies", func() { BeforeEach(func() { cookie := &http.Cookie{ @@ -712,6 +868,12 @@ func (mh *mockHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { type mockPlayers struct { core.Players transcoding *model.Transcoding + touched bool +} + +func (mp *mockPlayers) Touch(_ context.Context, plr model.Player, _, _, _ string) (*model.Player, *model.Transcoding, error) { + mp.touched = true + return &plr, mp.transcoding, nil } func (mp *mockPlayers) Get(ctx context.Context, playerId string) (*model.Player, error) { diff --git a/server/subsonic/opensubsonic.go b/server/subsonic/opensubsonic.go index 2b2a31bf3..21c407039 100644 --- a/server/subsonic/opensubsonic.go +++ b/server/subsonic/opensubsonic.go @@ -16,6 +16,7 @@ func (api *Router) GetOpenSubsonicExtensions(_ *http.Request) (*responses.Subson {Name: "transcoding", Versions: []int32{1}}, {Name: "playbackReport", Versions: []int32{1}}, {Name: "topSongsByArtistId", Versions: []int32{1}}, + {Name: "apiKeyAuthentication", Versions: []int32{1}}, } if api.sonic != nil && api.sonic.HasProvider() { extensions = append(extensions, responses.OpenSubsonicExtension{ diff --git a/server/subsonic/opensubsonic_test.go b/server/subsonic/opensubsonic_test.go index 2615a652d..8740c9971 100644 --- a/server/subsonic/opensubsonic_test.go +++ b/server/subsonic/opensubsonic_test.go @@ -44,44 +44,13 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { router = subsonic.New(nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) }) - It("should return the base 6 OpenSubsonicExtensions without sonicSimilarity", func() { + It("should return the base 8 OpenSubsonicExtensions without sonicSimilarity", func() { router.ServeHTTP(w, r) // Make sure the endpoint is public, by not passing any authentication Expect(w.Code).To(Equal(http.StatusOK)) Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) - var response responses.JsonWrapper - err := json.Unmarshal(w.Body.Bytes(), &response) - Expect(err).NotTo(HaveOccurred()) - Expect(*response.Subsonic.OpenSubsonicExtensions).To(SatisfyAll( - HaveLen(7), - ContainElement(responses.OpenSubsonicExtension{Name: "transcodeOffset", Versions: []int32{1}}), - ContainElement(responses.OpenSubsonicExtension{Name: "formPost", Versions: []int32{1}}), - ContainElement(responses.OpenSubsonicExtension{Name: "songLyrics", Versions: []int32{1, 2}}), - ContainElement(responses.OpenSubsonicExtension{Name: "indexBasedQueue", Versions: []int32{1}}), - ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), - ContainElement(responses.OpenSubsonicExtension{Name: "playbackReport", Versions: []int32{1}}), - ContainElement(responses.OpenSubsonicExtension{Name: "topSongsByArtistId", Versions: []int32{1}}), - )) - Expect(*response.Subsonic.OpenSubsonicExtensions).NotTo( - ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), - ) - }) - }) - - Context("with sonic similarity plugin", func() { - BeforeEach(func() { - sonicService := sonicsvc.New(nil, &mockSonicPluginLoader{names: []string{"test-plugin"}}, nil) - router = subsonic.New(nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, sonicService) - }) - - It("should return 7 extensions including sonicSimilarity", func() { - router.ServeHTTP(w, r) - - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) - var response responses.JsonWrapper err := json.Unmarshal(w.Body.Bytes(), &response) Expect(err).NotTo(HaveOccurred()) @@ -93,8 +62,41 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { ContainElement(responses.OpenSubsonicExtension{Name: "indexBasedQueue", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "playbackReport", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "topSongsByArtistId", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "apiKeyAuthentication", Versions: []int32{1}}), + )) + Expect(*response.Subsonic.OpenSubsonicExtensions).NotTo( + ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), + ) + }) + }) + + Context("with sonic similarity plugin", func() { + BeforeEach(func() { + sonicService := sonicsvc.New(nil, &mockSonicPluginLoader{names: []string{"test-plugin"}}, nil) + router = subsonic.New(nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, sonicService) + }) + + It("should return 9 extensions including sonicSimilarity", func() { + router.ServeHTTP(w, r) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) + + var response responses.JsonWrapper + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).NotTo(HaveOccurred()) + Expect(*response.Subsonic.OpenSubsonicExtensions).To(SatisfyAll( + HaveLen(9), + ContainElement(responses.OpenSubsonicExtension{Name: "transcodeOffset", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "formPost", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "songLyrics", Versions: []int32{1, 2}}), + ContainElement(responses.OpenSubsonicExtension{Name: "indexBasedQueue", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "playbackReport", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "topSongsByArtistId", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "apiKeyAuthentication", Versions: []int32{1}}), )) }) }) diff --git a/server/subsonic/responses/.snapshots/Responses TokenInfo should match .JSON b/server/subsonic/responses/.snapshots/Responses TokenInfo should match .JSON new file mode 100644 index 000000000..f2e251f49 --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses TokenInfo should match .JSON @@ -0,0 +1,10 @@ +{ + "status": "ok", + "version": "1.16.1", + "type": "navidrome", + "serverVersion": "v0.55.0", + "openSubsonic": true, + "tokenInfo": { + "username": "deluan" + } +} diff --git a/server/subsonic/responses/.snapshots/Responses TokenInfo should match .XML b/server/subsonic/responses/.snapshots/Responses TokenInfo should match .XML new file mode 100644 index 000000000..7ea786bb9 --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses TokenInfo should match .XML @@ -0,0 +1,3 @@ + + + diff --git a/server/subsonic/responses/errors.go b/server/subsonic/responses/errors.go index 42e5427b3..9c9dd10f6 100644 --- a/server/subsonic/responses/errors.go +++ b/server/subsonic/responses/errors.go @@ -1,25 +1,29 @@ package responses const ( - ErrorGeneric int32 = 0 - ErrorMissingParameter int32 = 10 - ErrorClientTooOld int32 = 20 - ErrorServerTooOld int32 = 30 - ErrorAuthenticationFail int32 = 40 - ErrorAuthorizationFail int32 = 50 - ErrorTrialExpired int32 = 60 - ErrorDataNotFound int32 = 70 + ErrorGeneric int32 = 0 + ErrorMissingParameter int32 = 10 + ErrorClientTooOld int32 = 20 + ErrorServerTooOld int32 = 30 + ErrorAuthenticationFail int32 = 40 + ErrorMultipleAuthMechanismsProvided int32 = 43 + ErrorInvalidAPIKey int32 = 44 + ErrorAuthorizationFail int32 = 50 + ErrorTrialExpired int32 = 60 + ErrorDataNotFound int32 = 70 ) -var errors = map[int32]string{ - ErrorGeneric: "A generic error", - ErrorMissingParameter: "Required parameter is missing", - ErrorClientTooOld: "Incompatible Subsonic REST protocol version. Client must upgrade", - ErrorServerTooOld: "Incompatible Subsonic REST protocol version. Server must upgrade", - ErrorAuthenticationFail: "Wrong username or password", - ErrorAuthorizationFail: "User is not authorized for the given operation", - ErrorTrialExpired: "The trial period for the Subsonic server is over. Please upgrade to Subsonic Premium. Visit subsonic.org for details", - ErrorDataNotFound: "The requested data was not found", +var errors = map[int32]string{ //nolint:gosec // G101 false positive: error messages, not credentials + ErrorGeneric: "A generic error", + ErrorMissingParameter: "Required parameter is missing", + ErrorClientTooOld: "Incompatible Subsonic REST protocol version. Client must upgrade", + ErrorServerTooOld: "Incompatible Subsonic REST protocol version. Server must upgrade", + ErrorAuthenticationFail: "Wrong username or password", + ErrorMultipleAuthMechanismsProvided: "Multiple conflicting authentication mechanisms provided", + ErrorInvalidAPIKey: "Invalid API key", + ErrorAuthorizationFail: "User is not authorized for the given operation", + ErrorTrialExpired: "The trial period for the Subsonic server is over. Please upgrade to Subsonic Premium. Visit subsonic.org for details", + ErrorDataNotFound: "The requested data was not found", } func ErrorMsg(code int32) string { diff --git a/server/subsonic/responses/responses.go b/server/subsonic/responses/responses.go index 252eee4c6..51d0020b2 100644 --- a/server/subsonic/responses/responses.go +++ b/server/subsonic/responses/responses.go @@ -63,6 +63,7 @@ type Subsonic struct { PlayQueueByIndex *PlayQueueByIndex `xml:"playQueueByIndex,omitempty" json:"playQueueByIndex,omitempty"` TranscodeDecision *TranscodeDecision `xml:"transcodeDecision,omitempty" json:"transcodeDecision,omitempty"` SonicMatches *Array[SonicMatch] `xml:"sonicMatch,omitempty" json:"sonicMatch,omitempty"` + TokenInfo *TokenInfo `xml:"tokenInfo,omitempty" json:"tokenInfo,omitempty"` } const ( @@ -596,6 +597,10 @@ type OpenSubsonicExtension struct { type OpenSubsonicExtensions []OpenSubsonicExtension +type TokenInfo struct { + Username string `xml:"username,attr" json:"username"` +} + type ItemGenre struct { Name string `xml:"name,attr" json:"name"` } diff --git a/server/subsonic/responses/responses_test.go b/server/subsonic/responses/responses_test.go index 586e46b63..027ac11d5 100644 --- a/server/subsonic/responses/responses_test.go +++ b/server/subsonic/responses/responses_test.go @@ -1015,6 +1015,20 @@ var _ = Describe("Responses", func() { }) }) + Describe("TokenInfo", func() { + BeforeEach(func() { + response.OpenSubsonic = true + response.TokenInfo = &TokenInfo{Username: "deluan"} + }) + + It("should match .XML", func() { + Expect(xml.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + It("should match .JSON", func() { + Expect(json.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + }) + Describe("InternetRadioStations", func() { BeforeEach(func() { response.InternetRadioStations = &InternetRadioStations{} diff --git a/server/subsonic/system.go b/server/subsonic/system.go index e14099942..4a59e7898 100644 --- a/server/subsonic/system.go +++ b/server/subsonic/system.go @@ -3,6 +3,7 @@ package subsonic import ( "net/http" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/subsonic/responses" ) @@ -15,3 +16,10 @@ func (api *Router) GetLicense(_ *http.Request) (*responses.Subsonic, error) { response.License = &responses.License{Valid: true} return response, nil } + +func (api *Router) TokenInfo(r *http.Request) (*responses.Subsonic, error) { + user, _ := request.UserFrom(r.Context()) + response := newResponse() + response.TokenInfo = &responses.TokenInfo{Username: user.UserName} + return response, nil +} diff --git a/server/subsonic/system_test.go b/server/subsonic/system_test.go new file mode 100644 index 000000000..a8c85238e --- /dev/null +++ b/server/subsonic/system_test.go @@ -0,0 +1,23 @@ +package subsonic + +import ( + "net/http/httptest" + + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("TokenInfo", func() { + It("returns the authenticated username", func() { + api := &Router{} + r := httptest.NewRequest("GET", "/tokenInfo", nil) + r = r.WithContext(request.WithUser(r.Context(), model.User{UserName: "deluan"})) + + resp, err := api.TokenInfo(r) + + Expect(err).ToNot(HaveOccurred()) + Expect(resp.TokenInfo.Username).To(Equal("deluan")) + }) +}) diff --git a/tests/mock_data_store.go b/tests/mock_data_store.go index db798eece..a5e4126cf 100644 --- a/tests/mock_data_store.go +++ b/tests/mock_data_store.go @@ -228,7 +228,7 @@ func (db *MockDataStore) Player() model.PlayerRepository { if db.RealDS != nil { return db.RealDS.Player() } - db.MockedPlayer = struct{ model.PlayerRepository }{} + db.MockedPlayer = CreateMockPlayerRepo() return db.MockedPlayer } diff --git a/tests/mock_library_repo.go b/tests/mock_library_repo.go index e21dcccce..6de2b9265 100644 --- a/tests/mock_library_repo.go +++ b/tests/mock_library_repo.go @@ -145,6 +145,17 @@ func (m *MockLibraryRepo) ScanEnd(_ context.Context, id int) error { return nil } +func (m *MockLibraryRepo) SetScannedPID(_ context.Context, id int, pid model.PIDConfig) error { + if m.Err != nil { + return m.Err + } + if lib, ok := m.Data[id]; ok { + lib.ScannedPIDAlbum, lib.ScannedPIDTrack = pid.Album, pid.Track + m.Data[id] = lib + } + return nil +} + func (m *MockLibraryRepo) ScanInProgress(_ context.Context) (bool, error) { if m.Err != nil { return false, m.Err diff --git a/tests/mock_player_repo.go b/tests/mock_player_repo.go new file mode 100644 index 000000000..56835f0cc --- /dev/null +++ b/tests/mock_player_repo.go @@ -0,0 +1,75 @@ +package tests + +import ( + "context" + "maps" + "slices" + + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/id" +) + +func CreateMockPlayerRepo() *MockPlayerRepo { + return &MockPlayerRepo{Data: map[string]*model.Player{}, APIKeys: map[string]string{}} +} + +// MockPlayerRepo keeps API keys in plaintext (key -> player ID); hashing belongs to the real repository. +type MockPlayerRepo struct { + model.PlayerRepository + Error error + Data map[string]*model.Player + APIKeys map[string]string +} + +func (m *MockPlayerRepo) Get(_ context.Context, id string) (*model.Player, error) { + if m.Error != nil { + return nil, m.Error + } + p, ok := m.Data[id] + if !ok { + return nil, model.ErrNotFound + } + cp := *p + cp.HasAPIKey = slices.Contains(slices.Collect(maps.Values(m.APIKeys)), id) + return &cp, nil +} + +func (m *MockPlayerRepo) Put(_ context.Context, p *model.Player) error { + if m.Error != nil { + return m.Error + } + if p.ID == "" { + p.ID = id.NewRandom() + } + cp := *p + m.Data[p.ID] = &cp + return nil +} + +func (m *MockPlayerRepo) FindByAPIKey(ctx context.Context, key string) (*model.Player, error) { + if m.Error != nil { + return nil, m.Error + } + if playerID, ok := m.APIKeys[key]; ok { + return m.Get(ctx, playerID) + } + return nil, model.ErrNotFound +} + +func (m *MockPlayerRepo) SetAPIKey(_ context.Context, playerID, key string) error { + if m.Error != nil { + return m.Error + } + if _, ok := m.Data[playerID]; !ok { + return model.ErrNotFound + } + m.removeKeys(playerID) + if key != "" { + m.APIKeys[key] = playerID + } + return nil +} + +func (m *MockPlayerRepo) removeKeys(playerID string) { + maps.DeleteFunc(m.APIKeys, func(_ string, v string) bool { return v == playerID }) +} diff --git a/ui/src/App.jsx b/ui/src/App.jsx index d10aa5a33..4de369394 100644 --- a/ui/src/App.jsx +++ b/ui/src/App.jsx @@ -141,7 +141,7 @@ const Admin = (props) => { , permissions === 'admin' ? ( { return (
-
+
({ + default: { + getAlbumInfo: () => + Promise.resolve({ + json: { 'subsonic-response': { status: 'ok', albumInfo: {} } }, + }), + getCoverArtUrl: () => '', + }, +})) + +vi.mock('react-admin', async () => { + const actual = await vi.importActual('react-admin') + return { + ...actual, + useDataProvider: () => ({ getOne: vi.fn() }), + useNotify: () => vi.fn(), + useRefresh: () => vi.fn(), + } +}) // Mock useMediaQuery vi.mock('@material-ui/core', async () => { @@ -343,3 +365,49 @@ describe('Details component', () => { }) }) }) + +describe('AlbumDetails cover animation', () => { + const albumRecord = { + id: '123', + name: 'Test Album', + songCount: 12, + duration: 3600, + size: 102400, + } + const originalEnableCoverAnimation = config.enableCoverAnimation + + beforeEach(() => { + vi.mocked(useMediaQuery).mockReturnValue(false) + }) + + afterEach(() => { + config.enableCoverAnimation = originalEnableCoverAnimation + }) + + const renderAlbum = () => + render( + + + + + , + ) + + test('applies noCoverAnimation when enableCoverAnimation is false', () => { + config.enableCoverAnimation = false + const { container } = renderAlbum() + const cover = container.querySelector('[class*="coverParent"]') + + expect(cover).not.toBeNull() + expect(cover.className).toMatch(/noCoverAnimation/) + }) + + test('omits noCoverAnimation when enableCoverAnimation is true', () => { + config.enableCoverAnimation = true + const { container } = renderAlbum() + const cover = container.querySelector('[class*="coverParent"]') + + expect(cover).not.toBeNull() + expect(cover.className).not.toMatch(/noCoverAnimation/) + }) +}) diff --git a/ui/src/album/AlbumList.jsx b/ui/src/album/AlbumList.jsx index 3cb8a648e..81e4dcd14 100644 --- a/ui/src/album/AlbumList.jsx +++ b/ui/src/album/AlbumList.jsx @@ -163,6 +163,7 @@ const AlbumFilter = (props) => { /> + {config.enableFavourites && ( ({ + inputRoot: { + '&:hover $notchedOutline': { + borderColor: theme.palette.divider, + }, + }, + notchedOutline: { + borderColor: theme.palette.divider, + }, + }), + { name: 'NDReadOnlyField' }, +) + +const identity = (v) => v + +// Renders a record value as a dimmed, non-editable input, so it lines up with the inputs in a form +export const ReadOnlyTextField = ({ + source, + label, + resource, + className, + fullWidth, + format = identity, + ...props +}) => { + const classes = useStyles(props) + const record = useRecordContext(props) + const value = get(record, source) + + return ( + } + value={value == null ? '' : format(value)} + variant="outlined" + margin="dense" + fullWidth={fullWidth} + focused={false} + helperText=" " + InputProps={{ + readOnly: true, + classes: { + root: classes.inputRoot, + notchedOutline: classes.notchedOutline, + }, + }} + inputProps={{ tabIndex: -1 }} + /> + ) +} + +ReadOnlyTextField.propTypes = { + source: PropTypes.string.isRequired, + label: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]), + record: PropTypes.object, + resource: PropTypes.string, + className: PropTypes.string, + classes: PropTypes.object, + fullWidth: PropTypes.bool, + format: PropTypes.func, +} + +export const ReadOnlyDateField = (props) => { + const locale = useDateLocale() + const format = (v) => (isDateSet(v) ? formatDateTime(v, locale) : '') + return +} + +export const ReadOnlyNumberField = (props) => { + const locale = useDateLocale() + return ( + formatNumber(v, locale)} {...props} /> + ) +} + +export const ReadOnlySizeField = (props) => ( + +) + +export const ReadOnlyDurationField = (props) => ( + +) diff --git a/ui/src/common/ReadOnlyFields.test.jsx b/ui/src/common/ReadOnlyFields.test.jsx new file mode 100644 index 000000000..00543a786 --- /dev/null +++ b/ui/src/common/ReadOnlyFields.test.jsx @@ -0,0 +1,121 @@ +import React from 'react' +import { render, screen } from '@testing-library/react' +import { describe, it, expect, beforeEach, vi } from 'vitest' +import { + ReadOnlyDateField, + ReadOnlyDurationField, + ReadOnlyNumberField, + ReadOnlySizeField, + ReadOnlyTextField, +} from './ReadOnlyFields' + +vi.mock('react-admin', async (importOriginal) => ({ + ...(await importOriginal()), + useLocale: vi.fn(), +})) + +describe('ReadOnlyFields', () => { + const record = { + id: '1', + client: 'NavidromeUI', + createdAt: '2026-09-17T14:30:00Z', + lastVisitedAt: '0001-01-01T00:00:00Z', + count: 1234567, + zero: 0, + size: 1536000, + duration: 3725, + } + + beforeEach(async () => { + vi.clearAllMocks() + vi.spyOn(navigator, 'languages', 'get').mockReturnValue([]) + const { useLocale } = await import('react-admin') + vi.mocked(useLocale).mockReturnValue('de') + }) + + const renderField = (Field, props) => + render() + + describe('', () => { + it('shows the record value with the translated field label', () => { + renderField(ReadOnlyTextField, { source: 'client' }) + const input = screen.getByLabelText('resources.player.fields.client') + expect(input).toHaveValue('NavidromeUI') + }) + + it('cannot be edited or reached with the Tab key', () => { + renderField(ReadOnlyTextField, { source: 'client' }) + const input = screen.getByRole('textbox') + expect(input).toHaveAttribute('readonly') + expect(input).toHaveAttribute('tabindex', '-1') + }) + + it('shows an empty value when the record has no value', () => { + renderField(ReadOnlyTextField, { source: 'userName' }) + expect(screen.getByRole('textbox')).toHaveValue('') + }) + + it('uses an explicit label when given', () => { + renderField(ReadOnlyTextField, { source: 'client', label: 'Custom' }) + expect(screen.getByLabelText('Custom')).toBeInTheDocument() + }) + + it('applies a custom format', () => { + renderField(ReadOnlyTextField, { + source: 'client', + format: (v) => v.toUpperCase(), + }) + expect(screen.getByRole('textbox')).toHaveValue('NAVIDROMEUI') + }) + + it('exposes theme-overridable class names', () => { + const { container } = renderField(ReadOnlyTextField, { + source: 'client', + }) + expect( + container.querySelector('[class*="NDReadOnlyField-inputRoot"]'), + ).toBeInTheDocument() + expect( + container.querySelector('[class*="NDReadOnlyField-notchedOutline"]'), + ).toBeInTheDocument() + }) + }) + + describe('', () => { + it('formats the date using the selected language', () => { + renderField(ReadOnlyDateField, { source: 'createdAt' }) + expect(screen.getByRole('textbox').value).toMatch(/^17\.9\.2026, /) + }) + + it('shows an empty value when the date is not set', () => { + renderField(ReadOnlyDateField, { source: 'lastVisitedAt' }) + expect(screen.getByRole('textbox')).toHaveValue('') + }) + }) + + describe('', () => { + it('formats the number using the selected language', () => { + renderField(ReadOnlyNumberField, { source: 'count' }) + expect(screen.getByRole('textbox')).toHaveValue('1.234.567') + }) + + it('shows zero', () => { + renderField(ReadOnlyNumberField, { source: 'zero' }) + expect(screen.getByRole('textbox')).toHaveValue('0') + }) + }) + + describe('', () => { + it('formats bytes as a human-readable size', () => { + renderField(ReadOnlySizeField, { source: 'size' }) + expect(screen.getByRole('textbox')).toHaveValue('1.46 MB') + }) + }) + + describe('', () => { + it('formats seconds as a human-readable duration', () => { + renderField(ReadOnlyDurationField, { source: 'duration' }) + expect(screen.getByRole('textbox')).toHaveValue('1h 2m 5s') + }) + }) +}) diff --git a/ui/src/common/index.js b/ui/src/common/index.js index 0177df326..fb8f40f00 100644 --- a/ui/src/common/index.js +++ b/ui/src/common/index.js @@ -15,6 +15,7 @@ export * from './perPageStore' export * from './PlayButton' export * from './QuickFilter' export * from './RangeField' +export * from './ReadOnlyFields' export * from './ShuffleAllButton' export * from './SimpleList' export * from './SizeField' diff --git a/ui/src/config.js b/ui/src/config.js index e406e47cf..62b3cb822 100644 --- a/ui/src/config.js +++ b/ui/src/config.js @@ -32,6 +32,8 @@ const defaultConfig = { listenBrainzEnabled: true, enableExternalServices: true, enableCoverAnimation: true, + pidAlbum: 'musicbrainz_albumid|albumartistid,album,albumversion,releasedate', // See consts.DefaultAlbumPID + pidTrack: 'musicbrainz_trackid|albumid,discnumber,tracknumber,title', // See consts.DefaultTrackPID enableNowPlaying: true, playbackReportIntervalMs: 60000, devShowArtistPage: true, diff --git a/ui/src/dataProvider/wrapperDataProvider.js b/ui/src/dataProvider/wrapperDataProvider.js index f0e44ce1d..7de20bcce 100644 --- a/ui/src/dataProvider/wrapperDataProvider.js +++ b/ui/src/dataProvider/wrapperDataProvider.js @@ -148,6 +148,12 @@ const updateUser = async (params) => { return userResponse } +// ra-data-json-server merges the request body into the result; re-read so the plaintext key is never cached +const createPlayer = async (resource, params) => { + const { data } = await dataProvider.create(resource, params) + return dataProvider.getOne(resource, { id: data.id }) +} + const wrapperDataProvider = { ...dataProvider, getList: (resource, params) => { @@ -194,6 +200,9 @@ const wrapperDataProvider = { return createUser(params) } const [r, p] = mapResource(resource, params) + if (resource === 'player') { + return createPlayer(r, p) + } return dataProvider.create(r, p) }, delete: (resource, params) => { diff --git a/ui/src/dataProvider/wrapperDataProvider.test.js b/ui/src/dataProvider/wrapperDataProvider.test.js index 4225a5a54..1c33aad5d 100644 --- a/ui/src/dataProvider/wrapperDataProvider.test.js +++ b/ui/src/dataProvider/wrapperDataProvider.test.js @@ -88,6 +88,21 @@ describe('wrapperDataProvider', () => { }) }) + describe('create player', () => { + it('returns the server record, never the plaintext API key', async () => { + const data = { name: 'Phone', apiKey: 'nds_0123456789abcdefghijkl' } + const saved = { id: 'p1', name: 'Phone', hasApiKey: true, userId: 'u1' } + mockProvider.create.mockResolvedValue({ data: { ...data, id: 'p1' } }) + mockProvider.getOne.mockResolvedValue({ data: saved }) + + const result = await wrapperDataProvider.create('player', { data }) + + expect(mockProvider.create).toHaveBeenCalledWith('player', { data }) + expect(mockProvider.getOne).toHaveBeenCalledWith('player', { id: 'p1' }) + expect(result.data).toEqual(saved) + }) + }) + describe('refreshMetadata', () => { it('posts to the album metadata refresh endpoint', () => { mockHttpClient.mockResolvedValue({ json: {} }) diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index f5af05b7b..f694ea75f 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -83,7 +83,8 @@ "grouping": "Grouping", "media": "Media", "mood": "Mood", - "missing": "Missing" + "missing": "Missing", + "played": "Played" }, "actions": { "playAll": "Play", @@ -182,6 +183,7 @@ }, "player": { "name": "Player |||| Players", + "menuName": "Players & API keys", "fields": { "name": "Name", "transcodingId": "Transcoding", @@ -190,7 +192,29 @@ "userName": "Username", "lastSeen": "Last Seen At", "reportRealPath": "Report Real Path", - "scrobbleEnabled": "Send Scrobbles to external services" + "scrobbleEnabled": "Send Scrobbles to external services", + "hasApiKey": "API Key" + }, + "actions": { + "generateApiKey": "Generate API key", + "regenerateApiKey": "Regenerate", + "revokeApiKey": "Revoke", + "copyApiKey": "Copy" + }, + "message": { + "apiKeyActive": "This player has an API key. Use it in your app as the API key, or as the password if the app does not use token authentication.", + "apiKeyNone": "No API key. Generate one to connect an app to this player.", + "apiKeyNoneOther": "No API key.", + "apiKeyPending": "Copy this key now. It is saved when you click Save and will not be shown again.", + "apiKeyRevokePending": "The API key will be removed when you save.", + "deleteWithKeyTitle": "Delete player", + "deleteWithKeyContent": "This player has an API key. Apps using it will stop working." + }, + "notifications": { + "apiKeyCopied": "API key copied to clipboard" + }, + "validation": { + "apiKeyFormat": "Invalid API key format" } }, "transcoding": { @@ -307,11 +331,22 @@ "totalDuration": "Duration", "defaultNewUsers": "Default for New Users", "createdAt": "Created", - "updatedAt": "Updated" + "updatedAt": "Updated", + "pidAlbum": "Album grouping", + "pidTrack": "Track identity" }, "sections": { "basic": "Basic Information", - "statistics": "Statistics" + "statistics": "Statistics", + "pid": "Persistent IDs" + }, + "pid": { + "global": "Use global setting (%{value})", + "folder": "Folder (one album per folder)", + "custom": "Custom", + "spec": "PID spec", + "help": "Tags and attributes that identify an item. See the documentation for the syntax:", + "docs": "Persistent IDs" }, "actions": { "scan": "Scan Library", @@ -341,7 +376,9 @@ "messages": { "deleteConfirm": "Are you sure you want to delete this library? This will remove all associated data and user access.", "scanInProgress": "Scan in progress...", - "noLibrariesAssigned": "No libraries assigned to this user" + "noLibrariesAssigned": "No libraries assigned to this user", + "pidChangeTitle": "Change persistent IDs?", + "pidChangeConfirm": "This regroups albums and tracks in this library. A full rescan of this library starts now. Track stars, ratings and play counts are kept. Album stars and ratings move to the new albums where an old album maps to a new one." } }, "plugin": { diff --git a/ui/src/layout/AppBar.jsx b/ui/src/layout/AppBar.jsx index 7de111e67..460d33bb9 100644 --- a/ui/src/layout/AppBar.jsx +++ b/ui/src/layout/AppBar.jsx @@ -102,9 +102,11 @@ const CustomUserMenu = ({ onClick, ...rest }) => { } const renderSettingsMenuItemLink = (resource, id) => { - const label = translate(`resources.${resource.name}.name`, { - smart_count: id ? 1 : 2, - }) + const label = resource.options.label + ? translate(resource.options.label) + : translate(`resources.${resource.name}.name`, { + smart_count: id ? 1 : 2, + }) const link = id ? `/${resource.name}/${id}` : `/${resource.name}` return ( ({ resources: [] })) + vi.mock('react-admin', () => ({ AppBar: ({ userMenu }) =>
{userMenu}
, + MenuItemLink: ({ primaryText }) =>
{primaryText}
, useTranslate: () => (x) => x, usePermissions: () => ({ permissions: 'admin' }), - getResources: () => [], + getResources: () => mocks.resources, })) vi.mock('./NowPlayingPanel', () => ({ @@ -41,6 +44,7 @@ describe('', () => { config.devActivityPanel = true config.enableNowPlaying = true config.enableQuickConnect = false + mocks.resources = [] store = createStore(combineReducers({ activity: activityReducer }), { activity: { nowPlayingCount: 0 }, }) @@ -84,4 +88,22 @@ describe('', () => { expect(screen.queryAllByText('menu.quickConnect.name')).toHaveLength(0) expect(screen.queryAllByText('menu.about')).not.toHaveLength(0) }) + + it('uses the resource label for settings items when set', () => { + mocks.resources = [ + { + name: 'player', + hasList: true, + options: { subMenu: 'settings', label: 'resources.player.menuName' }, + }, + { name: 'transcoding', hasList: true, options: { subMenu: 'settings' } }, + ] + render( + + + , + ) + expect(screen.getByText('resources.player.menuName')).toBeInTheDocument() + expect(screen.getByText('resources.transcoding.name')).toBeInTheDocument() + }) }) diff --git a/ui/src/layout/Notification.jsx b/ui/src/layout/Notification.jsx index 001d3fc01..ff641ec22 100644 --- a/ui/src/layout/Notification.jsx +++ b/ui/src/layout/Notification.jsx @@ -1,11 +1,26 @@ import React from 'react' import { Notification as RANotification } from 'react-admin' +import { makeStyles } from '@material-ui/core/styles' -const Notification = (props) => ( - +// RA's primary.light Undo is unreadable on the light snackbar of dark themes +const useStyles = makeStyles( + { + undo: { + color: 'inherit', + }, + }, + { name: 'NDNotification' }, ) +const Notification = (props) => { + const classes = useStyles() + return ( + + ) +} + export default Notification diff --git a/ui/src/library/LibraryCreate.jsx b/ui/src/library/LibraryCreate.jsx index 0e69964b6..8166bb2f3 100644 --- a/ui/src/library/LibraryCreate.jsx +++ b/ui/src/library/LibraryCreate.jsx @@ -1,4 +1,5 @@ import React, { useCallback } from 'react' +import PropTypes from 'prop-types' import { Create, SimpleForm, @@ -10,7 +11,34 @@ import { useNotify, useRedirect, } from 'react-admin' +import { Typography } from '@material-ui/core' +import { makeStyles } from '@material-ui/core/styles' import { Title } from '../common' +import { PIDInputs } from './PIDInput' + +const useStyles = makeStyles((theme) => ({ + spaced: { marginTop: theme.spacing(3) }, +})) + +// SimpleForm passes form props (variant, record, ...) to its children, so Typography can't be used directly +const SectionTitle = ({ label, spaced }) => { + const translate = useTranslate() + const classes = useStyles() + return ( + + {translate(label)} + + ) +} + +SectionTitle.propTypes = { + label: PropTypes.string.isRequired, + spaced: PropTypes.bool, +} const LibraryCreate = (props) => { const translate = useTranslate() @@ -73,9 +101,12 @@ const LibraryCreate = (props) => { return ( } {...props}> + + + ) diff --git a/ui/src/library/LibraryEdit.jsx b/ui/src/library/LibraryEdit.jsx index 7e89c892c..c42c7ac4b 100644 --- a/ui/src/library/LibraryEdit.jsx +++ b/ui/src/library/LibraryEdit.jsx @@ -1,12 +1,13 @@ -import React, { useCallback } from 'react' +import React, { useCallback, useState } from 'react' +import PropTypes from 'prop-types' import { Edit, FormWithRedirect, TextInput, BooleanInput, + Confirm, required, SaveButton, - DateField, useTranslate, useMutation, useNotify, @@ -16,8 +17,16 @@ import { import { Typography, Box } from '@material-ui/core' import { makeStyles } from '@material-ui/core/styles' import DeleteLibraryButton from './DeleteLibraryButton' -import { Title } from '../common' -import { formatBytes, formatDuration2, formatNumber } from '../utils/index.js' +import { + ReadOnlyDateField, + ReadOnlyDurationField, + ReadOnlyNumberField, + ReadOnlySizeField, + Title, +} from '../common' +import config from '../config' +import { PIDInputs } from './PIDInput' +import { pidConfigChanged } from './pidPresets' const useStyles = makeStyles({ toolbar: { @@ -26,6 +35,8 @@ const useStyles = makeStyles({ }, }) +const readOnlyProps = { resource: 'library', fullWidth: true } + const LibraryTitle = ({ record }) => { const translate = useTranslate() const resourceName = translate('resources.library.name', { smart_count: 1 }) @@ -47,8 +58,131 @@ const CustomToolbar = ({ showDelete, ...props }) => ( ) -const LibraryEdit = (props) => { +export const LibraryEditForm = ({ formProps, canEditPath, canDelete }) => { const translate = useTranslate() + const [confirmOpen, setConfirmOpen] = useState(false) + + // Every submit path (Save button and Enter key) goes through here, so a PID change always asks first + const submit = () => { + if ( + pidConfigChanged( + formProps.form.getState().values, + formProps.record, + config, + ) + ) { + setConfirmOpen(true) + return + } + formProps.handleSubmit() + } + + const handleConfirm = () => { + setConfirmOpen(false) + formProps.handleSubmit() + } + + return ( +
{ + event.preventDefault() + submit() + }} + > + + + + {/* Basic Information */} + + {translate('resources.library.sections.basic')} + + + + + + + + + {translate('resources.library.sections.pid')} + + + + + + {/* Statistics - Two Column Layout */} + + {translate('resources.library.sections.statistics')} + + + + + + + + + + + + + + + + + + + + + setConfirmOpen(false)} + /> + + ) +} + +LibraryEditForm.propTypes = { + formProps: PropTypes.object.isRequired, + canEditPath: PropTypes.bool, + canDelete: PropTypes.bool, +} + +const LibraryEdit = (props) => { const [mutate] = useMutation() const notify = useNotify() const redirect = useRedirect() @@ -87,183 +221,11 @@ const LibraryEdit = (props) => { {...props} save={save} render={(formProps) => ( -
- - - - {/* Basic Information */} - - {translate('resources.library.sections.basic')} - - - - - - - - - {/* Statistics - Two Column Layout */} - - {translate('resources.library.sections.statistics')} - - - - - - - - - - - - - - - - - formatBytes(v, 2)} - fullWidth - variant="outlined" - /> - - - - - - - - - - - - - {/* Timestamps Section */} - - - {translate('resources.library.fields.lastScanAt')} - - - - - - - {translate('resources.library.fields.updatedAt')} - - - - - - - {translate('resources.library.fields.createdAt')} - - - - - - - - - + )} /> diff --git a/ui/src/library/LibraryEdit.test.jsx b/ui/src/library/LibraryEdit.test.jsx new file mode 100644 index 000000000..926adc839 --- /dev/null +++ b/ui/src/library/LibraryEdit.test.jsx @@ -0,0 +1,125 @@ +import * as React from 'react' +import { TestContext } from 'ra-test' +import { + FormWithRedirect, + RecordContextProvider, + SaveContextProvider, +} from 'react-admin' +import { + cleanup, + fireEvent, + render, + screen, + waitFor, + within, +} from '@testing-library/react' +import { describe, it, expect, vi, afterEach } from 'vitest' +import { LibraryEditForm } from './LibraryEdit' +import config from '../config' + +const record = { + id: '2', + name: 'Jazz', + path: '/music/jazz', + pidAlbum: '', + pidTrack: '', +} + +// Edit provides a save context in the app. SaveButton only reads these setters from it +const saveContext = { + save: vi.fn(), + setOnSuccess: vi.fn(), + setOnFailure: vi.fn(), + setTransform: vi.fn(), +} + +const renderForm = (save) => + render( + + + + ( + + )} + /> + + + , + ) + +const chooseAlbumGrouping = (optionText) => { + fireEvent.mouseDown( + screen.getByLabelText('resources.library.fields.pidAlbum'), + ) + fireEvent.click(within(screen.getByRole('listbox')).getByText(optionText)) +} + +const dialogTitle = 'resources.library.messages.pidChangeTitle' + +describe('LibraryEditForm', () => { + afterEach(cleanup) + + it('saves directly when the PID config did not change', async () => { + const save = vi.fn() + renderForm(save) + fireEvent.change(screen.getByLabelText(/resources.library.fields.name/), { + target: { value: 'Jazz Renamed' }, + }) + fireEvent.click(screen.getByText('ra.action.save')) + await waitFor(() => expect(save).toHaveBeenCalled()) + expect(screen.queryByText(dialogTitle)).not.toBeInTheDocument() + }) + + it('asks before saving a PID change, and Cancel keeps the edits', async () => { + const save = vi.fn() + renderForm(save) + chooseAlbumGrouping('resources.library.pid.folder') + fireEvent.click(screen.getByText('ra.action.save')) + + expect(await screen.findByText(dialogTitle)).toBeInTheDocument() + expect(save).not.toHaveBeenCalled() + + fireEvent.click(screen.getByText('ra.action.cancel')) + await waitFor(() => + expect(screen.queryByText(dialogTitle)).not.toBeInTheDocument(), + ) + expect(save).not.toHaveBeenCalled() + expect(screen.getByText('resources.library.pid.folder')).toBeInTheDocument() + }) + + it('saves the PID change after Confirm', async () => { + const save = vi.fn() + renderForm(save) + chooseAlbumGrouping('resources.library.pid.folder') + fireEvent.click(screen.getByText('ra.action.save')) + fireEvent.click(await screen.findByText('ra.action.confirm')) + + await waitFor(() => expect(save).toHaveBeenCalled()) + expect(save.mock.calls[0][0]).toMatchObject({ pidAlbum: 'folder' }) + }) + + it('pre-fills a Custom spec with the global spec', () => { + renderForm(vi.fn()) + chooseAlbumGrouping('resources.library.pid.custom') + expect(screen.getByLabelText(/resources.library.pid.spec/)).toHaveValue( + config.pidAlbum, + ) + }) + + it('asks before saving when the form is submitted with Enter', async () => { + const save = vi.fn() + const { container } = renderForm(save) + chooseAlbumGrouping('resources.library.pid.folder') + fireEvent.submit(container.querySelector('form')) + + expect(await screen.findByText(dialogTitle)).toBeInTheDocument() + expect(save).not.toHaveBeenCalled() + }) +}) diff --git a/ui/src/library/PIDInput.jsx b/ui/src/library/PIDInput.jsx new file mode 100644 index 000000000..6481dc392 --- /dev/null +++ b/ui/src/library/PIDInput.jsx @@ -0,0 +1,114 @@ +import React, { useState } from 'react' +import PropTypes from 'prop-types' +import { TextInput, required, useTranslate } from 'react-admin' +import { useField } from 'react-final-form' +import { FormHelperText, Link, MenuItem, TextField } from '@material-ui/core' +import { makeStyles } from '@material-ui/core/styles' +import { + PID_CUSTOM, + PID_FOLDER, + PID_GLOBAL, + pidModeFromValue, + pidValueForMode, +} from './pidPresets' +import config from '../config' +import { docsUrl } from '../utils' + +const PID_DOCS_URL = docsUrl('/docs/usage/pids/') + +const useStyles = makeStyles((theme) => ({ + help: { marginBottom: theme.spacing(1) }, +})) + +// PIDInput edits a library PID override: use the global setting, a preset, or a custom spec +export const PIDInput = ({ source, label, globalValue, allowFolder }) => { + const translate = useTranslate() + const classes = useStyles() + const { input } = useField(source) + // Local state, so choosing Custom shows the text box before anything is typed + const [mode, setMode] = useState(() => + pidModeFromValue(input.value, allowFolder), + ) + + const choices = [ + { + id: PID_GLOBAL, + name: translate('resources.library.pid.global', { value: globalValue }), + }, + ...(allowFolder + ? [{ id: PID_FOLDER, name: translate('resources.library.pid.folder') }] + : []), + { id: PID_CUSTOM, name: translate('resources.library.pid.custom') }, + ] + + const handleModeChange = (event) => { + const newMode = event.target.value + setMode(newMode) + input.onChange(pidValueForMode(newMode, globalValue)) + } + + return ( + <> + + {choices.map((choice) => ( + + {choice.name} + + ))} + + {mode === PID_CUSTOM && ( + <> + + + {translate('resources.library.pid.help')}{' '} + + {translate('resources.library.pid.docs')} + + + + )} + + ) +} + +PIDInput.propTypes = { + source: PropTypes.string.isRequired, + label: PropTypes.string.isRequired, + globalValue: PropTypes.string, + allowFolder: PropTypes.bool, +} + +export const PIDInputs = () => { + const translate = useTranslate() + return ( + <> + + + + ) +} diff --git a/ui/src/library/pidPresets.js b/ui/src/library/pidPresets.js new file mode 100644 index 000000000..0483fc691 --- /dev/null +++ b/ui/src/library/pidPresets.js @@ -0,0 +1,33 @@ +export const PID_GLOBAL = 'global' +export const PID_FOLDER = 'folder' +export const PID_CUSTOM = 'custom' + +export const pidModeFromValue = (value, allowFolder) => { + const v = (value || '').trim() + if (v === '') return PID_GLOBAL + if (allowFolder && v === PID_FOLDER) return PID_FOLDER + return PID_CUSTOM +} + +// Returns the value to store for a dropdown choice. Custom starts from the global spec +export const pidValueForMode = (mode, globalValue) => { + switch (mode) { + case PID_GLOBAL: + return '' + case PID_FOLDER: + return PID_FOLDER + default: + return globalValue || '' + } +} + +// Reports whether the form values change the effective PID spec of the saved record. Like the +// server, it trims, treats empty as the global value and compares case-insensitively +export const pidConfigChanged = (values, record, globals) => { + const effective = (value, field) => + ((value || '').trim() || globals[field] || '').toLowerCase() + return ['pidAlbum', 'pidTrack'].some( + (field) => + effective(values[field], field) !== effective(record[field], field), + ) +} diff --git a/ui/src/library/pidPresets.test.js b/ui/src/library/pidPresets.test.js new file mode 100644 index 000000000..dab1c82e2 --- /dev/null +++ b/ui/src/library/pidPresets.test.js @@ -0,0 +1,65 @@ +import { describe, it, expect } from 'vitest' +import { + PID_CUSTOM, + PID_FOLDER, + PID_GLOBAL, + pidConfigChanged, + pidModeFromValue, + pidValueForMode, +} from './pidPresets' + +describe('pidModeFromValue', () => { + it('maps an empty value to the global setting', () => { + expect(pidModeFromValue('', true)).toBe(PID_GLOBAL) + expect(pidModeFromValue(undefined, true)).toBe(PID_GLOBAL) + }) + it('maps folder to the Folder preset when allowed', () => { + expect(pidModeFromValue('folder', true)).toBe(PID_FOLDER) + }) + it('maps folder to Custom when the Folder preset is not offered', () => { + expect(pidModeFromValue('folder', false)).toBe(PID_CUSTOM) + }) + it('maps any other value to Custom', () => { + expect(pidModeFromValue('album|title', true)).toBe(PID_CUSTOM) + }) +}) + +describe('pidValueForMode', () => { + it('stores an empty value for the global setting', () => { + expect(pidValueForMode(PID_GLOBAL, 'album')).toBe('') + }) + it('stores folder for the Folder preset', () => { + expect(pidValueForMode(PID_FOLDER, '')).toBe('folder') + }) + it('starts Custom from the global spec', () => { + expect(pidValueForMode(PID_CUSTOM, 'album|title')).toBe('album|title') + expect(pidValueForMode(PID_CUSTOM, undefined)).toBe('') + }) +}) + +describe('pidConfigChanged', () => { + const record = { pidAlbum: 'folder', pidTrack: '' } + const globals = { + pidAlbum: 'musicbrainz_albumid|albumartistid,album', + pidTrack: 'musicbrainz_trackid|albumid,discnumber,tracknumber,title', + } + it.each([ + ['nothing changed', { pidAlbum: 'folder', pidTrack: '' }, false], + ['a missing value equals an empty one', { pidAlbum: 'folder' }, false], + [ + 'Custom set to the global value', + { pidAlbum: 'folder', pidTrack: globals.pidTrack }, + false, + ], + ['a case-only change', { pidAlbum: 'FOLDER', pidTrack: '' }, false], + [ + 'a whitespace-only change', + { pidAlbum: ' folder ', pidTrack: ' ' }, + false, + ], + ['the album PID changed', { pidAlbum: '', pidTrack: '' }, true], + ['the track PID changed', { pidAlbum: 'folder', pidTrack: 'title' }, true], + ])('%s', (_, values, expected) => { + expect(pidConfigChanged(values, record, globals)).toBe(expected) + }) +}) diff --git a/ui/src/player/ApiKeyInput.jsx b/ui/src/player/ApiKeyInput.jsx new file mode 100644 index 000000000..1f2dd1ed2 --- /dev/null +++ b/ui/src/player/ApiKeyInput.jsx @@ -0,0 +1,107 @@ +import React from 'react' +import PropTypes from 'prop-types' +import { useInput, useNotify, useTranslate } from 'react-admin' +import { Button, TextField } from '@material-ui/core' +import { FaKey } from 'react-icons/fa' +import { MdContentCopy, MdDelete, MdRefresh } from 'react-icons/md' +import { isWritable } from '../common/playlistUtils' +import { generateApiKey } from './apiKey' + +const identity = (v) => v +const MASK = '•'.repeat(26) + +const ApiKeyInput = ({ record, isCreate, fullWidth, className, ...props }) => { + const translate = useTranslate() + const notify = useNotify() + // Identity format/parse keep "" (revoke) distinct from undefined (untouched) + const { + input: { value, onChange }, + meta: { error, touched }, + } = useInput({ ...props, format: identity, parse: identity }) + + const isOwner = isCreate || record?.userId === localStorage.getItem('userId') + const pending = !!value + const revoking = value === '' && !!record?.hasApiKey + const saved = value == null && !!record?.hasApiKey + const hasKey = pending || saved + + const copy = () => { + const fallback = () => + prompt(translate('message.shareCopyToClipboard'), value) + if (navigator.clipboard && window.isSecureContext) { + navigator.clipboard + .writeText(value) + .then( + () => notify('resources.player.notifications.apiKeyCopied'), + fallback, + ) + } else { + fallback() + } + } + + const helperText = pending + ? 'resources.player.message.apiKeyPending' + : revoking + ? 'resources.player.message.apiKeyRevokePending' + : saved + ? 'resources.player.message.apiKeyActive' + : isOwner + ? 'resources.player.message.apiKeyNone' + : 'resources.player.message.apiKeyNoneOther' + + return ( +
+ +
+ {pending && ( + + )} + {isOwner && ( + + )} + {saved && isWritable(record?.userId) && ( + + )} +
+
+ ) +} + +ApiKeyInput.propTypes = { + source: PropTypes.string.isRequired, + record: PropTypes.object, + isCreate: PropTypes.bool, + fullWidth: PropTypes.bool, + className: PropTypes.string, + validate: PropTypes.oneOfType([PropTypes.func, PropTypes.array]), +} + +export default ApiKeyInput diff --git a/ui/src/player/ApiKeyInput.test.jsx b/ui/src/player/ApiKeyInput.test.jsx new file mode 100644 index 000000000..dd4269d6e --- /dev/null +++ b/ui/src/player/ApiKeyInput.test.jsx @@ -0,0 +1,172 @@ +import * as React from 'react' +import { render, screen, fireEvent, waitFor } from '@testing-library/react' +import { Form } from 'react-final-form' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import ApiKeyInput from './ApiKeyInput' + +const hooks = vi.hoisted(() => ({ notify: vi.fn() })) +const KEY = 'nds_0123456789abcdefghijkl' +const KEY_FORMAT = /^nds_[0-9A-Za-z]{22}$/ + +vi.mock('react-admin', async () => { + const actual = await vi.importActual('react-admin') + return { + ...actual, + useNotify: () => hooks.notify, + useTranslate: () => (key) => key, + } +}) + +const renderInput = ({ + record, + initialValues = {}, + isCreate = false, + fullWidth, +}) => { + let values + const utils = render( +
{}} + initialValues={initialValues} + render={({ values: v }) => { + values = v + return ( + + ) + }} + />, + ) + return { ...utils, values: () => values } +} + +const text = (key) => screen.queryByText(key) + +describe('ApiKeyInput', () => { + beforeEach(() => { + vi.clearAllMocks() + localStorage.setItem('userId', 'owner') + localStorage.setItem('role', 'regular') + }) + + it('shows a pending key with copy and regenerate on create', () => { + const { values } = renderInput({ + record: {}, + isCreate: true, + initialValues: { apiKey: KEY }, + }) + expect(screen.getByDisplayValue(KEY)).toBeInTheDocument() + expect(text('resources.player.message.apiKeyPending')).toBeInTheDocument() + expect( + screen.getByRole('button', { + name: 'resources.player.actions.copyApiKey', + }), + ).toBeInTheDocument() + expect( + text('resources.player.actions.revokeApiKey'), + ).not.toBeInTheDocument() + + fireEvent.click( + screen.getByText('resources.player.actions.regenerateApiKey'), + ) + expect(values().apiKey).toMatch(KEY_FORMAT) + expect(values().apiKey).not.toBe(KEY) + }) + + it('masks a saved key and lets the owner regenerate or revoke', () => { + const { values } = renderInput({ + record: { id: 'p1', userId: 'owner', hasApiKey: true }, + }) + expect(screen.queryByDisplayValue(/^nds_/)).not.toBeInTheDocument() + expect(text('resources.player.message.apiKeyActive')).toBeInTheDocument() + expect( + text('resources.player.actions.regenerateApiKey'), + ).toBeInTheDocument() + + fireEvent.click(screen.getByText('resources.player.actions.revokeApiKey')) + expect(values().apiKey).toBe('') + expect( + text('resources.player.message.apiKeyRevokePending'), + ).toBeInTheDocument() + expect(text('resources.player.actions.generateApiKey')).toBeInTheDocument() + }) + + it('lets the owner generate a key when there is none', () => { + const { values } = renderInput({ + record: { id: 'p1', userId: 'owner', hasApiKey: false }, + }) + expect(values().apiKey).toBeUndefined() + expect(text('resources.player.message.apiKeyNone')).toBeInTheDocument() + + fireEvent.click(screen.getByText('resources.player.actions.generateApiKey')) + expect(values().apiKey).toMatch(KEY_FORMAT) + expect(text('resources.player.message.apiKeyPending')).toBeInTheDocument() + }) + + it('lets an admin revoke but not set a key on another user player', () => { + localStorage.setItem('role', 'admin') + renderInput({ record: { id: 'p1', userId: 'someone', hasApiKey: true } }) + expect( + text('resources.player.actions.regenerateApiKey'), + ).not.toBeInTheDocument() + expect(text('resources.player.actions.revokeApiKey')).toBeInTheDocument() + }) + + it('falls back to a prompt when the clipboard write fails', async () => { + vi.stubGlobal('isSecureContext', true) + vi.stubGlobal('prompt', vi.fn()) + Object.defineProperty(navigator, 'clipboard', { + configurable: true, + value: { writeText: vi.fn().mockRejectedValue(new Error('denied')) }, + }) + try { + renderInput({ + record: {}, + isCreate: true, + initialValues: { apiKey: KEY }, + }) + fireEvent.click( + screen.getByRole('button', { + name: 'resources.player.actions.copyApiKey', + }), + ) + await waitFor(() => + expect(window.prompt).toHaveBeenCalledWith( + 'message.shareCopyToClipboard', + KEY, + ), + ) + expect(hooks.notify).not.toHaveBeenCalled() + } finally { + vi.unstubAllGlobals() + delete navigator.clipboard + } + }) + + it('shows a neutral message to an admin viewing another user player with no key', () => { + localStorage.setItem('role', 'admin') + renderInput({ record: { id: 'p1', userId: 'someone', hasApiKey: false } }) + expect(text('resources.player.message.apiKeyNoneOther')).toBeInTheDocument() + expect(text('resources.player.message.apiKeyNone')).not.toBeInTheDocument() + expect(screen.queryAllByRole('button')).toHaveLength(0) + }) + + it('shows no actions to another regular user', () => { + renderInput({ record: { id: 'p1', userId: 'someone', hasApiKey: true } }) + expect(screen.queryAllByRole('button')).toHaveLength(0) + }) + + it('is not full width unless asked', () => { + const record = { id: 'p1', userId: 'owner', hasApiKey: true } + const { container, unmount } = renderInput({ record }) + expect(container.querySelector('.MuiFormControl-fullWidth')).toBeNull() + unmount() + + const { container: wide } = renderInput({ record, fullWidth: true }) + expect(wide.querySelector('.MuiFormControl-fullWidth')).not.toBeNull() + }) +}) diff --git a/ui/src/player/PlayerCreate.jsx b/ui/src/player/PlayerCreate.jsx new file mode 100644 index 000000000..bf02cde1b --- /dev/null +++ b/ui/src/player/PlayerCreate.jsx @@ -0,0 +1,33 @@ +import React, { useMemo } from 'react' +import { Create, SimpleForm, required, useTranslate } from 'react-admin' +import { Title } from '../common' +import { playerInputs } from './playerInputs' +import ApiKeyInput from './ApiKeyInput' +import { generateApiKey } from './apiKey' + +const PlayerCreateTitle = () => { + const translate = useTranslate() + const resourceName = translate('resources.player.name', { smart_count: 1 }) + return ( + + ) +} + +const PlayerCreate = (props) => { + // Memoized so re-renders don't swap the key the user may have already copied + const initialValues = useMemo(() => ({ apiKey: generateApiKey() }), []) + return ( + <Create title={<PlayerCreateTitle />} {...props}> + <SimpleForm + variant="outlined" + redirect="list" + initialValues={initialValues} + > + {playerInputs()} + <ApiKeyInput source="apiKey" isCreate validate={required()} /> + </SimpleForm> + </Create> + ) +} + +export default PlayerCreate diff --git a/ui/src/player/PlayerCreate.test.jsx b/ui/src/player/PlayerCreate.test.jsx new file mode 100644 index 000000000..dc1f817db --- /dev/null +++ b/ui/src/player/PlayerCreate.test.jsx @@ -0,0 +1,47 @@ +import * as React from 'react' +import { render } from '@testing-library/react' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import PlayerCreate from './PlayerCreate' +import ApiKeyInput from './ApiKeyInput' + +const hooks = vi.hoisted(() => ({ forms: [] })) + +vi.mock('react-admin', async () => { + const actual = await vi.importActual('react-admin') + return { + ...actual, + Create: ({ children }) => children, + SimpleForm: (props) => { + hooks.forms.push(props) + return null + }, + } +}) + +describe('PlayerCreate', () => { + beforeEach(() => { + hooks.forms = [] + }) + + it('pre-fills one generated API key that survives re-renders', () => { + const { rerender } = render(<PlayerCreate resource="player" />) + rerender(<PlayerCreate resource="player" />) + + const [first, second] = hooks.forms.map((f) => f.initialValues) + expect(hooks.forms).toHaveLength(2) + expect(first.apiKey).toMatch(/^nds_[0-9A-Za-z]{22}$/) + expect(second).toBe(first) + }) + + it('requires the API key', () => { + render(<PlayerCreate resource="player" />) + + const input = React.Children.toArray(hooks.forms[0].children).find( + (child) => child.type === ApiKeyInput, + ) + expect(input.props.source).toBe('apiKey') + expect(input.props.isCreate).toBe(true) + expect(input.props.validate('')).toBeTruthy() + expect(input.props.validate('nds_0123456789abcdefghijkl')).toBeUndefined() + }) +}) diff --git a/ui/src/player/PlayerEdit.jsx b/ui/src/player/PlayerEdit.jsx index 1826500bd..d785eb04e 100644 --- a/ui/src/player/PlayerEdit.jsx +++ b/ui/src/player/PlayerEdit.jsx @@ -1,17 +1,16 @@ import { - TextInput, - BooleanInput, - TextField, Edit, - required, SimpleForm, - SelectInput, - ReferenceInput, useTranslate, + DeleteButton, + DeleteWithConfirmButton, + SaveButton, + Toolbar, } from 'react-admin' -import { Title } from '../common' -import config from '../config' -import { BITRATE_CHOICES } from '../consts' +import { makeStyles } from '@material-ui/core/styles' +import { ReadOnlyTextField, Title } from '../common' +import ApiKeyInput from './ApiKeyInput' +import { playerInputs } from './playerInputs' const PlayerTitle = ({ record }) => { const translate = useTranslate() @@ -19,24 +18,35 @@ const PlayerTitle = ({ record }) => { return <Title subTitle={`${resourceName} ${record ? record.name : ''}`} /> } +const useToolbarStyles = makeStyles({ + toolbar: { + display: 'flex', + justifyContent: 'space-between', + }, +}) + +const PlayerEditToolbar = (props) => ( + <Toolbar {...props} classes={useToolbarStyles()}> + <SaveButton /> + {props.record?.hasApiKey ? ( + <DeleteWithConfirmButton + mutationMode="pessimistic" + confirmTitle="resources.player.message.deleteWithKeyTitle" + confirmContent="resources.player.message.deleteWithKeyContent" + /> + ) : ( + <DeleteButton /> + )} + </Toolbar> +) + const PlayerEdit = (props) => ( - <Edit title={<PlayerTitle />} {...props}> - <SimpleForm variant={'outlined'}> - <TextInput source="name" validate={[required()]} /> - <ReferenceInput - source="transcodingId" - reference="transcoding" - sort={{ field: 'name', order: 'ASC' }} - > - <SelectInput source="name" resettable /> - </ReferenceInput> - <SelectInput source="maxBitRate" resettable choices={BITRATE_CHOICES} /> - <BooleanInput source="reportRealPath" fullWidth /> - {(config.lastFMEnabled || config.listenBrainzEnabled) && ( - <BooleanInput source="scrobbleEnabled" fullWidth /> - )} - <TextField source="client" /> - <TextField source="userName" /> + <Edit title={<PlayerTitle />} mutationMode="pessimistic" {...props}> + <SimpleForm variant={'outlined'} toolbar={<PlayerEditToolbar />}> + {playerInputs()} + <ReadOnlyTextField source="client" /> + <ReadOnlyTextField source="userName" /> + <ApiKeyInput source="apiKey" /> </SimpleForm> </Edit> ) diff --git a/ui/src/player/PlayerEdit.test.jsx b/ui/src/player/PlayerEdit.test.jsx new file mode 100644 index 000000000..38fa2cc8e --- /dev/null +++ b/ui/src/player/PlayerEdit.test.jsx @@ -0,0 +1,25 @@ +import * as React from 'react' +import { render } from '@testing-library/react' +import { describe, it, expect, vi } from 'vitest' +import PlayerEdit from './PlayerEdit' + +const hooks = vi.hoisted(() => ({ editProps: null })) + +vi.mock('react-admin', async () => { + const actual = await vi.importActual('react-admin') + return { + ...actual, + Edit: (props) => { + hooks.editProps = props + return null + }, + } +}) + +describe('PlayerEdit', () => { + // An optimistic or undoable save would put the new key in react-admin's cache + it('saves pessimistically', () => { + render(<PlayerEdit resource="player" id="p1" />) + expect(hooks.editProps.mutationMode).toBe('pessimistic') + }) +}) diff --git a/ui/src/player/PlayerList.jsx b/ui/src/player/PlayerList.jsx index a2b009bad..c7bd32570 100644 --- a/ui/src/player/PlayerList.jsx +++ b/ui/src/player/PlayerList.jsx @@ -2,18 +2,20 @@ import React from 'react' import { Datagrid, TextField, - DateField, FunctionField, ReferenceField, Filter, SearchInput, + NullableBooleanInput, } from 'react-admin' import { useMediaQuery } from '@material-ui/core' -import { SimpleList, List } from '../common' +import { FaKey } from 'react-icons/fa' +import { SimpleList, List, DateField } from '../common' const PlayerFilter = (props) => ( <Filter {...props} variant={'outlined'}> <SearchInput id="search" source="name" alwaysOn /> + <NullableBooleanInput source="hasApiKey" alwaysOn /> </Filter> ) @@ -30,7 +32,11 @@ const PlayerList = ({ permissions, ...props }) => { <SimpleList primaryText={(r) => r.name} secondaryText={(r) => r.userName} - tertiaryText={(r) => (r.maxBitRate ? r.maxBitRate : '-')} + tertiaryText={(r) => ( + <> + {r.hasApiKey && <FaKey />} {r.maxBitRate ? r.maxBitRate : '-'} + </> + )} /> ) : ( <Datagrid rowClick="edit"> @@ -43,6 +49,11 @@ const PlayerList = ({ permissions, ...props }) => { source="maxBitRate" render={(r) => (r.maxBitRate ? r.maxBitRate : '-')} /> + <FunctionField + source="hasApiKey" + sortable={false} + render={(r) => (r.hasApiKey ? <FaKey /> : null)} + /> <DateField source="lastSeen" showTime sortByOrder={'DESC'} /> </Datagrid> )} diff --git a/ui/src/player/apiKey.js b/ui/src/player/apiKey.js new file mode 100644 index 000000000..27d683052 --- /dev/null +++ b/ui/src/player/apiKey.js @@ -0,0 +1,15 @@ +const API_KEY_PREFIX = 'nds_' +const ALPHABET = + '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz' +const KEY_LENGTH = 22 + +// Bytes >= 248 are dropped so every character is equally likely (248 = 4 * 62). +export const generateApiKey = () => { + let key = '' + while (key.length < KEY_LENGTH) { + for (const b of window.crypto.getRandomValues(new Uint8Array(32))) { + if (b < 248 && key.length < KEY_LENGTH) key += ALPHABET[b % 62] + } + } + return API_KEY_PREFIX + key +} diff --git a/ui/src/player/apiKey.test.js b/ui/src/player/apiKey.test.js new file mode 100644 index 000000000..4c41871e7 --- /dev/null +++ b/ui/src/player/apiKey.test.js @@ -0,0 +1,15 @@ +import { describe, it, expect } from 'vitest' +import { generateApiKey } from './apiKey' + +describe('generateApiKey', () => { + it('returns the prefix plus 22 base62 characters', () => { + for (let i = 0; i < 50; i++) { + expect(generateApiKey()).toMatch(/^nds_[0-9A-Za-z]{22}$/) + } + }) + + it('returns a different key each time', () => { + const keys = new Set(Array.from({ length: 100 }, generateApiKey)) + expect(keys.size).toBe(100) + }) +}) diff --git a/ui/src/player/index.js b/ui/src/player/index.js index aaa3d58d7..c28cfa414 100644 --- a/ui/src/player/index.js +++ b/ui/src/player/index.js @@ -1,9 +1,11 @@ import { BsFillMusicPlayerFill } from 'react-icons/bs' import PlayerList from './PlayerList' import PlayerEdit from './PlayerEdit' +import PlayerCreate from './PlayerCreate' export default { list: PlayerList, edit: PlayerEdit, + create: PlayerCreate, icon: BsFillMusicPlayerFill, } diff --git a/ui/src/player/playerInputs.jsx b/ui/src/player/playerInputs.jsx new file mode 100644 index 000000000..a8b719a81 --- /dev/null +++ b/ui/src/player/playerInputs.jsx @@ -0,0 +1,34 @@ +import React from 'react' +import { + BooleanInput, + ReferenceInput, + SelectInput, + TextInput, + required, +} from 'react-admin' +import config from '../config' +import { BITRATE_CHOICES } from '../consts' + +// Returned as an array, not a component, so SimpleForm still injects its props into each input. +export const playerInputs = () => + [ + <TextInput key="name" source="name" validate={[required()]} />, + <ReferenceInput + key="transcodingId" + source="transcodingId" + reference="transcoding" + sort={{ field: 'name', order: 'ASC' }} + > + <SelectInput source="name" resettable /> + </ReferenceInput>, + <SelectInput + key="maxBitRate" + source="maxBitRate" + resettable + choices={BITRATE_CHOICES} + />, + <BooleanInput key="reportRealPath" source="reportRealPath" fullWidth />, + (config.lastFMEnabled || config.listenBrainzEnabled) && ( + <BooleanInput key="scrobbleEnabled" source="scrobbleEnabled" fullWidth /> + ), + ].filter(Boolean) diff --git a/ui/src/playlist/PlaylistEdit.jsx b/ui/src/playlist/PlaylistEdit.jsx index f6882e366..c7594fa13 100644 --- a/ui/src/playlist/PlaylistEdit.jsx +++ b/ui/src/playlist/PlaylistEdit.jsx @@ -3,7 +3,6 @@ import { FormDataConsumer, SimpleForm, TextInput, - TextField, BooleanInput, required, useTranslate, @@ -11,13 +10,14 @@ import { ReferenceInput, SelectInput, } from 'react-admin' -import { isWritable, Title } from '../common' +import { isWritable, ReadOnlyTextField, Title } from '../common' const SyncFragment = ({ formData, variant, ...rest }) => { + if (!formData.path) return null return ( <> - {formData.path && <BooleanInput source="sync" {...rest} />} - {formData.path && <TextField source="path" {...rest} />} + <BooleanInput source="sync" {...rest} /> + <ReadOnlyTextField source="path" {...rest} /> </> ) } @@ -56,10 +56,10 @@ const PlaylistEditForm = (props) => { /> </ReferenceInput> ) : ( - <TextField source="ownerName" /> + <ReadOnlyTextField source="ownerName" /> )} <BooleanInput source="public" disabled={!isWritable(record.ownerId)} /> - <FormDataConsumer> + <FormDataConsumer fullWidth> {(formDataProps) => <SyncFragment {...formDataProps} />} </FormDataConsumer> </SimpleForm> diff --git a/ui/src/playlist/PlaylistList.jsx b/ui/src/playlist/PlaylistList.jsx index d2b17b108..e1695e980 100644 --- a/ui/src/playlist/PlaylistList.jsx +++ b/ui/src/playlist/PlaylistList.jsx @@ -67,15 +67,15 @@ const PlaylistFilter = (props) => { ) } -const TogglePublicInput = ({ resource, source }) => { +export const ToggleField = ({ resource, source }) => { const record = useRecordContext() const notify = useNotify() - const [togglePublic] = useUpdate( + const [toggle] = useUpdate( resource, - record.id, + record?.id, { ...record, - public: !record.public, + [source]: !record?.[source], }, { undoable: false, @@ -86,48 +86,25 @@ const TogglePublicInput = ({ resource, source }) => { ) const handleClick = (e) => { - togglePublic() + toggle() e.stopPropagation() } + if (!record) return null + return ( <Switch checked={record[source]} + color="primary" onClick={handleClick} disabled={!isWritable(record.ownerId)} /> ) } -const ToggleAutoImport = ({ resource, source }) => { +export const ToggleAutoImport = (props) => { const record = useRecordContext() - const notify = useNotify() - const [ToggleAutoImport] = useUpdate( - resource, - record.id, - { - ...record, - sync: !record.sync, - }, - { - undoable: false, - onFailure: (error) => { - notify('ra.page.error', 'warning') - }, - }, - ) - const handleClick = (e) => { - ToggleAutoImport() - e.stopPropagation() - } - - return record.path ? ( - <Switch - checked={record[source]} - onClick={handleClick} - disabled={!isWritable(record.ownerId)} - /> - ) : null + return record?.path ? <ToggleField {...props} /> : null } const PlaylistListBulkActions = (props) => { @@ -169,9 +146,7 @@ const PlaylistList = (props) => { updatedAt: isDesktop && ( <DateField source="updatedAt" sortByOrder={'DESC'} /> ), - public: !isXsmall && ( - <TogglePublicInput source="public" sortByOrder={'DESC'} /> - ), + public: !isXsmall && <ToggleField source="public" sortByOrder={'DESC'} />, comment: <TextField source="comment" />, sync: !isXsmall && ( <ToggleAutoImport source="sync" sortByOrder={'DESC'} /> diff --git a/ui/src/playlist/PlaylistList.test.jsx b/ui/src/playlist/PlaylistList.test.jsx index 4fbc6d516..c05833166 100644 --- a/ui/src/playlist/PlaylistList.test.jsx +++ b/ui/src/playlist/PlaylistList.test.jsx @@ -1,7 +1,9 @@ import React from 'react' import { render, screen } from '@testing-library/react' import { describe, it, expect, vi } from 'vitest' -import { PlaylistLove } from './PlaylistList' +import { TestContext } from 'ra-test' +import { RecordContextProvider } from 'react-admin' +import { PlaylistLove, ToggleField, ToggleAutoImport } from './PlaylistList' vi.mock('../config', () => ({ default: { enableFavourites: true }, @@ -13,6 +15,7 @@ vi.mock('../common', () => ({ {record?.starred ? 'starred' : 'not-starred'} </button> ), + isWritable: (ownerId) => ownerId === 'me', })) describe('<PlaylistLove />', () => { @@ -32,3 +35,50 @@ describe('<PlaylistLove />', () => { }) }) }) + +// react-admin evicts records older than 10 minutes while the list still holds +// their ids, so rows can render with no record. +describe('playlist toggles without a record', () => { + it('<ToggleField /> renders nothing', () => { + const { container } = render( + <TestContext> + <ToggleField resource="playlist" source="public" /> + </TestContext>, + ) + expect(container.innerHTML).toBe('') + }) + + it('<ToggleAutoImport /> renders nothing', () => { + const { container } = render( + <TestContext> + <ToggleAutoImport resource="playlist" source="sync" /> + </TestContext>, + ) + expect(container.innerHTML).toBe('') + }) +}) + +// Secondary is a surface color in many themes, so these toggles must use primary +describe('<ToggleField />', () => { + const renderToggle = (record) => + render( + <TestContext> + <RecordContextProvider value={record}> + <ToggleField resource="playlist" source="public" /> + </RecordContextProvider> + </TestContext>, + ) + + it.each([ + ['owner', 'me', false], + ['non-owner', 'someone-else', true], + ])('renders a primary-colored switch for the %s', (_, ownerId, disabled) => { + renderToggle({ id: 'pl-1', public: true, ownerId }) + const input = screen.getByRole('checkbox') + const switchBase = input.closest('.MuiSwitch-switchBase') + expect(input.checked).toBe(true) + expect(input.disabled).toBe(disabled) + expect(switchBase.classList).toContain('MuiSwitch-colorPrimary') + expect(switchBase.classList).not.toContain('MuiSwitch-colorSecondary') + }) +}) diff --git a/ui/src/radio/RadioEdit.jsx b/ui/src/radio/RadioEdit.jsx index bbe001e6f..af879deaa 100644 --- a/ui/src/radio/RadioEdit.jsx +++ b/ui/src/radio/RadioEdit.jsx @@ -1,5 +1,4 @@ import { - DateField, Edit, required, SimpleForm, @@ -9,7 +8,12 @@ import { import { CardMedia } from '@material-ui/core' import { makeStyles } from '@material-ui/core/styles' import { urlValidate } from '../utils/validations' -import { Title, ImageUploadOverlay, useImageLoadingState } from '../common' +import { + Title, + ImageUploadOverlay, + ReadOnlyDateField, + useImageLoadingState, +} from '../common' import subsonic from '../subsonic' import config from '../config' import { RADIO_PLACEHOLDER_IMAGE } from '../consts' @@ -65,8 +69,8 @@ const RadioEdit = (props) => { fullWidth validate={[urlValidate]} /> - <DateField variant="body1" source="updatedAt" showTime /> - <DateField variant="body1" source="createdAt" showTime /> + <ReadOnlyDateField source="updatedAt" /> + <ReadOnlyDateField source="createdAt" /> </SimpleForm> </Edit> ) diff --git a/ui/src/share/ShareEdit.jsx b/ui/src/share/ShareEdit.jsx index 2cf7f2df7..a222d3369 100644 --- a/ui/src/share/ShareEdit.jsx +++ b/ui/src/share/ShareEdit.jsx @@ -2,13 +2,16 @@ import { DateTimeInput, BooleanInput, Edit, - NumberField, SimpleForm, TextInput, } from 'react-admin' import { sharePlayerUrl } from '../utils' import { Link } from '@material-ui/core' -import { DateField } from '../common' +import { + ReadOnlyDateField, + ReadOnlyNumberField, + ReadOnlyTextField, +} from '../common' import config from '../config' export const ShareEdit = (props) => { @@ -16,20 +19,26 @@ export const ShareEdit = (props) => { const url = sharePlayerUrl(id) return ( <Edit {...props}> - <SimpleForm {...rest}> - <Link source="URL" href={url} target="_blank" rel="noopener noreferrer"> + <SimpleForm variant={'outlined'} {...rest}> + <Link + source="URL" + href={url} + target="_blank" + rel="noopener noreferrer" + variant="inherit" + > {url} </Link> <TextInput source="description" /> {config.enableDownloads && <BooleanInput source="downloadable" />} <DateTimeInput source="expiresAt" /> - <TextInput source="contents" disabled /> - <TextInput source="format" disabled /> - <TextInput source="maxBitRate" disabled /> - <TextInput source="username" disabled /> - <NumberField source="visitCount" disabled /> - <DateField source="lastVisitedAt" disabled showTime /> - <DateField source="createdAt" disabled showTime /> + <ReadOnlyTextField source="contents" /> + <ReadOnlyTextField source="format" /> + <ReadOnlyTextField source="maxBitRate" /> + <ReadOnlyTextField source="username" /> + <ReadOnlyNumberField source="visitCount" /> + <ReadOnlyDateField source="lastVisitedAt" /> + <ReadOnlyDateField source="createdAt" /> </SimpleForm> </Edit> ) diff --git a/ui/src/themes/amusic.js b/ui/src/themes/amusic.js index 55205baf3..eef7f90d6 100644 --- a/ui/src/themes/amusic.js +++ b/ui/src/themes/amusic.js @@ -79,6 +79,16 @@ export default { color: '#eee', backgroundColor: '#ff4e6b', }, + containedPrimary: { + color: '#fff', + backgroundColor: '#D60017', + '&:hover': { + backgroundColor: '#a30011', + '@media (hover: none)': { + backgroundColor: '#D60017', + }, + }, + }, textSizeSmall: { fontSize: '0.8rem', paddingRight: '0.5rem', @@ -192,6 +202,11 @@ export default { paddingBottom: '1rem', }, }, + NDNotification: { + undo: { + color: '#fff', + }, + }, RaConfirm: { confirmPrimary: { color: '#fff', diff --git a/ui/src/themes/dracula.js b/ui/src/themes/dracula.js index 2e4ae38e5..45559c3af 100644 --- a/ui/src/themes/dracula.js +++ b/ui/src/themes/dracula.js @@ -185,16 +185,6 @@ export default { color: `${foreground} !important`, }, }, - MuiSwitch: { - colorSecondary: { - '&$checked': { - color: green, - }, - '&$checked + $track': { - backgroundColor: green, - }, - }, - }, NDAlbumGridView: { albumName: { marginTop: '0.5rem', diff --git a/ui/src/themes/gruvboxDark.js b/ui/src/themes/gruvboxDark.js index 0f4cbd7c4..3e2955dcd 100644 --- a/ui/src/themes/gruvboxDark.js +++ b/ui/src/themes/gruvboxDark.js @@ -121,16 +121,6 @@ export default { boxShadow: '3px 3px 5px #3c3836', }, }, - MuiSwitch: { - colorSecondary: { - '&$checked': { - color: '#458588', - }, - '&$checked + $track': { - backgroundColor: '#458588', - }, - }, - }, NDMobileArtistDetails: { bgContainer: { background: diff --git a/ui/src/themes/nuclear.js b/ui/src/themes/nuclear.js index b34896c97..bf804139e 100644 --- a/ui/src/themes/nuclear.js +++ b/ui/src/themes/nuclear.js @@ -86,6 +86,13 @@ export default { }, }, }, + NDNotification: { + undo: { + '& .MuiButton-label': { + color: 'inherit', + }, + }, + }, MuiChip: { root: { backgroundColor: nukeCol['accent'], diff --git a/ui/src/themes/tokyoNight.js b/ui/src/themes/tokyoNight.js index 07d372a6b..9f6424b77 100644 --- a/ui/src/themes/tokyoNight.js +++ b/ui/src/themes/tokyoNight.js @@ -184,16 +184,6 @@ export default { color: `${foreground} !important`, }, }, - MuiSwitch: { - colorSecondary: { - '&$checked': { - color: blue, - }, - '&$checked + $track': { - backgroundColor: blue, - }, - }, - }, NDAlbumGridView: { albumName: { marginTop: '0.5rem', diff --git a/ui/src/themes/tokyoNightLight.js b/ui/src/themes/tokyoNightLight.js index f84cd0be9..a61c0fe87 100644 --- a/ui/src/themes/tokyoNightLight.js +++ b/ui/src/themes/tokyoNightLight.js @@ -184,16 +184,6 @@ export default { color: `${foreground} !important`, }, }, - MuiSwitch: { - colorSecondary: { - '&$checked': { - color: blue, - }, - '&$checked + $track': { - backgroundColor: blue, - }, - }, - }, NDAlbumGridView: { albumName: { marginTop: '0.5rem', diff --git a/ui/src/themes/useCurrentTheme.js b/ui/src/themes/useCurrentTheme.js index 4ccefe820..fbb5e9bc8 100644 --- a/ui/src/themes/useCurrentTheme.js +++ b/ui/src/themes/useCurrentTheme.js @@ -63,6 +63,8 @@ const useCurrentTheme = () => { ...theme.props, MuiUseMediaQuery: { noSsr: true }, MuiPopover: { disableScrollLock: true }, + // MUI defaults to secondary, which many themes use as a surface color + MuiSwitch: { color: 'primary' }, }, }), [theme], diff --git a/ui/src/themes/useCurrentTheme.test.jsx b/ui/src/themes/useCurrentTheme.test.jsx index 65c3be8c6..6553d9866 100644 --- a/ui/src/themes/useCurrentTheme.test.jsx +++ b/ui/src/themes/useCurrentTheme.test.jsx @@ -3,6 +3,10 @@ import { Provider } from 'react-redux' import { createStore } from 'redux' import mediaQuery from 'css-mediaquery' import { renderHook } from '@testing-library/react-hooks' +import { render, screen } from '@testing-library/react' +import { createMuiTheme, ThemeProvider } from '@material-ui/core/styles' +import Switch from '@material-ui/core/Switch' +import themes from './index' import useCurrentTheme from './useCurrentTheme' import { themeReducer } from '../reducers/themeReducer' import { AUTO_THEME_ID } from '../consts' @@ -161,4 +165,27 @@ describe('useCurrentTheme', () => { expect(document.body.style.backgroundColor).toBe('rgb(18, 18, 18)') }) }) + describe('switch color', () => { + it.each(Object.keys(themes))( + 'renders switches with the primary color in %s', + (theme) => { + const { result } = renderHook(() => useCurrentTheme(), { + wrapper: ({ children }) => ( + <Provider store={createStore(themeReducer, { theme })}> + {children} + </Provider> + ), + }) + render( + <ThemeProvider theme={createMuiTheme(result.current)}> + <Switch checked onChange={() => {}} /> + </ThemeProvider>, + ) + const switchBase = screen + .getByRole('checkbox') + .closest('.MuiSwitch-switchBase') + expect(switchBase.classList).toContain('MuiSwitch-colorPrimary') + }, + ) + }) }) diff --git a/ui/src/user/UserEdit.jsx b/ui/src/user/UserEdit.jsx index c5d9c75a4..feadafff1 100644 --- a/ui/src/user/UserEdit.jsx +++ b/ui/src/user/UserEdit.jsx @@ -3,7 +3,6 @@ import { makeStyles } from '@material-ui/core/styles' import { TextInput, BooleanInput, - DateField, PasswordInput, Edit, required, @@ -21,7 +20,7 @@ import { useRecordContext, } from 'react-admin' import { Typography } from '@material-ui/core' -import { Title } from '../common' +import { ReadOnlyDateField, Title } from '../common' import DeleteUserButton from './DeleteUserButton' import { LibrarySelectionField } from './LibrarySelectionField.jsx' import { validateUserForm } from './userValidation' @@ -183,10 +182,10 @@ const UserEdit = (props) => { helperText={translate('resources.user.helperTexts.scrobbleFilter')} /> - <DateField variant="body1" source="lastLoginAt" showTime /> - <DateField variant="body1" source="lastAccessAt" showTime /> - <DateField variant="body1" source="updatedAt" showTime /> - <DateField variant="body1" source="createdAt" showTime /> + <ReadOnlyDateField source="lastLoginAt" /> + <ReadOnlyDateField source="lastAccessAt" /> + <ReadOnlyDateField source="updatedAt" /> + <ReadOnlyDateField source="createdAt" /> </SimpleForm> </Edit> ) diff --git a/ui/src/user/UserEdit.test.jsx b/ui/src/user/UserEdit.test.jsx index 74405cc13..837b25ad6 100644 --- a/ui/src/user/UserEdit.test.jsx +++ b/ui/src/user/UserEdit.test.jsx @@ -51,9 +51,6 @@ vi.mock('react-admin', () => ({ BooleanInput: ({ source }) => ( <input type="checkbox" data-testid={`boolean-input-${source}`} /> ), - DateField: ({ source }) => ( - <div data-testid={`date-field-${source}`}>Date</div> - ), PasswordInput: ({ source }) => ( <input type="password" data-testid={`password-input-${source}`} /> ), @@ -82,6 +79,9 @@ vi.mock('./DeleteUserButton', () => ({ vi.mock('../common', () => ({ Title: ({ subTitle }) => <div data-testid="title">{subTitle}</div>, + ReadOnlyDateField: ({ source }) => ( + <div data-testid={`date-field-${source}`}>Date</div> + ), })) // Mock Material-UI