fix(share): add ownership checks to Delete and Update (#5189)

* test(share): add failing tests for Delete ownership checks * fix(share): add ownership check to Delete * test(share): add failing tests for Update ownership checks * fix(share): add ownership check to Update * refactor(share): extract checkOwnership helper with lightweight query - Deduplicate ownership check from Delete and Update into a single helper - Use a minimal single-column SELECT instead of Get (avoids loadMedia overhead) - Use positive bypass form (IsAdmin || invalidUserId) matching codebase convention * fix(share): convert model.ErrNotFound to rest.ErrNotFound in checkOwnership Ensure consistent 404 responses when a nonexistent share ID is passed to Delete or Update, by handling the conversion in checkOwnership rather than relying on the subsequent write operation.
2026-03-15 00:12:58 -04:00
parent 197d357f02
commit 6b8fcc37c6
2 changed files with 117 additions and 1 deletions
@@ -4,6 +4,7 @@ import (
 	"context"
 	"time"

+	"github.com/deluan/rest"
 	"github.com/navidrome/navidrome/conf/configtest"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
@@ -130,4 +131,91 @@ var _ = Describe("ShareRepository", func() {
 			Expect(share.Albums).To(BeEmpty())
 		})
 	})
+
+	Describe("Ownership Checks", func() {
+		var ownerUser = model.User{ID: "2222", UserName: "regular-user"}
+		var otherUser = model.User{ID: "3333", UserName: "third-user"}
+
+		insertShare := func(shareID, userID string) {
+			_, err := GetDBXBuilder().NewQuery(`
+				INSERT INTO share (id, user_id, description, resource_type, resource_ids, created_at, updated_at)
+				VALUES ({:id}, {:user}, {:desc}, {:type}, {:ids}, {:created}, {:updated})
+			`).Bind(map[string]any{
+				"id":      shareID,
+				"user":    userID,
+				"desc":    "Test Share",
+				"type":    "media_file",
+				"ids":     "1001",
+				"created": time.Now(),
+				"updated": time.Now(),
+			}).Execute()
+			Expect(err).ToNot(HaveOccurred())
+		}
+
+		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)
+				repo := NewShareRepository(ctx, GetDBXBuilder())
+				err := repo.(rest.Persistable).Delete("own-share-del")
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			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)
+				repo := NewShareRepository(ctx, GetDBXBuilder())
+				err := repo.(rest.Persistable).Delete("other-share-del")
+				Expect(err).To(Equal(rest.ErrPermissionDenied))
+			})
+
+			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)
+				repo := NewShareRepository(ctx, GetDBXBuilder())
+				err := repo.(rest.Persistable).Delete("admin-del-share")
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			It("allows headless context (no user) to delete a share", func() {
+				insertShare("headless-del-share", ownerUser.ID)
+				repo := NewShareRepository(context.Background(), GetDBXBuilder())
+				err := repo.(rest.Persistable).Delete("headless-del-share")
+				Expect(err).ToNot(HaveOccurred())
+			})
+		})
+
+		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)
+				repo := NewShareRepository(ctx, GetDBXBuilder())
+				err := repo.(rest.Persistable).Update("own-share-upd", &model.Share{Description: "Updated"}, "description")
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			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)
+				repo := NewShareRepository(ctx, GetDBXBuilder())
+				err := repo.(rest.Persistable).Update("other-share-upd", &model.Share{Description: "Hacked"}, "description")
+				Expect(err).To(Equal(rest.ErrPermissionDenied))
+			})
+
+			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)
+				repo := NewShareRepository(ctx, GetDBXBuilder())
+				err := repo.(rest.Persistable).Update("admin-upd-share", &model.Share{Description: "Admin Updated"}, "description")
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			It("allows headless context (no user) to update a share", func() {
+				insertShare("headless-upd-share", ownerUser.ID)
+				repo := NewShareRepository(context.Background(), GetDBXBuilder())
+				err := repo.(rest.Persistable).Update("headless-upd-share", &model.Share{Description: "Headless"}, "description")
+				Expect(err).ToNot(HaveOccurred())
+			})
+		})
+	})
 })