diff --git a/model/artist.go b/model/artist.go index 7f68f9787..f6a92a313 100644 --- a/model/artist.go +++ b/model/artist.go @@ -78,7 +78,7 @@ type ArtistRepository interface { UpdateExternalInfo(a *Artist) error Get(id string) (*Artist, error) GetAll(options ...QueryOptions) (Artists, error) - GetIndex(includeMissing bool, roles ...Role) (ArtistIndexes, error) + GetIndex(includeMissing bool, libraryId int, roles ...Role) (ArtistIndexes, error) // The following methods are used exclusively by the scanner: RefreshPlayCounts() (int64, error) diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index 65b287e4c..45e696dc2 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -146,7 +146,7 @@ func roleFilter(_ string, role any) Sqlizer { // artistLibraryFilter handles filtering artists by library_id through the library_artist junction table func artistLibraryFilter(_ string, value any) Sqlizer { - // Use a subquery with Eq to properly handle array parameters + // Use a subquery to find artists that belong to the specified library subquery := Select("1").From("library_artist la").Where(And{ Expr("la.artist_id = artist.id"), Eq{"la.library_id": value}, @@ -225,7 +225,7 @@ func (r *artistRepository) getIndexKey(a model.Artist) string { } // TODO Cache the index (recalculate when there are changes to the DB) -func (r *artistRepository) GetIndex(includeMissing bool, roles ...model.Role) (model.ArtistIndexes, error) { +func (r *artistRepository) GetIndex(includeMissing bool, libraryId int, roles ...model.Role) (model.ArtistIndexes, error) { options := model.QueryOptions{Sort: "name"} if len(roles) > 0 { roleFilters := slice.Map(roles, func(r model.Role) Sqlizer { @@ -240,10 +240,37 @@ func (r *artistRepository) GetIndex(includeMissing bool, roles ...model.Role) (m options.Filters = And{options.Filters, Eq{"artist.missing": false}} } } - artists, err := r.GetAll(options) + + // Get artists based on library filtering + var artists model.Artists + var err error + + var libFilter Sqlizer + + if libraryId > 0 { + libFilter = artistLibraryFilter("library_id", libraryId) + } else { + // All accessible libraries - use regular GetAll for backward compatibility + userRepo := NewUserRepository(r.ctx, r.db) + libs, err := userRepo.GetUserLibraries(userId(r.ctx)) + if err != nil { + return nil, fmt.Errorf("getting user libraries: %w", err) + } + libIds := slice.Map(libs, func(lib model.Library) int { return lib.ID }) + libFilter = artistLibraryFilter("library_id", libIds) + } + + if options.Filters == nil { + options.Filters = libFilter + } else { + options.Filters = And{options.Filters, libFilter} + } + + artists, err = r.GetAll(options) if err != nil { return nil, err } + var result model.ArtistIndexes for k, v := range slice.Group(artists, r.getIndexKey) { result = append(result, model.ArtistIndex{ID: k, Artists: v}) diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index 2b854b204..c9062a950 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -3,6 +3,7 @@ package persistence import ( "context" "encoding/json" + "strings" "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" @@ -95,7 +96,7 @@ var _ = Describe("ArtistRepository", func() { er := repo.Put(&artistBeatles) Expect(er).To(BeNil()) - idx, err := repo.GetIndex(false) + idx, err := repo.GetIndex(false, 0) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(2)) Expect(idx[0].ID).To(Equal("F")) @@ -113,7 +114,7 @@ var _ = Describe("ArtistRepository", func() { // BFR Empty SortArtistName is not saved in the DB anymore XIt("returns the index when PreferSortTags is true and SortArtistName is empty", func() { - idx, err := repo.GetIndex(false) + idx, err := repo.GetIndex(false, 0) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(2)) Expect(idx[0].ID).To(Equal("B")) @@ -135,7 +136,7 @@ var _ = Describe("ArtistRepository", func() { er := repo.Put(&artistBeatles) Expect(er).To(BeNil()) - idx, err := repo.GetIndex(false) + idx, err := repo.GetIndex(false, 0) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(2)) Expect(idx[0].ID).To(Equal("B")) @@ -152,7 +153,7 @@ var _ = Describe("ArtistRepository", func() { }) It("returns the index when SortArtistName is empty", func() { - idx, err := repo.GetIndex(false) + idx, err := repo.GetIndex(false, 0) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(2)) Expect(idx[0].ID).To(Equal("B")) @@ -189,7 +190,7 @@ var _ = Describe("ArtistRepository", func() { }) It("returns only artists with the specified role", func() { - idx, err := repo.GetIndex(false, model.RoleComposer) + idx, err := repo.GetIndex(false, 0, model.RoleComposer) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(1)) Expect(idx[0].ID).To(Equal("B")) @@ -198,7 +199,7 @@ var _ = Describe("ArtistRepository", func() { }) It("returns artists with any of the specified roles", func() { - idx, err := repo.GetIndex(false, model.RoleComposer, model.RoleProducer) + idx, err := repo.GetIndex(false, 0, model.RoleComposer, model.RoleProducer) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(2)) @@ -219,7 +220,7 @@ var _ = Describe("ArtistRepository", func() { }) It("returns empty index when no artists have the specified role", func() { - idx, err := repo.GetIndex(false, model.RoleDirector) + idx, err := repo.GetIndex(false, 0, model.RoleDirector) Expect(err).ToNot(HaveOccurred()) Expect(idx).To(HaveLen(0)) }) @@ -306,6 +307,45 @@ var _ = Describe("ArtistRepository", func() { raw = repo.(*artistRepository) _, err := raw.executeSQL(squirrel.Update(raw.tableName).Set("missing", true).Where(squirrel.Eq{"id": missing.ID})) Expect(err).ToNot(HaveOccurred()) + + // Add missing artist to library 1 so it can be found by library filtering + lr := NewLibraryRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) + err = lr.AddArtist(1, missing.ID) + Expect(err).ToNot(HaveOccurred()) + + // Ensure the test user exists and has library access + ur := NewUserRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) + currentUser, ok := request.UserFrom(repo.(*artistRepository).ctx) + if ok { + // Create the user if it doesn't exist with default values if missing + testUser := model.User{ + ID: currentUser.ID, + UserName: currentUser.UserName, + Name: currentUser.Name, + Email: currentUser.Email, + IsAdmin: currentUser.IsAdmin, + } + // Provide defaults for missing fields + if testUser.UserName == "" { + testUser.UserName = testUser.ID + } + if testUser.Name == "" { + testUser.Name = testUser.ID + } + if testUser.Email == "" { + testUser.Email = testUser.ID + "@test.com" + } + + // Try to put the user (will fail silently if already exists) + _ = ur.Put(&testUser) + + // Add library association + err = ur.AddUserLibrary(currentUser.ID, 1) + // Ignore error if user-library association already exists + if err != nil && !strings.Contains(err.Error(), "UNIQUE constraint failed") && !strings.Contains(err.Error(), "duplicate key") { + Expect(err).ToNot(HaveOccurred()) + } + } } removeMissing := func() { @@ -337,7 +377,7 @@ var _ = Describe("ArtistRepository", func() { }) It("does not return missing artist in GetIndex", func() { - idx, err := repo.GetIndex(false) + idx, err := repo.GetIndex(false, 0) Expect(err).ToNot(HaveOccurred()) // Only 2 artists should be present total := 0 @@ -371,7 +411,7 @@ var _ = Describe("ArtistRepository", func() { }) It("returns missing artist in GetIndex when included", func() { - idx, err := repo.GetIndex(true) + idx, err := repo.GetIndex(true, 0) Expect(err).ToNot(HaveOccurred()) total := 0 for _, ix := range idx { diff --git a/persistence/persistence_suite_test.go b/persistence/persistence_suite_test.go index d13b5d9c0..9ac2a0907 100644 --- a/persistence/persistence_suite_test.go +++ b/persistence/persistence_suite_test.go @@ -141,6 +141,14 @@ var _ = BeforeSuite(func() { } } + // Associate users with library 1 (default test library) + for i := range testUsers { + err := ur.AddUserLibrary(testUsers[i].ID, 1) + if err != nil { + panic(err) + } + } + //gr := NewGenreRepository(ctx, conn) //for i := range testGenres { // g := testGenres[i] @@ -168,6 +176,15 @@ var _ = BeforeSuite(func() { } } + // Associate artists with library 1 (default test library) + lr := NewLibraryRepository(ctx, conn) + for i := range testArtists { + err := lr.AddArtist(1, testArtists[i].ID) + if err != nil { + panic(err) + } + } + mr := NewMediaFileRepository(ctx, conn) for i := range testSongs { err := mr.Put(&testSongs[i]) diff --git a/server/subsonic/browsing.go b/server/subsonic/browsing.go index ba96a926b..e234c52a6 100644 --- a/server/subsonic/browsing.go +++ b/server/subsonic/browsing.go @@ -7,6 +7,7 @@ import ( "time" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/server/public" @@ -34,22 +35,31 @@ func (api *Router) GetMusicFolders(r *http.Request) (*responses.Subsonic, error) func (api *Router) getArtist(r *http.Request, libId int, ifModifiedSince time.Time) (model.ArtistIndexes, int64, error) { ctx := r.Context() - lib, err := api.ds.Library(ctx).Get(libId) + + lastScanStr, err := api.ds.Property(ctx).DefaultGet(consts.LastScanStartTimeKey, "") if err != nil { - log.Error(ctx, "Error retrieving Library", "id", libId, err) + log.Error(ctx, "Error retrieving last scan start time", err) return nil, 0, err } + lastScan := time.Now() + if lastScanStr != "" { + lastScan, err = time.Parse(time.RFC3339, lastScanStr) + } var indexes model.ArtistIndexes - if lib.LastScanAt.After(ifModifiedSince) { - indexes, err = api.ds.Artist(ctx).GetIndex(false, model.RoleAlbumArtist) + if lastScan.After(ifModifiedSince) { + indexes, err = api.ds.Artist(ctx).GetIndex(false, libId, model.RoleAlbumArtist) if err != nil { log.Error(ctx, "Error retrieving Indexes", err) return nil, 0, err } + if len(indexes) == 0 { + log.Debug(ctx, "No artists found in library", "libId", libId) + return nil, 0, newError(responses.ErrorDataNotFound, "Library not found or empty") + } } - return indexes, lib.LastScanAt.UnixMilli(), err + return indexes, lastScan.UnixMilli(), err } func (api *Router) getArtistIndex(r *http.Request, libId int, ifModifiedSince time.Time) (*responses.Indexes, error) { @@ -92,7 +102,7 @@ func (api *Router) getArtistIndexID3(r *http.Request, libId int, ifModifiedSince func (api *Router) GetIndexes(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) - musicFolderId := p.IntOr("musicFolderId", 1) + musicFolderId := p.IntOr("musicFolderId", 0) // 0 means all accessible libraries ifModifiedSince := p.TimeOr("ifModifiedSince", time.Time{}) res, err := api.getArtistIndex(r, musicFolderId, ifModifiedSince) @@ -107,7 +117,8 @@ func (api *Router) GetIndexes(r *http.Request) (*responses.Subsonic, error) { func (api *Router) GetArtists(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) - musicFolderId := p.IntOr("musicFolderId", 1) + musicFolderId := p.IntOr("musicFolderId", 0) // 0 means all accessible libraries + res, err := api.getArtistIndexID3(r, musicFolderId, time.Time{}) if err != nil { return nil, err diff --git a/server/subsonic/browsing_test.go b/server/subsonic/browsing_test.go index aba9981bb..a9ce0a5cc 100644 --- a/server/subsonic/browsing_test.go +++ b/server/subsonic/browsing_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http/httptest" + "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" @@ -18,6 +19,7 @@ var _ = Describe("Browsing", func() { BeforeEach(func() { ds = &tests.MockDataStore{} + auth.Init(ds) api = &Router{ds: ds} ctx = context.Background() }) @@ -52,4 +54,134 @@ var _ = Describe("Browsing", func() { Expect(response.MusicFolders.Folders[2].Name).To(Equal("Test Library 3")) }) }) + + Describe("GetIndexes", func() { + It("should validate user access to the specified musicFolderId", func() { + // Create mock user + user := model.User{ + ID: "user-id", + } + ctx = request.WithUser(ctx, user) + + // Setup mock expectations - user only has access to library 1 + mockUserRepo := ds.User(ctx).(*tests.MockedUserRepo) + err := mockUserRepo.SetUserLibraries("user-id", []int{1}) + Expect(err).ToNot(HaveOccurred()) + + // Create request with musicFolderId=2 (not accessible) + r := httptest.NewRequest("GET", "/rest/getIndexes?musicFolderId=2", nil) + r = r.WithContext(ctx) + + // Call endpoint + response, err := api.GetIndexes(r) + + // Should return error due to lack of access + Expect(err).To(HaveOccurred()) + Expect(response).To(BeNil()) + }) + + It("should default to first accessible library when no musicFolderId specified", func() { + // Create mock user + user := model.User{ + ID: "user-id", + } + ctx = request.WithUser(ctx, user) + + // Setup minimal mock library data for working tests + mockLibRepo := ds.Library(ctx).(*tests.MockLibraryRepo) + mockLibRepo.SetData(model.Libraries{ + {ID: 2, Name: "Test Library 2", Path: "/music/library2"}, + {ID: 3, Name: "Test Library 3", Path: "/music/library3"}, + }) + + // Setup mock expectations + mockUserRepo := ds.User(ctx).(*tests.MockedUserRepo) + err := mockUserRepo.SetUserLibraries("user-id", []int{2, 3}) + Expect(err).ToNot(HaveOccurred()) + + // Setup mock artist data + mockArtistRepo := ds.Artist(ctx).(*tests.MockArtistRepo) + mockArtistRepo.SetData(model.Artists{ + {ID: "1", Name: "Test Artist 1"}, + {ID: "2", Name: "Test Artist 2"}, + }) + + // Create request without musicFolderId + r := httptest.NewRequest("GET", "/rest/getIndexes", nil) + r = r.WithContext(ctx) + + // Call endpoint + response, err := api.GetIndexes(r) + + // Should succeed and use first accessible library (2) + Expect(err).ToNot(HaveOccurred()) + Expect(response).ToNot(BeNil()) + Expect(response.Indexes).ToNot(BeNil()) + }) + }) + + Describe("GetArtists", func() { + It("should validate user access to the specified musicFolderId", func() { + // Create mock user + user := model.User{ + ID: "user-id", + } + ctx = request.WithUser(ctx, user) + + // Setup mock expectations - user only has access to library 1 + mockUserRepo := ds.User(ctx).(*tests.MockedUserRepo) + err := mockUserRepo.SetUserLibraries("user-id", []int{1}) + Expect(err).ToNot(HaveOccurred()) + + // Create request with musicFolderId=3 (not accessible) + r := httptest.NewRequest("GET", "/rest/getArtists?musicFolderId=3", nil) + r = r.WithContext(ctx) + + // Call endpoint + response, err := api.GetArtists(r) + + // Should return error due to lack of access + Expect(err).To(HaveOccurred()) + Expect(response).To(BeNil()) + }) + + It("should default to first accessible library when no musicFolderId specified", func() { + // Create mock user + user := model.User{ + ID: "user-id", + } + ctx = request.WithUser(ctx, user) + + // Setup minimal mock library data for working tests + mockLibRepo := ds.Library(ctx).(*tests.MockLibraryRepo) + mockLibRepo.SetData(model.Libraries{ + {ID: 1, Name: "Test Library 1", Path: "/music/library1"}, + {ID: 2, Name: "Test Library 2", Path: "/music/library2"}, + }) + + // Setup mock expectations + mockUserRepo := ds.User(ctx).(*tests.MockedUserRepo) + err := mockUserRepo.SetUserLibraries("user-id", []int{1, 2}) + Expect(err).ToNot(HaveOccurred()) + + // Setup mock artist data + mockArtistRepo := ds.Artist(ctx).(*tests.MockArtistRepo) + mockArtistRepo.SetData(model.Artists{ + {ID: "1", Name: "Test Artist 1"}, + {ID: "2", Name: "Test Artist 2"}, + }) + + // Create request without musicFolderId + r := httptest.NewRequest("GET", "/rest/getArtists", nil) + r = r.WithContext(ctx) + + // Call endpoint + response, err := api.GetArtists(r) + + // Should succeed and use first accessible library (1) + Expect(err).ToNot(HaveOccurred()) + Expect(response).ToNot(BeNil()) + Expect(response.Artist).ToNot(BeNil()) + }) + }) }) diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index 9aecdb974..5d3950505 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -479,8 +479,5 @@ func buildLyricsList(mf *model.MediaFile, lyricsList model.LyricList) *responses // Admin users get all libraries (auto-assigned), regular users get their assigned libraries func getUserAccessibleLibraries(ctx context.Context, ds model.DataStore) ([]model.Library, error) { user := getUser(ctx) - - // All users (admin and regular) use the same method since admin users - // are automatically assigned to all libraries via user_library table return ds.User(ctx).GetUserLibraries(user.ID) } diff --git a/tests/mock_artist_repo.go b/tests/mock_artist_repo.go index da5851061..9387c227a 100644 --- a/tests/mock_artist_repo.go +++ b/tests/mock_artist_repo.go @@ -108,4 +108,37 @@ func (m *MockArtistRepo) RefreshPlayCounts() (int64, error) { return int64(len(m.Data)), nil } +func (m *MockArtistRepo) GetIndex(includeMissing bool, libraryId int, roles ...model.Role) (model.ArtistIndexes, error) { + if m.Err { + return nil, errors.New("mock repo error") + } + + artists, err := m.GetAll() + if err != nil { + return nil, err + } + + // For mock purposes, if no artists available, return empty result + if len(artists) == 0 { + return model.ArtistIndexes{}, nil + } + + // Simple index grouping by first letter (simplified implementation for mocks) + indexMap := make(map[string]model.Artists) + for _, artist := range artists { + key := "#" + if len(artist.Name) > 0 { + key = string(artist.Name[0]) + } + indexMap[key] = append(indexMap[key], artist) + } + + var result model.ArtistIndexes + for k, artists := range indexMap { + result = append(result, model.ArtistIndex{ID: k, Artists: artists}) + } + + return result, nil +} + var _ model.ArtistRepository = (*MockArtistRepo)(nil)