nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-08-01 07:21:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge branch 'master' into update-translations

Browse Source
This commit is contained in:
Deluan Quintão 2026-06-05 18:07:15 -04:00 committed by GitHub
commit 05191d2a22
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
16 changed files with 469 additions and 127 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
persistence/mediafile_repository.go
View File

@ -104,6 +104,7 @@ var mediaFileFilter = sync.OnceValue(func() map[string]filterFunc {
"missing": booleanFilter,
"artists_id": artistFilter,
"library_id": libraryIdFilter,
"path": startsWithFilter("media_file.path"),
}
// Add all album tags as filters
for tag := range model.TagMappings() {

28
persistence/mediafile_repository_test.go
View File

@ -524,6 +524,34 @@ var _ = Describe("MediaRepository", func() {
}
})
})
Describe("path", func() {
It("matches files whose path starts with the given prefix", func() {
res, err := mr.(model.ResourceRepository).ReadAll(rest.QueryOptions{
Filters: map[string]any{"path": "test/"},
})
Expect(err).ToNot(HaveOccurred())
files := res.(model.MediaFiles)
var found bool
for _, f := range files {
Expect(f.Path).To(HavePrefix("test/"))
if f.ID == mfWithoutAnnotation.ID {
found = true
}
}
Expect(found).To(BeTrue(), "MediaFile with matching path prefix should be included")
})
It("excludes files whose path does not start with the given prefix", func() {
res, err := mr.(model.ResourceRepository).ReadAll(rest.QueryOptions{
Filters: map[string]any{"path": "no-such-prefix/"},
})
Expect(err).ToNot(HaveOccurred())
files := res.(model.MediaFiles)
Expect(files).To(BeEmpty())
})
})
})
Describe("Search", func() {

18
persistence/player_repository.go
View File

@ -62,17 +62,6 @@ func (r *playerRepository) newRestSelect(options ...model.QueryOptions) SelectBu
return s.Where(r.addRestriction())
}
func (r *playerRepository) addRestriction(sql ...Sqlizer) Sqlizer {
s := And{}
if len(sql) > 0 {
s = append(s, sql[0])
}
if owner := r.ownerFilter(); owner != nil {
s = append(s, owner)
}
return s
}
func (r *playerRepository) CountByClient(options ...model.QueryOptions) (map[string]int64, error) {
sel := r.newSelect(options...).
Columns(
@ -152,12 +141,7 @@ func (r *playerRepository) Update(id string, entity any, cols ...string) error {
}
func (r *playerRepository) Delete(id string) error {
filter := r.addRestriction(And{Eq{"player.id": id}})
err := r.delete(filter)
if errors.Is(err, model.ErrNotFound) {
return rest.ErrNotFound
}
return err
return r.deleteOwned(id)
}
var _ model.PlayerRepository = (*playerRepository)(nil)

46
persistence/player_repository_test.go
View File

@ -110,33 +110,43 @@ var _ = Describe("PlayerRepository", func() {
})
Describe("Delete", func() {
DescribeTable("item type", func(player model.Player) {
err := repo.Delete(player.ID)
It("deletes a player owned by the current user", func() {
err := repo.Delete(userPlayer.ID)
Expect(err).To(BeNil())
isReal := player.UserId != ""
canDelete := admin || player.UserId == userPlayer.UserId
count, err := repo.Count()
Expect(err).To(BeNil())
Expect(count).To(Equal(baseCount - 1))
if isReal && canDelete {
Expect(count).To(Equal(baseCount - 1))
} else {
Expect(count).To(Equal(baseCount))
}
_, err = repo.Get(userPlayer.ID)
Expect(err).To(Equal(model.ErrNotFound))
})
item, err := repo.Get(player.ID)
if !isReal || canDelete {
It("does not delete another user's player when not admin", func() {
err := repo.Delete(otherPlayer.ID)
if admin {
// Admins may delete any player.
Expect(err).To(BeNil())
Expect(repo.Count()).To(Equal(baseCount - 1))
_, err = repo.Get(otherPlayer.ID)
Expect(err).To(Equal(model.ErrNotFound))
} else {
Expect(*item).To(Equal(player))
// The ownership-restricted delete matches no owned row, so it reports
// permission-denied and leaves the other user's player untouched.
Expect(err).To(Equal(rest.ErrPermissionDenied))
Expect(repo.Count()).To(Equal(baseCount))
item, err := repo.Get(otherPlayer.ID)
Expect(err).To(BeNil())
Expect(*item).To(Equal(otherPlayer))
}
},
Entry("same user", userPlayer),
Entry("other item", otherPlayer),
Entry("fake item", model.Player{}),
)
})
It("returns not-found for a nonexistent player", func() {
err := repo.Delete("i don't exist")
Expect(err).To(Equal(rest.ErrNotFound))
Expect(repo.Count()).To(Equal(baseCount))
})
})
Describe("Read", func() {

41
persistence/share_repository.go
View File

@ -30,51 +30,18 @@ func NewShareRepository(ctx context.Context, db dbx.Builder) model.ShareReposito
return r
}
// TODO: Ownership checks should be moved to the service layer (core/share.go)
func (r *shareRepository) checkOwnership(id string) error {
usr := loggedUser(r.ctx)
if usr.IsAdmin || usr.ID == invalidUserId {
return nil
}
sel := r.newSelect().Columns("user_id").Where(Eq{"id": id})
var share struct {
UserID string `db:"user_id"`
}
err := r.queryOne(sel, &share)
if err != nil {
if errors.Is(err, model.ErrNotFound) {
return rest.ErrNotFound
}
return err
}
if share.UserID != usr.ID {
return rest.ErrPermissionDenied
}
return nil
}
// TODO: this still uses the legacy checkOwnership SELECT-then-delete pattern (a TOCTOU window),
// the same shape removed from Update. Once a base-repo deleteOwned exists (built on ownerFilter,
// mirroring updateOwned), route Delete through it and drop checkOwnership entirely. playerRepository
// .Delete (which restricts via addRestriction) should adopt the same primitive.
func (r *shareRepository) Delete(id string) error {
if err := r.checkOwnership(id); err != nil {
return err
}
err := r.delete(Eq{"id": id})
if errors.Is(err, model.ErrNotFound) {
return rest.ErrNotFound
}
return err
return r.deleteOwned(id)
}
func (r *shareRepository) selectShare(options ...model.QueryOptions) SelectBuilder {
return r.newSelect(options...).Join("user u on u.id = share.user_id").
Columns("share.*", "user_name as username")
Columns("share.*", "user_name as username").
Where(r.addRestriction())
}
func (r *shareRepository) Exists(id string) (bool, error) {
return r.exists(Eq{"id": id})
return r.exists(r.addRestriction(And{Eq{"id": id}}))
}
func (r *shareRepository) Get(id string) (*model.Share, error) {

160
persistence/share_repository_test.go
View File

@ -20,7 +20,7 @@ var _ = Describe("ShareRepository", func() {
BeforeEach(func() {
DeferCleanup(configtest.SetupConfig())
ctx = request.WithUser(log.NewContext(context.TODO()), adminUser)
ctx = request.WithUser(log.NewContext(GinkgoT().Context()), adminUser)
repo = NewShareRepository(ctx, GetDBXBuilder())
// Insert the admin user into the database (required for foreign key constraint)
@ -38,7 +38,7 @@ var _ = Describe("ShareRepository", func() {
Context("Repository creation and basic operations", func() {
It("should create repository successfully with no user context", func() {
// Create repository with no user context (headless)
headlessRepo := NewShareRepository(context.Background(), GetDBXBuilder())
headlessRepo := NewShareRepository(GinkgoT().Context(), GetDBXBuilder())
Expect(headlessRepo).ToNot(BeNil())
})
@ -60,7 +60,7 @@ var _ = Describe("ShareRepository", func() {
Expect(err).ToNot(HaveOccurred())
// Headless process should see all shares
headlessRepo := NewShareRepository(context.Background(), GetDBXBuilder())
headlessRepo := NewShareRepository(GinkgoT().Context(), GetDBXBuilder())
shares, err := headlessRepo.GetAll()
Expect(err).ToNot(HaveOccurred())
@ -92,7 +92,7 @@ var _ = Describe("ShareRepository", func() {
Expect(err).ToNot(HaveOccurred())
// Headless process should be able to get the share
headlessRepo := NewShareRepository(context.Background(), GetDBXBuilder())
headlessRepo := NewShareRepository(GinkgoT().Context(), GetDBXBuilder())
share, err := headlessRepo.Get(shareID)
Expect(err).ToNot(HaveOccurred())
Expect(share.ID).To(Equal(shareID))
@ -155,7 +155,7 @@ var _ = Describe("ShareRepository", func() {
Describe("Delete", func() {
It("allows a non-admin user to delete their own share", func() {
insertShare("own-share-del", ownerUser.ID)
ctx := request.WithUser(log.NewContext(context.TODO()), ownerUser)
ctx := request.WithUser(log.NewContext(GinkgoT().Context()), ownerUser)
repo := NewShareRepository(ctx, GetDBXBuilder())
err := repo.(rest.Persistable).Delete("own-share-del")
Expect(err).ToNot(HaveOccurred())
@ -163,15 +163,21 @@ var _ = Describe("ShareRepository", func() {
It("denies a non-admin user from deleting another user's share", func() {
insertShare("other-share-del", ownerUser.ID)
ctx := request.WithUser(log.NewContext(context.TODO()), otherUser)
ctx := request.WithUser(log.NewContext(GinkgoT().Context()), otherUser)
repo := NewShareRepository(ctx, GetDBXBuilder())
err := repo.(rest.Persistable).Delete("other-share-del")
Expect(err).To(Equal(rest.ErrPermissionDenied))
// The share was not deleted: the owner can still read it.
ownerCtx := request.WithUser(log.NewContext(GinkgoT().Context()), ownerUser)
ownerRepo := NewShareRepository(ownerCtx, GetDBXBuilder())
_, err = ownerRepo.(rest.Repository).Read("other-share-del")
Expect(err).ToNot(HaveOccurred())
})
It("allows an admin to delete any user's share", func() {
insertShare("admin-del-share", ownerUser.ID)
ctx := request.WithUser(log.NewContext(context.TODO()), adminUser)
ctx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser)
repo := NewShareRepository(ctx, GetDBXBuilder())
err := repo.(rest.Persistable).Delete("admin-del-share")
Expect(err).ToNot(HaveOccurred())
@ -179,7 +185,7 @@ var _ = Describe("ShareRepository", func() {
It("allows headless context (no user) to delete a share", func() {
insertShare("headless-del-share", ownerUser.ID)
repo := NewShareRepository(context.Background(), GetDBXBuilder())
repo := NewShareRepository(GinkgoT().Context(), GetDBXBuilder())
err := repo.(rest.Persistable).Delete("headless-del-share")
Expect(err).ToNot(HaveOccurred())
})
@ -188,7 +194,7 @@ var _ = Describe("ShareRepository", func() {
Describe("Update", func() {
It("allows a non-admin user to update their own share", func() {
insertShare("own-share-upd", ownerUser.ID)
ctx := request.WithUser(log.NewContext(context.TODO()), ownerUser)
ctx := request.WithUser(log.NewContext(GinkgoT().Context()), ownerUser)
repo := NewShareRepository(ctx, GetDBXBuilder())
err := repo.(rest.Persistable).Update("own-share-upd", &model.Share{Description: "Updated"}, "description")
Expect(err).ToNot(HaveOccurred())
@ -196,7 +202,7 @@ var _ = Describe("ShareRepository", func() {
It("denies a non-admin user from updating another user's share", func() {
insertShare("other-share-upd", ownerUser.ID)
ctx := request.WithUser(log.NewContext(context.TODO()), otherUser)
ctx := request.WithUser(log.NewContext(GinkgoT().Context()), otherUser)
repo := NewShareRepository(ctx, GetDBXBuilder())
err := repo.(rest.Persistable).Update("other-share-upd", &model.Share{Description: "Hacked"}, "description")
Expect(err).To(Equal(rest.ErrPermissionDenied))
@ -204,7 +210,7 @@ var _ = Describe("ShareRepository", func() {
It("allows an admin to update any user's share", func() {
insertShare("admin-upd-share", ownerUser.ID)
ctx := request.WithUser(log.NewContext(context.TODO()), adminUser)
ctx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser)
repo := NewShareRepository(ctx, GetDBXBuilder())
err := repo.(rest.Persistable).Update("admin-upd-share", &model.Share{Description: "Admin Updated"}, "description")
Expect(err).ToNot(HaveOccurred())
@ -212,7 +218,7 @@ var _ = Describe("ShareRepository", func() {
It("allows headless context (no user) to update a share", func() {
insertShare("headless-upd-share", ownerUser.ID)
repo := NewShareRepository(context.Background(), GetDBXBuilder())
repo := NewShareRepository(GinkgoT().Context(), GetDBXBuilder())
err := repo.(rest.Persistable).Update("headless-upd-share", &model.Share{Description: "Headless"}, "description")
Expect(err).ToNot(HaveOccurred())
})
@ -255,5 +261,135 @@ var _ = Describe("ShareRepository", func() {
Expect(got.(*model.Share).UserID).To(Equal(ownerUser.ID))
})
})
Describe("Read scoping", func() {
BeforeEach(func() {
// Persist owner/other users so the JOIN in selectShare resolves.
ur := NewUserRepository(ctx, GetDBXBuilder())
Expect(ur.Put(&ownerUser)).To(Succeed())
Expect(ur.Put(&otherUser)).To(Succeed())
insertShare("share-owner-1", ownerUser.ID)
insertShare("share-owner-2", ownerUser.ID)
insertShare("share-other-1", otherUser.ID)
})
Context("non-admin user", func() {
var nonAdminRepo model.ShareRepository
var nonAdminRest rest.Repository
BeforeEach(func() {
nonAdminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), ownerUser)
nonAdminRepo = NewShareRepository(nonAdminCtx, GetDBXBuilder())
nonAdminRest = nonAdminRepo.(rest.Repository)
})
It("GetAll returns only own shares", func() {
shares, err := nonAdminRepo.GetAll()
Expect(err).ToNot(HaveOccurred())
ids := make([]string, len(shares))
for i, s := range shares {
ids[i] = s.ID
}
Expect(ids).To(ConsistOf("share-owner-1", "share-owner-2"))
})
It("ReadAll returns only own shares", func() {
res, err := nonAdminRest.ReadAll()
Expect(err).ToNot(HaveOccurred())
shares := res.(model.Shares)
ids := make([]string, len(shares))
for i, s := range shares {
ids[i] = s.ID
}
Expect(ids).To(ConsistOf("share-owner-1", "share-owner-2"))
})
It("Get returns own share", func() {
s, err := nonAdminRepo.Get("share-owner-1")
Expect(err).ToNot(HaveOccurred())
Expect(s.ID).To(Equal("share-owner-1"))
})
It("Get returns ErrNotFound for another user's share", func() {
_, err := nonAdminRepo.Get("share-other-1")
Expect(err).To(MatchError(model.ErrNotFound))
})
It("Read returns ErrNotFound for another user's share", func() {
_, err := nonAdminRest.Read("share-other-1")
Expect(err).To(MatchError(model.ErrNotFound))
})
It("Exists returns true for own share", func() {
exists, err := nonAdminRepo.Exists("share-owner-1")
Expect(err).ToNot(HaveOccurred())
Expect(exists).To(BeTrue())
})
It("Exists returns false for another user's share", func() {
exists, err := nonAdminRepo.Exists("share-other-1")
Expect(err).ToNot(HaveOccurred())
Expect(exists).To(BeFalse())
})
It("CountAll counts only own shares", func() {
count, err := nonAdminRepo.CountAll()
Expect(err).ToNot(HaveOccurred())
Expect(count).To(BeNumerically("==", 2))
})
It("Count (rest) counts only own shares", func() {
count, err := nonAdminRest.Count()
Expect(err).ToNot(HaveOccurred())
Expect(count).To(BeNumerically("==", 2))
})
})
Context("admin user", func() {
It("GetAll returns all shares", func() {
adminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser)
adminRepo := NewShareRepository(adminCtx, GetDBXBuilder())
shares, err := adminRepo.GetAll()
Expect(err).ToNot(HaveOccurred())
ids := make([]string, len(shares))
for i, s := range shares {
ids[i] = s.ID
}
Expect(ids).To(ConsistOf("share-owner-1", "share-owner-2", "share-other-1"))
})
It("CountAll counts all shares", func() {
adminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser)
adminRepo := NewShareRepository(adminCtx, GetDBXBuilder())
count, err := adminRepo.CountAll()
Expect(err).ToNot(HaveOccurred())
Expect(count).To(BeNumerically("==", 3))
})
})
Context("headless context (public share route)", func() {
It("GetAll returns all shares", func() {
headlessRepo := NewShareRepository(GinkgoT().Context(), GetDBXBuilder())
shares, err := headlessRepo.GetAll()
Expect(err).ToNot(HaveOccurred())
Expect(shares).To(HaveLen(3))
})
It("Get returns another user's share", func() {
headlessRepo := NewShareRepository(GinkgoT().Context(), GetDBXBuilder())
s, err := headlessRepo.Get("share-other-1")
Expect(err).ToNot(HaveOccurred())
Expect(s.ID).To(Equal("share-other-1"))
})
It("Exists returns true for any share", func() {
headlessRepo := NewShareRepository(GinkgoT().Context(), GetDBXBuilder())
exists, err := headlessRepo.Exists("share-other-1")
Expect(err).ToNot(HaveOccurred())
Expect(exists).To(BeTrue())
})
})
})
})
})

72
persistence/sql_base_repository.go
View File

@ -61,6 +61,9 @@ func loggedUser(ctx context.Context) *model.User {
// ownerFilter returns the predicate restricting access to rows owned by the logged-in user, for
// tables with a user_id column. It returns nil for admins and for headless/system contexts (invalid
// user), meaning "no ownership restriction". Callers should skip the WHERE clause when it is nil.
//
// The predicate uses an unqualified user_id, so it only works on queries where that column is
// unambiguous (no join introducing a second user_id).
func (r sqlRepository) ownerFilter() Sqlizer {
if usr := loggedUser(r.ctx); !usr.IsAdmin && usr.ID != invalidUserId {
return Eq{"user_id": usr.ID}
@ -68,6 +71,20 @@ func (r sqlRepository) ownerFilter() Sqlizer {
return nil
}
// addRestriction combines an optional caller predicate with the ownership filter, producing the
// WHERE clause for owner-scoped reads. For admins and headless contexts ownerFilter() is nil and
// only the caller's predicate (if any) remains.
func (r sqlRepository) addRestriction(sql ...Sqlizer) Sqlizer {
s := And{}
if len(sql) > 0 {
s = append(s, sql[0])
}
if owner := r.ownerFilter(); owner != nil {
s = append(s, owner)
}
return s
}
func (r *sqlRepository) registerModel(instance any, filters map[string]filterFunc) {
if r.tableName == "" {
r.tableName = strings.TrimPrefix(reflect.TypeOf(instance).String(), "*model.")
@ -197,15 +214,6 @@ func (r sqlRepository) applyFilters(sq SelectBuilder, options ...model.QueryOpti
return sq
}
func (r *sqlRepository) withTableName(filter filterFunc) filterFunc {
return func(field string, value any) Sqlizer {
if r.tableName != "" {
field = r.tableName + "." + field
}
return filter(field, value)
}
}
// libraryIdFilter is a filter function to be added to resources that have a library_id column.
func libraryIdFilter(_ string, value any) Sqlizer {
return Eq{"library_id": value}
@ -411,29 +419,47 @@ func (r sqlRepository) updateOwned(id string, m any, colsToUpdate ...string) err
}
updateValues := filterUpdateValues(values, id, colsToUpdate...)
delete(updateValues, "user_id") // ownership is immutable on update
update := Update(r.tableName).Where(Eq{"id": id}).SetMap(updateValues)
if owner := r.ownerFilter(); owner != nil {
update = update.Where(owner)
}
update := Update(r.tableName).Where(r.addRestriction(Eq{"id": id})).SetMap(updateValues)
count, err := r.executeSQL(update)
if err != nil {
return err
}
if count == 0 {
// The update matched no row: either the id is missing, or it exists but is owned by
// someone else. Disambiguate to return the more accurate error.
exists, err := r.exists(Eq{"id": id})
if err != nil {
return err
}
if exists {
return rest.ErrPermissionDenied
}
return rest.ErrNotFound
return r.classifyOwnedWriteMiss(id)
}
return nil
}
// deleteOwned performs an atomic, ownership-restricted delete of the row identified by id, for
// repositories whose table has a user_id column. Non-admins can only delete rows they own: the
// ownership predicate is part of the DELETE's WHERE clause, so a row owned by another user simply
// 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(id string) error {
count, err := r.executeSQL(Delete(r.tableName).Where(r.addRestriction(Eq{"id": id})))
if err != nil {
return err
}
if count == 0 {
return r.classifyOwnedWriteMiss(id)
}
return nil
}
// classifyOwnedWriteMiss explains why an ownership-filtered write (updateOwned/deleteOwned) matched
// no row: rest.ErrPermissionDenied if the row exists but is owned by another user, otherwise
// rest.ErrNotFound. It runs only on the failure path (count == 0), where no write occurred.
func (r sqlRepository) classifyOwnedWriteMiss(id string) error {
exists, err := r.exists(Eq{"id": id})
if err != nil {
return err
}
if exists {
return rest.ErrPermissionDenied
}
return rest.ErrNotFound
}
func (r sqlRepository) count(countQuery SelectBuilder, options ...model.QueryOptions) (int64, error) {
countQuery = countQuery.
RemoveColumns().Columns("count(distinct " + r.tableName + ".id) as count").

8
persistence/sql_restful.go
View File

@ -46,7 +46,7 @@ func (r *sqlRepository) parseRestFilters(ctx context.Context, options rest.Query
continue
}
// Default to a "starts with" filter
filters = append(filters, startsWithFilter(f, v))
filters = append(filters, Like{f: fmt.Sprintf("%s%%", v)})
}
return filters
}
@ -91,8 +91,10 @@ func eqFilter(field string, value any) Sqlizer {
return Eq{field: value}
}
func startsWithFilter(field string, value any) Sqlizer {
return Like{field: fmt.Sprintf("%s%%", value)}
func startsWithFilter(field string) func(string, any) Sqlizer {
return func(_ string, value any) Sqlizer {
return Like{field: fmt.Sprintf("%s%%", value)}
}
}
func containsFilter(field string) func(string, any) Sqlizer {

2
persistence/user_repository.go
View File

@ -59,7 +59,7 @@ func NewUserRepository(ctx context.Context, db dbx.Builder) model.UserRepository
r.registerModel(&model.User{}, map[string]filterFunc{
"id": idFilter(r.tableName),
"password": invalidFilter(ctx),
"name": r.withTableName(startsWithFilter),
"name": startsWithFilter(r.tableName + ".name"),
})
once.Do(func() {
_ = r.initPasswordEncryptionKey()

47
persistence/user_repository_test.go
View File

@ -11,6 +11,7 @@ import (
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/id"
"github.com/navidrome/navidrome/model/request"
"github.com/navidrome/navidrome/tests"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
@ -207,6 +208,52 @@ var _ = Describe("UserRepository", func() {
})
})
Describe("ReadAll name filter", func() {
var adminRepo model.ResourceRepository
BeforeEach(func() {
adminCtx := request.WithUser(GinkgoT().Context(), model.User{ID: "admin-id", UserName: "admin", IsAdmin: true})
adminRepo = NewUserRepository(adminCtx, GetDBXBuilder()).(model.ResourceRepository)
for _, u := range []model.User{
{ID: "filter-alice", UserName: "alice_filter", Name: "Alice Filter", NewPassword: "x"},
{ID: "filter-bob", UserName: "bob_filter", Name: "Bob Filter", NewPassword: "x"},
} {
Expect(adminRepo.(model.UserRepository).Put(&u)).To(Succeed())
}
})
AfterEach(func() {
ur := adminRepo.(model.UserRepository)
_ = ur.Delete("filter-alice")
_ = ur.Delete("filter-bob")
})
It("matches users whose name starts with the given prefix", func() {
res, err := adminRepo.ReadAll(rest.QueryOptions{Filters: map[string]any{"name": "Alice"}})
Expect(err).ToNot(HaveOccurred())
users := res.(model.Users)
var names []string
for _, u := range users {
names = append(names, u.Name)
}
Expect(names).To(ContainElement("Alice Filter"))
Expect(names).ToNot(ContainElement("Bob Filter"))
})
It("does not match names by mid-string substring (startsWith, not contains)", func() {
res, err := adminRepo.ReadAll(rest.QueryOptions{Filters: map[string]any{"name": "Filter"}})
Expect(err).ToNot(HaveOccurred())
users := res.(model.Users)
for _, u := range users {
Expect(u.ID).ToNot(Or(Equal("filter-alice"), Equal("filter-bob")),
"a mid-string substring should not match a startsWith filter")
}
})
})
Describe("validateUsernameUnique", func() {
var repo *tests.MockedUserRepo
var existingUser *model.User

79
server/e2e/subsonic_sharing_test.go
View File

@ -125,3 +125,82 @@ var _ = Describe("Sharing Endpoints", Ordered, func() {
Expect(resp.Error).ToNot(BeNil())
})
})
var _ = Describe("Sharing Cross-User Isolation", Ordered, func() {
var userA, userB model.User
var shareID string
var albumID string
BeforeAll(func() {
conf.Server.EnableSharing = true
setupTestDB()
userA = createUser("share-user-a", "share-user-a", "Share User A", false)
userB = createUser("share-user-b", "share-user-b", "Share User B", false)
albums, err := ds.Album(ctx).GetAll(model.QueryOptions{
Filters: squirrel.Eq{"album.name": "Abbey Road"},
})
Expect(err).ToNot(HaveOccurred())
Expect(albums).ToNot(BeEmpty())
albumID = albums[0].ID
resp := doReqWithUser(userA, "createShare", "id", albumID, "description", "User A's share")
Expect(resp.Status).To(Equal(responses.StatusOK))
Expect(resp.Shares.Share).To(HaveLen(1))
shareID = resp.Shares.Share[0].ID
Expect(resp.Shares.Share[0].Username).To(Equal(userA.UserName))
})
It("userB's getShares does not leak userA's share", func() {
resp := doReqWithUser(userB, "getShares")
Expect(resp.Status).To(Equal(responses.StatusOK))
Expect(resp.Shares).ToNot(BeNil())
Expect(resp.Shares.Share).To(BeEmpty())
})
It("userA still sees own share", func() {
resp := doReqWithUser(userA, "getShares")
Expect(resp.Status).To(Equal(responses.StatusOK))
Expect(resp.Shares.Share).To(HaveLen(1))
Expect(resp.Shares.Share[0].ID).To(Equal(shareID))
Expect(resp.Shares.Share[0].Description).To(Equal("User A's share"))
})
It("admin sees userA's share", func() {
resp := doReqWithUser(adminUser, "getShares")
Expect(resp.Status).To(Equal(responses.StatusOK))
ids := make([]string, len(resp.Shares.Share))
for i, s := range resp.Shares.Share {
ids[i] = s.ID
}
Expect(ids).To(ContainElement(shareID))
})
It("userB cannot updateShare on userA's share", func() {
resp := doReqWithUser(userB, "updateShare", "id", shareID, "description", "hijacked")
Expect(resp.Status).To(Equal(responses.StatusFailed))
Expect(resp.Error).ToNot(BeNil())
// Confirm description unchanged for userA.
check := doReqWithUser(userA, "getShares")
Expect(check.Shares.Share).To(HaveLen(1))
Expect(check.Shares.Share[0].Description).To(Equal("User A's share"))
})
It("userB cannot deleteShare on userA's share", func() {
resp := doReqWithUser(userB, "deleteShare", "id", shareID)
Expect(resp.Status).To(Equal(responses.StatusFailed))
Expect(resp.Error).ToNot(BeNil())
// Confirm share still present for userA.
check := doReqWithUser(userA, "getShares")
Expect(check.Shares.Share).To(HaveLen(1))
Expect(check.Shares.Share[0].ID).To(Equal(shareID))
})
})

5
server/subsonic/api.go
View File

@ -9,6 +9,7 @@ import (
"regexp"
"strconv"
"github.com/deluan/rest"
"github.com/go-chi/chi/v5"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/core"
@ -301,9 +302,9 @@ func mapToSubsonicError(err error) subError {
err = newError(responses.ErrorMissingParameter, err.Error())
case errors.Is(err, req.ErrInvalidParam):
err = newError(responses.ErrorGeneric, err.Error())
case errors.Is(err, model.ErrNotFound):
case errors.Is(err, model.ErrNotFound), errors.Is(err, rest.ErrNotFound):
err = newError(responses.ErrorDataNotFound, "data not found")
case errors.Is(err, model.ErrNotAuthorized):
case errors.Is(err, model.ErrNotAuthorized), errors.Is(err, rest.ErrPermissionDenied):
err = newError(responses.ErrorAuthorizationFail)
case errors.Is(err, stream.ErrTooManyTranscodes):
err = newError(responses.ErrorGeneric, "too many concurrent transcodes, please retry shortly")

24
server/subsonic/api_test.go
View File

@ -4,13 +4,16 @@ import (
"context"
"encoding/json"
"encoding/xml"
"errors"
"fmt"
"math"
"net/http"
"net/http/httptest"
"strings"
"github.com/deluan/rest"
"github.com/navidrome/navidrome/core/stream"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/server/subsonic/responses"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
@ -187,3 +190,24 @@ var _ = Describe("sendResponse", func() {
Expect(pointer).To(Equal(responses.ErrorDataNotFound))
})
})
var _ = Describe("mapToSubsonicError", func() {
DescribeTable("maps repository errors to the correct Subsonic error code",
func(err error, expectedCode int32) {
subErr := mapToSubsonicError(err)
Expect(subErr.code).To(Equal(expectedCode))
},
Entry("rest.ErrPermissionDenied -> not authorized (50)",
rest.ErrPermissionDenied, responses.ErrorAuthorizationFail),
Entry("rest.ErrNotFound -> data not found (70)",
rest.ErrNotFound, responses.ErrorDataNotFound),
Entry("model.ErrNotAuthorized -> not authorized (50)",
model.ErrNotAuthorized, responses.ErrorAuthorizationFail),
Entry("model.ErrNotFound -> data not found (70)",
model.ErrNotFound, responses.ErrorDataNotFound),
Entry("wrapped rest.ErrPermissionDenied is still mapped",
fmt.Errorf("update share: %w", rest.ErrPermissionDenied), responses.ErrorAuthorizationFail),
Entry("unknown error -> generic (0)",
errors.New("boom"), responses.ErrorGeneric),
)
})

2
server/subsonic/filter/filters.go
View File

@ -92,7 +92,7 @@ func SongsByAlbum(albumId string) Options {
func SongsByRandom(genre string, fromYear, toYear int) Options {
options := Options{
Sort: "random",
Sort: "random()",
}
ff := And{}
if genre != "" {

2
utils/cache/cached_http_client_test.go vendored
View File

@ -20,6 +20,8 @@ var _ = Describe("HTTPClient", func() {
var header string
BeforeEach(func() {
requestsReceived = 0
header = ""
ts = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requestsReceived++
header = r.Header.Get("head")

61
utils/cache/file_haunter_test.go vendored
View File

@ -29,15 +29,15 @@ var _ = Describe("FileHaunter", func() {
Expect(err).ToNot(HaveOccurred())
DeferCleanup(func() { _ = os.RemoveAll(tempDir) })
// Use a short haunter period so cleanup runs promptly; the assertions
// below poll with Eventually instead of racing a fixed sleep.
fsCache, err = fscache.NewCacheWithHaunter(fs, fscache.NewLRUHaunterStrategy(
cache.NewFileHaunter("", maxItems, maxSize, 300*time.Millisecond),
cache.NewFileHaunter("", maxItems, maxSize, 100*time.Millisecond),
))
Expect(err).ToNot(HaveOccurred())
DeferCleanup(fsCache.Clean)
Expect(createTestFiles(fsCache)).To(Succeed())
<-time.After(400 * time.Millisecond)
})
Context("When maxSize is defined", func() {
@ -46,24 +46,39 @@ var _ = Describe("FileHaunter", func() {
})
It("removes files", func() {
Expect(os.ReadDir(cacheDir)).To(HaveLen(4))
Expect(fsCache.Exists("stream-5")).To(BeFalse(), "stream-5 (empty file) should have been scrubbed")
// TODO Fix flaky tests
//Expect(fsCache.Exists("stream-0")).To(BeFalse(), "stream-0 should have been scrubbed")
// stream-0..4 hold "hello" (5 bytes each) and stream-5 is empty.
// With maxSize=20, the haunter scrubs the empty file plus enough of
// the oldest files to bring the total size down to <= 20 bytes.
// Which files survive (and therefore the exact count) depends on
// access-time ordering, so we only assert the haunter's guarantees:
// the empty file is always scrubbed and the total size stays within
// the configured limit.
Eventually(func(g Gomega) {
g.Expect(fsCache.Exists("stream-5")).To(BeFalse(), "stream-5 (empty file) should have been scrubbed")
size, err := dirSize(cacheDir)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(size).To(BeNumerically("<=", maxSize))
}).WithTimeout(5 * time.Second).WithPolling(50 * time.Millisecond).Should(Succeed())
})
})
XContext("When maxItems is defined", func() {
Context("When maxItems is defined", func() {
BeforeEach(func() {
maxItems = 3
})
It("removes files", func() {
Expect(os.ReadDir(cacheDir)).To(HaveLen(maxItems))
Expect(fsCache.Exists("stream-5")).To(BeFalse(), "stream-5 (empty file) should have been scrubbed")
// TODO Fix flaky tests
//Expect(fsCache.Exists("stream-0")).To(BeFalse(), "stream-0 should have been scrubbed")
//Expect(fsCache.Exists("stream-1")).To(BeFalse(), "stream-1 should have been scrubbed")
// With maxItems=3, the haunter scrubs the empty file plus enough of
// the oldest files to bring the count within the limit. As above, the
// exact survivors depend on access-time ordering, so we assert the
// guaranteed invariants: the empty file is gone and the item count
// stays within the configured limit.
Eventually(func(g Gomega) {
g.Expect(fsCache.Exists("stream-5")).To(BeFalse(), "stream-5 (empty file) should have been scrubbed")
entries, readErr := os.ReadDir(cacheDir)
g.Expect(readErr).ToNot(HaveOccurred())
g.Expect(len(entries)).To(BeNumerically("<=", maxItems))
}).WithTimeout(5 * time.Second).WithPolling(50 * time.Millisecond).Should(Succeed())
})
})
})
@ -93,6 +108,26 @@ func createTestFiles(c *fscache.FSCache) error {
return nil
}
// dirSize returns the total size in bytes of all regular files in dir.
func dirSize(dir string) (uint64, error) {
entries, err := os.ReadDir(dir)
if err != nil {
return 0, err
}
var total uint64
for _, e := range entries {
info, err := e.Info()
if err != nil {
return 0, err
}
if !info.Mode().IsRegular() {
continue
}
total += uint64(info.Size())
}
return total, nil
}
func createCachedStream(c *fscache.FSCache, name string, contents string) fscache.ReadAtCloser {
r, w, _ := c.Get(name)
_, _ = w.Write([]byte(contents))