fix(profiles): enforce per-account profile name uniqueness (#342)

* fix(profiles): enforce per-account profile name uniqueness Profile create and rename accepted any name, so one account could hold unlimited profiles all called "Laura" (every client allowed it too). Reject a create or rename whose trimmed, case-insensitive name matches another profile on the same account with 409 name_conflict. Scoping is per account by construction — the check runs against a single user's profile store, so different accounts can still each have a "Laura". Renames exclude the profile being updated, so re-saving a profile under its own name (e.g. avatar-only edits that resubmit the name) still works. Also reject whitespace-only names on create and rename; a name of " " previously passed the blank check. Additive-only per the v1 API rules: new 409 error code on existing endpoints, following the profile_limit_reached pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(profiles): store the trimmed profile name The conflict check compared trimmed names but create/rename persisted the raw input, so " Laura " could land with stray whitespace and render inconsistently. Normalize to the trimmed form before storage on both paths. Addresses the CodeRabbit review finding on PR #342. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(profiles): cover whitespace-only rejection and rename trimming Also document the check-then-write race in profileNameConflicts: the userstore backends carry no unique index on name, so concurrent creates can still race past the guard, same as profile_limit_reached. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: rxwatcher <rxwatcher@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Quick <31828688+Quick104@users.noreply.github.com>
2026-07-08 11:20:27 -04:00
parent 021e54a03c
commit 0deff23985
2 changed files with 281 additions and 3 deletions
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"log/slog"
 	"net/http"
+	"strings"
 	"time"

 	"github.com/go-chi/chi/v5"
@@ -213,6 +214,32 @@ func writeProfileManagementPermissionError(w http.ResponseWriter, err error) {
 	writeError(w, http.StatusInternalServerError, "internal_error", "Failed to check profile permissions")
 }

+// profileNameConflicts reports whether a profile other than excludeID already
+// uses name within this account's store, comparing the trimmed forms
+// case-insensitively so "Laura" and " laura " count as the same household
+// member. Scoping is per account by construction: callers pass the profile
+// list of a single user's store, so another account's profiles can never
+// conflict.
+//
+// This is a check-then-write guard with no store-level uniqueness constraint
+// (the userstore's dual Postgres/SQLite backends carry no unique index on
+// name), so two concurrent requests can both pass and insert duplicates —
+// the same window the profile_limit_reached check accepts. Good enough for
+// interactive profile management; a functional unique index is the fix if
+// that ever stops being true.
+func profileNameConflicts(profiles []userstore.Profile, name, excludeID string) bool {
+	trimmed := strings.TrimSpace(name)
+	for _, p := range profiles {
+		if p.ID == excludeID {
+			continue
+		}
+		if strings.EqualFold(strings.TrimSpace(p.Name), trimmed) {
+			return true
+		}
+	}
+	return false
+}
+
 // isAllowedSelfServiceProfileUpdate reports whether a non-admin update request
 // only touches fields the user is allowed to change on their own profiles.
 // Admin-only fields (access policy: library restrictions, content rating,
@@ -272,7 +299,7 @@ func (h *ProfileHandler) HandleCreateProfile(w http.ResponseWriter, r *http.Requ
 		return
 	}

-	if req.Name == "" {
+	if strings.TrimSpace(req.Name) == "" {
 		writeError(w, http.StatusBadRequest, "bad_request", "Profile name is required")
 		return
 	}
@@ -346,6 +373,16 @@ func (h *ProfileHandler) HandleCreateProfile(w http.ResponseWriter, r *http.Requ
 		}
 	}

+	if profileNameConflicts(existingProfiles, req.Name, "") {
+		writeError(
+			w,
+			http.StatusConflict,
+			"name_conflict",
+			"A profile with this name already exists",
+		)
+		return
+	}
+
 	showForcedSubtitles := true
 	if req.ShowForcedSubtitles != nil {
 		showForcedSubtitles = *req.ShowForcedSubtitles
@@ -353,8 +390,10 @@ func (h *ProfileHandler) HandleCreateProfile(w http.ResponseWriter, r *http.Requ

 	profileID := uuid.New().String()
 	profile := userstore.Profile{
-		ID:                         profileID,
-		Name:                       req.Name,
+		ID: profileID,
+		// Store the trimmed form the conflict check compared, so " Laura "
+		// doesn't persist with stray whitespace.
+		Name:                       strings.TrimSpace(req.Name),
 		Avatar:                     avatarRef,
 		IsChild:                    req.IsChild,
 		MaxContentRating:           req.MaxContentRating,
@@ -502,6 +541,31 @@ func (h *ProfileHandler) HandleUpdateProfile(w http.ResponseWriter, r *http.Requ
 		}
 	}

+	if req.Name != nil {
+		// Normalize to the trimmed form up front: the conflict check compares
+		// it and the store persists it, so " Laura " never lands verbatim.
+		trimmedName := strings.TrimSpace(*req.Name)
+		if trimmedName == "" {
+			writeError(w, http.StatusBadRequest, "bad_request", "Profile name is required")
+			return
+		}
+		req.Name = &trimmedName
+		existingProfiles, err := store.ListProfiles(r.Context())
+		if err != nil {
+			writeError(w, http.StatusInternalServerError, "internal_error", "Failed to list profiles")
+			return
+		}
+		if profileNameConflicts(existingProfiles, *req.Name, profileID) {
+			writeError(
+				w,
+				http.StatusConflict,
+				"name_conflict",
+				"A profile with this name already exists",
+			)
+			return
+		}
+	}
+
 	input := userstore.UpdateProfileInput{
 		Name:                       req.Name,
 		Avatar:                     avatarRef,
@@ -165,6 +165,103 @@ func TestHandleCreateProfile_AllowsNonAdminUpToCap(t *testing.T) {
 	}
 }

+func TestHandleCreateProfile_RejectsDuplicateName(t *testing.T) {
+	store := newProfileTestStore(t)
+	handler := NewProfileHandler(testUserStoreProvider{store: store})
+
+	// The seeded store already holds "Main"; a trimmed, case-insensitive
+	// match must be rejected within the same account.
+	req := newAuthorizedProfileRequestWithRole(
+		http.MethodPost,
+		"/profiles",
+		`{"name":" main "}`,
+		"user",
+		"profile-1",
+	)
+	rr := httptest.NewRecorder()
+
+	handler.HandleCreateProfile(rr, req)
+
+	if rr.Code != http.StatusConflict {
+		t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String())
+	}
+
+	var resp errorResponse
+	if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil {
+		t.Fatalf("decode response: %v", err)
+	}
+	if resp.Error != "name_conflict" {
+		t.Fatalf("error = %q, want %q", resp.Error, "name_conflict")
+	}
+
+	profiles, err := store.ListProfiles(context.Background())
+	if err != nil {
+		t.Fatalf("list profiles: %v", err)
+	}
+	if len(profiles) != 1 {
+		t.Fatalf("profile count = %d, want 1", len(profiles))
+	}
+}
+
+func TestHandleCreateProfile_RejectsWhitespaceOnlyName(t *testing.T) {
+	store := newProfileTestStore(t)
+	handler := NewProfileHandler(testUserStoreProvider{store: store})
+
+	req := newAuthorizedProfileRequestWithRole(
+		http.MethodPost,
+		"/profiles",
+		`{"name":"   "}`,
+		"user",
+		"profile-1",
+	)
+	rr := httptest.NewRecorder()
+
+	handler.HandleCreateProfile(rr, req)
+
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String())
+	}
+
+	profiles, err := store.ListProfiles(context.Background())
+	if err != nil {
+		t.Fatalf("list profiles: %v", err)
+	}
+	if len(profiles) != 1 {
+		t.Fatalf("profile count = %d, want 1", len(profiles))
+	}
+}
+
+func TestHandleCreateProfile_TrimsStoredName(t *testing.T) {
+	store := newProfileTestStore(t)
+	handler := NewProfileHandler(testUserStoreProvider{store: store})
+
+	req := newAuthorizedProfileRequestWithRole(
+		http.MethodPost,
+		"/profiles",
+		`{"name":" Laura "}`,
+		"user",
+		"profile-1",
+	)
+	rr := httptest.NewRecorder()
+
+	handler.HandleCreateProfile(rr, req)
+
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String())
+	}
+
+	profiles, err := store.ListProfiles(context.Background())
+	if err != nil {
+		t.Fatalf("list profiles: %v", err)
+	}
+	for _, p := range profiles {
+		if p.Name == "Laura" {
+			return
+		}
+	}
+	t.Fatalf("no profile stored with trimmed name, profiles: %+v", profiles)
+}
+
 func TestHandleCreateProfile_BlocksNonPrimaryNonAdmin(t *testing.T) {
 	store := newProfileTestStore(t)
 	if err := store.CreateProfile(context.Background(), userstore.Profile{ID: "profile-2", Name: "Kids"}); err != nil {
@@ -437,6 +534,123 @@ func TestHandleUpdateProfile_AllowsNonAdminToUpdateAnyOwnedProfile(t *testing.T)
 	}
 }

+func TestHandleUpdateProfile_RejectsDuplicateNameOnRename(t *testing.T) {
+	store := newProfileTestStore(t)
+	if err := store.CreateProfile(context.Background(), userstore.Profile{ID: "profile-2", Name: "Kids"}); err != nil {
+		t.Fatalf("create profile: %v", err)
+	}
+	handler := NewProfileHandler(testUserStoreProvider{store: store})
+
+	req := newAuthorizedProfileRequestWithRole(
+		http.MethodPut,
+		"/profiles/profile-2",
+		`{"name":"MAIN"}`,
+		"user",
+		"profile-1",
+	)
+	rr := httptest.NewRecorder()
+
+	handler.HandleUpdateProfile(rr, withProfileRouteParam(req, "id", "profile-2"))
+
+	if rr.Code != http.StatusConflict {
+		t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String())
+	}
+
+	var resp errorResponse
+	if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil {
+		t.Fatalf("decode response: %v", err)
+	}
+	if resp.Error != "name_conflict" {
+		t.Fatalf("error = %q, want %q", resp.Error, "name_conflict")
+	}
+
+	profile, err := store.GetProfile(context.Background(), "profile-2")
+	if err != nil {
+		t.Fatalf("get profile: %v", err)
+	}
+	if profile == nil || profile.Name != "Kids" {
+		t.Fatalf("profile name changed despite conflict: %+v", profile)
+	}
+}
+
+func TestHandleUpdateProfile_AllowsKeepingOwnNameOnUpdate(t *testing.T) {
+	store := newProfileTestStore(t)
+	handler := NewProfileHandler(testUserStoreProvider{store: store})
+
+	// Saving a profile without renaming resubmits its own name; the
+	// conflict check must exclude the profile being updated.
+	req := newAuthorizedProfileRequestWithRole(
+		http.MethodPut,
+		"/profiles/profile-1",
+		`{"name":"Main","subtitle_mode":"always"}`,
+		"user",
+		"profile-1",
+	)
+	rr := httptest.NewRecorder()
+
+	handler.HandleUpdateProfile(rr, withProfileRouteParam(req, "id", "profile-1"))
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String())
+	}
+}
+
+func TestHandleUpdateProfile_RejectsWhitespaceOnlyName(t *testing.T) {
+	store := newProfileTestStore(t)
+	handler := NewProfileHandler(testUserStoreProvider{store: store})
+
+	req := newAuthorizedProfileRequestWithRole(
+		http.MethodPut,
+		"/profiles/profile-1",
+		`{"name":"   "}`,
+		"user",
+		"profile-1",
+	)
+	rr := httptest.NewRecorder()
+
+	handler.HandleUpdateProfile(rr, withProfileRouteParam(req, "id", "profile-1"))
+
+	if rr.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String())
+	}
+
+	profile, err := store.GetProfile(context.Background(), "profile-1")
+	if err != nil {
+		t.Fatalf("get profile: %v", err)
+	}
+	if profile == nil || profile.Name != "Main" {
+		t.Fatalf("profile name changed despite rejection: %+v", profile)
+	}
+}
+
+func TestHandleUpdateProfile_TrimsStoredNameOnRename(t *testing.T) {
+	store := newProfileTestStore(t)
+	handler := NewProfileHandler(testUserStoreProvider{store: store})
+
+	req := newAuthorizedProfileRequestWithRole(
+		http.MethodPut,
+		"/profiles/profile-1",
+		`{"name":" Laura "}`,
+		"user",
+		"profile-1",
+	)
+	rr := httptest.NewRecorder()
+
+	handler.HandleUpdateProfile(rr, withProfileRouteParam(req, "id", "profile-1"))
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String())
+	}
+
+	profile, err := store.GetProfile(context.Background(), "profile-1")
+	if err != nil {
+		t.Fatalf("get profile: %v", err)
+	}
+	if profile == nil || profile.Name != "Laura" {
+		t.Fatalf("stored name not trimmed: %+v", profile)
+	}
+}
+
 func TestHandleDeleteProfile_AllowsPrimaryToDeleteOther(t *testing.T) {
 	store := newProfileTestStore(t)
 	if err := store.CreateProfile(context.Background(), userstore.Profile{ID: "profile-2", Name: "Kids"}); err != nil {