feat: transaction history audit log + timeline UI#119
feat: transaction history audit log + timeline UI#119
Conversation
…dundant created_at
Adds ConnectRPC handler for TransactionHistoryService.ListHistory with auth gate, plus *Mapper.MapTransactionHistoryEvent that converts database.TransactionHistory rows to historyv1.TransactionHistoryEvent (snapshot/diff mapped via structpb.NewStruct, defensive on error). New handler interfaces TransactionHistorySvc + TransactionHistoryMapper. Also bumps go-money-pb modules to pull history/v1 codegen. Wiring in main.go deferred to Task 12.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #119 +/- ##
==========================================
- Coverage 86.80% 86.32% -0.49%
==========================================
Files 86 92 +6
Lines 7155 7474 +319
==========================================
+ Hits 6211 6452 +241
- Misses 717 774 +57
- Partials 227 248 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an append-only transaction history system that tracks all mutations to transactions, including those triggered by rules, bulk operations, and importers. It features a new backend service for recording and retrieving history events using JSON diffs, along with a frontend timeline view. Key feedback includes critical security concerns regarding missing authorization checks in the history retrieval API, which could allow users to view other users' transaction logs. Performance improvements are also suggested to address N+1 query issues in bulk operations and the computational overhead of generating snapshots during rule execution. Finally, it is recommended to use TIMESTAMPTZ in the database schema for more robust time zone management.
| return nil, connect.NewError(connect.CodePermissionDenied, auth.ErrInvalidToken) | ||
| } | ||
|
|
||
| rows, err := a.svc.List(ctx, c.Msg.TransactionId) |
There was a problem hiding this comment.
The ListHistory endpoint is vulnerable to Insecure Direct Object Reference (IDOR). It fetches transaction history based solely on the TransactionId provided in the request without verifying that the transaction belongs to the authenticated user (jwtData.UserID). An attacker could guess transaction IDs to view audit logs of other users' transactions.
| func (s *Service) List(ctx context.Context, transactionID int64) ([]*database.TransactionHistory, error) { | ||
| var rows []*database.TransactionHistory | ||
| if err := database.GetDbWithContext(ctx, database.DbTypeReadonly). | ||
| Where("transaction_id = ?", transactionID). | ||
| Order("occurred_at ASC, id ASC"). | ||
| Find(&rows).Error; err != nil { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| return rows, nil |
There was a problem hiding this comment.
The List method should enforce ownership by verifying that the requested transactionID belongs to the user context. Currently, it performs a global query on the transaction_history table. Consider joining with the transactions table to filter by user_id or passing the userID as a parameter to this method.
| func (s *Executor) ruleProducedChange(prev, curr *database.Transaction) (bool, error) { | ||
| prevSnap, err := history.Snapshot(prev) | ||
| if err != nil { | ||
| return false, errors.Wrap(err, "snapshot prev for rule change check") | ||
| } | ||
| currSnap, err := history.Snapshot(curr) | ||
| if err != nil { | ||
| return false, errors.Wrap(err, "snapshot curr for rule change check") | ||
| } | ||
| diff, err := history.Diff(prevSnap, currSnap) | ||
| if err != nil { | ||
| return false, errors.Wrap(err, "diff for rule change check") | ||
| } | ||
| return diff != nil, nil | ||
| } |
There was a problem hiding this comment.
The ruleProducedChange method is called for every rule execution and performs two full snapshots and a JSON diff. Since Snapshot involves JSON marshaling and unmarshaling, this will significantly degrade performance during bulk transaction processing or when many rules are applied. Consider a more efficient way to detect changes, such as comparing specific fields or using a dirty-tracking mechanism on the transaction object.
| var prev database.Transaction | ||
| if err := tx.Where("id = ? AND deleted_at IS NULL", a.TransactionID).First(&prev).Error; err != nil { | ||
| return errors.Wrapf(err, "failed to set category on transaction %d: load", a.TransactionID) | ||
| } |
There was a problem hiding this comment.
Fetching the previous state of each transaction individually inside the loop creates an N+1 query problem. For bulk operations, it is more efficient to fetch all transactions by their IDs in a single query before entering the loop.
| actor_extra TEXT, | ||
| snapshot JSONB NOT NULL, | ||
| diff JSONB, | ||
| occurred_at TIMESTAMP NOT NULL DEFAULT now() |
There was a problem hiding this comment.
For audit logs and history tables, it is highly recommended to use TIMESTAMPTZ (timestamp with time zone) instead of TIMESTAMP. This ensures that events are correctly ordered and interpreted regardless of the database server's local time zone configuration.
1 participant
Summary
Append-only audit log for every transaction mutation, surfaced as a timeline tab on the transaction details page.
transaction_historytable (snapshot + RFC 6902 JSON Patch diff).context.Context(USER from JWT middleware, IMPORTER / SCHEDULER / BULK / RULE set at respective entry points).TransactionHistoryService.ListHistorygRPC + frontend timeline with PrimeNGTimelineand per-op diff rendering.xskydev/go-money-pb.Design doc:
docs/plans/2026-04-19-transaction-history.md.Test plan
/transactions/:id, switch to History tab, verify timeline renders.SELECT event_type, actor_type, actor_extra FROM transaction_history ORDER BY occurred_at DESC LIMIT 20;Verification gate (already green locally)
make lint— 0 issues.go test -p 1 -timeout 300s ./pkg/transactions/... ./pkg/importers/... ./pkg/mappers/... ./cmd/server/...— all pass.go build ./...— clean.ng build— clean.