From 962910f0de2d1f8eae91c3faef5749c90d6f1ced Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 18:53:38 -0400 Subject: [PATCH 01/21] feat(config): add EnableUserAvatarUpload flag --- conf/configuration.go | 2 ++ conf/configuration_test.go | 7 +++++++ consts/consts.go | 3 +++ core/metrics/insights.go | 1 + core/metrics/insights/data.go | 1 + server/serve_index.go | 1 + 6 files changed, 15 insertions(+) diff --git a/conf/configuration.go b/conf/configuration.go index ff119417a..9ef0e37de 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -91,6 +91,7 @@ type configOptions struct { EnableStarRating bool EnableUserEditing bool EnableArtworkUpload bool + EnableUserAvatarUpload bool MaxImageUploadSize string MaxImageSize string EnableSharing bool @@ -1028,6 +1029,7 @@ func setViperDefaults() { viper.SetDefault("enablenowplaying", true) viper.SetDefault("uiplaybackreportinterval", consts.DefaultUIPlaybackReportInterval) viper.SetDefault("enableartworkupload", true) + viper.SetDefault("enableuseravatarupload", true) viper.SetDefault("maximageuploadsize", consts.DefaultMaxImageUploadSize) viper.SetDefault("maximagesize", consts.DefaultMaxImageSize) viper.SetDefault("enablesharing", true) diff --git a/conf/configuration_test.go b/conf/configuration_test.go index 2c7f8edaa..ca97f2efb 100644 --- a/conf/configuration_test.go +++ b/conf/configuration_test.go @@ -76,6 +76,13 @@ var _ = Describe("Configuration", func() { }) }) + Describe("user avatar upload", func() { + It("is enabled by default", func() { + conf.Load(true) + Expect(conf.Server.EnableUserAvatarUpload).To(BeTrue()) + }) + }) + Describe("ValidateURL", func() { It("accepts a valid http URL", func() { fn := conf.ValidateURL("TestOption", "http://example.com/path") diff --git a/consts/consts.go b/consts/consts.go index 486ea66bc..938f2ad57 100644 --- a/consts/consts.go +++ b/consts/consts.go @@ -89,6 +89,7 @@ const ( I18nFolder = "i18n" ScanIgnoreFile = ".ndignore" ArtworkFolder = "artwork" + AvatarFolder = "avatar" // HashedArtworkFolder is a subtree of ArtworkFolder, kept apart from the name-addressed // upload folders beside it so Prune's sweep never reaches them. HashedArtworkFolder = "hashed" @@ -118,6 +119,7 @@ const ( DefaultUICoverArtSize = 300 DefaultMaxImageUploadSize = "10MB" DefaultMaxImageSize = "20MB" + MaxAvatarSize = 512 ) // Prometheus options @@ -143,6 +145,7 @@ const ( EntityArtist = "artist" EntityPlaylist = "playlist" EntityRadio = "radio" + EntityUser = "user" ) const ( diff --git a/core/metrics/insights.go b/core/metrics/insights.go index d952f517a..ce45f1263 100644 --- a/core/metrics/insights.go +++ b/core/metrics/insights.go @@ -202,6 +202,7 @@ var staticData = sync.OnceValue(func() insights.Data { data.Config.DefaultBackgroundURLSet = conf.Server.UILoginBackgroundURL == consts.DefaultUILoginBackgroundURL data.Config.EnableArtworkPrecache = conf.Server.EnableArtworkPrecache data.Config.EnableArtworkUpload = conf.Server.EnableArtworkUpload + data.Config.EnableUserAvatarUpload = conf.Server.EnableUserAvatarUpload data.Config.CoverArtQuality = conf.Server.CoverArtQuality data.Config.EnableWebPEncoding = conf.Server.EnableWebPEncoding data.Config.UICoverArtSize = conf.Server.UICoverArtSize diff --git a/core/metrics/insights/data.go b/core/metrics/insights/data.go index 8559d4204..fd99f9e88 100644 --- a/core/metrics/insights/data.go +++ b/core/metrics/insights/data.go @@ -67,6 +67,7 @@ type Data struct { EnableJukebox bool `json:"enableJukebox,omitempty"` EnablePrometheus bool `json:"enablePrometheus,omitempty"` EnableArtworkUpload bool `json:"enableArtworkUpload,omitempty"` + EnableUserAvatarUpload bool `json:"enableUserAvatarUpload,omitempty"` CoverArtQuality int `json:"coverArtQuality,omitempty"` EnableWebPEncoding bool `json:"enableWebPEncoding,omitempty"` UICoverArtSize int `json:"uiCoverArtSize,omitempty"` diff --git a/server/serve_index.go b/server/serve_index.go index a538daf1a..750885697 100644 --- a/server/serve_index.go +++ b/server/serve_index.go @@ -64,6 +64,7 @@ func serveIndex(ds model.DataStore, fs fs.FS, shareInfo *model.Share) http.Handl "devActivityPanel": conf.Server.DevActivityPanel, "enableUserEditing": conf.Server.EnableUserEditing, "enableArtworkUpload": conf.Server.EnableArtworkUpload, + "enableUserAvatarUpload": conf.Server.EnableUserAvatarUpload, "enableSharing": conf.Server.EnableSharing, "shareURL": conf.Server.ShareURL, "defaultDownloadableShare": conf.Server.DefaultDownloadableShare, From 42008a1b881ec763f972b052e62b96de44e9dae3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 18:59:22 -0400 Subject: [PATCH 02/21] feat(model): add uploaded_image column and avatar helpers to user --- ...20260907225736_add_user_uploaded_image.sql | 5 ++++ model/image.go | 3 ++ model/image_test.go | 29 +++++++++++++++++++ model/user.go | 21 ++++++++++++++ model/user_test.go | 23 +++++++++++++++ 5 files changed, 81 insertions(+) create mode 100644 db/migrations/20260907225736_add_user_uploaded_image.sql create mode 100644 model/image_test.go diff --git a/db/migrations/20260907225736_add_user_uploaded_image.sql b/db/migrations/20260907225736_add_user_uploaded_image.sql new file mode 100644 index 000000000..aee8efa7c --- /dev/null +++ b/db/migrations/20260907225736_add_user_uploaded_image.sql @@ -0,0 +1,5 @@ +-- +goose Up +ALTER TABLE user ADD COLUMN uploaded_image VARCHAR(255) DEFAULT ''; + +-- +goose Down +ALTER TABLE user DROP COLUMN uploaded_image; diff --git a/model/image.go b/model/image.go index 30307fcea..5986d1b7e 100644 --- a/model/image.go +++ b/model/image.go @@ -13,5 +13,8 @@ func UploadedImagePath(entityType, filename string) string { if filename == "" { return "" } + if entityType == consts.EntityUser { + return filepath.Join(conf.Server.DataFolder.String(), consts.AvatarFolder, filename) + } return filepath.Join(conf.Server.DataFolder.String(), consts.ArtworkFolder, entityType, filename) } diff --git a/model/image_test.go b/model/image_test.go new file mode 100644 index 000000000..9d5001051 --- /dev/null +++ b/model/image_test.go @@ -0,0 +1,29 @@ +package model_test + +import ( + "path/filepath" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("UploadedImagePath", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DataFolder = conf.NewDir("/data") + }) + + It("puts user avatars in the avatar folder, not under artwork", func() { + Expect(model.UploadedImagePath(consts.EntityUser, "abc_deluan.png")). + To(Equal(filepath.Join("/data", "avatar", "abc_deluan.png"))) + }) + + It("keeps other entities under the artwork folder", func() { + Expect(model.UploadedImagePath(consts.EntityArtist, "abc_bowie.png")). + To(Equal(filepath.Join("/data", "artwork", "artist", "abc_bowie.png"))) + }) +}) diff --git a/model/user.go b/model/user.go index 37bdca33d..3ea3985e6 100644 --- a/model/user.go +++ b/model/user.go @@ -1,7 +1,12 @@ package model import ( + "crypto/md5" + "encoding/hex" + "fmt" "time" + + "github.com/navidrome/navidrome/consts" ) type User struct { @@ -17,6 +22,10 @@ type User struct { // Smart-playlist criteria JSON; matching songs are not sent to external scrobblers ScrobbleFilter string `structs:"scrobble_filter" json:"scrobbleFilter"` + // structs:"-" because userRepository.Put writes every mapped column with no column list, so a + // profile save from the UI would blank this. UpdateImage is the only writer. + UploadedImage string `structs:"-" json:"uploadedImage,omitempty"` + // Library associations (many-to-many relationship) Libraries Libraries `structs:"-" json:"libraries,omitempty"` @@ -43,6 +52,18 @@ func (u User) HasLibraryAccess(libraryID int) bool { return false } +func (u User) UploadedImagePath() string { + return UploadedImagePath(consts.EntityUser, u.UploadedImage) +} + +func (u User) AvatarTag() string { + if u.UploadedImage == "" { + return "" + } + sum := md5.Sum(fmt.Appendf(nil, "%s|%d", u.UploadedImage, u.UpdatedAt.UnixNano())) + return hex.EncodeToString(sum[:])[:16] +} + type Users []User type UserRepository interface { diff --git a/model/user_test.go b/model/user_test.go index ab66a29a9..0abd679b6 100644 --- a/model/user_test.go +++ b/model/user_test.go @@ -1,6 +1,8 @@ package model_test import ( + "time" + "github.com/navidrome/navidrome/model" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -80,4 +82,25 @@ var _ = Describe("User", func() { }) }) }) + + Describe("AvatarTag", func() { + It("returns an empty tag when there is no avatar", func() { + u := model.User{ID: "1", UpdatedAt: time.Now()} + Expect(u.AvatarTag()).To(BeEmpty()) + }) + + It("changes the tag when the image changes", func() { + at := time.Unix(1000, 0) + a := model.User{ID: "1", UploadedImage: "1_deluan.png", UpdatedAt: at} + b := model.User{ID: "1", UploadedImage: "1_deluan.jpg", UpdatedAt: at} + Expect(a.AvatarTag()).NotTo(BeEmpty()) + Expect(a.AvatarTag()).NotTo(Equal(b.AvatarTag())) + }) + + It("changes the tag when the same file is replaced", func() { + a := model.User{ID: "1", UploadedImage: "1_deluan.png", UpdatedAt: time.Unix(1000, 0)} + b := model.User{ID: "1", UploadedImage: "1_deluan.png", UpdatedAt: time.Unix(2000, 0)} + Expect(a.AvatarTag()).NotTo(Equal(b.AvatarTag())) + }) + }) }) From ab2c77f7b2c528c5600d36e2e198f3a48452e659 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 19:05:14 -0400 Subject: [PATCH 03/21] feat(persistence): add UserRepository.UpdateImage Adds the interface method, real repository implementation, and mock together so the build stays green. UpdateImage sets updated_at explicitly in the same UPDATE statement, since AvatarTag's ETag derives from uploaded_image + updated_at. --- model/user.go | 2 ++ persistence/user_repository.go | 15 +++++++++++ persistence/user_repository_test.go | 40 +++++++++++++++++++++++++++++ tests/mock_user_repo.go | 14 ++++++++++ 4 files changed, 71 insertions(+) diff --git a/model/user.go b/model/user.go index 3ea3985e6..452b60b0a 100644 --- a/model/user.go +++ b/model/user.go @@ -73,6 +73,8 @@ type UserRepository interface { Get(id string) (*User, error) GetAll(options ...QueryOptions) (Users, error) Put(*User) error + // UpdateImage is the only writer of uploaded_image. See the field comment on User. + UpdateImage(id string, filename string) error UpdateLastLoginAt(id string) error UpdateLastAccessAt(id string) error FindFirstAdmin() (*User, error) diff --git a/persistence/user_repository.go b/persistence/user_repository.go index 9de37876b..df53a34f3 100644 --- a/persistence/user_repository.go +++ b/persistence/user_repository.go @@ -189,6 +189,21 @@ func (r *userRepository) Put(u *model.User) error { return nil } +func (r *userRepository) UpdateImage(id string, filename string) error { + upd := Update(r.tableName). + Set("uploaded_image", filename). + Set("updated_at", time.Now()). + Where(Eq{"id": id}) + count, err := r.executeSQL(upd) + if err != nil { + return err + } + if count == 0 { + return model.ErrNotFound + } + return nil +} + func (r *userRepository) FindFirstAdmin() (*model.User, error) { sel := r.selectUserWithLibraries(model.QueryOptions{Sort: "updated_at", Max: 1}).Where(Eq{"user.is_admin": true}) var usr dbUser diff --git a/persistence/user_repository_test.go b/persistence/user_repository_test.go index dc519d0a1..69f136a15 100644 --- a/persistence/user_repository_test.go +++ b/persistence/user_repository_test.go @@ -5,6 +5,7 @@ import ( "errors" "slices" "sync" + "time" "github.com/Masterminds/squirrel" "github.com/deluan/rest" @@ -94,6 +95,45 @@ var _ = Describe("UserRepository", func() { }) }) + Describe("UpdateImage", func() { + It("stores the filename and bumps updated_at", func() { + before := time.Now().Add(-time.Hour) + Expect(repo.UpdateImage(adminUser.ID, "u1_admin.png")).To(Succeed()) + + usr, err := repo.Get(adminUser.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(usr.UploadedImage).To(Equal("u1_admin.png")) + Expect(usr.UpdatedAt).To(BeTemporally(">", before)) + }) + + It("is not erased by a later full-row Put", func() { + Expect(repo.UpdateImage(adminUser.ID, "u1_admin.png")).To(Succeed()) + + usr, err := repo.Get(adminUser.ID) + Expect(err).ToNot(HaveOccurred()) + usr.Name = "Renamed" + Expect(repo.Put(usr)).To(Succeed()) + + usr, err = repo.Get(adminUser.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(usr.UploadedImage).To(Equal("u1_admin.png")) + }) + + It("clears the filename when given an empty string", func() { + Expect(repo.UpdateImage(adminUser.ID, "u1_admin.png")).To(Succeed()) + Expect(repo.UpdateImage(adminUser.ID, "")).To(Succeed()) + + usr, err := repo.Get(adminUser.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(usr.UploadedImage).To(BeEmpty()) + }) + + It("returns ErrNotFound for an unknown id", func() { + err := repo.UpdateImage("no-such-user", "x.png") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + }) + Describe("validatePasswordChange", func() { var loggedUser *model.User diff --git a/tests/mock_user_repo.go b/tests/mock_user_repo.go index 2d6ff3c02..7bd32d229 100644 --- a/tests/mock_user_repo.go +++ b/tests/mock_user_repo.go @@ -92,6 +92,20 @@ func (u *MockedUserRepo) GetAll(options ...model.QueryOptions) (model.Users, err return users, nil } +func (u *MockedUserRepo) UpdateImage(id string, filename string) error { + if u.Error != nil { + return u.Error + } + for _, usr := range u.Data { + if usr.ID == id { + usr.UploadedImage = filename + usr.UpdatedAt = time.Now() + return nil + } + } + return model.ErrNotFound +} + func (u *MockedUserRepo) UpdateLastLoginAt(id string) error { for _, usr := range u.Data { if usr.ID == id { From 15d6a2a728c8824944bfa920cb9bbd9bda204515 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 19:10:31 -0400 Subject: [PATCH 04/21] fix(persistence): make the UpdateImage/Put guard test a real guard The user object was fetched after UpdateImage, so its in-memory UploadedImage already matched and Put's structs:"-" tag was never exercised. Fetch the stale object first and assert the DB value it would have clobbered survives. --- persistence/user_repository_test.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/persistence/user_repository_test.go b/persistence/user_repository_test.go index 69f136a15..c6702db79 100644 --- a/persistence/user_repository_test.go +++ b/persistence/user_repository_test.go @@ -107,16 +107,18 @@ var _ = Describe("UserRepository", func() { }) It("is not erased by a later full-row Put", func() { - Expect(repo.UpdateImage(adminUser.ID, "u1_admin.png")).To(Succeed()) + // Fetched before UpdateImage, so its in-memory UploadedImage is stale: Put must not write it back. + stale, err := repo.Get(adminUser.ID) + Expect(err).ToNot(HaveOccurred()) + + Expect(repo.UpdateImage(adminUser.ID, "u2_admin.png")).To(Succeed()) + + stale.Name = "Renamed" + Expect(repo.Put(stale)).To(Succeed()) usr, err := repo.Get(adminUser.ID) Expect(err).ToNot(HaveOccurred()) - usr.Name = "Renamed" - Expect(repo.Put(usr)).To(Succeed()) - - usr, err = repo.Get(adminUser.ID) - Expect(err).ToNot(HaveOccurred()) - Expect(usr.UploadedImage).To(Equal("u1_admin.png")) + Expect(usr.UploadedImage).To(Equal("u2_admin.png")) }) It("clears the filename when given an empty string", func() { From cac170781bd426c51ec301846b16d8e773d65e90 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 19:15:54 -0400 Subject: [PATCH 05/21] feat(artwork): add SetAvatar to the image uploader --- core/artwork/uploader.go | 20 ++++++++++ core/artwork/uploader_test.go | 69 +++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/core/artwork/uploader.go b/core/artwork/uploader.go index 76526af3c..90e10b555 100644 --- a/core/artwork/uploader.go +++ b/core/artwork/uploader.go @@ -1,6 +1,7 @@ package artwork import ( + "bytes" "context" "fmt" "io" @@ -31,6 +32,9 @@ func parseSize(value, fallback string) int64 { // Uploader stores a user-uploaded entity image and invalidates that entity's artwork state. type Uploader interface { SetImage(ctx context.Context, entityType string, entityID string, name string, oldPath string, reader io.Reader, ext string) (filename string, err error) + // SetAvatar resizes a user avatar to consts.MaxAvatarSize (aspect ratio preserved, + // no square padding) and stores it under the avatar folder. + SetAvatar(ctx context.Context, userID, username, oldPath string, reader io.Reader, ext string) (filename string, err error) RemoveImage(ctx context.Context, path string) error // EnqueueArtwork re-resolves the item's artwork. Call it AFTER persisting the new // filename, or the worker resolves the old one. @@ -77,6 +81,22 @@ func (s *uploader) SetImage(ctx context.Context, entityType string, entityID str return filename, nil } +func (s *uploader) SetAvatar(ctx context.Context, userID, username, oldPath string, reader io.Reader, ext string) (string, error) { + data, err := io.ReadAll(io.LimitReader(reader, MaxImageUploadSize())) + if err != nil { + return "", fmt.Errorf("reading avatar: %w", err) + } + // square=false: square padding would put transparent bars around a non-square photo. + resized, _, err := resizeStaticImage(data, consts.MaxAvatarSize, false) + if err != nil { + return "", fmt.Errorf("resizing avatar: %w", err) + } + if resized == nil { + resized = bytes.NewReader(data) // already within bounds: resizeStaticImage returns nil, not the original + } + return s.SetImage(ctx, consts.EntityUser, userID, username, oldPath, resized, ext) +} + func (s *uploader) EnqueueArtwork(ctx context.Context, entityType, id string) { kind, ok := uploadEntityKind[entityType] if !ok { diff --git a/core/artwork/uploader_test.go b/core/artwork/uploader_test.go index 44f5ede26..d7c4f466a 100644 --- a/core/artwork/uploader_test.go +++ b/core/artwork/uploader_test.go @@ -1,7 +1,10 @@ package artwork import ( + "bytes" "context" + "image" + "image/png" "os" "path/filepath" "strings" @@ -93,6 +96,65 @@ var _ = Describe("Uploader", func() { }) }) + Describe("SetAvatar", func() { + It("writes into the avatar folder, named after id and username", func() { + ctx := context.Background() + big := makePNG(1024, 1024) + filename, err := svc.SetAvatar(ctx, "u1", "deluan", "", bytes.NewReader(big), ".png") + Expect(err).ToNot(HaveOccurred()) + Expect(filename).To(Equal("u1_deluan.png")) + Expect(filepath.Join(tmpDir, "avatar", filename)).To(BeAnExistingFile()) + }) + + It("shrinks a large image to MaxAvatarSize, preserving aspect ratio", func() { + ctx := context.Background() + big := makePNG(1024, 768) + filename, err := svc.SetAvatar(ctx, "u1", "deluan", "", bytes.NewReader(big), ".png") + Expect(err).ToNot(HaveOccurred()) + + f, err := os.Open(filepath.Join(tmpDir, "avatar", filename)) + Expect(err).ToNot(HaveOccurred()) + defer f.Close() + cfg, _, err := image.DecodeConfig(f) + Expect(err).ToNot(HaveOccurred()) + Expect(cfg.Width).To(Equal(consts.MaxAvatarSize)) + Expect(cfg.Height).To(Equal(384)) // aspect ratio kept, not padded to a square + }) + + It("keeps a small image intact instead of writing an empty file", func() { + ctx := context.Background() + small := makePNG(64, 64) + filename, err := svc.SetAvatar(ctx, "u1", "deluan", "", bytes.NewReader(small), ".png") + Expect(err).ToNot(HaveOccurred()) + + data, err := os.ReadFile(filepath.Join(tmpDir, "avatar", filename)) + Expect(err).ToNot(HaveOccurred()) + Expect(data).ToNot(BeEmpty()) + + cfg, _, err := image.DecodeConfig(bytes.NewReader(data)) + Expect(err).ToNot(HaveOccurred()) + Expect(cfg.Width).To(Equal(64)) + Expect(cfg.Height).To(Equal(64)) + }) + + It("removes the previous file", func() { + ctx := context.Background() + old := filepath.Join(tmpDir, "avatar", "u1_old.png") + Expect(os.MkdirAll(filepath.Dir(old), 0755)).To(Succeed()) + Expect(os.WriteFile(old, []byte("x"), 0600)).To(Succeed()) + + _, err := svc.SetAvatar(ctx, "u1", "deluan", old, bytes.NewReader(makePNG(64, 64)), ".png") + Expect(err).ToNot(HaveOccurred()) + Expect(old).ToNot(BeAnExistingFile()) + }) + + It("rejects a body that is not a decodable image", func() { + ctx := context.Background() + _, err := svc.SetAvatar(ctx, "u1", "deluan", "", strings.NewReader("not an image"), ".png") + Expect(err).To(HaveOccurred()) + }) + }) + Describe("EnqueueArtwork", func() { It("clears artwork state and enqueues a Bump", func() { ctx := context.Background() @@ -172,3 +234,10 @@ var _ = Describe("MaxImageUploadSize", func() { Expect(MaxImageUploadSize()).To(Equal(int64(52_428_800))) }) }) + +func makePNG(w, h int) []byte { + img := image.NewRGBA(image.Rect(0, 0, w, h)) + var buf bytes.Buffer + Expect(png.Encode(&buf, img)).To(Succeed()) + return buf.Bytes() +} From 61bb68fc6bfb9013eaa44c72a1f26ccc5a6ab600 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 19:24:05 -0400 Subject: [PATCH 06/21] feat(server): add shared user avatar serving helper ServeUserAvatar lives in server/imghttp so both server/subsonic and server/jellyfin can call it without importing each other. It delegates ETag/If-None-Match handling and Content-Type detection to http.ServeContent instead of hand-parsing the header, avoiding a false-304 on multi-valued or wildcard If-None-Match headers. --- server/imghttp/avatar.go | 34 ++++++++++++++ server/imghttp/avatar_test.go | 87 +++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 server/imghttp/avatar.go create mode 100644 server/imghttp/avatar_test.go diff --git a/server/imghttp/avatar.go b/server/imghttp/avatar.go new file mode 100644 index 000000000..e6201249e --- /dev/null +++ b/server/imghttp/avatar.go @@ -0,0 +1,34 @@ +package imghttp + +import ( + "net/http" + "os" + + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" +) + +// ServeUserAvatar writes the user's uploaded avatar, reporting whether it answered the request; +// when false the caller must fall back. http.ServeContent gives correct ETag/If-None-Match handling. +func ServeUserAvatar(w http.ResponseWriter, r *http.Request, u *model.User) bool { + path := u.UploadedImagePath() + if path == "" { + return false + } + f, err := os.Open(path) + if err != nil { + log.Warn(r.Context(), "Could not open user avatar", "user", u.UserName, err) + return false + } + defer f.Close() + info, err := f.Stat() + if err != nil { + log.Warn(r.Context(), "Could not stat user avatar", "user", u.UserName, err) + return false + } + + w.Header().Set("ETag", `"`+u.AvatarTag()+`"`) + w.Header().Set("Cache-Control", "private, no-cache") + http.ServeContent(w, r, info.Name(), info.ModTime(), f) + return true +} diff --git a/server/imghttp/avatar_test.go b/server/imghttp/avatar_test.go new file mode 100644 index 000000000..63a6c967a --- /dev/null +++ b/server/imghttp/avatar_test.go @@ -0,0 +1,87 @@ +package imghttp_test + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "time" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/server/imghttp" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("ServeUserAvatar", func() { + var w *httptest.ResponseRecorder + var r *http.Request + var usr model.User + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DataFolder = conf.NewDir(GinkgoT().TempDir()) + w = httptest.NewRecorder() + r = httptest.NewRequest("GET", "/avatar", nil) + usr = model.User{ID: "u1", UserName: "deluan", UpdatedAt: time.Unix(1000, 0)} + }) + + It("returns false when the user has no avatar", func() { + Expect(imghttp.ServeUserAvatar(w, r, &usr)).To(BeFalse()) + }) + + It("returns false when the file is missing on disk", func() { + usr.UploadedImage = "u1_deluan.png" + Expect(imghttp.ServeUserAvatar(w, r, &usr)).To(BeFalse()) + }) + + It("serves the file with a content type and an ETag", func() { + usr.UploadedImage = writeAvatar(usr, "png") + Expect(imghttp.ServeUserAvatar(w, r, &usr)).To(BeTrue()) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Type")).To(Equal("image/png")) + Expect(w.Header().Get("ETag")).To(Equal(`"` + usr.AvatarTag() + `"`)) + }) + + It("answers 304 when the ETag matches", func() { + usr.UploadedImage = writeAvatar(usr, "png") + r.Header.Set("If-None-Match", `"`+usr.AvatarTag()+`"`) + Expect(imghttp.ServeUserAvatar(w, r, &usr)).To(BeTrue()) + Expect(w.Code).To(Equal(http.StatusNotModified)) + }) + + It("does not leak the absolute filesystem path in the response", func() { + usr.UploadedImage = writeAvatar(usr, "png") + Expect(imghttp.ServeUserAvatar(w, r, &usr)).To(BeTrue()) + Expect(w.Body.String()).NotTo(ContainSubstring(conf.Server.DataFolder.String())) + for _, values := range w.Header() { + for _, v := range values { + Expect(v).NotTo(ContainSubstring(conf.Server.DataFolder.String())) + } + } + }) + + It("does not false-304 on an unrelated multi-value If-None-Match", func() { + usr.UploadedImage = writeAvatar(usr, "png") + r.Header.Set("If-None-Match", `"deadbeefdeadbeef", "cafecafecafecafe"`) + Expect(imghttp.ServeUserAvatar(w, r, &usr)).To(BeTrue()) + Expect(w.Code).To(Equal(http.StatusOK)) + }) + + It("treats If-None-Match: * as matching the current representation", func() { + usr.UploadedImage = writeAvatar(usr, "png") + r.Header.Set("If-None-Match", "*") + Expect(imghttp.ServeUserAvatar(w, r, &usr)).To(BeTrue()) + Expect(w.Code).To(Equal(http.StatusNotModified)) + }) +}) + +func writeAvatar(u model.User, ext string) string { + name := u.ID + "_" + u.UserName + "." + ext + path := filepath.Join(conf.Server.DataFolder.String(), "avatar", name) + Expect(os.MkdirAll(filepath.Dir(path), 0755)).To(Succeed()) + Expect(os.WriteFile(path, []byte{0x89, 'P', 'N', 'G'}, 0600)).To(Succeed()) + return name +} From 16c62e075e41483069fb7ec4bfe6da2679bd0a95 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 19:31:51 -0400 Subject: [PATCH 07/21] feat(nativeapi): add user avatar upload and delete endpoints Adds POST/DELETE /api/user/{id}/image, gated by EnableUserAvatarUpload (never EnableArtworkUpload) and restricted to the target user or an admin. Reuses the artist/playlist/radio image-upload handlers via a new gate opt-out so their EnableArtworkUpload behavior is unchanged. --- server/nativeapi/image_upload.go | 16 ++++- server/nativeapi/native_api.go | 2 +- server/nativeapi/users.go | 87 ++++++++++++++++++++++++ server/nativeapi/users_test.go | 109 +++++++++++++++++++++++++++++++ 4 files changed, 211 insertions(+), 3 deletions(-) create mode 100644 server/nativeapi/users.go create mode 100644 server/nativeapi/users_test.go diff --git a/server/nativeapi/image_upload.go b/server/nativeapi/image_upload.go index c4ff62e42..95d15b1e2 100644 --- a/server/nativeapi/image_upload.go +++ b/server/nativeapi/image_upload.go @@ -31,10 +31,16 @@ func checkImageUploadPermission(w http.ResponseWriter, r *http.Request) bool { } func handleImageUpload(saveFn func(ctx context.Context, reader io.Reader, ext string) error) http.HandlerFunc { + return handleImageUploadGated(true, saveFn) +} + +// handleImageUploadGated is handleImageUpload with an opt-out from the EnableArtworkUpload gate, +// so avatar uploads (gated separately by EnableUserAvatarUpload) can reuse this handler. +func handleImageUploadGated(checkArtworkFlag bool, saveFn func(ctx context.Context, reader io.Reader, ext string) error) http.HandlerFunc { maxImageSize := artwork.MaxImageUploadSize() return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - if !checkImageUploadPermission(w, r) { + if checkArtworkFlag && !checkImageUploadPermission(w, r) { return } r.Body = http.MaxBytesReader(w, r.Body, maxImageSize) @@ -97,9 +103,15 @@ func handleImageUpload(saveFn func(ctx context.Context, reader io.Reader, ext st } func handleImageDelete(deleteFn func(ctx context.Context) error) http.HandlerFunc { + return handleImageDeleteGated(true, deleteFn) +} + +// handleImageDeleteGated is handleImageDelete with the same EnableArtworkUpload opt-out as +// handleImageUploadGated. +func handleImageDeleteGated(checkArtworkFlag bool, deleteFn func(ctx context.Context) error) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - if !checkImageUploadPermission(w, r) { + if checkArtworkFlag && !checkImageUploadPermission(w, r) { return } if err := deleteFn(ctx); err != nil { diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 57a712a20..0bd897865 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -67,7 +67,7 @@ func (api *Router) routes() http.Handler { r.Use(server.Authenticator(api.ds)) r.Use(server.JWTRefresher) r.Use(server.UpdateLastAccessMiddleware(api.ds)) - api.RX(r, "/user", api.users.NewRepository, true) + api.addUserRoute(r) api.R(r, "/song", model.MediaFile{}, false) api.R(r, "/album", model.Album{}, false) api.addArtistRoute(r) diff --git a/server/nativeapi/users.go b/server/nativeapi/users.go new file mode 100644 index 000000000..fe7e241d0 --- /dev/null +++ b/server/nativeapi/users.go @@ -0,0 +1,87 @@ +package nativeapi + +import ( + "context" + "errors" + "io" + "net/http" + + "github.com/deluan/rest" + "github.com/go-chi/chi/v5" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/server" +) + +func (api *Router) addUserRoute(r chi.Router) { + constructor := func(ctx context.Context) rest.Repository { + return api.users.NewRepository(ctx) + } + r.Route("/user", func(r chi.Router) { + r.Get("/", rest.GetAll(constructor)) + r.Post("/", rest.Post(constructor)) + r.Route("/{id}", func(r chi.Router) { + r.Use(server.URLParamsMiddleware) + r.Get("/", rest.Get(constructor)) + r.Put("/", rest.Put(constructor)) + r.Delete("/", rest.Delete(constructor)) + r.Post("/image", api.uploadUserAvatar()) + r.Delete("/image", api.deleteUserAvatar()) + }) + }) +} + +// canEditAvatar allows the target user or any admin, and honors the feature flag for everyone. +func canEditAvatar(ctx context.Context, targetID string) error { + if !conf.Server.EnableUserAvatarUpload { + return model.ErrNotAuthorized + } + usr, _ := request.UserFrom(ctx) + if !usr.IsAdmin && usr.ID != targetID { + return model.ErrNotAuthorized + } + return nil +} + +func (api *Router) uploadUserAvatar() http.HandlerFunc { + // false: avatars are gated by EnableUserAvatarUpload, never by EnableArtworkUpload. + return handleImageUploadGated(false, func(ctx context.Context, reader io.Reader, ext string) error { + userID := chi.URLParamFromCtx(ctx, "id") + if err := canEditAvatar(ctx, userID); err != nil { + return err + } + usr, err := api.ds.User(ctx).Get(userID) + if err != nil { + if errors.Is(err, model.ErrNotFound) { + return model.ErrNotFound + } + return err + } + filename, err := api.imgUpload.SetAvatar(ctx, usr.ID, usr.UserName, usr.UploadedImagePath(), reader, ext) + if err != nil { + return err + } + return api.ds.User(ctx).UpdateImage(usr.ID, filename) + }) +} + +func (api *Router) deleteUserAvatar() http.HandlerFunc { + return handleImageDeleteGated(false, func(ctx context.Context) error { + userID := chi.URLParamFromCtx(ctx, "id") + if err := canEditAvatar(ctx, userID); err != nil { + return err + } + usr, err := api.ds.User(ctx).Get(userID) + if err != nil { + if errors.Is(err, model.ErrNotFound) { + return model.ErrNotFound + } + return err + } + if err := api.imgUpload.RemoveImage(ctx, usr.UploadedImagePath()); err != nil { + return err + } + return api.ds.User(ctx).UpdateImage(usr.ID, "") + }) +} diff --git a/server/nativeapi/users_test.go b/server/nativeapi/users_test.go new file mode 100644 index 000000000..d0ddf06b2 --- /dev/null +++ b/server/nativeapi/users_test.go @@ -0,0 +1,109 @@ +package nativeapi + +import ( + "bytes" + "context" + "image" + "image/png" + "io" + "mime/multipart" + "net/http" + "net/http/httptest" + + "github.com/go-chi/chi/v5" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/core/artwork" + "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" +) + +func testRouter(ds model.DataStore) http.Handler { + api := &Router{ds: ds, imgUpload: artwork.NewUploader(ds)} + r := chi.NewRouter() + api.addUserRoute(r) + return r +} + +var _ = Describe("User avatar routes", func() { + var router http.Handler + var ds *tests.MockDataStore + + newRequest := func(method, path string, body io.Reader, contentType string, asUser model.User) *http.Request { + r := httptest.NewRequest(method, path, body) + if contentType != "" { + r.Header.Set("Content-Type", contentType) + } + return r.WithContext(request.WithUser(r.Context(), asUser)) + } + + pngUpload := func() (io.Reader, string) { + var buf bytes.Buffer + mw := multipart.NewWriter(&buf) + part, err := mw.CreateFormFile("image", "avatar.png") + Expect(err).ToNot(HaveOccurred()) + Expect(png.Encode(part, image.NewRGBA(image.Rect(0, 0, 8, 8)))).To(Succeed()) + Expect(mw.Close()).To(Succeed()) + return &buf, mw.FormDataContentType() + } + + regularUser := model.User{ID: "u1", UserName: "regular"} + adminUser := model.User{ID: "admin", UserName: "admin", IsAdmin: true} + otherUser := model.User{ID: "u2", UserName: "other"} + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DataFolder = conf.NewDir(GinkgoT().TempDir()) + conf.Server.EnableUserAvatarUpload = true + ds = &tests.MockDataStore{} + Expect(ds.User(context.Background()).Put(®ularUser)).To(Succeed()) + Expect(ds.User(context.Background()).Put(&adminUser)).To(Succeed()) + Expect(ds.User(context.Background()).Put(&otherUser)).To(Succeed()) + router = testRouter(ds) + }) + + It("lets a user upload their own avatar", func() { + body, ct := pngUpload() + w := httptest.NewRecorder() + router.ServeHTTP(w, newRequest("POST", "/user/u1/image", body, ct, regularUser)) + Expect(w.Code).To(Equal(http.StatusOK)) + + usr, _ := ds.User(context.Background()).Get("u1") + Expect(usr.UploadedImage).To(Equal("u1_regular.png")) + }) + + It("lets an admin upload someone else's avatar", func() { + body, ct := pngUpload() + w := httptest.NewRecorder() + router.ServeHTTP(w, newRequest("POST", "/user/u1/image", body, ct, adminUser)) + Expect(w.Code).To(Equal(http.StatusOK)) + }) + + It("refuses a third party", func() { + body, ct := pngUpload() + w := httptest.NewRecorder() + router.ServeHTTP(w, newRequest("POST", "/user/u1/image", body, ct, otherUser)) + Expect(w.Code).To(Equal(http.StatusForbidden)) + }) + + It("refuses everyone, admins included, when the flag is off", func() { + conf.Server.EnableUserAvatarUpload = false + body, ct := pngUpload() + w := httptest.NewRecorder() + router.ServeHTTP(w, newRequest("POST", "/user/u1/image", body, ct, adminUser)) + Expect(w.Code).To(Equal(http.StatusForbidden)) + }) + + It("clears the filename on delete", func() { + Expect(ds.User(context.Background()).UpdateImage("u1", "u1_regular.png")).To(Succeed()) + w := httptest.NewRecorder() + router.ServeHTTP(w, newRequest("DELETE", "/user/u1/image", nil, "", regularUser)) + Expect(w.Code).To(Equal(http.StatusOK)) + + usr, _ := ds.User(context.Background()).Get("u1") + Expect(usr.UploadedImage).To(BeEmpty()) + }) +}) From 1421604d6ae0b881f9cb746f35ec677a556a702f Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 19:38:59 -0400 Subject: [PATCH 08/21] feat(subsonic): serve uploaded avatars from getAvatar Check the uploaded avatar before the Gravatar/placeholder fallback so getAvatar honors an upload even when Gravatar is disabled. --- server/subsonic/media_retrieval.go | 10 ++- server/subsonic/media_retrieval_test.go | 82 +++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index 8a5152a9d..619545b03 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -20,9 +20,6 @@ import ( ) func (api *Router) GetAvatar(w http.ResponseWriter, r *http.Request) (*responses.Subsonic, error) { - if !conf.Server.EnableGravatar { - return api.getPlaceHolderAvatar(w, r) - } p := req.Params(r) username, err := p.String("username") if err != nil { @@ -33,6 +30,13 @@ func (api *Router) GetAvatar(w http.ResponseWriter, r *http.Request) (*responses if err != nil { return nil, err } + // An uploaded avatar wins regardless of Gravatar settings, so it must be checked first. + if imghttp.ServeUserAvatar(w, r, u) { + return nil, nil + } + if !conf.Server.EnableGravatar { + return api.getPlaceHolderAvatar(w, r) + } if u.Email == "" { log.Warn(ctx, "User needs an email for gravatar to work", "username", username) return api.getPlaceHolderAvatar(w, r) diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index 7610c866a..da22130ff 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -7,7 +7,9 @@ import ( "encoding/json" "errors" "io" + "net/http" "net/http/httptest" + "os" "path/filepath" "slices" "time" @@ -29,16 +31,19 @@ var _ = Describe("MediaRetrievalController", func() { mockRepo := &mockedMediaFile{MockMediaFileRepo: tests.MockMediaFileRepo{}} var artwork *fakeArtwork var w *httptest.ResponseRecorder + var userRepo *tests.MockedUserRepo BeforeEach(func() { albumRepo := &tests.MockAlbumRepo{} albumRepo.SetData(model.Albums{{ID: "34"}}) // the id the specs request, made accessible radioRepo := tests.CreateMockedRadioRepo() Expect(radioRepo.Put(&model.Radio{ID: "rd1", Name: "Radio"})).To(Succeed()) + userRepo = tests.CreateMockUserRepo() ds = &tests.MockDataStore{ MockedMediaFile: mockRepo, MockedAlbum: albumRepo, MockedRadio: radioRepo, + MockedUser: userRepo, } artwork = &fakeArtwork{data: "image data"} router = New(ds, artwork, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, lyrics.NewLyrics(ds, nil), nil, nil) @@ -180,6 +185,74 @@ var _ = Describe("MediaRetrievalController", func() { }) }) + Describe("GetAvatar", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DataFolder = conf.NewDir(GinkgoT().TempDir()) + }) + + It("serves an uploaded avatar even when Gravatar is disabled", func() { + conf.Server.EnableGravatar = false + usr := &model.User{ID: "u1", UserName: "deluan"} + usr.UploadedImage = writeUserAvatar(usr) + Expect(userRepo.Put(usr)).To(Succeed()) + + _, err := router.GetAvatar(w, newGetRequest("username=deluan")) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("ETag")).ToNot(BeEmpty()) + }) + + It("prefers the uploaded avatar over Gravatar", func() { + conf.Server.EnableGravatar = true + usr := &model.User{ID: "u1", UserName: "deluan", Email: "deluan@navidrome.org"} + usr.UploadedImage = writeUserAvatar(usr) + Expect(userRepo.Put(usr)).To(Succeed()) + + _, err := router.GetAvatar(w, newGetRequest("username=deluan")) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(http.StatusOK)) // not 302 + }) + + It("still redirects to Gravatar when there is no upload", func() { + conf.Server.EnableGravatar = true + Expect(userRepo.Put(&model.User{ID: "u2", UserName: "noavatar", Email: "noavatar@navidrome.org"})).To(Succeed()) + + _, err := router.GetAvatar(w, newGetRequest("username=noavatar")) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(http.StatusFound)) + }) + + It("still serves the placeholder when there is nothing at all", func() { + conf.Server.EnableGravatar = false + Expect(userRepo.Put(&model.User{ID: "u2", UserName: "noavatar"})).To(Succeed()) + + _, err := router.GetAvatar(w, newGetRequest("username=noavatar")) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(http.StatusOK)) + }) + + It("serves the placeholder when Gravatar is on but the user has no email", func() { + conf.Server.EnableGravatar = true + Expect(userRepo.Put(&model.User{ID: "u2", UserName: "noavatar"})).To(Succeed()) + + _, err := router.GetAvatar(w, newGetRequest("username=noavatar")) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(http.StatusOK)) + }) + + It("returns the same error as before for an unknown username", func() { + _, err := router.GetAvatar(w, newGetRequest("username=ghost")) + + Expect(err).To(MatchError(model.ErrNotFound)) + }) + }) + Describe("GetLyrics", func() { It("should return data for given artist & title", func() { r := newGetRequest("artist=Rick+Astley", "title=Never+Gonna+Give+You+Up") @@ -255,6 +328,15 @@ var _ = Describe("MediaRetrievalController", func() { }) }) +// writeUserAvatar seeds a fake avatar file and returns the UploadedImage filename for usr. +func writeUserAvatar(usr *model.User) string { + name := usr.ID + "_" + usr.UserName + ".png" + path := filepath.Join(conf.Server.DataFolder.String(), "avatar", name) + Expect(os.MkdirAll(filepath.Dir(path), 0755)).To(Succeed()) + Expect(os.WriteFile(path, []byte{0x89, 'P', 'N', 'G'}, 0600)).To(Succeed()) + return name +} + type fakeArtwork struct { artwork.Artwork data string From e7f3245acd1b6de9fd9f3d61879b736a8bdf0e8a Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 19:43:18 -0400 Subject: [PATCH 09/21] fix(subsonic): keep placeholder for unknown username when Gravatar is off The old getAvatar never looked up the user when Gravatar was disabled (the default), so an unknown username served the placeholder. Restore that for the unresolvable-user case now that lookup always happens. --- server/subsonic/media_retrieval.go | 5 +++++ server/subsonic/media_retrieval_test.go | 13 ++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index 619545b03..ccc73c162 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -28,6 +28,11 @@ func (api *Router) GetAvatar(w http.ResponseWriter, r *http.Request) (*responses ctx := r.Context() u, err := api.ds.User(ctx).FindByUsername(username) if err != nil { + // Preserve the pre-upload-avatar behaviour: an unresolvable user must not surface + // as an error when Gravatar is off, since the old handler never looked it up. + if !conf.Server.EnableGravatar { + return api.getPlaceHolderAvatar(w, r) + } return nil, err } // An uploaded avatar wins regardless of Gravatar settings, so it must be checked first. diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index da22130ff..0b961e834 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -246,11 +246,22 @@ var _ = Describe("MediaRetrievalController", func() { Expect(w.Code).To(Equal(http.StatusOK)) }) - It("returns the same error as before for an unknown username", func() { + It("returns the same error as before for an unknown username when Gravatar is on", func() { + conf.Server.EnableGravatar = true + _, err := router.GetAvatar(w, newGetRequest("username=ghost")) Expect(err).To(MatchError(model.ErrNotFound)) }) + + It("serves the placeholder for an unknown username when Gravatar is off, as before", func() { + conf.Server.EnableGravatar = false + + _, err := router.GetAvatar(w, newGetRequest("username=ghost")) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(http.StatusOK)) + }) }) Describe("GetLyrics", func() { From d57bda78f8f95b5494c6bb1d188f33a492721dd7 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 19:52:35 -0400 Subject: [PATCH 10/21] feat(jellyfin): serve user avatars from GET /userimage Registered unauthenticated (matching real Jellyfin's login-picker use case), but narrowed to conf.Server.Jellyfin.ExposedPublicUsers for anonymous callers. Also fills PrimaryImageTag on userToDto and getPublicUsers. --- server/jellyfin/api.go | 2 + server/jellyfin/auth.go | 1 + server/jellyfin/user_image.go | 67 ++++++++++ server/jellyfin/user_image_test.go | 188 +++++++++++++++++++++++++++++ server/jellyfin/users.go | 16 +-- 5 files changed, 264 insertions(+), 10 deletions(-) create mode 100644 server/jellyfin/user_image.go create mode 100644 server/jellyfin/user_image_test.go diff --git a/server/jellyfin/api.go b/server/jellyfin/api.go index 6484a3bb4..11f884ba4 100644 --- a/server/jellyfin/api.go +++ b/server/jellyfin/api.go @@ -86,6 +86,8 @@ func (api *Router) routes() http.Handler { inner.Post("/users/authenticatebyname", api.authenticateByName) } inner.Get("/users/public", api.getPublicUsers) + // Unauthenticated on purpose, matching Jellyfin, but narrowed to ExposedPublicUsers. + inner.Get("/userimage", api.getUserImage) // Images are intentionally public: artwork isn't sensitive, matching Jellyfin's image handling. // Bound concurrency like Subsonic's getCoverArt: image decode/resize is CPU- and memory-heavy, diff --git a/server/jellyfin/auth.go b/server/jellyfin/auth.go index e7070d341..c1f69d1ef 100644 --- a/server/jellyfin/auth.go +++ b/server/jellyfin/auth.go @@ -61,6 +61,7 @@ func userToDto(u *model.User, serverName, serverID string) *dto.UserDto { ServerName: serverName, HasPassword: true, HasConfiguredPassword: true, + PrimaryImageTag: u.AvatarTag(), Policy: userPolicy(u), Configuration: userConfiguration(), } diff --git a/server/jellyfin/user_image.go b/server/jellyfin/user_image.go new file mode 100644 index 000000000..b21c2c04c --- /dev/null +++ b/server/jellyfin/user_image.go @@ -0,0 +1,67 @@ +package jellyfin + +import ( + "net/http" + "strings" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/server/imghttp" + "github.com/navidrome/navidrome/server/jellyfin/dto" +) + +// getUserImage is registered unauthenticated, like Jellyfin's own GET /UserImage, so a login picker +// can show avatars; anonymous callers are limited to the ExposedPublicUsers allowlist. +func (api *Router) getUserImage(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + // This route skips the authenticate middleware, so a caller's token (if any) is resolved directly. + caller, authenticated := api.userFromToken(r) + + rawID := r.URL.Query().Get("userid") + if rawID == "" { + if !authenticated { + http.Error(w, "UserId is required if unauthenticated", http.StatusBadRequest) + return + } + rawID = dto.EncodeID(caller.ID) + } + id, ok := dto.DecodeID(rawID) + if !ok { + http.Error(w, "Bad Request", http.StatusBadRequest) + return + } + + usr, err := api.ds.User(ctx).Get(id) + if err != nil { + http.Error(w, "Not Found", http.StatusNotFound) + return + } + if !authenticated && !isPublicUser(usr.UserName) { + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + if !imghttp.ServeUserAvatar(w, r, usr) { + http.Error(w, "Not Found", http.StatusNotFound) + } +} + +// publicUsernames splits and normalizes the raw ExposedPublicUsers config: comma-separated, trimmed, +// skipping empty entries. Shared with getPublicUsers so the two allowlist checks can't drift apart. +func publicUsernames() []string { + var names []string + for name := range strings.SplitSeq(conf.Server.Jellyfin.ExposedPublicUsers, ",") { + name = strings.TrimSpace(name) + if name != "" { + names = append(names, name) + } + } + return names +} + +func isPublicUser(username string) bool { + for _, name := range publicUsernames() { + if strings.EqualFold(name, username) { + return true + } + } + return false +} diff --git a/server/jellyfin/user_image_test.go b/server/jellyfin/user_image_test.go new file mode 100644 index 000000000..da78816b6 --- /dev/null +++ b/server/jellyfin/user_image_test.go @@ -0,0 +1,188 @@ +package jellyfin + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/server/jellyfin/dto" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// writeUserAvatar seeds a fake avatar file on disk and returns the UploadedImage filename to store +// on the user record. +func writeUserAvatar(u *model.User) string { + name := u.ID + "_" + u.UserName + ".png" + path := filepath.Join(conf.Server.DataFolder.String(), consts.AvatarFolder, name) + Expect(os.MkdirAll(filepath.Dir(path), 0755)).To(Succeed()) + Expect(os.WriteFile(path, []byte{0x89, 'P', 'N', 'G'}, 0600)).To(Succeed()) + return name +} + +var _ = Describe("GET /userimage", func() { + var api *Router + var ds *tests.MockDataStore + var ur *tests.MockedUserRepo + var pub, priv *model.User + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DataFolder = conf.NewDir(GinkgoT().TempDir()) + conf.Server.Jellyfin.ExposedPublicUsers = "publicuser" + + ds = &tests.MockDataStore{} + auth.Init(ds) + ur = ds.User(context.Background()).(*tests.MockedUserRepo) + + pub = &model.User{ID: testID("pub1"), UserName: "publicuser"} + pub.UploadedImage = writeUserAvatar(pub) + Expect(ur.Put(pub)).To(Succeed()) + + priv = &model.User{ID: testID("u1"), UserName: "alice"} + Expect(ur.Put(priv)).To(Succeed()) + + api = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + }) + + get := func(query string) *httptest.ResponseRecorder { + w := httptest.NewRecorder() + api.ServeHTTP(w, httptest.NewRequest("GET", "/userimage"+query, nil)) + return w + } + + It("serves an allowlisted user's avatar without authentication", func() { + w := get("?userId=" + dto.EncodeID(pub.ID)) + Expect(w.Code).To(Equal(http.StatusOK)) + }) + + // This is the leak this whole design exists to prevent: anonymous access must be denied for + // anyone not on the allowlist, not just fall through to a 404. + It("refuses an anonymous request for a user not on the allowlist with 401", func() { + w := get("?userId=" + dto.EncodeID(priv.ID)) + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) + + It("refuses every anonymous request when the allowlist is empty", func() { + conf.Server.Jellyfin.ExposedPublicUsers = "" + w := get("?userId=" + dto.EncodeID(pub.ID)) + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) + + It("rejects an anonymous request with no userId with 400", func() { + w := get("") + Expect(w.Code).To(Equal(http.StatusBadRequest)) + }) + + It("returns 404 for an authenticated user with no avatar", func() { + tok, err := auth.CreateToken(priv) + Expect(err).ToNot(HaveOccurred()) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/userimage?userId="+dto.EncodeID(priv.ID), nil) + r.Header.Set("X-Emby-Token", tok) + api.ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusNotFound)) + }) + + It("matches an allowlist entry regardless of surrounding whitespace and casing", func() { + conf.Server.Jellyfin.ExposedPublicUsers = " PublicUser " + w := get("?userId=" + dto.EncodeID(pub.ID)) + Expect(w.Code).To(Equal(http.StatusOK)) + }) + + It("404s an authenticated request for an id that decodes but does not exist", func() { + tok, err := auth.CreateToken(priv) + Expect(err).ToNot(HaveOccurred()) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/userimage?userId="+dto.EncodeID(testID("ghost")), nil) + r.Header.Set("X-Emby-Token", tok) + api.ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusNotFound)) + }) +}) + +var _ = Describe("isPublicUser", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + + It("matches a configured name case-insensitively and trims whitespace", func() { + conf.Server.Jellyfin.ExposedPublicUsers = " Alice ,bob" + Expect(isPublicUser("alice")).To(BeTrue()) + Expect(isPublicUser("ALICE")).To(BeTrue()) + Expect(isPublicUser("bob")).To(BeTrue()) + }) + + It("rejects a user not on the allowlist", func() { + conf.Server.Jellyfin.ExposedPublicUsers = "alice" + Expect(isPublicUser("eve")).To(BeFalse()) + }) + + It("rejects everyone when the allowlist is empty", func() { + conf.Server.Jellyfin.ExposedPublicUsers = "" + Expect(isPublicUser("alice")).To(BeFalse()) + }) +}) + +var _ = Describe("PrimaryImageTag", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + + It("is present on userToDto when the user has an avatar", func() { + u := model.User{ID: "u1", UserName: "deluan", UploadedImage: "u1_deluan.png"} + got := userToDto(&u, "srv", "sid") + Expect(got.PrimaryImageTag).To(Equal(u.AvatarTag())) + Expect(got.PrimaryImageTag).ToNot(BeEmpty()) + }) + + It("is empty on userToDto when the user has no avatar", func() { + u := model.User{ID: "u1", UserName: "deluan"} + Expect(userToDto(&u, "srv", "sid").PrimaryImageTag).To(BeEmpty()) + }) + + It("is present on getPublicUsers when the listed user has an avatar", func() { + ds := &tests.MockDataStore{} + ur := ds.User(context.Background()).(*tests.MockedUserRepo) + u := &model.User{ID: testID("pub1"), UserName: "publicuser", UploadedImage: "x.png"} + Expect(ur.Put(u)).To(Succeed()) + conf.Server.Jellyfin.ExposedPublicUsers = "publicuser" + + api := &Router{ds: ds} + w := httptest.NewRecorder() + api.getPublicUsers(w, httptest.NewRequest("GET", "/users/public", nil)) + Expect(w.Code).To(Equal(http.StatusOK)) + + var users []dto.UserDto + Expect(json.Unmarshal(w.Body.Bytes(), &users)).To(Succeed()) + Expect(users).To(HaveLen(1)) + Expect(users[0].PrimaryImageTag).To(Equal(u.AvatarTag())) + Expect(users[0].PrimaryImageTag).ToNot(BeEmpty()) + }) + + It("is empty on getPublicUsers when the listed user has no avatar", func() { + ds := &tests.MockDataStore{} + ur := ds.User(context.Background()).(*tests.MockedUserRepo) + Expect(ur.Put(&model.User{ID: testID("pub1"), UserName: "publicuser"})).To(Succeed()) + conf.Server.Jellyfin.ExposedPublicUsers = "publicuser" + + api := &Router{ds: ds} + w := httptest.NewRecorder() + api.getPublicUsers(w, httptest.NewRequest("GET", "/users/public", nil)) + Expect(w.Code).To(Equal(http.StatusOK)) + + var users []dto.UserDto + Expect(json.Unmarshal(w.Body.Bytes(), &users)).To(Succeed()) + Expect(users).To(HaveLen(1)) + Expect(users[0].PrimaryImageTag).To(BeEmpty()) + }) +}) diff --git a/server/jellyfin/users.go b/server/jellyfin/users.go index bbc60c892..622715fbf 100644 --- a/server/jellyfin/users.go +++ b/server/jellyfin/users.go @@ -4,7 +4,6 @@ import ( "net/http" "strings" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/jellyfin/dto" @@ -35,11 +34,7 @@ func (api *Router) getPublicUsers(w http.ResponseWriter, r *http.Request) { serverID := api.serverID(ctx) seen := make(map[string]bool) users := []dto.UserDto{} - for name := range strings.SplitSeq(conf.Server.Jellyfin.ExposedPublicUsers, ",") { - name = strings.TrimSpace(name) - if name == "" { - continue - } + for _, name := range publicUsernames() { key := strings.ToLower(name) if seen[key] { continue @@ -51,10 +46,11 @@ func (api *Router) getPublicUsers(w http.ResponseWriter, r *http.Request) { continue } users = append(users, dto.UserDto{ - Name: usr.UserName, - Id: dto.EncodeID(usr.ID), - ServerId: serverID, - HasPassword: true, + Name: usr.UserName, + Id: dto.EncodeID(usr.ID), + ServerId: serverID, + HasPassword: true, + PrimaryImageTag: usr.AvatarTag(), }) } api.ok(w, r, users) From 669d9216ec83f7ee6947d82a0874fdef10ef1698 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 20:06:50 -0400 Subject: [PATCH 11/21] feat(jellyfin): accept avatar upload and delete on /userimage Adds POST/DELETE /userimage (authenticated-only), enforcing the self-or-admin write rule and the EnableUserAvatarUpload flag. Also strengthens the anonymous-401 GET /userimage test to seed the private user with a real avatar file, so it fails for the right reason if the isPublicUser gate is ever removed. --- cmd/wire_gen.go | 2 +- server/jellyfin/api.go | 7 +- server/jellyfin/api_test.go | 10 +- server/jellyfin/e2e/e2e_suite_test.go | 4 +- server/jellyfin/routing_test.go | 2 +- server/jellyfin/socket_test.go | 2 +- server/jellyfin/user_image.go | 96 ++++++++++++++++++ server/jellyfin/user_image_test.go | 140 +++++++++++++++++++++++++- 8 files changed, 248 insertions(+), 15 deletions(-) diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index de4c55a1e..e47b18cf1 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -135,7 +135,7 @@ func CreateJellyfinAPIRouter(ctx context.Context) *jellyfin.Router { provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher, broker) sonicSonic := sonic.New(dataStore, manager, matcherMatcher) lyricsLyrics := lyrics.NewLyrics(dataStore, manager) - router := jellyfin.New(dataStore, artworkArtwork, mediaStreamer, transcodeDecider, players, playTracker, playlistsPlaylists, provider, sonicSonic, lyricsLyrics, broker) + router := jellyfin.New(dataStore, artworkArtwork, mediaStreamer, transcodeDecider, players, playTracker, playlistsPlaylists, provider, sonicSonic, lyricsLyrics, broker, uploader) return router } diff --git a/server/jellyfin/api.go b/server/jellyfin/api.go index 11f884ba4..b290dde7d 100644 --- a/server/jellyfin/api.go +++ b/server/jellyfin/api.go @@ -40,6 +40,7 @@ type Router struct { sonic sonic.Engine lyrics lyrics.Lyrics broker events.Broker + imgUpload artwork.Uploader lyricsCache cache.SimpleCache[string, model.LyricList] similarFlight singleflight.Group serverIDMu sync.Mutex @@ -49,11 +50,11 @@ type Router struct { func New(ds model.DataStore, artwork artwork.Artwork, streamer stream.MediaStreamer, transcodeDecider stream.TranscodeDecider, players core.Players, scrobbler scrobbler.PlayTracker, playlists playlists.Playlists, provider external.Provider, - sonicSvc sonic.Engine, lyricsSvc lyrics.Lyrics, broker events.Broker) *Router { + sonicSvc sonic.Engine, lyricsSvc lyrics.Lyrics, broker events.Broker, imgUpload artwork.Uploader) *Router { r := &Router{ ds: ds, artwork: artwork, streamer: streamer, transcodeDecider: transcodeDecider, players: players, scrobbler: scrobbler, playlists: playlists, provider: provider, - sonic: sonicSvc, lyrics: lyricsSvc, broker: broker, + sonic: sonicSvc, lyrics: lyricsSvc, broker: broker, imgUpload: imgUpload, lyricsCache: cache.NewSimpleCache[string, model.LyricList](cache.Options{ SizeLimit: 1000, DefaultTTL: 5 * time.Minute, @@ -111,6 +112,8 @@ func (api *Router) routes() http.Handler { r.Get("/users/{userId}/views", api.getUserViews) r.Get("/users/me", api.getCurrentUser) r.Get("/users/{userId}", api.getCurrentUser) + r.Post("/userimage", api.postUserImage) + r.Delete("/userimage", api.deleteUserImage) // Cursor-backed collections: each streams straight from the DB, holding a connection for the // whole client-paced response, so enough slow clients would take the entire pool and stall the diff --git a/server/jellyfin/api_test.go b/server/jellyfin/api_test.go index fe69c8321..3d9c04126 100644 --- a/server/jellyfin/api_test.go +++ b/server/jellyfin/api_test.go @@ -18,7 +18,7 @@ import ( var _ = Describe("Router", func() { It("serves the public handshake through the mounted handler", func() { ds := &tests.MockDataStore{} - api := New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + api := New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/System/Info/Public", nil) api.ServeHTTP(w, r) @@ -26,7 +26,7 @@ var _ = Describe("Router", func() { }) It("returns 404 JSON for unknown routes", func() { - api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/Nonexistent/Route", nil) api.ServeHTTP(w, r) @@ -36,7 +36,7 @@ var _ = Describe("Router", func() { }) It("returns 404 JSON for a known path with an unsupported method", func() { - api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) w := httptest.NewRecorder() r := httptest.NewRequest("PATCH", "/System/Info/Public", nil) api.ServeHTTP(w, r) @@ -53,7 +53,7 @@ var _ = Describe("Router", func() { Expect(err).ToNot(HaveOccurred()) fp := &fakePlayers{} - api := New(ds, nil, nil, nil, fp, nil, nil, nil, nil, nil, nil) + api := New(ds, nil, nil, nil, fp, nil, nil, nil, nil, nil, nil, nil) w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/Users/Me", nil) @@ -70,7 +70,7 @@ var _ = Describe("Router", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.AuthRequestLimit = 2 conf.Server.AuthWindowLength = time.Minute - api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) login := func() int { w := httptest.NewRecorder() diff --git a/server/jellyfin/e2e/e2e_suite_test.go b/server/jellyfin/e2e/e2e_suite_test.go index 1ee19ea01..b46390125 100644 --- a/server/jellyfin/e2e/e2e_suite_test.go +++ b/server/jellyfin/e2e/e2e_suite_test.go @@ -326,6 +326,7 @@ func setupTestDB() { sonicProviderFake = &fakeSonicProvider{} sonicSvc := sonic.New(ds, &fakeSonicLoader{provider: sonicProviderFake}, matcher.New(ds)) decider := stream.NewTranscodeDecider(ds, harness.NoopFFmpeg{}) + imgUpload := artwork.NewUploader(ds) router = jellyfin.New( ds, artworkSpy, @@ -333,11 +334,12 @@ func setupTestDB() { decider, core.NewPlayers(ds), scrobbler.NewPlayTracker(ds, events.NoopBroker(), nil), - playlists.NewPlaylists(ds, artwork.NewUploader(ds)), + playlists.NewPlaylists(ds, imgUpload), providerFake, sonicSvc, lyrics.NewLyrics(ds, nil), events.NoopBroker(), + imgUpload, ) } diff --git a/server/jellyfin/routing_test.go b/server/jellyfin/routing_test.go index 62b52cfff..70da5b1eb 100644 --- a/server/jellyfin/routing_test.go +++ b/server/jellyfin/routing_test.go @@ -19,7 +19,7 @@ var _ = Describe("Case-insensitive routing", func() { var api *Router BeforeEach(func() { - api = New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + api = New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) }) It("serves a fully lowercase path directly", func() { diff --git a/server/jellyfin/socket_test.go b/server/jellyfin/socket_test.go index 401c791fd..b4be0e244 100644 --- a/server/jellyfin/socket_test.go +++ b/server/jellyfin/socket_test.go @@ -94,7 +94,7 @@ var _ = Describe("handleSocket", func() { Expect(err).ToNot(HaveOccurred()) token = t - api = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + api = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) }) It("upgrades when authenticated via the api_key query parameter", func() { diff --git a/server/jellyfin/user_image.go b/server/jellyfin/user_image.go index b21c2c04c..e58b8e4c3 100644 --- a/server/jellyfin/user_image.go +++ b/server/jellyfin/user_image.go @@ -1,10 +1,16 @@ package jellyfin import ( + "bytes" + "io" "net/http" "strings" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/core/artwork" + "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/server/jellyfin/dto" ) @@ -65,3 +71,93 @@ func isPublicUser(username string) bool { } return false } + +var avatarExtByContentType = map[string]string{ + "image/png": ".png", + "image/jpeg": ".jpg", + "image/jpg": ".jpg", + "image/gif": ".gif", + "image/webp": ".webp", +} + +// targetUser resolves the userid param, defaulting to the caller, and applies the +// self-or-admin write rule plus the feature flag (which refuses everyone, admins included). +func (api *Router) targetUser(w http.ResponseWriter, r *http.Request) (*model.User, bool) { + ctx := r.Context() + caller, _ := request.UserFrom(ctx) + id := caller.ID + if raw := r.URL.Query().Get("userid"); raw != "" { + decoded, ok := dto.DecodeID(raw) + if !ok { + http.Error(w, "Bad Request", http.StatusBadRequest) + return nil, false + } + id = decoded + } + if !conf.Server.EnableUserAvatarUpload || (!caller.IsAdmin && caller.ID != id) { + http.Error(w, "Forbidden", http.StatusForbidden) + return nil, false + } + usr, err := api.ds.User(ctx).Get(id) + if err != nil { + http.Error(w, "Not Found", http.StatusNotFound) + return nil, false + } + return usr, true +} + +func (api *Router) postUserImage(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + usr, ok := api.targetUser(w, r) + if !ok { + return + } + mimeType := strings.TrimSpace(strings.SplitN(r.Header.Get("Content-Type"), ";", 2)[0]) + ext, known := avatarExtByContentType[strings.ToLower(mimeType)] + if !known { + http.Error(w, "Incorrect ContentType.", http.StatusBadRequest) + return + } + + // Jellyfin clients base64-encode the wire body (4/3 bigger), so the read cap allows for inflation. + limit := artwork.MaxImageUploadSize() + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, limit*4/3+4)) + if err != nil { + http.Error(w, "file too large", http.StatusBadRequest) + return + } + imgBytes, err := decodeImageBody(body) + if err != nil { + http.Error(w, "Bad Request", http.StatusBadRequest) + return + } + + filename, err := api.imgUpload.SetAvatar(ctx, usr.ID, usr.UserName, usr.UploadedImagePath(), bytes.NewReader(imgBytes), ext) + if err != nil { + log.Error(ctx, "Jellyfin API: could not save avatar", "user", usr.UserName, err) + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } + if err := api.ds.User(ctx).UpdateImage(usr.ID, filename); err != nil { + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) +} + +func (api *Router) deleteUserImage(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + usr, ok := api.targetUser(w, r) + if !ok { + return + } + if err := api.imgUpload.RemoveImage(ctx, usr.UploadedImagePath()); err != nil { + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } + if err := api.ds.User(ctx).UpdateImage(usr.ID, ""); err != nil { + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) +} diff --git a/server/jellyfin/user_image_test.go b/server/jellyfin/user_image_test.go index da78816b6..d746f9476 100644 --- a/server/jellyfin/user_image_test.go +++ b/server/jellyfin/user_image_test.go @@ -1,16 +1,23 @@ package jellyfin import ( + "bytes" "context" + "encoding/base64" "encoding/json" + "image" + "image/png" + "io" "net/http" "net/http/httptest" "os" "path/filepath" + "strings" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/server/jellyfin/dto" @@ -33,7 +40,7 @@ var _ = Describe("GET /userimage", func() { var api *Router var ds *tests.MockDataStore var ur *tests.MockedUserRepo - var pub, priv *model.User + var pub, priv, noAvatar *model.User BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) @@ -48,10 +55,16 @@ var _ = Describe("GET /userimage", func() { pub.UploadedImage = writeUserAvatar(pub) Expect(ur.Put(pub)).To(Succeed()) + // Has a real avatar so the anonymous-401 test below fails on a served image (200), not a + // 404, if the isPublicUser gate is ever removed. priv = &model.User{ID: testID("u1"), UserName: "alice"} + priv.UploadedImage = writeUserAvatar(priv) Expect(ur.Put(priv)).To(Succeed()) - api = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + noAvatar = &model.User{ID: testID("u4"), UserName: "carol"} + Expect(ur.Put(noAvatar)).To(Succeed()) + + api = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) }) get := func(query string) *httptest.ResponseRecorder { @@ -84,10 +97,10 @@ var _ = Describe("GET /userimage", func() { }) It("returns 404 for an authenticated user with no avatar", func() { - tok, err := auth.CreateToken(priv) + tok, err := auth.CreateToken(noAvatar) Expect(err).ToNot(HaveOccurred()) w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/userimage?userId="+dto.EncodeID(priv.ID), nil) + r := httptest.NewRequest("GET", "/userimage?userId="+dto.EncodeID(noAvatar.ID), nil) r.Header.Set("X-Emby-Token", tok) api.ServeHTTP(w, r) Expect(w.Code).To(Equal(http.StatusNotFound)) @@ -110,6 +123,125 @@ var _ = Describe("GET /userimage", func() { }) }) +var _ = Describe("POST /userimage and DELETE /userimage", func() { + var router *Router + var ds *tests.MockDataStore + var ur *tests.MockedUserRepo + var caller *model.User + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DataFolder = conf.NewDir(GinkgoT().TempDir()) + conf.Server.EnableUserAvatarUpload = true + + ds = &tests.MockDataStore{} + auth.Init(ds) + ur = ds.User(context.Background()).(*tests.MockedUserRepo) + + caller = &model.User{ID: "u1", UserName: "regular"} + Expect(ur.Put(caller)).To(Succeed()) + // A real canonical id, unlike caller's literal "u1", so it round-trips through dto.EncodeID. + Expect(ur.Put(&model.User{ID: testID("u2"), UserName: "other"})).To(Succeed()) + + router = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, artwork.NewUploader(ds)) + }) + + tokenFor := func(u *model.User) string { + tok, err := auth.CreateToken(u) + Expect(err).ToNot(HaveOccurred()) + return tok + } + authenticatedRequestWithBody := func(method, target string, body io.Reader) *http.Request { + r := httptest.NewRequest(method, target, body) + r.Header.Set("X-Emby-Token", tokenFor(caller)) + return r + } + authenticatedRequest := func(method, target string) *http.Request { + return authenticatedRequestWithBody(method, target, nil) + } + pngBytes := func() []byte { + var buf bytes.Buffer + Expect(png.Encode(&buf, image.NewRGBA(image.Rect(0, 0, 8, 8)))).To(Succeed()) + return buf.Bytes() + } + + Describe("POST /userimage", func() { + It("accepts a base64 body with a charset suffix", func() { + body := base64.StdEncoding.EncodeToString(pngBytes()) + r := authenticatedRequestWithBody("POST", "/userimage", strings.NewReader(body)) + r.Header.Set("Content-Type", "image/png; charset=utf-8") + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusNoContent)) + + usr, _ := ds.User(context.Background()).Get("u1") + Expect(usr.UploadedImage).To(Equal("u1_regular.png")) + }) + + It("accepts raw image bytes too", func() { + r := authenticatedRequestWithBody("POST", "/userimage", bytes.NewReader(pngBytes())) + r.Header.Set("Content-Type", "image/png") + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusNoContent)) + }) + + It("rejects an unknown content type", func() { + r := authenticatedRequestWithBody("POST", "/userimage", strings.NewReader("x")) + r.Header.Set("Content-Type", "text/plain") + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusBadRequest)) + }) + + It("refuses a third party", func() { + body := base64.StdEncoding.EncodeToString(pngBytes()) + r := authenticatedRequestWithBody("POST", "/userimage?userId="+dto.EncodeID(testID("u2")), strings.NewReader(body)) + r.Header.Set("Content-Type", "image/png") + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusForbidden)) + }) + + It("refuses even an admin when the flag is off", func() { + conf.Server.EnableUserAvatarUpload = false + admin := &model.User{ID: "admin1", UserName: "boss", IsAdmin: true} + Expect(ur.Put(admin)).To(Succeed()) + + body := base64.StdEncoding.EncodeToString(pngBytes()) + r := httptest.NewRequest("POST", "/userimage", strings.NewReader(body)) + r.Header.Set("X-Emby-Token", tokenFor(admin)) + r.Header.Set("Content-Type", "image/png") + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusForbidden)) + }) + + It("is not reachable anonymously", func() { + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("POST", "/userimage", strings.NewReader("x"))) + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) + }) + + Describe("DELETE /userimage", func() { + It("clears the avatar and removes the file from disk", func() { + name := writeUserAvatar(caller) + Expect(ur.UpdateImage(caller.ID, name)).To(Succeed()) + path := caller.UploadedImagePath() + + w := httptest.NewRecorder() + router.ServeHTTP(w, authenticatedRequest("DELETE", "/userimage")) + Expect(w.Code).To(Equal(http.StatusNoContent)) + + usr, _ := ds.User(context.Background()).Get("u1") + Expect(usr.UploadedImage).To(BeEmpty()) + _, err := os.Stat(path) + Expect(os.IsNotExist(err)).To(BeTrue()) + }) + }) +}) + var _ = Describe("isPublicUser", func() { BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) From c5a29983a54195453b35e2307b18a7e0fb1c32ba Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 20:14:13 -0400 Subject: [PATCH 12/21] fix(jellyfin): reject oversized raw avatar uploads before resizing postUserImage capped the read for base64 inflation but never checked the decoded body against MaxImageUploadSize, so a raw upload up to ~33% over the limit reached SetAvatar, whose io.LimitReader silently truncated it. Add the same explicit size check postItemImage already uses. --- server/jellyfin/user_image.go | 8 ++++++++ server/jellyfin/user_image_test.go | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/server/jellyfin/user_image.go b/server/jellyfin/user_image.go index e58b8e4c3..317082024 100644 --- a/server/jellyfin/user_image.go +++ b/server/jellyfin/user_image.go @@ -6,6 +6,7 @@ import ( "net/http" "strings" + "github.com/dustin/go-humanize" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/log" @@ -131,6 +132,13 @@ func (api *Router) postUserImage(w http.ResponseWriter, r *http.Request) { http.Error(w, "Bad Request", http.StatusBadRequest) return } + // Raw (non-base64) bodies skip the inflation math above, so the decoded size still needs its own check. + if int64(len(imgBytes)) > limit { + log.Warn(ctx, "Jellyfin API: avatar upload rejected: image exceeds MaxImageUploadSize", + "user", usr.UserName, "size", humanize.Bytes(uint64(len(imgBytes))), "limit", humanize.Bytes(uint64(limit))) + http.Error(w, "file too large", http.StatusBadRequest) + return + } filename, err := api.imgUpload.SetAvatar(ctx, usr.ID, usr.UserName, usr.UploadedImagePath(), bytes.NewReader(imgBytes), ext) if err != nil { diff --git a/server/jellyfin/user_image_test.go b/server/jellyfin/user_image_test.go index d746f9476..816551d49 100644 --- a/server/jellyfin/user_image_test.go +++ b/server/jellyfin/user_image_test.go @@ -12,6 +12,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strconv" "strings" "github.com/navidrome/navidrome/conf" @@ -186,6 +187,21 @@ var _ = Describe("POST /userimage and DELETE /userimage", func() { Expect(w.Code).To(Equal(http.StatusNoContent)) }) + It("rejects raw bytes over MaxImageUploadSize instead of letting SetAvatar truncate them", func() { + img := pngBytes() + // One byte under the image size: still well inside the base64-inflation read cap + // (limit*4/3+4), so only the post-decode size check can catch this. + conf.Server.MaxImageUploadSize = strconv.Itoa(len(img)-1) + "B" + r := authenticatedRequestWithBody("POST", "/userimage", bytes.NewReader(img)) + r.Header.Set("Content-Type", "image/png") + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusBadRequest)) + + usr, _ := ds.User(context.Background()).Get("u1") + Expect(usr.UploadedImage).To(BeEmpty()) + }) + It("rejects an unknown content type", func() { r := authenticatedRequestWithBody("POST", "/userimage", strings.NewReader("x")) r.Header.Set("Content-Type", "text/plain") From c5303e06e1db756d89168e1db0288e82893cd5a4 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 20:18:14 -0400 Subject: [PATCH 13/21] feat(server): return avatarTag in the login payload --- server/auth.go | 3 +++ server/auth_test.go | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/server/auth.go b/server/auth.go index 37a318a83..0a9fb369a 100644 --- a/server/auth.go +++ b/server/auth.go @@ -80,6 +80,9 @@ func buildAuthPayload(user *model.User) map[string]any { if conf.Server.EnableGravatar && user.Email != "" { payload["avatar"] = gravatar.Url(user.Email, 50) } + if tag := user.AvatarTag(); tag != "" { + payload["avatarTag"] = tag + } bytes := make([]byte, 3) _, err := rand.Read(bytes) diff --git a/server/auth_test.go b/server/auth_test.go index e021c82a8..518e148c8 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -216,6 +216,18 @@ var _ = Describe("Auth", func() { Expect(parsed["token"]).ToNot(BeEmpty()) }) }) + + Describe("buildAuthPayload", func() { + It("includes avatarTag when the user has an uploaded avatar", func() { + u := &model.User{ID: "u1", UserName: "deluan", UploadedImage: "u1_deluan.png", UpdatedAt: time.Unix(1000, 0)} + Expect(buildAuthPayload(u)["avatarTag"]).To(Equal(u.AvatarTag())) + }) + + It("omits avatarTag when there is none", func() { + u := &model.User{ID: "u1", UserName: "deluan"} + Expect(buildAuthPayload(u)).ToNot(HaveKey("avatarTag")) + }) + }) }) Describe("tokenFromHeader", func() { From 89d8fbbd134c89c9cbe171c9faf11cc581323e22 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 20:27:02 -0400 Subject: [PATCH 14/21] refactor(ui): let ImageUploadOverlay take its own gate and messages --- ui/src/common/ImageUploadOverlay.jsx | 69 +++++++++++++++++++---- ui/src/common/ImageUploadOverlay.test.jsx | 36 ++++++++++++ 2 files changed, 94 insertions(+), 11 deletions(-) create mode 100644 ui/src/common/ImageUploadOverlay.test.jsx diff --git a/ui/src/common/ImageUploadOverlay.jsx b/ui/src/common/ImageUploadOverlay.jsx index a370e40fe..119426c73 100644 --- a/ui/src/common/ImageUploadOverlay.jsx +++ b/ui/src/common/ImageUploadOverlay.jsx @@ -3,6 +3,7 @@ import { makeStyles } from '@material-ui/core/styles' import PhotoCameraIcon from '@material-ui/icons/PhotoCamera' import DeleteIcon from '@material-ui/icons/Delete' import { useTranslate, useNotify, useRefresh } from 'react-admin' +import PropTypes from 'prop-types' import { useCallback, useRef } from 'react' import config from '../config' import { REST_URL } from '../consts' @@ -36,11 +37,22 @@ const useStyles = makeStyles(() => ({ }, })) +const defaultMessages = { + uploaded: 'message.coverUploaded', + uploadError: 'message.coverUploadError', + removed: 'message.coverRemoved', + removeError: 'message.coverRemoveError', + uploadLabel: 'message.uploadCover', + removeLabel: 'message.removeCover', +} + export const ImageUploadOverlay = ({ entityType, entityId, hasUploadedImage, onImageChange, + canEdit, + messages, }) => { const translate = useTranslate() const notify = useNotify() @@ -48,8 +60,11 @@ export const ImageUploadOverlay = ({ const classes = useStyles() const fileInputRef = useRef(null) - const canEdit = - config.enableArtworkUpload || localStorage.getItem('role') === 'admin' + const msg = { ...defaultMessages, ...messages } + // Callers pass no canEdit today; `??` (not `||`) keeps canEdit={false} from being ignored. + const allowed = + canEdit ?? + (config.enableArtworkUpload || localStorage.getItem('role') === 'admin') const handleUploadClick = useCallback((e) => { e.stopPropagation() @@ -72,16 +87,24 @@ export const ImageUploadOverlay = ({ headers: new Headers({}), body: formData, }) - notify(`message.coverUploaded`, 'success') + notify(msg.uploaded, 'success') if (onImageChange) onImageChange() refresh() } catch (err) { - notify(`message.coverUploadError`, 'warning') + notify(msg.uploadError, 'warning') } e.target.value = '' }, - [entityType, entityId, notify, refresh, onImageChange], + [ + entityType, + entityId, + notify, + refresh, + onImageChange, + msg.uploaded, + msg.uploadError, + ], ) const handleRemoveCover = useCallback( @@ -93,21 +116,29 @@ export const ImageUploadOverlay = ({ await httpClient(`${REST_URL}/${entityType}/${entityId}/image`, { method: 'DELETE', }) - notify(`message.coverRemoved`, 'success') + notify(msg.removed, 'success') if (onImageChange) onImageChange() refresh() } catch (err) { - notify(`message.coverRemoveError`, 'warning') + notify(msg.removeError, 'warning') } }, - [entityType, entityId, notify, refresh, onImageChange], + [ + entityType, + entityId, + notify, + refresh, + onImageChange, + msg.removed, + msg.removeError, + ], ) - if (!canEdit) return null + if (!allowed) return null return (
- + {hasUploadedImage && ( - + ) } + +ImageUploadOverlay.propTypes = { + entityType: PropTypes.string.isRequired, + entityId: PropTypes.string, + hasUploadedImage: PropTypes.bool, + onImageChange: PropTypes.func, + canEdit: PropTypes.bool, + messages: PropTypes.shape({ + uploaded: PropTypes.string, + uploadError: PropTypes.string, + removed: PropTypes.string, + removeError: PropTypes.string, + uploadLabel: PropTypes.string, + removeLabel: PropTypes.string, + }), +} diff --git a/ui/src/common/ImageUploadOverlay.test.jsx b/ui/src/common/ImageUploadOverlay.test.jsx new file mode 100644 index 000000000..904e49829 --- /dev/null +++ b/ui/src/common/ImageUploadOverlay.test.jsx @@ -0,0 +1,36 @@ +import React from 'react' +import { render, screen } from '@testing-library/react' +import { TestContext } from 'ra-test' +import { describe, it, expect, vi } from 'vitest' +import { ImageUploadOverlay } from './ImageUploadOverlay' +import config from '../config' + +vi.mock('react-admin', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useTranslate: () => (x) => x, + useNotify: () => vi.fn(), + useRefresh: () => vi.fn(), + } +}) + +const renderOverlay = (props) => + render( + + + , + ) + +describe('ImageUploadOverlay', () => { + it('renders nothing when canEdit is false', () => { + const { container } = renderOverlay({ canEdit: false }) + expect(container).toBeEmptyDOMElement() + }) + + it('renders when canEdit is true even if artwork upload is off', () => { + config.enableArtworkUpload = false + renderOverlay({ canEdit: true }) + expect(screen.getByRole('button')).toBeInTheDocument() + }) +}) From e405723c43cc3dad07d3c1113885d72e0487ee6c Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 20:29:01 -0400 Subject: [PATCH 15/21] feat(ui): add avatar upload to the user edit page --- ui/src/i18n/en.json | 6 ++++ ui/src/user/UserEdit.jsx | 58 +++++++++++++++++++++++++++++++++-- ui/src/user/UserEdit.test.jsx | 52 +++++++++++++++++++++++++++++-- 3 files changed, 112 insertions(+), 4 deletions(-) diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index de96d47c0..6c23f393b 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -571,6 +571,12 @@ "coverRemoved": "Cover art removed", "coverUploadError": "Error uploading cover art", "coverRemoveError": "Error removing cover art", + "uploadAvatar": "Upload avatar", + "removeAvatar": "Remove avatar", + "avatarUploaded": "Avatar updated", + "avatarUploadError": "Error updating avatar", + "avatarRemoved": "Avatar removed", + "avatarRemoveError": "Error removing avatar", "metadataRefreshStarted": "Refreshing metadata in the background", "note": "NOTE", "transcodingDisabled": "Changing the transcoding configuration through the web interface is disabled for security reasons. If you would like to change (edit or add) transcoding options, restart the server with the %{config} configuration option.", diff --git a/ui/src/user/UserEdit.jsx b/ui/src/user/UserEdit.jsx index c5d9c75a4..b6523bc2b 100644 --- a/ui/src/user/UserEdit.jsx +++ b/ui/src/user/UserEdit.jsx @@ -20,8 +20,10 @@ import { usePermissions, useRecordContext, } from 'react-admin' -import { Typography } from '@material-ui/core' -import { Title } from '../common' +import { Avatar, Typography } from '@material-ui/core' +import { Title, ImageUploadOverlay } from '../common' +import subsonic from '../subsonic' +import config from '../config' import DeleteUserButton from './DeleteUserButton' import { LibrarySelectionField } from './LibrarySelectionField.jsx' import { validateUserForm } from './userValidation' @@ -33,6 +35,17 @@ const useStyles = makeStyles({ }, }) +const useAvatarStyles = makeStyles({ + avatarParent: { + display: 'inline-flex', + position: 'relative', + width: '8rem', + height: '8rem', + marginBottom: '1em', + }, + avatar: { width: '100%', height: '100%' }, +}) + const UserTitle = ({ record }) => { const translate = useTranslate() const resourceName = translate('resources.user.name', { smart_count: 1 }) @@ -65,6 +78,46 @@ const NewPasswordInput = ({ formData, ...rest }) => { ) : null } +const AvatarField = () => { + const record = useRecordContext() + const { permissions } = usePermissions() + const isAdmin = permissions === 'admin' + const isMyself = localStorage.getItem('userId') === record?.id + // Mirrors server canEditAvatar: the flag gates everyone, admins included. + const canEdit = config.enableUserAvatarUpload && (isAdmin || isMyself) + const classes = useAvatarStyles() + + if (!record?.id) return null + + return ( +
+ + +
+ ) +} + const UserEdit = (props) => { const { permissions } = props const translate = useTranslate() @@ -118,6 +171,7 @@ const UserEdit = (props) => { save={save} validate={validateForm} > + {permissions === 'admin' && ( ({ notify: vi.fn(), redirect: vi.fn(), refresh: vi.fn(), + permissions: 'admin', + record: null, })) // Mock React-Admin completely with simpler implementations @@ -67,7 +70,8 @@ vi.mock('react-admin', () => ({ useNotify: () => hooks.notify, useRedirect: () => hooks.redirect, useRefresh: () => hooks.refresh, - usePermissions: () => ({ permissions: 'admin' }), + usePermissions: () => ({ permissions: hooks.permissions }), + useRecordContext: () => hooks.record, useTranslate: () => (key) => key, })) @@ -82,6 +86,14 @@ vi.mock('./DeleteUserButton', () => ({ vi.mock('../common', () => ({ Title: ({ subTitle }) =>
{subTitle}
, + ImageUploadOverlay: ({ canEdit, messages }) => + canEdit ? : null, +})) + +vi.mock('../subsonic', () => ({ + default: { + getAvatarUrl: (username) => `/rest/getAvatar?username=${username}`, + }, })) // Mock Material-UI @@ -91,6 +103,7 @@ vi.mock('@material-ui/core/styles', () => ({ vi.mock('@material-ui/core', () => ({ Typography: ({ children }) =>

{children}

, + Avatar: ({ src, alt }) => {alt}, })) describe('', () => { @@ -198,4 +211,39 @@ describe('', () => { expect(hooks.redirect).not.toHaveBeenCalled() }) }) + + describe('avatar upload', () => { + afterEach(() => { + localStorage.clear() + hooks.record = null + hooks.permissions = 'admin' + }) + + const renderUserEdit = ( + record, + { isMyself = false, role = 'user' } = {}, + ) => { + localStorage.setItem('userId', isMyself ? record.id : 'someone-else') + hooks.record = record + hooks.permissions = role + return render() + } + + it('shows the avatar upload control for the user themselves', () => { + config.enableUserAvatarUpload = true + renderUserEdit({ id: 'u1', userName: 'deluan' }, { isMyself: true }) + expect(screen.getByLabelText('message.uploadAvatar')).toBeInTheDocument() + }) + + it('hides the control when the feature is off and the viewer is not an admin', () => { + config.enableUserAvatarUpload = false + renderUserEdit( + { id: 'u1', userName: 'deluan' }, + { isMyself: true, role: 'regular' }, + ) + expect( + screen.queryByLabelText('message.uploadAvatar'), + ).not.toBeInTheDocument() + }) + }) }) From 7b225f530f42c18074eda681c4a29572b89c6ca5 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 20:36:28 -0400 Subject: [PATCH 16/21] feat(ui): show the uploaded avatar in the user menu Wire avatarTag through authProvider so identity.avatarTag is actually populated; Task 10 only added it to the backend login response. --- ui/src/authProvider.js | 6 ++++ ui/src/layout/UserMenu.jsx | 10 +++++-- ui/src/layout/UserMenu.test.jsx | 52 +++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 ui/src/layout/UserMenu.test.jsx diff --git a/ui/src/authProvider.js b/ui/src/authProvider.js index 18badbc9c..c5bc240ca 100644 --- a/ui/src/authProvider.js +++ b/ui/src/authProvider.js @@ -19,6 +19,10 @@ function storeAuthenticationInfo(authInfo) { localStorage.setItem('name', authInfo.name) localStorage.setItem('username', authInfo.username) authInfo.avatar && localStorage.setItem('avatar', authInfo.avatar) + // Not omitted like the fields above: a stale tag from a previous login must not survive avatar removal + authInfo.avatarTag + ? localStorage.setItem('avatarTag', authInfo.avatarTag) + : localStorage.removeItem('avatarTag') localStorage.setItem('role', authInfo.isAdmin ? 'admin' : 'regular') localStorage.setItem('subsonic-salt', authInfo.subsonicSalt) localStorage.setItem('subsonic-token', authInfo.subsonicToken) @@ -94,6 +98,7 @@ const authProvider = { id: localStorage.getItem('username'), fullName: localStorage.getItem('name'), avatar: localStorage.getItem('avatar'), + avatarTag: localStorage.getItem('avatarTag'), }) }, } @@ -104,6 +109,7 @@ const removeItems = () => { localStorage.removeItem('name') localStorage.removeItem('username') localStorage.removeItem('avatar') + localStorage.removeItem('avatarTag') localStorage.removeItem('role') localStorage.removeItem('subsonic-salt') localStorage.removeItem('subsonic-token') diff --git a/ui/src/layout/UserMenu.jsx b/ui/src/layout/UserMenu.jsx index e33185578..49c64928d 100644 --- a/ui/src/layout/UserMenu.jsx +++ b/ui/src/layout/UserMenu.jsx @@ -25,6 +25,7 @@ import config from '../config' import authProvider from '../authProvider' import { startEventStream } from '../eventStream' import { useDispatch } from 'react-redux' +import subsonic from '../subsonic' const useStyles = makeStyles((theme) => ({ user: {}, @@ -56,6 +57,11 @@ const UserMenu = (props) => { const { children, label, icon, logout } = props + // identity.id is the username (see authProvider.getIdentity); avatarTag is only set when an avatar was uploaded + const avatarUrl = identity?.avatarTag + ? `${subsonic.getAvatarUrl(identity.id)}&_=${identity.avatarTag}` + : identity?.avatar + useEffect(() => { if (config.devActivityPanel) { authProvider @@ -81,10 +87,10 @@ const UserMenu = (props) => { aria-haspopup={true} onClick={handleMenu} > - {loaded && identity.avatar ? ( + {loaded && avatarUrl ? ( ) : ( diff --git a/ui/src/layout/UserMenu.test.jsx b/ui/src/layout/UserMenu.test.jsx new file mode 100644 index 000000000..15c42e81b --- /dev/null +++ b/ui/src/layout/UserMenu.test.jsx @@ -0,0 +1,52 @@ +import React from 'react' +import { render, screen } from '@testing-library/react' +import { describe, it, expect, vi } from 'vitest' +import UserMenu from './UserMenu' + +vi.mock('../subsonic', () => ({ + default: { + getAvatarUrl: vi.fn((username) => `/app/rest/getAvatar.view?u=${username}`), + }, +})) + +vi.mock('react-redux', () => ({ + useDispatch: () => vi.fn(), +})) + +let mockIdentity +vi.mock('react-admin', () => ({ + useTranslate: () => (x) => x, + useGetIdentity: () => ({ loaded: true, identity: mockIdentity }), +})) + +const renderUserMenu = (identity) => { + mockIdentity = identity + render(Logout
} />) +} + +describe('', () => { + it('uses the uploaded avatar when the identity has an avatarTag', () => { + renderUserMenu({ id: 'u1', username: 'deluan', avatarTag: 'abc123' }) + expect(screen.getByRole('img')).toHaveAttribute( + 'src', + expect.stringContaining('getAvatar'), + ) + }) + + it('falls back to the generic icon with no avatar and no gravatar', () => { + renderUserMenu({ id: 'u1', username: 'deluan' }) + expect(screen.queryByRole('img')).not.toBeInTheDocument() + }) + + it('uses the gravatar url when there is no avatarTag', () => { + renderUserMenu({ + id: 'u1', + username: 'deluan', + avatar: 'https://gravatar/u1', + }) + expect(screen.getByRole('img')).toHaveAttribute( + 'src', + 'https://gravatar/u1', + ) + }) +}) From 755ae5106fac695a54a714f74224953407f00599 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 20:54:56 -0400 Subject: [PATCH 17/21] fix(server): sniff the avatar content type from the bytes The uploader re-encodes the image (PNG only when the source was PNG, WebP when EnableWebPEncoding is on, JPEG otherwise) but stores it under the caller's extension, so a .gif file can hold JPEG bytes. Deriving Content-Type from the extension made getAvatar and /UserImage announce a type they were not sending. --- server/imghttp/avatar.go | 15 ++++++++++++ server/imghttp/avatar_test.go | 46 ++++++++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/server/imghttp/avatar.go b/server/imghttp/avatar.go index e6201249e..11434a8b2 100644 --- a/server/imghttp/avatar.go +++ b/server/imghttp/avatar.go @@ -1,6 +1,8 @@ package imghttp import ( + "errors" + "io" "net/http" "os" @@ -27,6 +29,19 @@ func ServeUserAvatar(w http.ResponseWriter, r *http.Request, u *model.User) bool return false } + // The stored extension can disagree with the bytes: the uploader re-encodes while keeping the + // caller's extension, so the type is sniffed from the content instead. + head := make([]byte, 512) + n, err := io.ReadFull(f, head) + if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) { + log.Warn(r.Context(), "Could not read user avatar", "user", u.UserName, err) + return false + } + if _, err := f.Seek(0, io.SeekStart); err != nil { + log.Warn(r.Context(), "Could not rewind user avatar", "user", u.UserName, err) + return false + } + w.Header().Set("Content-Type", http.DetectContentType(head[:n])) w.Header().Set("ETag", `"`+u.AvatarTag()+`"`) w.Header().Set("Cache-Control", "private, no-cache") http.ServeContent(w, r, info.Name(), info.ModTime(), f) diff --git a/server/imghttp/avatar_test.go b/server/imghttp/avatar_test.go index 63a6c967a..81a5465d4 100644 --- a/server/imghttp/avatar_test.go +++ b/server/imghttp/avatar_test.go @@ -1,6 +1,11 @@ package imghttp_test import ( + "bytes" + "crypto/rand" + "image" + "image/jpeg" + "image/png" "net/http" "net/http/httptest" "os" @@ -45,6 +50,21 @@ var _ = Describe("ServeUserAvatar", func() { Expect(w.Header().Get("ETag")).To(Equal(`"` + usr.AvatarTag() + `"`)) }) + It("reports the content type of the bytes, not of the extension", func() { + usr.UploadedImage = writeAvatarBytes(usr, "gif", jpegBytes()) + Expect(imghttp.ServeUserAvatar(w, r, &usr)).To(BeTrue()) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Type")).To(Equal("image/jpeg")) + }) + + It("serves the whole file, not just what is left after sniffing", func() { + data := pngBytes() + Expect(len(data)).To(BeNumerically(">", 512)) + usr.UploadedImage = writeAvatarBytes(usr, "png", data) + Expect(imghttp.ServeUserAvatar(w, r, &usr)).To(BeTrue()) + Expect(w.Body.Bytes()).To(Equal(data)) + }) + It("answers 304 when the ETag matches", func() { usr.UploadedImage = writeAvatar(usr, "png") r.Header.Set("If-None-Match", `"`+usr.AvatarTag()+`"`) @@ -79,9 +99,33 @@ var _ = Describe("ServeUserAvatar", func() { }) func writeAvatar(u model.User, ext string) string { + return writeAvatarBytes(u, ext, pngBytes()) +} + +func writeAvatarBytes(u model.User, ext string, data []byte) string { name := u.ID + "_" + u.UserName + "." + ext path := filepath.Join(conf.Server.DataFolder.String(), "avatar", name) Expect(os.MkdirAll(filepath.Dir(path), 0755)).To(Succeed()) - Expect(os.WriteFile(path, []byte{0x89, 'P', 'N', 'G'}, 0600)).To(Succeed()) + Expect(os.WriteFile(path, data, 0600)).To(Succeed()) return name } + +func pngBytes() []byte { + var buf bytes.Buffer + Expect(png.Encode(&buf, noiseImage())).To(Succeed()) + return buf.Bytes() +} + +func jpegBytes() []byte { + var buf bytes.Buffer + Expect(jpeg.Encode(&buf, noiseImage(), nil)).To(Succeed()) + return buf.Bytes() +} + +// noiseImage compresses poorly on purpose, so the encoded file is larger than the 512-byte sniff window. +func noiseImage() image.Image { + img := image.NewRGBA(image.Rect(0, 0, 64, 64)) + _, err := rand.Read(img.Pix) + Expect(err).ToNot(HaveOccurred()) + return img +} From 41d7d1d48bca5b21756b6b3a925f427544f3f491 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 20:56:30 -0400 Subject: [PATCH 18/21] fix(subsonic): keep the placeholder for a missing username when Gravatar is off Parsing username before the EnableGravatar short-circuit turned a request with no username into Subsonic error 10, where it used to return a 200 placeholder without touching the DB. --- server/subsonic/media_retrieval.go | 5 +++++ server/subsonic/media_retrieval_test.go | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index ccc73c162..630598997 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -23,6 +23,11 @@ func (api *Router) GetAvatar(w http.ResponseWriter, r *http.Request) (*responses p := req.Params(r) username, err := p.String("username") if err != nil { + // Same reason as the unresolvable-user case below: the old handler short-circuited on + // EnableGravatar before it ever looked at the parameter. + if !conf.Server.EnableGravatar { + return api.getPlaceHolderAvatar(w, r) + } return nil, err } ctx := r.Context() diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index 0b961e834..d2edc2256 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -21,6 +21,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" + "github.com/navidrome/navidrome/utils/req" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -262,6 +263,23 @@ var _ = Describe("MediaRetrievalController", func() { Expect(err).ToNot(HaveOccurred()) Expect(w.Code).To(Equal(http.StatusOK)) }) + + It("serves the placeholder when the username is missing and Gravatar is off, as before", func() { + conf.Server.EnableGravatar = false + + _, err := router.GetAvatar(w, newGetRequest()) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(http.StatusOK)) + }) + + It("returns the same error as before for a missing username when Gravatar is on", func() { + conf.Server.EnableGravatar = true + + _, err := router.GetAvatar(w, newGetRequest()) + + Expect(err).To(MatchError(req.ErrMissingParam)) + }) }) Describe("GetLyrics", func() { From 114208e6d6c39a841391ad88829eb22eb6b6f2b4 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 20:59:59 -0400 Subject: [PATCH 19/21] fix(nativeapi): check avatar permission before reading the request body The gate ran inside saveFn, so a request destined for 403 still spooled up to 10MB of multipart body to temp first. Artist, playlist and radio already gate before parsing; the shared handler now takes the gate as a parameter. --- server/nativeapi/image_upload.go | 19 +++++++++---------- server/nativeapi/users.go | 21 ++++++++++++--------- server/nativeapi/users_test.go | 19 +++++++++++++++++++ 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/server/nativeapi/image_upload.go b/server/nativeapi/image_upload.go index 95d15b1e2..1c7d116ce 100644 --- a/server/nativeapi/image_upload.go +++ b/server/nativeapi/image_upload.go @@ -31,16 +31,16 @@ func checkImageUploadPermission(w http.ResponseWriter, r *http.Request) bool { } func handleImageUpload(saveFn func(ctx context.Context, reader io.Reader, ext string) error) http.HandlerFunc { - return handleImageUploadGated(true, saveFn) + return handleImageUploadGated(checkImageUploadPermission, saveFn) } -// handleImageUploadGated is handleImageUpload with an opt-out from the EnableArtworkUpload gate, -// so avatar uploads (gated separately by EnableUserAvatarUpload) can reuse this handler. -func handleImageUploadGated(checkArtworkFlag bool, saveFn func(ctx context.Context, reader io.Reader, ext string) error) http.HandlerFunc { +// handleImageUploadGated is handleImageUpload with a custom permission gate, checked before the +// request body is read, so avatar uploads (gated by EnableUserAvatarUpload) can reuse this handler. +func handleImageUploadGated(gate func(http.ResponseWriter, *http.Request) bool, saveFn func(ctx context.Context, reader io.Reader, ext string) error) http.HandlerFunc { maxImageSize := artwork.MaxImageUploadSize() return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - if checkArtworkFlag && !checkImageUploadPermission(w, r) { + if !gate(w, r) { return } r.Body = http.MaxBytesReader(w, r.Body, maxImageSize) @@ -103,15 +103,14 @@ func handleImageUploadGated(checkArtworkFlag bool, saveFn func(ctx context.Conte } func handleImageDelete(deleteFn func(ctx context.Context) error) http.HandlerFunc { - return handleImageDeleteGated(true, deleteFn) + return handleImageDeleteGated(checkImageUploadPermission, deleteFn) } -// handleImageDeleteGated is handleImageDelete with the same EnableArtworkUpload opt-out as -// handleImageUploadGated. -func handleImageDeleteGated(checkArtworkFlag bool, deleteFn func(ctx context.Context) error) http.HandlerFunc { +// handleImageDeleteGated is handleImageDelete with the same custom gate as handleImageUploadGated. +func handleImageDeleteGated(gate func(http.ResponseWriter, *http.Request) bool, deleteFn func(ctx context.Context) error) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - if checkArtworkFlag && !checkImageUploadPermission(w, r) { + if !gate(w, r) { return } if err := deleteFn(ctx); err != nil { diff --git a/server/nativeapi/users.go b/server/nativeapi/users.go index fe7e241d0..e38deaf8f 100644 --- a/server/nativeapi/users.go +++ b/server/nativeapi/users.go @@ -44,13 +44,19 @@ func canEditAvatar(ctx context.Context, targetID string) error { return nil } +// checkAvatarPermission gates avatar uploads by EnableUserAvatarUpload, never by EnableArtworkUpload. +func checkAvatarPermission(w http.ResponseWriter, r *http.Request) bool { + ctx := r.Context() + if err := canEditAvatar(ctx, chi.URLParamFromCtx(ctx, "id")); err != nil { + http.Error(w, "not authorized", http.StatusForbidden) + return false + } + return true +} + func (api *Router) uploadUserAvatar() http.HandlerFunc { - // false: avatars are gated by EnableUserAvatarUpload, never by EnableArtworkUpload. - return handleImageUploadGated(false, func(ctx context.Context, reader io.Reader, ext string) error { + return handleImageUploadGated(checkAvatarPermission, func(ctx context.Context, reader io.Reader, ext string) error { userID := chi.URLParamFromCtx(ctx, "id") - if err := canEditAvatar(ctx, userID); err != nil { - return err - } usr, err := api.ds.User(ctx).Get(userID) if err != nil { if errors.Is(err, model.ErrNotFound) { @@ -67,11 +73,8 @@ func (api *Router) uploadUserAvatar() http.HandlerFunc { } func (api *Router) deleteUserAvatar() http.HandlerFunc { - return handleImageDeleteGated(false, func(ctx context.Context) error { + return handleImageDeleteGated(checkAvatarPermission, func(ctx context.Context) error { userID := chi.URLParamFromCtx(ctx, "id") - if err := canEditAvatar(ctx, userID); err != nil { - return err - } usr, err := api.ds.User(ctx).Get(userID) if err != nil { if errors.Is(err, model.ErrNotFound) { diff --git a/server/nativeapi/users_test.go b/server/nativeapi/users_test.go index d0ddf06b2..37a995fc5 100644 --- a/server/nativeapi/users_test.go +++ b/server/nativeapi/users_test.go @@ -28,6 +28,16 @@ func testRouter(ds model.DataStore) http.Handler { return r } +type trackingReader struct { + r io.Reader + read bool +} + +func (t *trackingReader) Read(p []byte) (int, error) { + t.read = true + return t.r.Read(p) +} + var _ = Describe("User avatar routes", func() { var router http.Handler var ds *tests.MockDataStore @@ -89,6 +99,15 @@ var _ = Describe("User avatar routes", func() { Expect(w.Code).To(Equal(http.StatusForbidden)) }) + It("refuses a third party before reading the request body", func() { + body, ct := pngUpload() + tracked := &trackingReader{r: body} + w := httptest.NewRecorder() + router.ServeHTTP(w, newRequest("POST", "/user/u1/image", tracked, ct, otherUser)) + Expect(w.Code).To(Equal(http.StatusForbidden)) + Expect(tracked.read).To(BeFalse()) + }) + It("refuses everyone, admins included, when the flag is off", func() { conf.Server.EnableUserAvatarUpload = false body, ct := pngUpload() From 56ec411cae34fab734e0db1f1f60f13803f40af3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 21:00:32 -0400 Subject: [PATCH 20/21] fix(jellyfin): return 400 for an avatar body that is not a decodable image The native endpoint answers 400 for the same input; only SetAvatar's resize was catching it here, which surfaced as 500. --- server/jellyfin/user_image.go | 7 +++++++ server/jellyfin/user_image_test.go | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/server/jellyfin/user_image.go b/server/jellyfin/user_image.go index 317082024..e4cbc43ba 100644 --- a/server/jellyfin/user_image.go +++ b/server/jellyfin/user_image.go @@ -2,6 +2,7 @@ package jellyfin import ( "bytes" + "image" "io" "net/http" "strings" @@ -140,6 +141,12 @@ func (api *Router) postUserImage(w http.ResponseWriter, r *http.Request) { return } + if _, _, err := image.DecodeConfig(bytes.NewReader(imgBytes)); err != nil { + log.Warn(ctx, "Jellyfin API: avatar upload rejected: not a valid image", "user", usr.UserName, err) + http.Error(w, "invalid image file", http.StatusBadRequest) + return + } + filename, err := api.imgUpload.SetAvatar(ctx, usr.ID, usr.UserName, usr.UploadedImagePath(), bytes.NewReader(imgBytes), ext) if err != nil { log.Error(ctx, "Jellyfin API: could not save avatar", "user", usr.UserName, err) diff --git a/server/jellyfin/user_image_test.go b/server/jellyfin/user_image_test.go index 816551d49..b2b8db5aa 100644 --- a/server/jellyfin/user_image_test.go +++ b/server/jellyfin/user_image_test.go @@ -202,6 +202,18 @@ var _ = Describe("POST /userimage and DELETE /userimage", func() { Expect(usr.UploadedImage).To(BeEmpty()) }) + It("rejects a body that is not a decodable image with 400", func() { + body := base64.StdEncoding.EncodeToString([]byte("this is not an image")) + r := authenticatedRequestWithBody("POST", "/userimage", strings.NewReader(body)) + r.Header.Set("Content-Type", "image/png") + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusBadRequest)) + + usr, _ := ds.User(context.Background()).Get("u1") + Expect(usr.UploadedImage).To(BeEmpty()) + }) + It("rejects an unknown content type", func() { r := authenticatedRequestWithBody("POST", "/userimage", strings.NewReader("x")) r.Header.Set("Content-Type", "text/plain") From ae8b736d07f37d9c81a8476d38eba44f606f3504 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 21:03:00 -0400 Subject: [PATCH 21/21] fix(ui): keep the user menu avatar in sync after upload or removal avatarTag was a login-time snapshot, so uploading an avatar left the menu on the old icon and removing one left it pointing at getAvatar, which now falls through to the Navidrome logo placeholder. The edit page now updates the stored tag when the record is the logged-in user, and the menu re-reads the identity on refresh. --- ui/src/common/ImageUploadOverlay.jsx | 4 +- ui/src/layout/UserMenu.jsx | 19 +++++++--- ui/src/layout/UserMenu.test.jsx | 54 +++++++++++++++++++++----- ui/src/setupTests.js | 3 ++ ui/src/user/UserEdit.jsx | 14 +++++++ ui/src/user/UserEdit.test.jsx | 57 ++++++++++++++++++++++++++-- 6 files changed, 132 insertions(+), 19 deletions(-) diff --git a/ui/src/common/ImageUploadOverlay.jsx b/ui/src/common/ImageUploadOverlay.jsx index 119426c73..03144ddc0 100644 --- a/ui/src/common/ImageUploadOverlay.jsx +++ b/ui/src/common/ImageUploadOverlay.jsx @@ -88,7 +88,7 @@ export const ImageUploadOverlay = ({ body: formData, }) notify(msg.uploaded, 'success') - if (onImageChange) onImageChange() + if (onImageChange) onImageChange(true) refresh() } catch (err) { notify(msg.uploadError, 'warning') @@ -117,7 +117,7 @@ export const ImageUploadOverlay = ({ method: 'DELETE', }) notify(msg.removed, 'success') - if (onImageChange) onImageChange() + if (onImageChange) onImageChange(false) refresh() } catch (err) { notify(msg.removeError, 'warning') diff --git a/ui/src/layout/UserMenu.jsx b/ui/src/layout/UserMenu.jsx index 49c64928d..1b465ed35 100644 --- a/ui/src/layout/UserMenu.jsx +++ b/ui/src/layout/UserMenu.jsx @@ -7,7 +7,7 @@ import { useState, } from 'react' import PropTypes from 'prop-types' -import { useTranslate, useGetIdentity } from 'react-admin' +import { useTranslate, useGetIdentity, useVersion } from 'react-admin' import { Tooltip, IconButton, @@ -52,15 +52,24 @@ const UserMenu = (props) => { const [anchorEl, setAnchorEl] = useState(null) const translate = useTranslate() const { loaded, identity } = useGetIdentity() + const version = useVersion() + const [avatarTag, setAvatarTag] = useState(null) const classes = useStyles(props) const dispatch = useDispatch() const { children, label, icon, logout } = props - // identity.id is the username (see authProvider.getIdentity); avatarTag is only set when an avatar was uploaded - const avatarUrl = identity?.avatarTag - ? `${subsonic.getAvatarUrl(identity.id)}&_=${identity.avatarTag}` - : identity?.avatar + // The tag is only set when an avatar was uploaded, and it changes mid-session on upload or + // removal, so the identity is re-read on every refresh instead of trusting the login snapshot. + useEffect(() => { + authProvider.getIdentity().then((user) => setAvatarTag(user?.avatarTag)) + }, [version]) + + // identity.id is the username (see authProvider.getIdentity) + const avatarUrl = + avatarTag && identity?.id + ? `${subsonic.getAvatarUrl(identity.id)}&_=${avatarTag}` + : identity?.avatar useEffect(() => { if (config.devActivityPanel) { diff --git a/ui/src/layout/UserMenu.test.jsx b/ui/src/layout/UserMenu.test.jsx index 15c42e81b..4a21b94ca 100644 --- a/ui/src/layout/UserMenu.test.jsx +++ b/ui/src/layout/UserMenu.test.jsx @@ -1,6 +1,6 @@ import React from 'react' import { render, screen } from '@testing-library/react' -import { describe, it, expect, vi } from 'vitest' +import { describe, it, expect, vi, beforeEach } from 'vitest' import UserMenu from './UserMenu' vi.mock('../subsonic', () => ({ @@ -14,34 +14,42 @@ vi.mock('react-redux', () => ({ })) let mockIdentity +let mockVersion vi.mock('react-admin', () => ({ useTranslate: () => (x) => x, useGetIdentity: () => ({ loaded: true, identity: mockIdentity }), + useVersion: () => mockVersion, })) const renderUserMenu = (identity) => { mockIdentity = identity - render(Logout} />) + return render(Logout} />) } describe('', () => { - it('uses the uploaded avatar when the identity has an avatarTag', () => { - renderUserMenu({ id: 'u1', username: 'deluan', avatarTag: 'abc123' }) - expect(screen.getByRole('img')).toHaveAttribute( + beforeEach(() => { + localStorage.clear() + mockVersion = 1 + }) + + it('uses the uploaded avatar when there is an avatar tag', async () => { + localStorage.setItem('avatarTag', 'abc123') + renderUserMenu({ id: 'deluan', fullName: 'Deluan' }) + expect(await screen.findByRole('img')).toHaveAttribute( 'src', expect.stringContaining('getAvatar'), ) }) it('falls back to the generic icon with no avatar and no gravatar', () => { - renderUserMenu({ id: 'u1', username: 'deluan' }) + renderUserMenu({ id: 'deluan', fullName: 'Deluan' }) expect(screen.queryByRole('img')).not.toBeInTheDocument() }) - it('uses the gravatar url when there is no avatarTag', () => { + it('uses the gravatar url when there is no avatar tag', () => { renderUserMenu({ - id: 'u1', - username: 'deluan', + id: 'deluan', + fullName: 'Deluan', avatar: 'https://gravatar/u1', }) expect(screen.getByRole('img')).toHaveAttribute( @@ -49,4 +57,32 @@ describe('', () => { 'https://gravatar/u1', ) }) + + it('picks up an avatar uploaded during the session', async () => { + const { rerender } = renderUserMenu({ id: 'deluan', fullName: 'Deluan' }) + expect(screen.queryByRole('img')).not.toBeInTheDocument() + + localStorage.setItem('avatarTag', 'newtag') + mockVersion = 2 + rerender(Logout} />) + + expect(await screen.findByRole('img')).toHaveAttribute( + 'src', + expect.stringContaining('newtag'), + ) + }) + + it('goes back to the generic icon when the avatar is removed during the session', async () => { + localStorage.setItem('avatarTag', 'abc123') + const { rerender } = renderUserMenu({ id: 'deluan', fullName: 'Deluan' }) + expect(await screen.findByRole('img')).toBeInTheDocument() + + localStorage.removeItem('avatarTag') + mockVersion = 2 + rerender(Logout} />) + + await vi.waitFor(() => + expect(screen.queryByRole('img')).not.toBeInTheDocument(), + ) + }) }) diff --git a/ui/src/setupTests.js b/ui/src/setupTests.js index ddb999f3c..7cb46e09c 100644 --- a/ui/src/setupTests.js +++ b/ui/src/setupTests.js @@ -14,6 +14,9 @@ const localStorageMock = (function () { setItem: function (key, value) { store[key] = value.toString() }, + removeItem: function (key) { + delete store[key] + }, clear: function () { store = {} }, diff --git a/ui/src/user/UserEdit.jsx b/ui/src/user/UserEdit.jsx index b6523bc2b..567709832 100644 --- a/ui/src/user/UserEdit.jsx +++ b/ui/src/user/UserEdit.jsx @@ -87,6 +87,19 @@ const AvatarField = () => { const canEdit = config.enableUserAvatarUpload && (isAdmin || isMyself) const classes = useAvatarStyles() + const handleImageChange = useCallback( + (hasImage) => { + if (!isMyself) return + // Only a cache-buster: the server sends the authoritative ETag, so any changing value works + if (hasImage) { + localStorage.setItem('avatarTag', Date.now().toString()) + } else { + localStorage.removeItem('avatarTag') + } + }, + [isMyself], + ) + if (!record?.id) return null return ( @@ -104,6 +117,7 @@ const AvatarField = () => { entityType="user" entityId={record.id} hasUploadedImage={!!record.uploadedImage} + onImageChange={handleImageChange} canEdit={canEdit} messages={{ uploaded: 'message.avatarUploaded', diff --git a/ui/src/user/UserEdit.test.jsx b/ui/src/user/UserEdit.test.jsx index 0f5dd5a1c..52e955ed7 100644 --- a/ui/src/user/UserEdit.test.jsx +++ b/ui/src/user/UserEdit.test.jsx @@ -1,5 +1,5 @@ import * as React from 'react' -import { render, screen } from '@testing-library/react' +import { render, screen, fireEvent } from '@testing-library/react' import UserEdit from './UserEdit' import config from '../config' import { describe, it, expect, vi, afterEach } from 'vitest' @@ -86,8 +86,23 @@ vi.mock('./DeleteUserButton', () => ({ vi.mock('../common', () => ({ Title: ({ subTitle }) =>
{subTitle}
, - ImageUploadOverlay: ({ canEdit, messages }) => - canEdit ? : null, + ImageUploadOverlay: ({ canEdit, messages, onImageChange }) => + canEdit ? ( + <> + + + + ) : null, })) vi.mock('../subsonic', () => ({ @@ -235,6 +250,42 @@ describe('', () => { expect(screen.getByLabelText('message.uploadAvatar')).toBeInTheDocument() }) + it('stores a new avatar tag when the user uploads their own avatar', () => { + config.enableUserAvatarUpload = true + renderUserEdit({ id: 'u1', userName: 'deluan' }, { isMyself: true }) + + fireEvent.click(screen.getByLabelText('message.uploadAvatar')) + + expect(localStorage.getItem('avatarTag')).toBeTruthy() + }) + + it('removes the avatar tag when the user removes their own avatar', () => { + config.enableUserAvatarUpload = true + localStorage.setItem('avatarTag', 'oldtag') + renderUserEdit( + { id: 'u1', userName: 'deluan', uploadedImage: 'u1_deluan.png' }, + { isMyself: true }, + ) + + fireEvent.click(screen.getByLabelText('message.removeAvatar')) + + expect(localStorage.getItem('avatarTag')).toBeNull() + }) + + it('does not touch the admin own tag when editing another user', () => { + config.enableUserAvatarUpload = true + renderUserEdit( + { id: 'u1', userName: 'deluan', uploadedImage: 'u1_deluan.png' }, + { isMyself: false, role: 'admin' }, + ) + localStorage.setItem('avatarTag', 'mytag') + + fireEvent.click(screen.getByLabelText('message.uploadAvatar')) + fireEvent.click(screen.getByLabelText('message.removeAvatar')) + + expect(localStorage.getItem('avatarTag')).toEqual('mytag') + }) + it('hides the control when the feature is off and the viewer is not an admin', () => { config.enableUserAvatarUpload = false renderUserEdit(