From ad0c9ae15379ab1747d7d24ef01b9ac070a27cdc Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 27 Jun 2025 09:21:15 -0400 Subject: [PATCH] refactor: simplify getUserAccessibleLibraries function and update related tests Signed-off-by: Deluan --- server/subsonic/browsing.go | 5 +-- server/subsonic/browsing_test.go | 73 ++++++++++---------------------- server/subsonic/helpers.go | 24 +++++++++-- 3 files changed, 44 insertions(+), 58 deletions(-) diff --git a/server/subsonic/browsing.go b/server/subsonic/browsing.go index e234c52a6..9bd4966a9 100644 --- a/server/subsonic/browsing.go +++ b/server/subsonic/browsing.go @@ -18,10 +18,7 @@ import ( ) func (api *Router) GetMusicFolders(r *http.Request) (*responses.Subsonic, error) { - libraries, err := getUserAccessibleLibraries(r.Context(), api.ds) - if err != nil { - return nil, err - } + libraries := getUserAccessibleLibraries(r.Context()) folders := make([]responses.MusicFolder, len(libraries)) for i, f := range libraries { diff --git a/server/subsonic/browsing_test.go b/server/subsonic/browsing_test.go index a9ce0a5cc..b8f510aed 100644 --- a/server/subsonic/browsing_test.go +++ b/server/subsonic/browsing_test.go @@ -2,6 +2,7 @@ package subsonic import ( "context" + "fmt" "net/http/httptest" "github.com/navidrome/navidrome/core/auth" @@ -12,6 +13,18 @@ import ( . "github.com/onsi/gomega" ) +func contextWithUser(ctx context.Context, userID string, libraryIDs ...int) context.Context { + libraries := make([]model.Library, len(libraryIDs)) + for i, id := range libraryIDs { + libraries[i] = model.Library{ID: id, Name: fmt.Sprintf("Test Library %d", id), Path: fmt.Sprintf("/music/library%d", id)} + } + user := model.User{ + ID: userID, + Libraries: libraries, + } + return request.WithUser(ctx, user) +} + var _ = Describe("Browsing", func() { var api *Router var ctx context.Context @@ -26,16 +39,8 @@ var _ = Describe("Browsing", func() { Describe("GetMusicFolders", func() { It("should return all libraries the user has access", func() { - // Create mock user - user := model.User{ - ID: "user-id", - } - ctx = request.WithUser(ctx, user) - - // Setup mock expectations - admin users get all libraries via GetUserLibraries - mockUserRepo := ds.User(ctx).(*tests.MockedUserRepo) - err := mockUserRepo.SetUserLibraries("user-id", []int{1, 2, 3}) - Expect(err).ToNot(HaveOccurred()) + // Create mock user with libraries + ctx := contextWithUser(ctx, "user-id", 1, 2, 3) // Create request r := httptest.NewRequest("GET", "/rest/getMusicFolders", nil) @@ -57,16 +62,8 @@ var _ = Describe("Browsing", func() { 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 mock user with access to library 1 only + ctx = contextWithUser(ctx, "user-id", 1) // Create request with musicFolderId=2 (not accessible) r := httptest.NewRequest("GET", "/rest/getIndexes?musicFolderId=2", nil) @@ -81,11 +78,8 @@ var _ = Describe("Browsing", func() { }) 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) + // Create mock user with access to libraries 2 and 3 + ctx = contextWithUser(ctx, "user-id", 2, 3) // Setup minimal mock library data for working tests mockLibRepo := ds.Library(ctx).(*tests.MockLibraryRepo) @@ -94,11 +88,6 @@ var _ = Describe("Browsing", func() { {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{ @@ -122,16 +111,8 @@ var _ = Describe("Browsing", func() { 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 mock user with access to library 1 only + ctx = contextWithUser(ctx, "user-id", 1) // Create request with musicFolderId=3 (not accessible) r := httptest.NewRequest("GET", "/rest/getArtists?musicFolderId=3", nil) @@ -146,11 +127,8 @@ var _ = Describe("Browsing", func() { }) 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) + // Create mock user with access to libraries 1 and 2 + ctx = contextWithUser(ctx, "user-id", 1, 2) // Setup minimal mock library data for working tests mockLibRepo := ds.Library(ctx).(*tests.MockLibraryRepo) @@ -159,11 +137,6 @@ var _ = Describe("Browsing", func() { {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{ diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index 5d3950505..e8436965c 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -17,6 +17,7 @@ import ( "github.com/navidrome/navidrome/server/public" "github.com/navidrome/navidrome/server/subsonic/responses" "github.com/navidrome/navidrome/utils/number" + "github.com/navidrome/navidrome/utils/req" "github.com/navidrome/navidrome/utils/slice" ) @@ -475,9 +476,24 @@ func buildLyricsList(mf *model.MediaFile, lyricsList model.LyricList) *responses return res } -// getUserAccessibleLibraries returns the list of libraries the current user has access to -// Admin users get all libraries (auto-assigned), regular users get their assigned libraries -func getUserAccessibleLibraries(ctx context.Context, ds model.DataStore) ([]model.Library, error) { +// getUserAccessibleLibraries returns the list of libraries the current user has access to. +func getUserAccessibleLibraries(ctx context.Context) []model.Library { user := getUser(ctx) - return ds.User(ctx).GetUserLibraries(user.ID) + return user.Libraries +} + +// selectedMusicFolderIds returns the music folder IDs from the request parameters. +// If no IDs are provided, it returns all libraries accessible to the user. +func selectedMusicFolderIds(r *http.Request) ([]int, error) { + user := getUser(r.Context()) + p := req.Params(r) + musicFolderIds, err := p.Ints("musicFolderId") + if !errors.Is(err, req.ErrMissingParam) { + return nil, err + } + + if len(musicFolderIds) > 0 { + return musicFolderIds, nil + } + return slice.Map(user.Libraries, func(lib model.Library) int { return lib.ID }), nil }