diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index c293129c3..1f75b8550 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -53,6 +53,35 @@ func NewArtwork(ds model.DataStore, cache cache.FileCache, store *ImageStore, ff return &service{ds: ds, cache: cache, store: store, ffmpeg: ffm} } +// EntityExists reports whether the entity an artwork id points at is still there: state rows +// outlive a deleted entity until the next prune, so a servable row is not evidence of its owner. +// The repositories are ctx-scoped, so a request context makes this a visibility check too. +func EntityExists(ctx context.Context, ds model.DataStore, artID model.ArtworkID) bool { + var found bool + var err error + switch artID.Kind { + case model.KindArtistArtwork: + found, err = ds.Artist(ctx).Exists(artID.ID) + case model.KindAlbumArtwork: + found, err = ds.Album(ctx).Exists(artID.ID) + case model.KindMediaFileArtwork: + found, err = ds.MediaFile(ctx).Exists(artID.ID) + case model.KindPlaylistArtwork: + found, err = ds.Playlist(ctx).Exists(artID.ID) + case model.KindRadioArtwork: + found, err = ds.Radio(ctx).Exists(artID.ID) + case model.KindDiscArtwork: + albumID, _, perr := model.ParseDiscArtworkID(artID.ID) + if perr != nil { + return false + } + found, err = ds.Album(ctx).Exists(albumID) + default: + return false + } + return err == nil && found +} + type service struct { ds model.DataStore cache cache.FileCache @@ -150,6 +179,11 @@ func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate // serveHash serves the bytes of a found state row. A mismatch/open error is dangling (a warm // cache still serves), but a cancelled request is not: it must not enqueue a re-resolution. func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *model.ItemArtwork, size int, square bool) (*Image, error) { + // Only this path can hand back a deleted entity's bytes: an absent row is already + // unavailable, and the provisional and disc paths load their entity to resolve at all. + if !EntityExists(ctx, s.ds, artID) { + return nil, ErrUnavailable + } art, err := s.ds.Artwork(ctx).GetImage(ia.Hash) if err != nil { if errors.Is(err, model.ErrNotFound) { diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go index 37c385aff..4ee99edec 100644 --- a/core/artwork/artwork_test.go +++ b/core/artwork/artwork_test.go @@ -36,6 +36,7 @@ var _ = Describe("Artwork", func() { svc Artwork repoRoot string coverBytes []byte + seedEntity func(kind, id string) ) primaryKey := func(kind, id string) string { return kind + "|" + id + "|" + model.ImageTypePrimary } @@ -48,9 +49,22 @@ var _ = Describe("Artwork", func() { Expect(store.Write(hash, "image/jpeg", bytes.NewReader(imgBytes))).To(Succeed()) Expect(artRepo.PutImage(&model.Artwork{Hash: hash, Mime: "image/jpeg"})).To(Succeed()) Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: kind, ItemID: id, Hash: hash, Source: "external"})).To(Succeed()) + seedEntity(kind, id) return hash } + // seedEntity registers the owning entity, without which the state row describes something + // that no longer exists and the service correctly refuses to serve it. + seedEntity = func(kind, id string) { + GinkgoHelper() + switch kind { + case "al": + Expect(albumRepo.Put(&model.Album{ID: id, Name: "Album"})).To(Succeed()) + case "mf": + Expect(mfRepo.Put(&model.MediaFile{ID: id})).To(Succeed()) + } + } + readAll := func(img *Image) []byte { GinkgoHelper() defer img.Close() @@ -139,6 +153,7 @@ var _ = Describe("Artwork", func() { Expect(os.WriteFile(imgPath, coverBytes, 0600)).To(Succeed()) mtime := fileMtime(imgPath) Expect(artRepo.PutImage(&model.Artwork{Hash: "aaaaaaaaaaaaaaaa", Mime: "image/jpeg"})).To(Succeed()) + seedEntity("al", "al2") Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: "al", ItemID: "al2", Hash: "aaaaaaaaaaaaaaaa", Source: "folder", SourcePath: imgPath, RefMtime: mtime, @@ -154,6 +169,7 @@ var _ = Describe("Artwork", func() { imgPath := filepath.Join(dir, "cover.jpg") Expect(os.WriteFile(imgPath, coverBytes, 0600)).To(Succeed()) Expect(artRepo.PutImage(&model.Artwork{Hash: "bbbbbbbbbbbbbbbb", Mime: "image/jpeg"})).To(Succeed()) + seedEntity("al", "al3") Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: "al", ItemID: "al3", Hash: "bbbbbbbbbbbbbbbb", Source: "folder", SourcePath: imgPath, RefMtime: fileMtime(imgPath) + 999, @@ -172,6 +188,7 @@ var _ = Describe("Artwork", func() { imgPath := filepath.Join(dir, "cover.jpg") Expect(os.WriteFile(imgPath, coverBytes, 0600)).To(Succeed()) Expect(artRepo.PutImage(&model.Artwork{Hash: "cccccccccccccccc", Mime: "image/jpeg"})).To(Succeed()) + seedEntity("al", "al3b") Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: "al", ItemID: "al3b", Hash: "cccccccccccccccc", Source: "folder", SourcePath: imgPath, RefMtime: fileMtime(imgPath) + 999, @@ -182,6 +199,17 @@ var _ = Describe("Artwork", func() { Expect(queueRepo.Data[primaryKey("al", "al3b")].Priority).To(Equal(model.ArtworkPriorityScan)) }) + // State rows and their bytes outlive a deleted entity until the next prune, so serving + // straight from the row would keep handing out a removed entity's image. + It("refuses to serve a found row whose entity is gone", func() { + hash := seedFoundStore("al", "alzz", coverBytes) + Expect(hash).ToNot(BeEmpty()) + albumRepo.SetData(model.Albums{}) // the album is deleted; its artwork row survives + + _, err := svc.Get(ctx, model.MustParseArtworkID("al-alzz"), 0, false) + Expect(err).To(MatchError(ErrUnavailable)) + }) + It("does not re-enqueue a recently-attempted absent state", func() { Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: "al", ItemID: "al4", AttemptedAt: time.Now(), @@ -480,3 +508,35 @@ func fileMtime(path string) int64 { Expect(err).ToNot(HaveOccurred()) return info.ModTime().UnixNano() } + +var _ = Describe("EntityExists", func() { + var ctx context.Context + var ds *tests.MockDataStore + + BeforeEach(func() { + ctx = context.Background() + albumRepo := tests.CreateMockAlbumRepo() + albumRepo.SetData(model.Albums{{ID: "al1"}}) + artistRepo := tests.CreateMockArtistRepo() + artistRepo.SetData(model.Artists{{ID: "ar1"}}) + radioRepo := tests.CreateMockedRadioRepo() + Expect(radioRepo.Put(&model.Radio{ID: "ra1", Name: "R"})).To(Succeed()) + ds = &tests.MockDataStore{MockedAlbum: albumRepo, MockedArtist: artistRepo, MockedRadio: radioRepo} + }) + + DescribeTable("reports whether the owning entity is still there", + func(id string, expected bool) { + Expect(EntityExists(ctx, ds, model.MustParseArtworkID(id))).To(Equal(expected)) + }, + Entry("existing album", "al-al1", true), + Entry("deleted album", "al-gone", false), + Entry("existing artist", "ar-ar1", true), + Entry("deleted artist", "ar-gone", false), + Entry("existing radio", "ra-ra1", true), + Entry("deleted radio", "ra-gone", false), + // Disc art has no entity of its own; it stands or falls with its album. + Entry("disc of an existing album", "dc-al1:1", true), + Entry("disc of a deleted album", "dc-gone:1", false), + Entry("malformed disc id", "dc-nodiscnum", false), + ) +}) diff --git a/model/radio.go b/model/radio.go index 5ee0e86fe..466ff48b0 100644 --- a/model/radio.go +++ b/model/radio.go @@ -32,6 +32,7 @@ type RadioRepository interface { ResourceRepository CountAll(options ...QueryOptions) (int64, error) Delete(id string) error + Exists(id string) (bool, error) Get(id string) (*Radio, error) GetAll(options ...QueryOptions) (Radios, error) GetAllIDs(options ...QueryOptions) ([]string, error) diff --git a/persistence/album_repository.go b/persistence/album_repository.go index fe919dd29..75f98d5d9 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -202,7 +202,10 @@ func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error) } func (r *albumRepository) Exists(id string) (bool, error) { - return r.exists(Eq{"album.id": id}) + // Filtered like CountAll: the plain exists() helper applies no library filter, so it + // would report a row in a library the caller cannot see. + c, err := r.count(r.applyLibraryFilter(r.newSelect().Where(Eq{"album.id": id}))) + return c > 0, err } func (r *albumRepository) Put(al *model.Album) error { diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index ef18cf426..66acef072 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -992,6 +992,23 @@ var _ = Describe("AlbumRepository", func() { Expect(got.RGAlbumPeak).To(BeNil()) }) }) + + // Exists used the unfiltered helper, so it reported albums in libraries the caller + // cannot see -- the same leak Get/GetAll/CountAll already guard against. + Describe("Exists library visibility", func() { + It("hides an album the user has no library access to", func() { + Expect(albumRepo.Put(&model.Album{ID: "vis-album", Name: "Vis", LibraryID: 1})).To(Succeed()) + DeferCleanup(func() { + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": "vis-album"})) + }) + + Expect(albumRepo.Exists("vis-album")).To(BeTrue(), "admin sees it") + + restricted := model.User{ID: "restricted_album_user", UserName: "ra", Name: "RA", Email: "ra@t.com"} + rctx := request.WithUser(GinkgoT().Context(), restricted) + Expect(NewAlbumRepository(rctx, GetDBXBuilder()).Exists("vis-album")).To(BeFalse()) + }) + }) }) func _p(id, name string, sortName ...string) model.Participant { diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 62b77a368..6b47b5228 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -163,7 +163,10 @@ func (r *mediaFileRepository) CountBySuffix(options ...model.QueryOptions) (map[ } func (r *mediaFileRepository) Exists(id string) (bool, error) { - return r.exists(Eq{"media_file.id": id}) + // Filtered like CountAll: the plain exists() helper applies no library filter, so it + // would report a row in a library the caller cannot see. + c, err := r.count(r.applyLibraryFilter(r.newSelect().Where(Eq{"media_file.id": id}))) + return c > 0, err } func (r *mediaFileRepository) Put(m *model.MediaFile) error { diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index 6e892c8f5..d0e7071cf 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -1118,4 +1118,16 @@ var _ = Describe("MediaRepository", func() { Expect(mf.AlbumImage.ImageHash).To(Equal("bbbbbbbbbbbbbbbb")) }) }) + + // Exists used the unfiltered helper, so it reported tracks in libraries the caller + // cannot see -- the same leak Get/GetAll/CountAll already guard against. + Describe("Exists library visibility", func() { + It("hides a track the user has no library access to", func() { + restricted := model.User{ID: "restricted_mf_user", UserName: "rm", Name: "RM", Email: "rm@t.com"} + rctx := request.WithUser(GinkgoT().Context(), restricted) + + Expect(mr.Exists(songAntenna.ID)).To(BeTrue(), "admin sees it") + Expect(NewMediaFileRepository(rctx, GetDBXBuilder()).Exists(songAntenna.ID)).To(BeFalse()) + }) + }) }) diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index 9905d3da6..05b40393f 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -384,4 +384,28 @@ var _ = Describe("PlaylistRepository", func() { Expect(mediaFileIDs).To(Equal([]string{"1001", "1002"})) }) }) + + // Exists is ctx-sensitive through userFilter: a private playlist is invisible to anyone but + // its owner or an admin. Callers that only want "does it still exist" -- the public image + // route serving a share -- must elevate, or a shared private playlist looks deleted. + Describe("Exists visibility", func() { + It("hides a private playlist from an unauthenticated context", func() { + // "userid" is the fixture user; playlist.owner_id has a FK to user(id). + owner := model.User{ID: "userid", UserName: "userid"} + octx := request.WithUser(GinkgoT().Context(), owner) + ownerRepo := NewPlaylistRepository(octx, GetDBXBuilder()) + pls := model.Playlist{Name: "Private One", OwnerID: owner.ID, Public: false} + Expect(ownerRepo.Put(&pls)).To(Succeed()) + DeferCleanup(func() { _ = ownerRepo.Delete(pls.ID) }) + + Expect(ownerRepo.Exists(pls.ID)).To(BeTrue(), "the owner sees it") + + anon := NewPlaylistRepository(GinkgoT().Context(), GetDBXBuilder()) + Expect(anon.Exists(pls.ID)).To(BeFalse(), "no user: userFilter hides it") + + admin := request.WithUser(GinkgoT().Context(), model.User{ID: "userid", IsAdmin: true}) + Expect(NewPlaylistRepository(admin, GetDBXBuilder()).Exists(pls.ID)).To(BeTrue(), + "elevating is what the public image route relies on") + }) + }) }) diff --git a/persistence/radio_repository.go b/persistence/radio_repository.go index 2ee5351fe..01558e541 100644 --- a/persistence/radio_repository.go +++ b/persistence/radio_repository.go @@ -38,6 +38,11 @@ func (r *radioRepository) CountAll(options ...model.QueryOptions) (int64, error) return r.count(sql, options...) } +// Exists needs no library or ownership filter: radios are visible to every user. +func (r *radioRepository) Exists(id string) (bool, error) { + return r.exists(Eq{"id": id}) +} + func (r *radioRepository) Delete(id string) error { if !r.isPermitted() { return rest.ErrPermissionDenied diff --git a/server/jellyfin/images_test.go b/server/jellyfin/images_test.go index 9d30e7d6c..f8cdf0701 100644 --- a/server/jellyfin/images_test.go +++ b/server/jellyfin/images_test.go @@ -75,6 +75,20 @@ var _ = Describe("Images", func() { Expect(fa.recvId).To(ContainSubstring("a1")) }) + // resolveArtworkID probes the entity tables, so a deleted item cannot produce an artwork id + // at all -- which is what stops artwork state outliving its entity from being served here. + It("asks for no artwork once the item is deleted, rather than its lingering state", func() { + ds := &tests.MockDataStore{} // no albums/artists/tracks/playlists at all + fa := &fakeArtwork{} + api := &Router{ds: ds, artwork: fa} + + w, r := newImageRequest(dto.EncodeID("deleted-item")) + api.getItemImage(w, r) + + Expect(fa.recvId).To(BeEmpty(), "an empty artwork id can only yield a placeholder") + Expect(w.Body.String()).ToNot(ContainSubstring("deleted-item")) + }) + It("sniffs the Content-Type instead of hardcoding it", func() { ds := &tests.MockDataStore{} ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) diff --git a/server/public/handle_images.go b/server/public/handle_images.go index 9bcaa5933..1c45898c9 100644 --- a/server/public/handle_images.go +++ b/server/public/handle_images.go @@ -11,6 +11,7 @@ import ( "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/imghttp" "github.com/navidrome/navidrome/utils/req" ) @@ -38,6 +39,10 @@ func (pub *Router) handleImages(w http.ResponseWriter, r *http.Request) { http.Error(w, "invalid request", http.StatusBadRequest) return } + // Elevated like the Jellyfin image route: the token is the authorization, so the service's + // entity check asks "is it still there", not "may this user see it" -- the latter would hide + // a shared private playlist, which is the case shares exist to serve. + ctx = request.WithUser(ctx, model.User{IsAdmin: true}) size := p.IntOr("size", 0) square := p.BoolOr("square", false) diff --git a/server/public/handle_images_test.go b/server/public/handle_images_test.go index 329ff58c9..cef4e0851 100644 --- a/server/public/handle_images_test.go +++ b/server/public/handle_images_test.go @@ -12,6 +12,7 @@ import ( "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -52,18 +53,25 @@ var _ = Describe("handleImages", func() { return httptest.NewRequest("GET", "/img?:id="+url.QueryEscape(token), nil) } + // The handler re-checks that the entity behind the token still exists, so every spec needs + // a store where "1" is a live album. + var ds *tests.MockDataStore + BeforeEach(func() { w = httptest.NewRecorder() + albumRepo := tests.CreateMockAlbumRepo() + albumRepo.SetData(model.Albums{{ID: "1"}}) + ds = &tests.MockDataStore{MockedAlbum: albumRepo} }) It("returns 404 when the artwork is unavailable", func() { - pub := &Router{artwork: &fakeArtwork{err: artwork.ErrUnavailable}} + pub := &Router{ds: ds, artwork: &fakeArtwork{err: artwork.ErrUnavailable}} pub.handleImages(w, newImageRequest("al-1")) Expect(w.Code).To(Equal(http.StatusNotFound)) }) It("returns 404 when the artwork is not found", func() { - pub := &Router{artwork: &fakeArtwork{err: model.ErrNotFound}} + pub := &Router{ds: ds, artwork: &fakeArtwork{err: model.ErrNotFound}} pub.handleImages(w, newImageRequest("al-1")) Expect(w.Code).To(Equal(http.StatusNotFound)) }) @@ -71,10 +79,11 @@ var _ = Describe("handleImages", func() { It("serves the image immutable when the token asserts the current hash", func() { const hash = "0123456789abcdef" img := &artwork.Image{ReadCloser: io.NopCloser(bytes.NewReader([]byte("IMG"))), Hash: hash} - pub := &Router{artwork: &fakeArtwork{img: img}} + pub := &Router{ds: ds, artwork: &fakeArtwork{img: img}} pub.handleImages(w, newImageRequest("al-1_"+hash)) Expect(w.Code).To(Equal(http.StatusOK)) Expect(w.Header().Get("Cache-Control")).To(Equal("public, max-age=31536000, immutable")) Expect(w.Header().Get("ETag")).To(Equal(`"` + hash + `"`)) }) + }) diff --git a/server/subsonic/e2e/subsonic_artwork_test.go b/server/subsonic/e2e/subsonic_artwork_test.go index 05a25f222..2679b4324 100644 --- a/server/subsonic/e2e/subsonic_artwork_test.go +++ b/server/subsonic/e2e/subsonic_artwork_test.go @@ -201,6 +201,21 @@ var _ = Describe("Artwork Serving", Ordered, func() { Expect(w.Header().Get("Cache-Control")).To(Equal("no-store")) }) + // The BeforeAll grants the artwork library to adminUser only, so regularUser cannot see + // this album -- but its artwork state and bytes are perfectly servable by id. The service + // resolves the entity through the caller's repositories, so the filter still applies. + It("serves the placeholder for an album in a library the caller cannot see", func() { + w := httptest.NewRecorder() + artRouter.ServeHTTP(w, buildReq(regularUser, "getCoverArt", "id", "al-"+artfulID)) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Body.Bytes()).To(Equal(placeholder), "must not leak the real cover") + Expect(w.Header().Get("Cache-Control")).To(Equal("no-store")) + + // ...while the admin, who does have access, gets the real bytes for the same id. + Expect(getCover("id", "al-"+artfulID).Body.Bytes()).ToNot(Equal(placeholder)) + }) + // An album with no art and an id naming no album are different answers; only the former // is a placeholder. It("answers error 70 for an id that matches no entity", func() { diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index 34e582638..8a5152a9d 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -10,7 +10,6 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/resources" @@ -80,13 +79,6 @@ func (api *Router) GetCoverArt(w http.ResponseWriter, r *http.Request) (*respons return nil, err } - // Access control: the serving path reads persisted state by id, bypassing the library and - // private-playlist filters. On this authenticated path, fall back to the placeholder for an - // entity the caller cannot see, so a guessed id can't leak artwork (matches the legacy load). - if id != "" && !img.Placeholder && !api.artworkAccessible(ctx, id) { - _ = img.Close() - img = artwork.PlaceholderFor(id) - } defer img.Close() artID, _ := model.ParseArtworkID(id) @@ -101,39 +93,6 @@ func (api *Router) GetCoverArt(w http.ResponseWriter, r *http.Request) (*respons return nil, err } -// artworkAccessible reports whether the caller may view the artwork for id, by resolving the -// underlying entity through the request-scoped (filtered) repositories. Radios are global and -// always accessible; an unparsable/unknown id defers to GetEntityByID. -func (api *Router) artworkAccessible(ctx context.Context, id string) bool { - artID, err := model.ParseArtworkID(id) - if err != nil { - _, err := model.GetEntityByID(ctx, api.ds, id) - return err == nil - } - var lookupErr error - switch artID.Kind { - case model.KindArtistArtwork: - _, lookupErr = api.ds.Artist(ctx).Get(artID.ID) - case model.KindAlbumArtwork: - _, lookupErr = api.ds.Album(ctx).Get(artID.ID) - case model.KindMediaFileArtwork: - _, lookupErr = api.ds.MediaFile(ctx).Get(artID.ID) - case model.KindPlaylistArtwork: - _, lookupErr = api.ds.Playlist(ctx).Get(artID.ID) - case model.KindRadioArtwork: - _, lookupErr = api.ds.Radio(ctx).Get(artID.ID) - case model.KindDiscArtwork: - albumID, _, perr := model.ParseDiscArtworkID(artID.ID) - if perr != nil { - return false - } - _, lookupErr = api.ds.Album(ctx).Get(albumID) - default: // anything else has no per-user artwork access control - return true - } - return lookupErr == nil -} - func (api *Router) GetLyrics(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) artist, _ := p.String("artist") diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index cec091a19..63ceaa8fa 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -17,6 +17,7 @@ import ( "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/lyrics" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -66,36 +67,21 @@ var _ = Describe("MediaRetrievalController", func() { Expect(w.Body.String()).To(Equal(artwork.data)) }) - It("serves radio artwork while the radio exists", func() { - r := newGetRequest("id=ra-rd1") + // Visibility now lives in the service, which resolves the entity through the + // request-scoped repositories. The handler's whole contribution is handing over the + // caller's context unchanged -- elevating here would bypass the library filter. + It("passes the caller's context to the service rather than elevating", func() { + r := newGetRequest("id=al-34") + usr := model.User{ID: "u1", UserName: "u1"} + r = r.WithContext(request.WithUser(r.Context(), usr)) + _, err := router.GetCoverArt(w, r) Expect(err).ToNot(HaveOccurred()) - Expect(w.Body.String()).To(Equal(artwork.data)) - }) - - // A deleted radio keeps its item_artwork row and uploaded file until the next prune, so - // existence has to be re-checked or the old id keeps serving the removed radio's image. - It("serves a placeholder once the radio is gone", func() { - r := newGetRequest("id=ra-deleted") - _, err := router.GetCoverArt(w, r) - - Expect(err).ToNot(HaveOccurred()) - Expect(w.Code).To(Equal(200)) - Expect(w.Body.String()).ToNot(Equal(artwork.data)) - Expect(w.Header().Get("Cache-Control")).To(Equal("no-store")) - }) - - It("serves a placeholder for an entity the caller cannot access", func() { - // al-99 is not in the (filtered) album repo, so the caller must not get its bytes. - r := newGetRequest("id=al-99") - _, err := router.GetCoverArt(w, r) - - Expect(err).ToNot(HaveOccurred()) - Expect(w.Code).To(Equal(200)) - Expect(w.Body.String()).ToNot(Equal(artwork.data)) - Expect(w.Header().Get("Cache-Control")).To(Equal("no-store")) - Expect(w.Header().Get("ETag")).To(BeEmpty()) + got, ok := request.UserFrom(artwork.recvCtx) + Expect(ok).To(BeTrue(), "the service must see who is asking") + Expect(got.ID).To(Equal("u1")) + Expect(got.IsAdmin).To(BeFalse(), "the handler must not elevate") }) It("should fail when the file is not found", func() { @@ -281,9 +267,11 @@ type fakeArtwork struct { recvId string recvSize int recvSquare bool + recvCtx context.Context } -func (c *fakeArtwork) GetOrPlaceholder(_ context.Context, id string, size int, square bool) (*artwork.Image, error) { +func (c *fakeArtwork) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*artwork.Image, error) { + c.recvCtx = ctx if c.err != nil { return nil, c.err }