From 09e78962b21358c90b424dbbbcf53189e75b26d4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 23 Oct 2026 13:58:09 -0400 Subject: [PATCH] fix(subsonic): getUser answers an admin's lookup with the requested user (#6292) An admin asking for another username got their own name and roles back. The handler now looks the user up by exact username and answers with that user's payload; an unknown username is ErrDataNotFound. A non-admin asking about someone else is still ErrNotAuthorized. Co-Authored-By: Claude Opus 5.5 --- internal/subsonic/subsonic.go | 4 ++- internal/subsonic/user.go | 41 +++++++++++++++++++-------- internal/subsonic/user_test.go | 51 ++++++++++++++++++++++++++++++++-- 3 files changed, 82 insertions(+), 14 deletions(-) diff --git a/internal/subsonic/subsonic.go b/internal/subsonic/subsonic.go index 3e5e6d95..9b1cd8bb 100644 --- a/internal/subsonic/subsonic.go +++ b/internal/subsonic/subsonic.go @@ -13,6 +13,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/auth" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/playevents" ) @@ -26,6 +27,7 @@ import ( func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, cfg Config, events *playevents.Writer, hops func() int) { b := &browseHandlers{pool: pool} m := newMediaHandlers(pool, events, logger) + u := &userHandlers{lookup: dbq.New(pool).GetUserByUsername} r.Route("/rest", func(sub chi.Router) { sub.Use(Middleware(pool, cfg, auth.NewLoginGuard(), hops)) register(sub, "/ping", handlePing) @@ -46,7 +48,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, cfg Config, ev register(sub, "/unstar", m.handleUnstar) register(sub, "/getStarred", m.handleGetStarred) register(sub, "/getStarred2", m.handleGetStarred2) - register(sub, "/getUser", handleGetUser) + register(sub, "/getUser", u.getUser) // Subsonic clients expect every /rest/* miss to come back as a // Subsonic-shaped error envelope, not chi's plain-text 404. Without diff --git a/internal/subsonic/user.go b/internal/subsonic/user.go index 6195333e..68dc4104 100644 --- a/internal/subsonic/user.go +++ b/internal/subsonic/user.go @@ -1,9 +1,13 @@ package subsonic import ( + "context" "encoding/xml" + "errors" "net/http" + "github.com/jackc/pgx/v5" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) @@ -38,26 +42,41 @@ type User struct { VideoConversionRole bool `json:"videoConversionRole" xml:"videoConversionRole,attr"` } -// handleGetUser returns the authenticated user when no username is provided -// or when the requested username matches; admins may look up any user. Per -// spec a non-admin asking about another user gets ErrNotAuthorized. -func handleGetUser(w http.ResponseWriter, r *http.Request) { +// userHandlers serves getUser. lookup finds a user by exact username; it is +// a field so tests can answer without a database. +type userHandlers struct { + lookup func(ctx context.Context, username string) (dbq.User, error) +} + +// getUser returns the authenticated user when no username is provided or +// when the requested username matches; admins may look up any user, and an +// unknown username is ErrDataNotFound. Per spec a non-admin asking about +// another user gets ErrNotAuthorized. +func (h *userHandlers) getUser(w http.ResponseWriter, r *http.Request) { caller, ok := UserFromContext(r.Context()) if !ok { WriteFail(w, r, ErrNotAuthorized, "Not authenticated") return } requested := r.URL.Query().Get("username") - if requested != "" && requested != caller.Username && !caller.IsAdmin { + if requested == "" || requested == caller.Username { + Write(w, r, UserResponse{Envelope: NewEnvelope("ok"), User: userPayload(caller)}) + return + } + if !caller.IsAdmin { WriteFail(w, r, ErrNotAuthorized, "Not authorized to view other users") return } - // No per-user lookup: an admin asking for another username is answered - // with the caller's own roles. - Write(w, r, UserResponse{ - Envelope: NewEnvelope("ok"), - User: userPayload(caller), - }) + target, err := h.lookup(r.Context(), requested) + if errors.Is(err, pgx.ErrNoRows) { + WriteFail(w, r, ErrDataNotFound, "User not found") + return + } + if err != nil { + WriteFail(w, r, ErrGeneric, "Failed to look up user") + return + } + Write(w, r, UserResponse{Envelope: NewEnvelope("ok"), User: userPayload(target)}) } func userPayload(u dbq.User) User { diff --git a/internal/subsonic/user_test.go b/internal/subsonic/user_test.go index 3bb3e8c5..c72e3c46 100644 --- a/internal/subsonic/user_test.go +++ b/internal/subsonic/user_test.go @@ -8,6 +8,8 @@ import ( "strings" "testing" + "github.com/jackc/pgx/v5" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) @@ -51,6 +53,41 @@ func TestHandleGetUser_NonAdminCannotLookupOthers(t *testing.T) { } } +func TestHandleGetUser_AdminLooksUpAnotherUser(t *testing.T) { + admin := dbq.User{Username: "admin", IsAdmin: true} + bob := dbq.User{Username: "bob", IsAdmin: false} + _, body := callGetUser(t, admin, "bob", bob) + if body.Resp.Status != "ok" { + t.Fatalf("status = %q, want ok (err=%+v)", body.Resp.Status, body.Resp.Error) + } + if body.Resp.User.Username != "bob" { + t.Errorf("username = %q, want bob: the answer must describe the user asked about, not the caller", body.Resp.User.Username) + } + if body.Resp.User.AdminRole || body.Resp.User.SettingsRole { + t.Errorf("bob is not an admin, but the answer carries admin/settings roles %v/%v", + body.Resp.User.AdminRole, body.Resp.User.SettingsRole) + } +} + +func TestHandleGetUser_AdminLooksUpUnknownUser(t *testing.T) { + admin := dbq.User{Username: "admin", IsAdmin: true} + _, body := callGetUser(t, admin, "nobody") + if body.Resp.Status != "failed" || body.Resp.Error == nil || + body.Resp.Error.Code != ErrDataNotFound { + t.Errorf("expected ErrDataNotFound, got status=%q err=%+v", + body.Resp.Status, body.Resp.Error) + } +} + +func TestHandleGetUser_OwnUsernameNeedsNoLookup(t *testing.T) { + // No known users: a lookup would fail, so success proves none was made. + user := dbq.User{Username: "alice", IsAdmin: false} + _, body := callGetUser(t, user, "alice") + if body.Resp.Status != "ok" || body.Resp.User.Username != "alice" { + t.Errorf("status=%q username=%q, want ok/alice", body.Resp.Status, body.Resp.User.Username) + } +} + type getUserPayload struct { Resp struct { Status string `json:"status"` @@ -59,8 +96,18 @@ type getUserPayload struct { } `json:"subsonic-response"` } -func callGetUser(t *testing.T, user dbq.User, requestedUsername string) (*httptest.ResponseRecorder, getUserPayload) { +// callGetUser runs getUser as caller, with the users in known as the only +// accounts a lookup can find. +func callGetUser(t *testing.T, user dbq.User, requestedUsername string, known ...dbq.User) (*httptest.ResponseRecorder, getUserPayload) { t.Helper() + h := &userHandlers{lookup: func(_ context.Context, username string) (dbq.User, error) { + for _, u := range known { + if u.Username == username { + return u, nil + } + } + return dbq.User{}, pgx.ErrNoRows + }} url := "/rest/getUser.view?f=json" if requestedUsername != "" { url += "&username=" + requestedUsername @@ -68,7 +115,7 @@ func callGetUser(t *testing.T, user dbq.User, requestedUsername string) (*httpte req := httptest.NewRequest("GET", url, nil) req = req.WithContext(context.WithValue(req.Context(), userCtxKey, user)) w := httptest.NewRecorder() - handleGetUser(w, req) + h.getUser(w, req) var body getUserPayload if err := json.NewDecoder(strings.NewReader(w.Body.String())).Decode(&body); err != nil {