fix(api): the quota/link admin sub-resources require a live user (404, not FK 500) (1918da29) · Commits · FelisMC / Felis · GitLab

internal/api/api_test.go

+21 −0

Changes for internal/api/api_test.go: 21 added lines, 0 removed lines.
Original line number	Diff line number	Diff line
		@@ -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
		}

internal/api/handlers_users.go

+8 −0

Changes for internal/api/handlers_users.go: 8 added lines, 0 removed lines.
Original line number	Diff line number	Diff line
		@@ -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
		}

internal/api/handlers_users_test.go

+46 −0

Changes for internal/api/handlers_users_test.go: 46 added lines, 0 removed lines.
Original line number	Diff line number	Diff line
		@@ -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())
		}
		})
		}

internal/api/pgrepo.go

+29 −1

Changes for internal/api/pgrepo.go: 29 added lines, 1 removed line.
Original line number	Diff line number	Diff line
		@@ -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,

internal/pgint/pgint_test.go

+42 −0

Changes for internal/pgint/pgint_test.go: 42 added lines, 0 removed lines.
Original line number	Diff line number	Diff line
		@@ -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.