diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 468b2dd..a61c87f 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -1106,7 +1106,22 @@ func (f *fakeRepo) RedeemMigration(_ context.Context, targetUserID, codeHash str // ---- quota admin fakes ---- +// liveUserExists mirrors PGRepo.requireLiveUser: the admin quota/link fakes +// only act on a live seeded row, so a unit test can drive the unknown-user 404 +// the real FK would otherwise turn into a 500. +func (f *fakeRepo) liveUserExists(id string) bool { + for _, su := range f.seededUsers { + if su.view.ID == id && su.detail.DeletedAt == nil { + return true + } + } + return false +} + func (f *fakeRepo) GetQuotas(_ context.Context, userID string) (*QuotaView, error) { + if !f.liveUserExists(userID) { + return nil, ErrNotFound + } v := &QuotaView{UserID: userID} if f.fakeQuotas == nil { return v, nil @@ -1121,6 +1136,9 @@ func (f *fakeRepo) GetQuotas(_ context.Context, userID string) (*QuotaView, erro } func (f *fakeRepo) SetQuotas(_ context.Context, userID string, qi QuotaInput, _ string) (*QuotaView, error) { + if !f.liveUserExists(userID) { + return nil, ErrNotFound + } if f.fakeQuotas == nil { f.fakeQuotas = map[string]*QuotaView{} } @@ -1173,6 +1191,9 @@ func (f *fakeRepo) UnlinkAccount(_ context.Context, userID, mcUUID string) error } func (f *fakeRepo) LinkAccount(_ context.Context, userID, mcUUID, authSource string) error { + if !f.liveUserExists(userID) { + return ErrNotFound + } if existing, ok := f.links[mcUUID]; ok && existing != userID { return ErrConflict } diff --git a/internal/api/handlers_users.go b/internal/api/handlers_users.go index c01cf45..80f62c5 100644 --- a/internal/api/handlers_users.go +++ b/internal/api/handlers_users.go @@ -281,6 +281,10 @@ func (a *API) handleGetQuotas(w http.ResponseWriter, r *http.Request) { } v, err := a.Repo.GetQuotas(r.Context(), id) if err != nil { + if errors.Is(err, ErrNotFound) { + writeError(w, r, newError(http.StatusNotFound, "not_found", "user not found")) + return + } writeError(w, r, err) return } @@ -477,6 +481,10 @@ func (a *API) handleLinkAccount(w http.ResponseWriter, r *http.Request) { "this UUID is already linked to a different user")) return } + if errors.Is(err, ErrNotFound) { + writeError(w, r, newError(http.StatusNotFound, "not_found", "user not found")) + return + } writeError(w, r, err) return } diff --git a/internal/api/handlers_users_test.go b/internal/api/handlers_users_test.go index 9fc2745..a18ec26 100644 --- a/internal/api/handlers_users_test.go +++ b/internal/api/handlers_users_test.go @@ -51,3 +51,49 @@ func TestOwnerAccountProtectedFromPanelMutations(t *testing.T) { } }) } + +// The user-scoped admin sub-resources (quotas, account links) must answer 404 +// for an unknown user id. Before the requireLiveUser guard the quota upsert and +// the link insert reached the users(id) foreign key and surfaced as an opaque +// 500 (found live against the drill cluster, audit #30), and the quotas read +// answered a zero-value "unlimited" view as if the id existed. +func TestAdminSubresourcesRequireLiveUser(t *testing.T) { + owner := &Principal{UserID: "usr-root", Role: "owner", ViaAdminAccess: true} + repo := newFakeRepo() + repo.seedUser(UserView{ID: "usr-root", Username: "root", Role: "owner"}) + repo.seedUser(UserView{ID: "u2", Username: "alice", Role: "user"}) + api := newTestAPI(repo, newFakeCluster()) + api.External = staticExternal{p: owner} + eh := api.ExternalHandler() + + t.Run("quotas read of an unknown user", func(t *testing.T) { + w := do(eh, "GET", "/api/v1/users/usr-nope/quotas", "", nil) + if w.Code != http.StatusNotFound { + t.Fatalf("code = %d body %s, want 404", w.Code, w.Body.String()) + } + }) + t.Run("quotas write of an unknown user", func(t *testing.T) { + w := do(eh, "PUT", "/api/v1/users/usr-nope/quotas", `{"max_servers":1}`, jsonHeader) + if w.Code != http.StatusNotFound { + t.Fatalf("code = %d body %s, want 404", w.Code, w.Body.String()) + } + }) + t.Run("account link of an unknown user", func(t *testing.T) { + w := do(eh, "POST", "/api/v1/users/usr-nope/links", + `{"mc_uuid":"22222222-3333-4444-5555-666666666666"}`, jsonHeader) + if w.Code != http.StatusNotFound { + t.Fatalf("code = %d body %s, want 404", w.Code, w.Body.String()) + } + }) + t.Run("control: a live user still accepts both writes", func(t *testing.T) { + w := do(eh, "PUT", "/api/v1/users/u2/quotas", `{"max_servers":2}`, jsonHeader) + if w.Code != http.StatusOK { + t.Fatalf("set quotas: code = %d body %s, want 200", w.Code, w.Body.String()) + } + w = do(eh, "POST", "/api/v1/users/u2/links", + `{"mc_uuid":"22222222-3333-4444-5555-666666666677"}`, jsonHeader) + if w.Code != http.StatusOK { + t.Fatalf("link: code = %d body %s, want 200", w.Code, w.Body.String()) + } + }) +} diff --git a/internal/api/pgrepo.go b/internal/api/pgrepo.go index de68490..a1a7728 100644 --- a/internal/api/pgrepo.go +++ b/internal/api/pgrepo.go @@ -1632,8 +1632,13 @@ func (p *PGRepo) SetUserDisabled(ctx context.Context, userID string, disabled bo // ---- quota admin ---- // GetQuotas returns the quotas row for a user, or a zero-value view when no -// row exists (meaning unlimited). +// row exists (meaning unlimited). An unknown or soft-deleted user id is +// ErrNotFound, never a zero-value "unlimited" answer — the admin sub-resource +// routes all 404 on a user that has no live row. func (p *PGRepo) GetQuotas(ctx context.Context, userID string) (*QuotaView, error) { + if err := p.requireLiveUser(ctx, userID); err != nil { + return nil, err + } const q = `SELECT user_id, max_servers, max_cpu_milli, max_memory_mb, max_storage_gb FROM quotas WHERE user_id = $1` v := QuotaView{UserID: userID} @@ -1647,9 +1652,29 @@ func (p *PGRepo) GetQuotas(ctx context.Context, userID string) (*QuotaView, erro return &v, nil } +// requireLiveUser gates the user-scoped admin sub-resources (quotas, account +// links) on a live users row. Without it a write would hit the user_id foreign +// key and surface as an opaque 500, and the read would answer as if a +// never-existed id did; every admin route answers ErrNotFound → 404 instead. +func (p *PGRepo) requireLiveUser(ctx context.Context, userID string) error { + var ok bool + if err := p.db.QueryRowContext(ctx, + `SELECT EXISTS(SELECT 1 FROM users WHERE id = $1 AND deleted_at IS NULL)`, + userID).Scan(&ok); err != nil { + return err + } + if !ok { + return ErrNotFound + } + return nil +} + // SetQuotas upserts a quotas row. Nil fields are left unchanged; a non-nil // zero-value field clears the cap. func (p *PGRepo) SetQuotas(ctx context.Context, userID string, qi QuotaInput, setBy string) (*QuotaView, error) { + if err := p.requireLiveUser(ctx, userID); err != nil { + return nil, err + } type col struct { name string value *int @@ -1754,6 +1779,9 @@ func (p *PGRepo) UnlinkAccount(ctx context.Context, userID, mcUUID string) error // NOTHING on the UNIQUE(mc_uuid) constraint, plus an idempotency check via // EXISTS). func (p *PGRepo) LinkAccount(ctx context.Context, userID, mcUUID, authSource string) error { + if err := p.requireLiveUser(ctx, userID); err != nil { + return err + } // Check idempotency first: already linked to this user → success. var exists bool if err := p.db.QueryRowContext(ctx, diff --git a/internal/pgint/pgint_test.go b/internal/pgint/pgint_test.go index 1d580df..be49d75 100644 --- a/internal/pgint/pgint_test.go +++ b/internal/pgint/pgint_test.go @@ -439,6 +439,48 @@ func TestUserAdminEmailEditClearsVerification(t *testing.T) { assertEmailProven(t, u.ID, next, false) } +// The user-scoped admin sub-resources must refuse an id that has no live users +// row with ErrNotFound (→ the API's 404). The quota upsert and the account-link +// insert touch user_id foreign keys, so before the requireLiveUser guard the +// live drill returned a 500 on both (audit #30); the quotas read answered a +// zero-value "unlimited" view for an id that never existed. +func TestAdminSubresourcesRequireLiveUser(t *testing.T) { + ctx := context.Background() + ghost := "usr-ghost-" + suffix(t) + + if _, err := repo.GetQuotas(ctx, ghost); !errors.Is(err, api.ErrNotFound) { + t.Fatalf("GetQuotas(ghost) = %v, want ErrNotFound", err) + } + three := 3 + if _, err := repo.SetQuotas(ctx, ghost, api.QuotaInput{MaxServers: &three}, "pgint"); !errors.Is(err, api.ErrNotFound) { + t.Fatalf("SetQuotas(ghost) = %v, want ErrNotFound", err) + } + if err := repo.LinkAccount(ctx, ghost, testUUID(t), "mojang"); !errors.Is(err, api.ErrNotFound) { + t.Fatalf("LinkAccount(ghost) = %v, want ErrNotFound", err) + } + + // Control: the same calls land for a live user. + u := newUser(t, "user", "subres") + if _, err := repo.SetQuotas(ctx, u.ID, api.QuotaInput{MaxServers: &three}, "pgint"); err != nil { + t.Fatalf("SetQuotas(live): %v", err) + } + got, err := repo.GetQuotas(ctx, u.ID) + if err != nil || got.MaxServers == nil || *got.MaxServers != 3 { + t.Fatalf("GetQuotas(live) = %+v, %v; want max_servers=3", got, err) + } + if err := repo.LinkAccount(ctx, u.ID, testUUID(t), "mojang"); err != nil { + t.Fatalf("LinkAccount(live): %v", err) + } + + // A soft-deleted user is no longer a live target either. + if err := repo.DeleteUser(ctx, u.ID, "pgint"); err != nil { + t.Fatalf("DeleteUser: %v", err) + } + if _, err := repo.SetQuotas(ctx, u.ID, api.QuotaInput{MaxServers: &three}, "pgint"); !errors.Is(err, api.ErrNotFound) { + t.Fatalf("SetQuotas(deleted) = %v, want ErrNotFound", err) + } +} + // The owner tier the panel gates on must actually be WRITTEN: until this // contract had a test, every provisioning path wrote 'admin', so the whole // owner surface (user administration) was unreachable in a fresh install.