ironmagma/quintodrome
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(smartplaylists): relax playlist visibility in inPlaylist/notInPlaylist rules (#5411)

Browse source 
* test(e2e): add end-to-end tests for smart playlists functionality

Signed-off-by: Deluan <deluan@navidrome.org>

* fix: enforce playlist visibility in smart playlist InPlaylist/NotInPlaylist rules

Previously, the InPlaylist/NotInPlaylist smart playlist criteria only
allowed referencing public playlists, regardless of who owned the smart
playlist. This was too restrictive for owners referencing their own
private playlists and for admins who should have unrestricted access.

The fix passes the smart playlist owner's identity and admin status into
the criteria SQL builder, so that: admins can reference any playlist,
regular users can reference public playlists plus their own private ones,
and inaccessible referenced playlists produce a warning instead of a hard
error. Also prevents recursive refresh of child playlists the owner
cannot access.

* test(e2e): clarify user roles and fix playlist visibility tests

Renamed testUser/otherUser to adminUser/regularUser to make the admin
vs regular user distinction explicit in test code. Fixed three playlist
visibility tests that were evaluating as admin (bypassing all access
checks) instead of as a regular user, so the public playlist path is
now actually exercised. All playlist operator tests now use explicit
evaluateRuleAs calls with the appropriate user role.

* fix: sync rulesSQL criteria after limitPercent resolution

The rulesSQL struct captures a copy of rules at creation time. When
limitPercent is resolved later, rules.Limit is updated but rulesSQL
still holds the stale value. This caused percentage-based smart playlist
limits to be silently ignored. Fix by updating rulesSQL.criteria after
the resolution.

* refactor: convert inList to a method on smartPlaylistCriteria

The inList function already receives ownerID and ownerIsAdmin from the
smartPlaylistCriteria caller. Making it a method lets it access those
fields directly from the receiver, simplifying the signature and staying
consistent with exprSQL which was already converted to a method.

* refactor: simplify function signatures by removing type parameters in criteria_sql.go

Signed-off-by: Deluan <deluan@navidrome.org>

---------

Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
Deluan Quintão 2026-04-25 14:59:06 -04:00 committed by GitHub
parent 251cc71e2d
commit ca09070a6c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 176 additions and 67 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

65
persistence/criteria_sql.go
View file

@ -8,7 +8,7 @@ import (
"strings" "strings"
"time" "time"
squirrel "github.com/Masterminds/squirrel" "github.com/Masterminds/squirrel"
"github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/criteria"
) )
@ -32,11 +32,24 @@ type smartPlaylistField struct {
} }
type smartPlaylistCriteria struct { type smartPlaylistCriteria struct {
criteria criteria.Criteria criteria criteria.Criteria
ownerID string
ownerIsAdmin bool
} }
func newSmartPlaylistCriteria(c criteria.Criteria) smartPlaylistCriteria { func newSmartPlaylistCriteria(c criteria.Criteria, opts ...func(*smartPlaylistCriteria)) smartPlaylistCriteria {
return smartPlaylistCriteria{criteria: c} cSQL := smartPlaylistCriteria{criteria: c}
for _, opt := range opts {
opt(&cSQL)
}
return cSQL
}
func withSmartPlaylistOwner(ownerID string, ownerIsAdmin bool) func(*smartPlaylistCriteria) {
return func(c *smartPlaylistCriteria) {
c.ownerID = ownerID
c.ownerIsAdmin = ownerIsAdmin
}
} }
var smartPlaylistFields = map[string]smartPlaylistField{ var smartPlaylistFields = map[string]smartPlaylistField{
@ -109,15 +122,15 @@ func (c smartPlaylistCriteria) Where() (squirrel.Sqlizer, error) {
if c.criteria.Expression == nil { if c.criteria.Expression == nil {
return squirrel.Expr("1 = 1"), nil return squirrel.Expr("1 = 1"), nil
} }
return exprSQL(c.criteria.Expression) return c.exprSQL(c.criteria.Expression)
} }
func exprSQL(expr criteria.Expression) (squirrel.Sqlizer, error) { func (c smartPlaylistCriteria) exprSQL(expr criteria.Expression) (squirrel.Sqlizer, error) {
switch e := expr.(type) { switch e := expr.(type) {
case criteria.All: case criteria.All:
and := squirrel.And{} and := squirrel.And{}
for _, child := range e { for _, child := range e {
cond, err := exprSQL(child) cond, err := c.exprSQL(child)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -127,7 +140,7 @@ func exprSQL(expr criteria.Expression) (squirrel.Sqlizer, error) {
case criteria.Any: case criteria.Any:
or := squirrel.Or{} or := squirrel.Or{}
for _, child := range e { for _, child := range e {
cond, err := exprSQL(child) cond, err := c.exprSQL(child)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -171,15 +184,15 @@ func exprSQL(expr criteria.Expression) (squirrel.Sqlizer, error) {
case criteria.NotInTheLast: case criteria.NotInTheLast:
return periodExpr(e, true) return periodExpr(e, true)
case criteria.InPlaylist: case criteria.InPlaylist:
return inList(e, false) return c.inList(e, false)
case criteria.NotInPlaylist: case criteria.NotInPlaylist:
return inList(e, true) return c.inList(e, true)
default: default:
return nil, fmt.Errorf("unknown criteria expression type %T", expr) return nil, fmt.Errorf("unknown criteria expression type %T", expr)
} }
} }
func isNotExpr[T ~map[string]any](values T) (squirrel.Sqlizer, error) { func isNotExpr(values map[string]any) (squirrel.Sqlizer, error) {
if _, value, info, ok := singleField(values); ok && (info.IsTag || info.IsRole) { if _, value, info, ok := singleField(values); ok && (info.IsTag || info.IsRole) {
return jsonExpr(info, squirrel.Eq{"value": value}, true), nil return jsonExpr(info, squirrel.Eq{"value": value}, true), nil
} }
@ -190,7 +203,7 @@ func isNotExpr[T ~map[string]any](values T) (squirrel.Sqlizer, error) {
return squirrel.NotEq(fields), nil return squirrel.NotEq(fields), nil
} }
func mapExpr[T ~map[string]any](values T, makeCond func(map[string]any) squirrel.Sqlizer, negateJSON bool) (squirrel.Sqlizer, error) { func mapExpr(values map[string]any, makeCond func(map[string]any) squirrel.Sqlizer, negateJSON bool) (squirrel.Sqlizer, error) {
if _, value, info, ok := singleField(values); ok && (info.IsTag || info.IsRole) { if _, value, info, ok := singleField(values); ok && (info.IsTag || info.IsRole) {
return jsonExpr(info, makeCond(map[string]any{"value": value}), negateJSON), nil return jsonExpr(info, makeCond(map[string]any{"value": value}), negateJSON), nil
} }
@ -201,7 +214,7 @@ func mapExpr[T ~map[string]any](values T, makeCond func(map[string]any) squirrel
return makeCond(fields), nil return makeCond(fields), nil
} }
func likeExpr[T ~map[string]any](values T, pattern string, negate bool) (squirrel.Sqlizer, error) { func likeExpr(values map[string]any, pattern string, negate bool) (squirrel.Sqlizer, error) {
if _, value, info, ok := singleField(values); ok && (info.IsTag || info.IsRole) { if _, value, info, ok := singleField(values); ok && (info.IsTag || info.IsRole) {
return jsonExpr(info, squirrel.Like{"value": fmt.Sprintf(pattern, value)}, negate), nil return jsonExpr(info, squirrel.Like{"value": fmt.Sprintf(pattern, value)}, negate), nil
} }
@ -223,7 +236,7 @@ func likeExpr[T ~map[string]any](values T, pattern string, negate bool) (squirre
return lk, nil return lk, nil
} }
func rangeExpr[T ~map[string]any](values T) (squirrel.Sqlizer, error) { func rangeExpr(values map[string]any) (squirrel.Sqlizer, error) {
fields, err := sqlFields(values) fields, err := sqlFields(values)
if err != nil { if err != nil {
return nil, err return nil, err
@ -242,7 +255,7 @@ func rangeExpr[T ~map[string]any](values T) (squirrel.Sqlizer, error) {
return and, nil return and, nil
} }
func periodExpr[T ~map[string]any](values T, negate bool) (squirrel.Sqlizer, error) { func periodExpr(values map[string]any, negate bool) (squirrel.Sqlizer, error) {
fields, err := sqlFields(values) fields, err := sqlFields(values)
if err != nil { if err != nil {
return nil, err return nil, err
@ -271,18 +284,26 @@ func startOfPeriod(numDays int64, from time.Time) string {
return from.Add(time.Duration(-24*numDays) * time.Hour).Format("2006-01-02") return from.Add(time.Duration(-24*numDays) * time.Hour).Format("2006-01-02")
} }
func inList[T ~map[string]any](values T, negate bool) (squirrel.Sqlizer, error) { func (c smartPlaylistCriteria) inList(values map[string]any, negate bool) (squirrel.Sqlizer, error) {
playlistID, ok := values["id"].(string) playlistID, ok := values["id"].(string)
if !ok { if !ok {
return nil, errors.New("playlist id not given") return nil, errors.New("playlist id not given")
} }
filters := squirrel.And{squirrel.Eq{"pl.playlist_id": playlistID}}
if !c.ownerIsAdmin {
if c.ownerID == "" {
filters = append(filters, squirrel.Eq{"playlist.public": 1})
} else {
filters = append(filters, squirrel.Or{
squirrel.Eq{"playlist.public": 1},
squirrel.Eq{"playlist.owner_id": c.ownerID},
})
}
}
subQuery := squirrel.Select("media_file_id"). subQuery := squirrel.Select("media_file_id").
From("playlist_tracks pl"). From("playlist_tracks pl").
LeftJoin("playlist on pl.playlist_id = playlist.id"). LeftJoin("playlist on pl.playlist_id = playlist.id").
Where(squirrel.And{ Where(filters)
squirrel.Eq{"pl.playlist_id": playlistID},
squirrel.Eq{"playlist.public": 1},
})
subSQL, subArgs, err := subQuery.PlaceholderFormat(squirrel.Question).ToSql() subSQL, subArgs, err := subQuery.PlaceholderFormat(squirrel.Question).ToSql()
if err != nil { if err != nil {
return nil, err return nil, err
@ -334,7 +355,7 @@ func (e roleCond) ToSql() (string, []any, error) {
return cond, args, err return cond, args, err
} }
func singleField[T ~map[string]any](values T) (string, any, criteria.FieldInfo, bool) { func singleField(values map[string]any) (string, any, criteria.FieldInfo, bool) {
if len(values) != 1 { if len(values) != 1 {
return "", nil, criteria.FieldInfo{}, false return "", nil, criteria.FieldInfo{}, false
} }
@ -345,7 +366,7 @@ func singleField[T ~map[string]any](values T) (string, any, criteria.FieldInfo,
return "", nil, criteria.FieldInfo{}, false return "", nil, criteria.FieldInfo{}, false
} }
func sqlFields[T ~map[string]any](values T) (map[string]any, error) { func sqlFields(values map[string]any) (map[string]any, error) {
fields := make(map[string]any, len(values)) fields := make(map[string]any, len(values))
for field, value := range values { for field, value := range values {
info, ok := criteria.LookupField(field) info, ok := criteria.LookupField(field)

28
persistence/criteria_sql_test.go
View file

@ -59,6 +59,34 @@ var _ = Describe("Smart playlist criteria SQL", func() {
Entry("role not contains", criteria.NotContains{"artist": "u2"}, "not exists (select 1 from json_tree(media_file.participants, '$.artist') where key='name' and value LIKE ?)", "%u2%"), Entry("role not contains", criteria.NotContains{"artist": "u2"}, "not exists (select 1 from json_tree(media_file.participants, '$.artist') where key='name' and value LIKE ?)", "%u2%"),
) )
Describe("playlist permissions", func() {
It("allows public or same-owner playlist references for regular users", func() {
sqlizer, err := newSmartPlaylistCriteria(
criteria.Criteria{Expression: criteria.InPlaylist{"id": "deadbeef-dead-beef"}},
withSmartPlaylistOwner("owner-id", false),
).Where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal("media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND (playlist.public = ? OR playlist.owner_id = ?)))"))
Expect(args).To(HaveExactElements("deadbeef-dead-beef", 1, "owner-id"))
})
It("allows all playlist references for admins", func() {
sqlizer, err := newSmartPlaylistCriteria(
criteria.Criteria{Expression: criteria.InPlaylist{"id": "deadbeef-dead-beef"}},
withSmartPlaylistOwner("admin-id", true),
).Where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal("media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ?))"))
Expect(args).To(HaveExactElements("deadbeef-dead-beef"))
})
})
It("builds relative date expressions", func() { It("builds relative date expressions", func() {
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheLast{"lastPlayed": 30}}).Where() sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheLast{"lastPlayed": 30}}).Where()
Expect(err).ToNot(HaveOccurred()) Expect(err).ToNot(HaveOccurred())

59
core/playlists/e2e/e2e_suite_test.go → persistence/e2e/e2e_suite_test.go
View file

@ -53,14 +53,14 @@ var (
snapshotPath string snapshotPath string
snapshotTables []string snapshotTables []string
testUser = model.User{ adminUser = model.User{
ID: "sp-test-user-1", ID: "sp-test-user-1",
UserName: "sptestuser", UserName: "sptestuser",
Name: "SP Test User", Name: "SP Test User",
IsAdmin: true, IsAdmin: true,
} }
otherUser = model.User{ regularUser = model.User{
ID: "sp-test-user-2", ID: "sp-test-user-2",
UserName: "spotheruser", UserName: "spotheruser",
Name: "SP Other User", Name: "SP Other User",
@ -147,25 +147,36 @@ func findMediaFileByTitle(title string) string {
} }
func evaluateRule(jsonRule string) []string { func evaluateRule(jsonRule string) []string {
titles := evaluateRuleOrdered(jsonRule) titles := evaluateRuleOrderedAs(adminUser, jsonRule)
sort.Strings(titles) sort.Strings(titles)
return titles return titles
} }
func evaluateRuleOrdered(jsonRule string) []string { func evaluateRuleOrdered(jsonRule string) []string {
return evaluateRuleOrderedAs(adminUser, jsonRule)
}
func evaluateRuleAs(owner model.User, jsonRule string) []string {
titles := evaluateRuleOrderedAs(owner, jsonRule)
sort.Strings(titles)
return titles
}
func evaluateRuleOrderedAs(owner model.User, jsonRule string) []string {
userCtx := request.WithUser(GinkgoT().Context(), owner)
var rules criteria.Criteria var rules criteria.Criteria
err := json.Unmarshal([]byte(jsonRule), &rules) err := json.Unmarshal([]byte(jsonRule), &rules)
Expect(err).ToNot(HaveOccurred(), "invalid criteria JSON: %s", jsonRule) Expect(err).ToNot(HaveOccurred(), "invalid criteria JSON: %s", jsonRule)
pls := &model.Playlist{ pls := &model.Playlist{
Name: "test-smart-playlist", Name: "test-smart-playlist",
OwnerID: testUser.ID, OwnerID: owner.ID,
Rules: &rules, Rules: &rules,
} }
err = ds.Playlist(ctx).Put(pls) err = ds.Playlist(userCtx).Put(pls)
Expect(err).ToNot(HaveOccurred()) Expect(err).ToNot(HaveOccurred())
loaded, err := ds.Playlist(ctx).GetWithTracks(pls.ID, true, false) loaded, err := ds.Playlist(userCtx).GetWithTracks(pls.ID, true, false)
Expect(err).ToNot(HaveOccurred()) Expect(err).ToNot(HaveOccurred())
titles := make([]string, len(loaded.Tracks)) titles := make([]string, len(loaded.Tracks))
@ -198,12 +209,20 @@ func createPrivatePlaylist(owner model.User, titles ...string) string {
} }
func createPublicSmartPlaylist(owner model.User, jsonRule string) string { func createPublicSmartPlaylist(owner model.User, jsonRule string) string {
return createSmartPlaylist(owner, true, jsonRule)
}
func createPrivateSmartPlaylist(owner model.User, jsonRule string) string {
return createSmartPlaylist(owner, false, jsonRule)
}
func createSmartPlaylist(owner model.User, public bool, jsonRule string) string {
var rules criteria.Criteria var rules criteria.Criteria
Expect(json.Unmarshal([]byte(jsonRule), &rules)).To(Succeed()) Expect(json.Unmarshal([]byte(jsonRule), &rules)).To(Succeed())
pls := &model.Playlist{ pls := &model.Playlist{
Name: "ref-smart-playlist", Name: "ref-smart-playlist",
OwnerID: owner.ID, OwnerID: owner.ID,
Public: true, Public: public,
Rules: &rules, Rules: &rules,
} }
Expect(ds.Playlist(ctx).Put(pls)).To(Succeed()) Expect(ds.Playlist(ctx).Put(pls)).To(Succeed())
@ -211,7 +230,7 @@ func createPublicSmartPlaylist(owner model.User, jsonRule string) string {
} }
var _ = BeforeSuite(func() { var _ = BeforeSuite(func() {
ctx = request.WithUser(GinkgoT().Context(), testUser) ctx = request.WithUser(GinkgoT().Context(), adminUser)
tmpDir := GinkgoT().TempDir() tmpDir := GinkgoT().TempDir()
dbFilePath = filepath.Join(tmpDir, "smartplaylist-e2e.db") dbFilePath = filepath.Join(tmpDir, "smartplaylist-e2e.db")
snapshotPath = filepath.Join(tmpDir, "smartplaylist-e2e.db.snapshot") snapshotPath = filepath.Join(tmpDir, "smartplaylist-e2e.db.snapshot")
@ -226,28 +245,28 @@ var _ = BeforeSuite(func() {
initDS := &tests.MockDataStore{RealDS: persistence.New(db.Db())} initDS := &tests.MockDataStore{RealDS: persistence.New(db.Db())}
userWithPass := testUser userWithPass := adminUser
userWithPass.NewPassword = "password" userWithPass.NewPassword = "password"
Expect(initDS.User(ctx).Put(&userWithPass)).To(Succeed()) Expect(initDS.User(ctx).Put(&userWithPass)).To(Succeed())
otherUserWithPass := otherUser regularUserWithPass := regularUser
otherUserWithPass.NewPassword = "password" regularUserWithPass.NewPassword = "password"
Expect(initDS.User(ctx).Put(&otherUserWithPass)).To(Succeed()) Expect(initDS.User(ctx).Put(&regularUserWithPass)).To(Succeed())
lib = model.Library{ID: 1, Name: "Music Library", Path: "fake:///music"} lib = model.Library{ID: 1, Name: "Music Library", Path: "fake:///music"}
Expect(initDS.Library(ctx).Put(&lib)).To(Succeed()) Expect(initDS.Library(ctx).Put(&lib)).To(Succeed())
Expect(initDS.User(ctx).SetUserLibraries(testUser.ID, []int{lib.ID})).To(Succeed()) Expect(initDS.User(ctx).SetUserLibraries(adminUser.ID, []int{lib.ID})).To(Succeed())
Expect(initDS.User(ctx).SetUserLibraries(otherUser.ID, []int{lib.ID})).To(Succeed()) Expect(initDS.User(ctx).SetUserLibraries(regularUser.ID, []int{lib.ID})).To(Succeed())
loadedUser, err := initDS.User(ctx).FindByUsername(testUser.UserName) loadedUser, err := initDS.User(ctx).FindByUsername(adminUser.UserName)
Expect(err).ToNot(HaveOccurred()) Expect(err).ToNot(HaveOccurred())
testUser.Libraries = loadedUser.Libraries adminUser.Libraries = loadedUser.Libraries
loadedOther, err := initDS.User(ctx).FindByUsername(otherUser.UserName) loadedOther, err := initDS.User(ctx).FindByUsername(regularUser.UserName)
Expect(err).ToNot(HaveOccurred()) Expect(err).ToNot(HaveOccurred())
otherUser.Libraries = loadedOther.Libraries regularUser.Libraries = loadedOther.Libraries
ctx = request.WithUser(GinkgoT().Context(), testUser) ctx = request.WithUser(GinkgoT().Context(), adminUser)
buildTestFS() buildTestFS()
s := scanner.New(ctx, initDS, artwork.NoopCacheWarmer(), events.NoopBroker(), s := scanner.New(ctx, initDS, artwork.NoopCacheWarmer(), events.NoopBroker(),
@ -315,7 +334,7 @@ func restoreDB() {
} }
func setupTestDB() { func setupTestDB() {
ctx = request.WithUser(GinkgoT().Context(), testUser) ctx = request.WithUser(GinkgoT().Context(), adminUser)
DeferCleanup(configtest.SetupConfig()) DeferCleanup(configtest.SetupConfig())
conf.Server.MusicFolder = "fake:///music" conf.Server.MusicFolder = "fake:///music"
conf.Server.DevExternalScanner = false conf.Server.DevExternalScanner = false

74
core/playlists/e2e/smartplaylist_test.go → persistence/e2e/smartplaylist_test.go
View file

@ -1,8 +1,10 @@
package e2e package e2e
import ( import (
"github.com/navidrome/navidrome/tests"
. "github.com/onsi/ginkgo/v2" . "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega" . "github.com/onsi/gomega"
"github.com/sirupsen/logrus"
) )
var _ = Describe("Smart Playlists", func() { var _ = Describe("Smart Playlists", func() {
@ -245,46 +247,82 @@ var _ = Describe("Smart Playlists", func() {
Describe("Playlist operators", func() { Describe("Playlist operators", func() {
It("matches tracks in a public regular playlist", func() { It("matches tracks in a public regular playlist", func() {
refID := createPublicPlaylist(testUser, "Come Together", "So What") refID := createPublicPlaylist(adminUser, "Come Together", "So What")
results := evaluateRule(`{"all":[{"inPlaylist":{"id":"` + refID + `"}}]}`) results := evaluateRuleAs(regularUser, `{"all":[{"inPlaylist":{"id":"`+refID+`"}}]}`)
Expect(results).To(ConsistOf("Come Together", "So What")) Expect(results).To(ConsistOf("Come Together", "So What"))
}) })
It("matches tracks not in a public regular playlist", func() { It("matches tracks not in a public regular playlist", func() {
refID := createPublicPlaylist(testUser, "Come Together", "So What") refID := createPublicPlaylist(adminUser, "Come Together", "So What")
results := evaluateRule(`{"all":[{"notInPlaylist":{"id":"` + refID + `"}}]}`) results := evaluateRuleAs(regularUser, `{"all":[{"notInPlaylist":{"id":"`+refID+`"}}]}`)
Expect(results).To(ConsistOf("Something", "Stairway To Heaven", "Black Dog", Expect(results).To(ConsistOf("Something", "Stairway To Heaven", "Black Dog",
"Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions")) "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions"))
}) })
It("recursively refreshes a referenced smart playlist owned by the same user", func() { It("recursively refreshes a referenced smart playlist owned by the same user", func() {
smartBID := createPublicSmartPlaylist(testUser, `{"all":[{"is":{"genre":"Jazz"}}]}`) smartBID := createPublicSmartPlaylist(adminUser, `{"all":[{"is":{"genre":"Jazz"}}]}`)
results := evaluateRule(`{"all":[{"inPlaylist":{"id":"` + smartBID + `"}}]}`) results := evaluateRuleAs(adminUser, `{"all":[{"inPlaylist":{"id":"`+smartBID+`"}}]}`)
Expect(results).To(ConsistOf("So What")) Expect(results).To(ConsistOf("So What"))
}) })
It("does not refresh a referenced smart playlist owned by another user", func() { It("does not refresh a referenced smart playlist owned by another user", func() {
smartBID := createPublicSmartPlaylist(otherUser, `{"all":[{"is":{"genre":"Jazz"}}]}`) smartBID := createPublicSmartPlaylist(regularUser, `{"all":[{"is":{"genre":"Jazz"}}]}`)
results := evaluateRule(`{"all":[{"inPlaylist":{"id":"` + smartBID + `"}}]}`) results := evaluateRuleAs(adminUser, `{"all":[{"inPlaylist":{"id":"`+smartBID+`"}}]}`)
Expect(results).To(BeEmpty()) Expect(results).To(BeEmpty())
}) })
It("does not match tracks from a private playlist", func() { It("does not refresh a playlist or its children when an admin views another user's smart playlist", func() {
refID := createPrivatePlaylist(testUser, "Come Together", "So What") smartBID := createPrivateSmartPlaylist(adminUser, `{"all":[{"is":{"genre":"Jazz"}}]}`)
results := evaluateRule(`{"all":[{"inPlaylist":{"id":"` + refID + `"}}]}`) smartAID := createPublicSmartPlaylist(regularUser, `{"all":[{"inPlaylist":{"id":"`+smartBID+`"}}]}`)
Expect(results).To(BeEmpty())
loadedA, err := ds.Playlist(ctx).GetWithTracks(smartAID, true, false)
Expect(err).ToNot(HaveOccurred())
Expect(loadedA.Tracks).To(BeEmpty())
Expect(loadedA.EvaluatedAt).To(BeNil())
loadedB, err := ds.Playlist(ctx).Get(smartBID)
Expect(err).ToNot(HaveOccurred())
Expect(loadedB.EvaluatedAt).To(BeNil())
}) })
It("matches tracks in a public playlist owned by another user", func() { It("matches tracks from a private playlist owned by the same user", func() {
refID := createPublicPlaylist(otherUser, "Bohemian Rhapsody") refID := createPrivatePlaylist(regularUser, "Come Together", "So What")
results := evaluateRule(`{"all":[{"inPlaylist":{"id":"` + refID + `"}}]}`) results := evaluateRuleAs(regularUser, `{"all":[{"inPlaylist":{"id":"`+refID+`"}}]}`)
Expect(results).To(ConsistOf("Come Together", "So What"))
})
It("allows admin-owned smart playlists to reference private playlists owned by other users", func() {
refID := createPrivatePlaylist(regularUser, "Bohemian Rhapsody")
results := evaluateRuleAs(adminUser, `{"all":[{"inPlaylist":{"id":"`+refID+`"}}]}`)
Expect(results).To(ConsistOf("Bohemian Rhapsody")) Expect(results).To(ConsistOf("Bohemian Rhapsody"))
}) })
It("does not match tracks from a private playlist owned by another user", func() { It("does not match tracks from a private playlist owned by another regular user", func() {
refID := createPrivatePlaylist(otherUser, "Bohemian Rhapsody") refID := createPrivatePlaylist(adminUser, "Come Together", "So What")
results := evaluateRule(`{"all":[{"inPlaylist":{"id":"` + refID + `"}}]}`) results := evaluateRuleAs(regularUser, `{"all":[{"inPlaylist":{"id":"`+refID+`"}}]}`)
Expect(results).To(BeEmpty()) Expect(results).To(BeEmpty())
}) })
It("warns when a referenced playlist is inaccessible to the smart playlist owner", func() {
hook, cleanup := tests.LogHook()
defer cleanup()
refID := createPrivatePlaylist(adminUser, "Come Together")
results := evaluateRuleAs(regularUser, `{"all":[{"notInPlaylist":{"id":"`+refID+`"}}]}`)
Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven", "Black Dog",
"So What", "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions"))
Expect(hook.LastEntry()).ToNot(BeNil())
Expect(hook.LastEntry().Level).To(Equal(logrus.WarnLevel))
Expect(hook.LastEntry().Message).To(Equal("Referenced playlist is not accessible to smart playlist owner"))
Expect(hook.LastEntry().Data).To(HaveKeyWithValue("childId", refID))
})
It("matches tracks in a public playlist owned by another user", func() {
refID := createPublicPlaylist(adminUser, "Bohemian Rhapsody")
results := evaluateRuleAs(regularUser, `{"all":[{"inPlaylist":{"id":"`+refID+`"}}]}`)
Expect(results).To(ConsistOf("Bohemian Rhapsody"))
})
}) })
}) })

17
persistence/playlist_repository.go
View file

@ -14,7 +14,6 @@ import (
"github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/criteria"
"github.com/pocketbase/dbx" "github.com/pocketbase/dbx"
) )
@ -228,13 +227,17 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool {
// Re-populate playlist based on Smart Playlist criteria // Re-populate playlist based on Smart Playlist criteria
rules := *pls.Rules rules := *pls.Rules
rulesSQL := newSmartPlaylistCriteria(rules) rulesSQL := newSmartPlaylistCriteria(rules, withSmartPlaylistOwner(pls.OwnerID, usr.IsAdmin))
// If the playlist depends on other playlists, recursively refresh them first // If the playlist depends on other playlists, recursively refresh them first
childPlaylistIds := rules.ChildPlaylistIds() childPlaylistIds := rules.ChildPlaylistIds()
for _, id := range childPlaylistIds { for _, id := range childPlaylistIds {
childPls, err := r.Get(id) childPls, err := r.Get(id)
if err != nil { if err != nil {
if errors.Is(err, model.ErrNotFound) {
log.Warn(r.ctx, "Referenced playlist is not accessible to smart playlist owner", "playlist", pls.Name, "id", pls.ID, "childId", id, "ownerId", pls.OwnerID)
continue
}
log.Error(r.ctx, "Error loading child playlist", "id", pls.ID, "childId", id, err) log.Error(r.ctx, "Error loading child playlist", "id", pls.ID, "childId", id, err)
return false return false
} }
@ -283,10 +286,11 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool {
log.Debug(r.ctx, "Resolved percentage limit", "playlist", pls.Name, "percent", rules.LimitPercent, "totalMatching", res.Count, "resolvedLimit", resolvedLimit) log.Debug(r.ctx, "Resolved percentage limit", "playlist", pls.Name, "percent", rules.LimitPercent, "totalMatching", res.Count, "resolvedLimit", resolvedLimit)
rules.Limit = resolvedLimit rules.Limit = resolvedLimit
rules.LimitPercent = 0 rules.LimitPercent = 0
rulesSQL.criteria = rules
} }
// Apply the criteria rules // Apply the criteria rules
sq, err = r.addCriteria(sq, rules) sq, err = r.addCriteria(sq, rulesSQL)
if err != nil { if err != nil {
log.Error(r.ctx, "Error building smart playlist criteria", "playlist", pls.Name, "id", pls.ID, err) log.Error(r.ctx, "Error building smart playlist criteria", "playlist", pls.Name, "id", pls.ID, err)
return false return false
@ -337,15 +341,14 @@ func (r *playlistRepository) addSmartPlaylistAnnotationJoins(sq SelectBuilder, j
return sq return sq
} }
func (r *playlistRepository) addCriteria(sql SelectBuilder, c criteria.Criteria) (SelectBuilder, error) { func (r *playlistRepository) addCriteria(sql SelectBuilder, cSQL smartPlaylistCriteria) (SelectBuilder, error) {
cSQL := newSmartPlaylistCriteria(c)
cond, err := cSQL.Where() cond, err := cSQL.Where()
if err != nil { if err != nil {
return sql, err return sql, err
} }
sql = sql.Where(cond) sql = sql.Where(cond)
if c.Limit > 0 { if cSQL.criteria.Limit > 0 {
sql = sql.Limit(uint64(c.Limit)).Offset(uint64(c.Offset)) sql = sql.Limit(uint64(cSQL.criteria.Limit)).Offset(uint64(cSQL.criteria.Offset))
} }
if order := cSQL.OrderBy(); order != "" { if order := cSQL.OrderBy(); order != "" {
sql = sql.OrderBy(order) sql = sql.OrderBy(order)