From 65bd4df49c53dc37e05762b5a1a89a20990ba114 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 27 May 2026 23:24:43 -0300 Subject: [PATCH] refactor(playlists): drop redundant entity-based owner-permission guard The case-insensitive sentFields predicate already prevents case-variant JSON keys like {"OwnerId":"x"} from bypassing the ownerChanged check, so the duplicated entity-content guard is no longer load-bearing. Strengthen the regression test into a DescribeTable covering canonical, PascalCase, all-upper, and all-lower spellings to lock in the case-insensitive contract. --- core/playlists/rest_adapter.go | 6 +----- core/playlists/rest_adapter_test.go | 29 ++++++++++++++++++----------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/core/playlists/rest_adapter.go b/core/playlists/rest_adapter.go index 8a6e39321..3f886aadd 100644 --- a/core/playlists/rest_adapter.go +++ b/core/playlists/rest_adapter.go @@ -106,11 +106,7 @@ func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity usr, _ := request.UserFrom(ctx) ownerChanged := sent("ownerId") && entity.OwnerID != "" && entity.OwnerID != current.OwnerID - // 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 { + if !usr.IsAdmin && ownerChanged { return rest.ErrPermissionDenied } diff --git a/core/playlists/rest_adapter_test.go b/core/playlists/rest_adapter_test.go index 83bf58827..79d72d147 100644 --- a/core/playlists/rest_adapter_test.go +++ b/core/playlists/rest_adapter_test.go @@ -125,17 +125,24 @@ 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)) - }) + DescribeTable("denies regular user from changing ownership under any case-variant JSON key", + func(colName string) { + // rest.Put's field-name extraction is case-sensitive, but Go's + // json decoder is case-insensitive on struct fields, so any + // {"OwnerId":"x"} / {"OWNERID":"x"} / {"ownerid":"x"} populates + // entity.OwnerID. sentFields normalizes both sides so the + // permission gate fires regardless of casing. + 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, colName) + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }, + Entry("canonical camelCase", "ownerId"), + Entry("PascalCase", "OwnerId"), + Entry("all upper", "OWNERID"), + Entry("all lower", "ownerid"), + ) It("updates smart playlist rules", func() { mockPlsRepo.Data["smart-1"] = &model.Playlist{