diff --git a/internal/repository/taxonomy_repository.go b/internal/repository/taxonomy_repository.go index 39d71cd8..502b91b5 100644 --- a/internal/repository/taxonomy_repository.go +++ b/internal/repository/taxonomy_repository.go @@ -1038,6 +1038,8 @@ func (r *TaxonomyRepository) RemoveNode( } // ListNodeRecords returns feedback records assigned to a visible taxonomy node or descendants. +// The node must be visible and belong to the tenant, otherwise a not-found error is returned — +// the same contract as every other node-scoped operation. func (r *TaxonomyRepository) ListNodeRecords( ctx context.Context, nodeID uuid.UUID, @@ -1048,7 +1050,29 @@ func (r *TaxonomyRepository) ListNodeRecords( limit = defaultTaxonomyNodeRecordLimit } - rows, err := r.db.Query(ctx, ` + // The records query below is already tenant-safe on its own (it filters on the run's tenant, so a + // foreign node simply matches no rows). The ownership guard is about the contract, not isolation: + // without it a foreign or removed node is indistinguishable from one that genuinely holds no + // records, which is what CountNodeRecords, GetTree, RenameNode and RemoveNode all avoid by + // resolving ownership first. + // + // Both reads share one REPEATABLE READ snapshot. On separate statements a RemoveNode committing + // between them would pass the guard and then match no rows in the recursive CTE, handing back the + // ambiguous 200-empty this contract exists to remove. Under one snapshot the node is either + // visible to both reads or to neither, so a removal mid-request lands as a 404. Read-only, so the + // deferred rollback is the only exit. + dbTx, err := r.db.BeginTx(ctx, pgx.TxOptions{IsoLevel: pgx.RepeatableRead, AccessMode: pgx.ReadOnly}) + if err != nil { + return nil, 0, fmt.Errorf("begin taxonomy node records tx: %w", err) + } + + defer rollbackQuietly(ctx, dbTx, "list taxonomy node records: rollback failed") + + if _, err := getNodeForTenant(ctx, dbTx, nodeID, tenantID); err != nil { + return nil, 0, err + } + + rows, err := dbTx.Query(ctx, ` WITH RECURSIVE visible_nodes AS ( SELECT id, run_id, cluster_id FROM taxonomy_nodes @@ -1342,6 +1366,41 @@ func scanTaxonomyNode(row scanner) (*models.TaxonomyNode, error) { return &node, nil } +// A node is addressable by a tenant when it is visible and its run belongs to that tenant. Shared by +// the locking write path and the read path so the two can never drift apart on what "yours" means. +// $1 is the node id, $2 the tenant id. +const taxonomyNodeForTenantWhere = ` + WHERE id = $1 AND removed_at IS NULL + AND EXISTS ( + SELECT 1 FROM taxonomy_runs + WHERE taxonomy_runs.id = taxonomy_nodes.run_id AND taxonomy_runs.tenant_id = $2 + )` + +// getNodeForTenant returns a visible taxonomy node addressable by the tenant, or a not-found error. +// The read-only counterpart of getNodeForUpdate: same ownership predicate, no row lock. It takes a +// queryer so the caller decides the snapshot — ListNodeRecords passes its transaction so the check +// and the records read cannot disagree about whether the node is still there. +func getNodeForTenant( + ctx context.Context, + q queryer, + nodeID uuid.UUID, + tenantID string, +) (*models.TaxonomyNode, error) { + node, err := queryTaxonomyNode(ctx, q, taxonomyNodeSelect+` + FROM taxonomy_nodes`+taxonomyNodeForTenantWhere, + nodeID, tenantID, + ) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, huberrors.NewNotFoundError("taxonomy_node", "taxonomy node not found") + } + + return nil, fmt.Errorf("get taxonomy node for tenant: %w", err) + } + + return node, nil +} + // getNodeForUpdate takes a tenantWriteTx (not the narrower queryer) so the // compiler enforces that the SELECT ... FOR UPDATE row lock is held for the // life of a transaction; outside one, the lock would release at statement end. @@ -1354,12 +1413,7 @@ func getNodeForUpdate( // The tenant predicate keeps the row lock tenant-scoped: a caller can never // lock another tenant's node row, even transiently. node, err := queryTaxonomyNode(ctx, transaction, taxonomyNodeSelect+` - FROM taxonomy_nodes - WHERE id = $1 AND removed_at IS NULL - AND EXISTS ( - SELECT 1 FROM taxonomy_runs - WHERE taxonomy_runs.id = taxonomy_nodes.run_id AND taxonomy_runs.tenant_id = $2 - ) + FROM taxonomy_nodes`+taxonomyNodeForTenantWhere+` FOR UPDATE`, nodeID, tenantID, ) diff --git a/openapi.yaml b/openapi.yaml index 51212dab..63bfb2aa 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -2080,7 +2080,9 @@ paths: summary: List feedback records for a taxonomy node description: | Returns the feedback records assigned to a node and all of its (visible) descendant nodes, via the - clusters those nodes reference. Tenant-scoped. The `limit` in the response reflects the applied cap. + clusters those nodes reference. Tenant-scoped; returns 404 if the node does not belong to the tenant + or has been removed. An empty `data` therefore means the node genuinely holds no records. The `limit` + in the response reflects the applied cap. operationId: list-taxonomy-node-records parameters: - name: node_id @@ -2129,6 +2131,12 @@ paths: application/problem+json: schema: $ref: '#/components/schemas/ErrorModel' + "404": + description: Not Found – no node with this ID for the tenant. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/ErrorModel' default: description: Error content: diff --git a/tests/taxonomy_api_test.go b/tests/taxonomy_api_test.go index d49361d1..d7f3127f 100644 --- a/tests/taxonomy_api_test.go +++ b/tests/taxonomy_api_test.go @@ -223,12 +223,11 @@ func TestTaxonomyAPI_TenantIsolation(t *testing.T) { taxonomyURL(harness.server.URL, "/v1/taxonomy/nodes/"+ids.BranchID.String(), removeQuery), harness.apiKey, nil, http.StatusNotFound, nil) }) - t.Run("node records return nothing for another tenant", func(t *testing.T) { + t.Run("node records 404 for another tenant", func(t *testing.T) { + // Not 200-with-empty: that would make a foreign node indistinguishable from one of your own + // that holds no records. Same contract as the run, tree, rename and remove cases above. recordsURL := taxonomyURL(harness.server.URL, "/v1/taxonomy/nodes/"+ids.RootID.String()+"/records", otherQuery) - - var resp models.TaxonomyNodeRecordsResponse - requestTaxonomyJSON(ctx, t, http.MethodGet, recordsURL, harness.apiKey, nil, http.StatusOK, &resp) - require.Empty(t, resp.Data) + requestTaxonomyJSON(ctx, t, http.MethodGet, recordsURL, harness.apiKey, nil, http.StatusNotFound, nil) }) } diff --git a/tests/taxonomy_persistence_test.go b/tests/taxonomy_persistence_test.go index 1c908892..67427d15 100644 --- a/tests/taxonomy_persistence_test.go +++ b/tests/taxonomy_persistence_test.go @@ -466,10 +466,27 @@ func TestTaxonomyRepository_ListNodeRecords(t *testing.T) { require.Len(t, records, 1) require.Equal(t, ids.FeedbackRecordID, records[0].ID) - // A different tenant sees nothing for the same node id. - otherTenantRecords, _, err := repo.ListNodeRecords(ctx, ids.RootID, "other-tenant-"+uuid.NewString(), 50) + // A different tenant cannot address the node at all — not even to be told it is empty. + _, _, err = repo.ListNodeRecords(ctx, ids.RootID, "other-tenant-"+uuid.NewString(), 50) + require.ErrorIs(t, err, huberrors.ErrNotFound, "node records must be tenant-scoped") + + // An unknown node id is the same not-found, so ownership is never enumerable. + _, _, err = repo.ListNodeRecords(ctx, uuid.New(), scope.TenantID, 50) + require.ErrorIs(t, err, huberrors.ErrNotFound) + + // A soft-removed node is not addressable either, matching rename and remove: the tree has already + // dropped it, so a caller asking for its records is working from stale state. + _, err = repo.RemoveNode(ctx, ids.LeafID, scope.TenantID, "actor-remove") + require.NoError(t, err) + + _, _, err = repo.ListNodeRecords(ctx, ids.LeafID, scope.TenantID, 50) + require.ErrorIs(t, err, huberrors.ErrNotFound) + + // The root survives and now genuinely holds no records, because the removed leaf carried the only + // cluster membership. That empty result is only meaningful because the cases above are 404s. + rootRecords, _, err := repo.ListNodeRecords(ctx, ids.RootID, scope.TenantID, 50) require.NoError(t, err) - require.Empty(t, otherTenantRecords, "node records must be tenant-scoped") + require.Empty(t, rootRecords) } // TestTaxonomyRepository_TenantIsolation proves every tenant-scoped read and mutation refuses @@ -501,6 +518,11 @@ func TestTaxonomyRepository_TenantIsolation(t *testing.T) { require.ErrorIs(t, err, huberrors.ErrNotFound) }) + t.Run("node records refuse another tenant", func(t *testing.T) { + _, _, err := repo.ListNodeRecords(ctx, ids.RootID, otherTenant, 50) + require.ErrorIs(t, err, huberrors.ErrNotFound) + }) + t.Run("rename and remove refuse another tenant", func(t *testing.T) { _, err := repo.RenameNode(ctx, ids.BranchID, otherTenant, "attacker", "Hijacked") require.ErrorIs(t, err, huberrors.ErrNotFound)