From 1aba1a41e6629a6df4b82239deb898fadbc063a6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 23 Oct 2026 14:16:41 -0400 Subject: [PATCH] fix(tracks): removing a track writes a track_remove audit row (#6294) RemoveTrack took the admin's id and dropped it, so a merge left an audit trail and the more destructive removal did not. It now reads the track's title, artist, album and file path before the delete and writes them, with whether the album and artist went too, under the new track_remove action. The audit action groups lose their plan-era U1/U2/U3 headings. Co-Authored-By: Claude Opus 5.5 --- internal/audit/audit.go | 21 +++++++++-------- internal/tracks/service.go | 32 ++++++++++++++++++++++--- internal/tracks/service_test.go | 41 +++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 12 deletions(-) diff --git a/internal/audit/audit.go b/internal/audit/audit.go index c88b2048..07b37ba2 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -20,14 +20,13 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) -// Action is the discriminator stored in audit_log.action. New values -// added in U2/U3 + future tasks; declare them here so callers don't -// stringly-type and so a future audit-search UI has a single source -// of truth for the vocabulary. +// Action is the discriminator stored in audit_log.action. Every value is +// declared here, so callers never stringly-type an action and the vocabulary +// has one source. type Action string const ( - // U1 + // Accounts and invites. ActionRegister Action = "register" ActionPromoteAdmin Action = "promote_admin" ActionDemoteAdmin Action = "demote_admin" @@ -35,15 +34,13 @@ const ( ActionInviteRedeem Action = "invite_redeem" ActionInviteRevoke Action = "invite_revoke" - // U2 (declared early; callers in U1 don't use them yet, but - // having them here means U2's diff is purely additive on the - // caller side, not also touching this file). + // Admin user management. ActionCreateUserAdmin Action = "create_user_admin" ActionDeleteUser Action = "delete_user" ActionPasswordResetAdmin Action = "password_reset_admin" ActionAutoApproveToggle Action = "auto_approve_toggle" - // U3 + // Credentials. ActionPasswordChangeSelf Action = "password_change_self" ActionTokenRegenerate Action = "token_regenerate" ActionForgotPasswordInit Action = "forgot_password_initiated" @@ -65,6 +62,12 @@ const ( ActionDuplicateDismiss Action = "duplicate_dismiss" ActionDuplicateWrongFile Action = "duplicate_wrong_file" + // An admin removing a track: its file and row are deleted, with every + // like, play and playlist entry that hung off the row. The metadata names + // the track and its file, so the log can say what went after the row is + // gone. + ActionTrackRemove Action = "track_remove" + // Subsonic password: generated in Settings for t/s-only clients. ActionSubsonicPasswordSet Action = "subsonic_password_set" ActionSubsonicPasswordClear Action = "subsonic_password_clear" diff --git a/internal/tracks/service.go b/internal/tracks/service.go index 63b76aa0..f1cafd71 100644 --- a/internal/tracks/service.go +++ b/internal/tracks/service.go @@ -13,11 +13,13 @@ import ( "fmt" "log/slog" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/audit" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/library" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) @@ -57,11 +59,32 @@ func NewService(pool *pgxpool.Pool, logger *slog.Logger, dataDir string) *Servic // file cannot be removed it is a *library.FileRemoveError and NOTHING was // deleted — see library.DeleteTrackFile for why that order is the contract. // -// adminID is accepted for the audit log but not yet written to it. +// A removal is written to the audit log with adminID as the actor. The +// track's names are read before the delete, since the row, and perhaps its +// album and artist, are gone after it. func (s *Service) RemoveTrack( - ctx context.Context, - trackID, adminID pgtype.UUID, //nolint:revive // adminID reserved for audit-log wiring in a follow-up + ctx context.Context, trackID, adminID pgtype.UUID, ) (deletedAlbumID *pgtype.UUID, deletedArtistID *pgtype.UUID, err error) { + q := dbq.New(s.pool) + track, err := q.GetTrackByID(ctx, trackID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, nil, ErrNotFound + } + return nil, nil, fmt.Errorf("get track: %w", err) + } + meta := map[string]any{ + "track_id": syncpkg.FormatUUID(track.ID), + "title": track.Title, + "file_path": track.FilePath, + } + if artist, err := q.GetArtistByID(ctx, track.ArtistID); err == nil { + meta["artist"] = artist.Name + } + if album, err := q.GetAlbumByID(ctx, track.AlbumID); err == nil { + meta["album"] = album.Title + } + deleted, err := library.DeleteTrackFile(ctx, s.pool, s.logger, s.dataDir, trackID) if err != nil { if errors.Is(err, library.ErrTrackNotFound) { @@ -69,6 +92,9 @@ func (s *Service) RemoveTrack( } return nil, nil, fmt.Errorf("delete track: %w", err) } + meta["album_deleted"] = deleted.AlbumID != nil + meta["artist_deleted"] = deleted.ArtistID != nil + audit.WriteOrLog(ctx, s.pool, s.logger, adminID, pgtype.UUID{}, audit.ActionTrackRemove, meta) return deleted.AlbumID, deleted.ArtistID, nil } diff --git a/internal/tracks/service_test.go b/internal/tracks/service_test.go index 0bd229b7..06a354f4 100644 --- a/internal/tracks/service_test.go +++ b/internal/tracks/service_test.go @@ -137,6 +137,47 @@ func TestRemoveTrack_LocalFile_HappyPath(t *testing.T) { } } +func TestRemoveTrack_WritesAuditRow(t *testing.T) { + pool := newPool(t) + admin := seedAdmin(t, pool, "alice") + if _, err := pool.Exec(context.Background(), `DELETE FROM audit_log`); err != nil { + t.Fatalf("reset audit_log: %v", err) + } + + dir := t.TempDir() + path := filepath.Join(dir, "track.mp3") + if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + _, _, track := seedArtistAlbumTrack(t, pool, "Audited", "", path) + + svc := NewService(pool, nil, "") + if _, _, err := svc.RemoveTrack(context.Background(), track.ID, admin.ID); err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + + var actor pgtype.UUID + var title, artist, album, filePath string + var albumDeleted bool + err := pool.QueryRow(context.Background(), ` + SELECT actor_id, metadata->>'title', metadata->>'artist', metadata->>'album', + metadata->>'file_path', (metadata->>'album_deleted')::boolean + FROM audit_log WHERE action = 'track_remove'`). + Scan(&actor, &title, &artist, &album, &filePath, &albumDeleted) + if err != nil { + t.Fatalf("read audit row: %v", err) + } + if actor != admin.ID { + t.Errorf("actor = %v, want the admin %v", actor, admin.ID) + } + if title != "Audited Track" || artist != "Audited Artist" || album != "Audited Album" || filePath != path { + t.Errorf("metadata = %q / %q / %q / %q", title, artist, album, filePath) + } + if !albumDeleted { + t.Errorf("album_deleted = false, want true: the track was its album's only one") + } +} + func TestRemoveTrack_FileAlreadyMissing(t *testing.T) { pool := newPool(t) admin := seedAdmin(t, pool, "alice")