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/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/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() +} 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/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..452b60b0a 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 { @@ -52,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/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())) + }) + }) }) 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..c6702db79 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,47 @@ 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() { + // 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()) + Expect(usr.UploadedImage).To(Equal("u2_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/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() { diff --git a/server/imghttp/avatar.go b/server/imghttp/avatar.go new file mode 100644 index 000000000..11434a8b2 --- /dev/null +++ b/server/imghttp/avatar.go @@ -0,0 +1,49 @@ +package imghttp + +import ( + "errors" + "io" + "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 + } + + // 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) + return true +} diff --git a/server/imghttp/avatar_test.go b/server/imghttp/avatar_test.go new file mode 100644 index 000000000..81a5465d4 --- /dev/null +++ b/server/imghttp/avatar_test.go @@ -0,0 +1,131 @@ +package imghttp_test + +import ( + "bytes" + "crypto/rand" + "image" + "image/jpeg" + "image/png" + "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("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()+`"`) + 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 { + 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, 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 +} diff --git a/server/jellyfin/api.go b/server/jellyfin/api.go index 6484a3bb4..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, @@ -86,6 +87,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, @@ -109,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/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/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 new file mode 100644 index 000000000..e4cbc43ba --- /dev/null +++ b/server/jellyfin/user_image.go @@ -0,0 +1,178 @@ +package jellyfin + +import ( + "bytes" + "image" + "io" + "net/http" + "strings" + + "github.com/dustin/go-humanize" + "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" +) + +// 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 +} + +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 + } + // 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 + } + + 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) + 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 new file mode 100644 index 000000000..b2b8db5aa --- /dev/null +++ b/server/jellyfin/user_image_test.go @@ -0,0 +1,348 @@ +package jellyfin + +import ( + "bytes" + "context" + "encoding/base64" + "encoding/json" + "image" + "image/png" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "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" + "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, noAvatar *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()) + + // 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()) + + 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 { + 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(noAvatar) + Expect(err).ToNot(HaveOccurred()) + w := httptest.NewRecorder() + 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)) + }) + + 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("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 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 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") + 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()) + }) + + 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) diff --git a/server/nativeapi/image_upload.go b/server/nativeapi/image_upload.go index c4ff62e42..1c7d116ce 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(checkImageUploadPermission, saveFn) +} + +// 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 !checkImageUploadPermission(w, r) { + if !gate(w, r) { return } r.Body = http.MaxBytesReader(w, r.Body, maxImageSize) @@ -97,9 +103,14 @@ func handleImageUpload(saveFn func(ctx context.Context, reader io.Reader, ext st } func handleImageDelete(deleteFn func(ctx context.Context) error) http.HandlerFunc { + return handleImageDeleteGated(checkImageUploadPermission, deleteFn) +} + +// 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 !checkImageUploadPermission(w, r) { + if !gate(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..e38deaf8f --- /dev/null +++ b/server/nativeapi/users.go @@ -0,0 +1,90 @@ +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 +} + +// 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 { + return handleImageUploadGated(checkAvatarPermission, func(ctx context.Context, reader io.Reader, ext string) error { + userID := chi.URLParamFromCtx(ctx, "id") + 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(checkAvatarPermission, func(ctx context.Context) error { + userID := chi.URLParamFromCtx(ctx, "id") + 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..37a995fc5 --- /dev/null +++ b/server/nativeapi/users_test.go @@ -0,0 +1,128 @@ +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 +} + +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 + + 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 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() + 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()) + }) +}) 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, diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index 8a5152a9d..630598997 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -20,19 +20,33 @@ 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 { + // 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() 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. + 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..d2edc2256 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" @@ -19,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" ) @@ -29,16 +32,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 +186,102 @@ 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 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)) + }) + + 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() { It("should return data for given artist & title", func() { r := newGetRequest("artist=Rick+Astley", "title=Never+Gonna+Give+You+Up") @@ -255,6 +357,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 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 { 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/common/ImageUploadOverlay.jsx b/ui/src/common/ImageUploadOverlay.jsx index a370e40fe..03144ddc0 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') - if (onImageChange) onImageChange() + notify(msg.uploaded, 'success') + if (onImageChange) onImageChange(true) 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') - if (onImageChange) onImageChange() + notify(msg.removed, 'success') + if (onImageChange) onImageChange(false) 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() + }) +}) 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/layout/UserMenu.jsx b/ui/src/layout/UserMenu.jsx index e33185578..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, @@ -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: {}, @@ -51,11 +52,25 @@ 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 + // 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) { authProvider @@ -81,10 +96,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..4a21b94ca --- /dev/null +++ b/ui/src/layout/UserMenu.test.jsx @@ -0,0 +1,88 @@ +import React from 'react' +import { render, screen } from '@testing-library/react' +import { describe, it, expect, vi, beforeEach } 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 +let mockVersion +vi.mock('react-admin', () => ({ + useTranslate: () => (x) => x, + useGetIdentity: () => ({ loaded: true, identity: mockIdentity }), + useVersion: () => mockVersion, +})) + +const renderUserMenu = (identity) => { + mockIdentity = identity + return render(Logout
} />) +} + +describe('', () => { + 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: 'deluan', fullName: 'Deluan' }) + expect(screen.queryByRole('img')).not.toBeInTheDocument() + }) + + it('uses the gravatar url when there is no avatar tag', () => { + renderUserMenu({ + id: 'deluan', + fullName: 'Deluan', + avatar: 'https://gravatar/u1', + }) + expect(screen.getByRole('img')).toHaveAttribute( + 'src', + '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 c5d9c75a4..567709832 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,60 @@ 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() + + 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 ( +
+ + +
+ ) +} + const UserEdit = (props) => { const { permissions } = props const translate = useTranslate() @@ -118,6 +185,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,29 @@ vi.mock('./DeleteUserButton', () => ({ vi.mock('../common', () => ({ Title: ({ subTitle }) =>
{subTitle}
, + ImageUploadOverlay: ({ canEdit, messages, onImageChange }) => + canEdit ? ( + <> + + + + ) : null, +})) + +vi.mock('../subsonic', () => ({ + default: { + getAvatarUrl: (username) => `/rest/getAvatar?username=${username}`, + }, })) // Mock Material-UI @@ -91,6 +118,7 @@ vi.mock('@material-ui/core/styles', () => ({ vi.mock('@material-ui/core', () => ({ Typography: ({ children }) =>

{children}

, + Avatar: ({ src, alt }) => {alt}, })) describe('', () => { @@ -198,4 +226,75 @@ 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('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( + { id: 'u1', userName: 'deluan' }, + { isMyself: true, role: 'regular' }, + ) + expect( + screen.queryByLabelText('message.uploadAvatar'), + ).not.toBeInTheDocument() + }) + }) })