test(playlists): cover rules-only PUT + case-variant owner-change guard

Follow-ups from manual testing and code review of the prior commit:

- Manual testing confirmed Feishin-style rules-only PUT works correctly
  on the fix; add ginkgo regression tests for rules-only update, name+
  rules combined, idempotent rules PUT (no-op), and bulk Make-Public
  preserving rules on smart playlists.
- Keep the non-admin owner-change permission check gated on the
  deserialized entity content (not on `sent("ownerId")`) so a
  case-variant JSON key like {"OwnerId":"x"} can't downgrade the 403
  to a silent 200. Go's json decoder is case-insensitive on struct
  field matching but rest.Put's field-name extraction is case-
  sensitive; the entity-based guard catches both spellings. The
  apply-side gating on ownerChanged still prevents the actual mutation,
  so this was a behavioral (not security) regression, but worth fixing.
  Adds a regression test asserting the case-variant key still returns
  rest.ErrPermissionDenied.
- Correct misleading doc on applyContentUpdate: the path does not
  rewrite the backing M3U file; it goes through updateMetadata which
  bumps updatedAt and invalidates cached cover-art URLs.
This commit is contained in:
Deluan 2026-05-27 20:09:29 -03:00
parent 067944817e
commit a75f820fa3
2 changed files with 92 additions and 3 deletions

View File

@ -105,7 +105,11 @@ func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity
usr, _ := request.UserFrom(ctx)
ownerChanged := sent("ownerId") && entity.OwnerID != "" && entity.OwnerID != current.OwnerID
if !usr.IsAdmin && ownerChanged {
// Permission check uses the deserialized entity directly (not gated by `sent`)
// so a non-admin can't smuggle in an owner change via a case-variant JSON key
// like {"OwnerId":"x"} — Go's json decoder is case-insensitive on field match
// but rest.Put's field-name extraction is case-sensitive.
if !usr.IsAdmin && entity.OwnerID != "" && entity.OwnerID != current.OwnerID {
return rest.ErrPermissionDenied
}
@ -121,8 +125,9 @@ func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity
}
// applyContentUpdate handles updates that change at least one of name/comment/
// owner/rules — the path that goes through updateMetadata and may rewrite the
// backing M3U file. Pointer args are nil for fields not present in the request.
// owner/rules. It goes through updateMetadata, which always bumps updatedAt
// (invalidating cached cover-art URLs). Pointer args are nil for fields not
// present in the request.
func (s *playlists) applyContentUpdate(ctx context.Context, current, entity *model.Playlist,
sent func(string) bool, nameChanged, commentChanged, ownerChanged, rulesChanged bool,
) error {

View File

@ -125,6 +125,18 @@ var _ = Describe("REST Adapter", func() {
Expect(err).To(Equal(rest.ErrPermissionDenied))
})
It("denies regular user even when ownerId arrives under a case-variant JSON key", func() {
// rest.Put's field-name extraction is case-sensitive, but Go's json
// decoder is case-insensitive on struct fields, so {"OwnerId":"x"}
// populates entity.OwnerID while cols carries "OwnerId" instead of
// "ownerId". The permission gate must still fire.
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
repo = ps.NewRepository(ctx).(rest.Persistable)
pls := &model.Playlist{OwnerID: "other-user"}
err := repo.Update("pls-1", pls, "OwnerId")
Expect(err).To(Equal(rest.ErrPermissionDenied))
})
It("updates smart playlist rules", func() {
mockPlsRepo.Data["smart-1"] = &model.Playlist{
ID: "smart-1",
@ -276,6 +288,78 @@ var _ = Describe("REST Adapter", func() {
Expect(mockPlsRepo.Last.Name).To(Equal("Original Name"))
})
It("updates rules-only on a smart playlist (Feishin-style edit)", func() {
mockPlsRepo.Data["smart-partial"] = &model.Playlist{
ID: "smart-partial",
Name: "Smart Original",
Comment: "smart comment",
OwnerID: "user-1",
Public: true,
Rules: &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}},
}
repo = ps.NewRepository(ctx).(rest.Persistable)
newRules := &criteria.Criteria{Expression: criteria.Is{"genre": "Jazz"}, Sort: "year DESC"}
err := repo.Update("smart-partial", &model.Playlist{Rules: newRules}, "rules")
Expect(err).ToNot(HaveOccurred())
Expect(mockPlsRepo.Last.Rules).To(Equal(newRules))
Expect(mockPlsRepo.Last.Name).To(Equal("Smart Original"))
Expect(mockPlsRepo.Last.Comment).To(Equal("smart comment"))
Expect(mockPlsRepo.Last.Public).To(BeTrue())
})
It("updates name and rules together (smart-playlist Edit form)", func() {
mockPlsRepo.Data["smart-edit"] = &model.Playlist{
ID: "smart-edit",
Name: "Smart Original",
Comment: "smart comment",
OwnerID: "user-1",
Rules: &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}},
}
repo = ps.NewRepository(ctx).(rest.Persistable)
newRules := &criteria.Criteria{Expression: criteria.Is{"artist": "Miles Davis"}, Sort: "album"}
err := repo.Update("smart-edit",
&model.Playlist{Name: "Smart Renamed", Rules: newRules},
"name", "rules")
Expect(err).ToNot(HaveOccurred())
Expect(mockPlsRepo.Last.Name).To(Equal("Smart Renamed"))
Expect(mockPlsRepo.Last.Rules).To(Equal(newRules))
Expect(mockPlsRepo.Last.Comment).To(Equal("smart comment"))
})
It("does not bump the saved rules on an idempotent rules-only PUT", func() {
rules := &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}}
mockPlsRepo.Data["smart-idempotent"] = &model.Playlist{
ID: "smart-idempotent",
Name: "Smart Idempotent",
OwnerID: "user-1",
Rules: rules,
}
repo = ps.NewRepository(ctx).(rest.Persistable)
// Same rules sent back — rulesEqual should report no change and
// the request should no-op (no Put call).
sameRules := &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}}
err := repo.Update("smart-idempotent", &model.Playlist{Rules: sameRules}, "rules")
Expect(err).ToNot(HaveOccurred())
Expect(mockPlsRepo.Last).To(BeNil()) // no Put happened
})
It("preserves rules when only public is sent (smart playlist + bulk Make Public)", func() {
rules := &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}}
mockPlsRepo.Data["smart-public"] = &model.Playlist{
ID: "smart-public",
Name: "Smart Public",
OwnerID: "user-1",
Public: false,
Rules: rules,
}
repo = ps.NewRepository(ctx).(rest.Persistable)
err := repo.Update("smart-public", &model.Playlist{Public: true}, "public")
Expect(err).ToNot(HaveOccurred())
Expect(mockPlsRepo.Last.Public).To(BeTrue())
Expect(mockPlsRepo.Last.Rules).To(Equal(rules))
Expect(mockPlsRepo.Last.Name).To(Equal("Smart Public"))
})
It("does not treat a missing ownerId as an ownership transfer attempt", func() {
// A non-admin user sending only {public:true} should not be blocked
// just because OwnerID is the zero value in the deserialized entity.