test(service): cover atomic attachment-ownership link semantics (W1-3)

Mechanical signature updates for LinkAttachmentsToMessage callsites, plus:
db-level OwnershipGuard test (owned links, foreign never links, legacy
NULL-uploader claimable, nonexistent skipped) and a service-level
SendMessage test proving skip semantics end-to-end including the
already-linked retry path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
J3vb
2026-07-19 08:29:26 +02:00
co-authored by Claude Fable 5
parent 4b37c8024e
commit 95f85e213f
3 changed files with 114 additions and 4 deletions
+47 -3
View File
@@ -57,7 +57,7 @@ func TestGetAttachmentByID_Found(t *testing.T) {
func TestLinkAttachmentsToMessage_Empty(t *testing.T) {
database := openMigratedMemory(t)
n, err := database.LinkAttachmentsToMessage(1, nil)
n, err := database.LinkAttachmentsToMessage(1, 1, nil)
if err != nil {
t.Fatalf("LinkAttachmentsToMessage(nil): %v", err)
}
@@ -84,7 +84,7 @@ func TestLinkAttachmentsToMessage_LinksUnlinked(t *testing.T) {
}
}
n, err := database.LinkAttachmentsToMessage(msgID, []string{"att-a", "att-b"})
n, err := database.LinkAttachmentsToMessage(msgID, userID, []string{"att-a", "att-b"})
if err != nil {
t.Fatalf("LinkAttachmentsToMessage: %v", err)
}
@@ -113,7 +113,7 @@ func TestLinkAttachmentsToMessage_SkipsAlreadyLinked(t *testing.T) {
)
// Try to re-link to a different message — should skip (WHERE message_id IS NULL).
n, err := database.LinkAttachmentsToMessage(msg2, []string{"att-linked"})
n, err := database.LinkAttachmentsToMessage(msg2, userID, []string{"att-linked"})
if err != nil {
t.Fatalf("LinkAttachmentsToMessage: %v", err)
}
@@ -122,6 +122,50 @@ func TestLinkAttachmentsToMessage_SkipsAlreadyLinked(t *testing.T) {
}
}
// TestLinkAttachmentsToMessage_OwnershipGuard locks the atomic IDOR guard
// (W1-3): the link UPDATE itself enforces ownership, so a foreign attachment
// can never be claimed, legacy NULL-uploader rows remain claimable, and
// nonexistent ids are skipped without failing the statement.
func TestLinkAttachmentsToMessage_OwnershipGuard(t *testing.T) {
database := openMigratedMemory(t)
owner := seedUser(t, database, "att-owner")
other := seedUser(t, database, "att-other")
chID := seedChannel(t, database, "att-owner-ch")
msgID, _ := database.CreateMessage(chID, owner, "attachment carrier", nil)
if err := database.CreateAttachment("att-owned", owner, "o.txt", "s-o.txt", "text/plain", 1, nil, nil); err != nil {
t.Fatalf("CreateAttachment att-owned: %v", err)
}
if err := database.CreateAttachment("att-foreign", other, "f.txt", "s-f.txt", "text/plain", 1, nil, nil); err != nil {
t.Fatalf("CreateAttachment att-foreign: %v", err)
}
// Legacy row from before uploader tracking: uploader_id IS NULL.
if _, err := database.Exec(
`INSERT INTO attachments (id, filename, stored_as, mime_type, size)
VALUES ('att-legacy', 'l.txt', 's-l.txt', 'text/plain', 1)`,
); err != nil {
t.Fatalf("inserting legacy attachment: %v", err)
}
n, err := database.LinkAttachmentsToMessage(msgID, owner,
[]string{"att-owned", "att-foreign", "att-legacy", "att-missing"})
if err != nil {
t.Fatalf("LinkAttachmentsToMessage: %v", err)
}
if n != 2 {
t.Errorf("expected 2 linked (owned + legacy), got %d", n)
}
if att, _ := database.GetAttachmentByID("att-owned"); att.MessageID == nil || *att.MessageID != msgID {
t.Error("owner's unlinked attachment should link")
}
if att, _ := database.GetAttachmentByID("att-foreign"); att.MessageID != nil {
t.Error("another user's attachment must never link (IDOR guard)")
}
if att, _ := database.GetAttachmentByID("att-legacy"); att.MessageID == nil {
t.Error("legacy NULL-uploader attachment should be claimable")
}
}
// ─── GetAttachmentsByMessageIDs ──────────────────────────────────────────────
func TestGetAttachmentsByMessageIDs_Empty(t *testing.T) {
+1 -1
View File
@@ -460,7 +460,7 @@ func TestDeleteOrphanedAttachments_KeepsLinked(t *testing.T) {
// Create attachment and link it to a message.
_ = database.CreateAttachment("linked-1", userID, "file.txt", "stored-linked.txt", "text/plain", 100, nil, nil)
msgID, _ := database.CreateMessage(chID, userID, "with attachment", nil)
_, _ = database.LinkAttachmentsToMessage(msgID, []string{"linked-1"})
_, _ = database.LinkAttachmentsToMessage(msgID, userID, []string{"linked-1"})
files, err := database.DeleteOrphanedAttachments("2099-01-01T00:00:00Z")
if err != nil {
+66
View File
@@ -56,6 +56,72 @@ func TestSendMessage_Valid(t *testing.T) {
}
}
// TestSendMessage_AttachmentOwnershipAtomic locks the W1-3 semantics: the
// link UPDATE itself enforces ownership, so a foreign, already-linked, or
// nonexistent attachment is skipped (never linked) while the message still
// sends — no check-then-link race, and retries cannot hard-fail.
func TestSendMessage_AttachmentOwnershipAtomic(t *testing.T) {
ms := store.NewMemStore()
ms.SeedRole(&db.Role{
ID: permissions.MemberRoleID,
Name: "member",
Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AttachFiles,
Position: 1,
})
ms.SeedUserRole(1, permissions.MemberRoleID)
ms.SeedUserRole(2, permissions.MemberRoleID)
ms.SeedUser(&db.User{ID: 1, Username: "alice", Status: "online"})
ms.SeedUser(&db.User{ID: 2, Username: "mallory", Status: "online"})
ms.SeedChannel(&db.Channel{ID: 10, Name: "general", Type: "text"})
checker := permissions.NewChecker(ms)
svc := NewMessageService(ms, NewPermissionService(ms, checker), nil)
if err := ms.CreateAttachment("att-own", 1, "a.png", "s-a.png", "image/png", 10, nil, nil); err != nil {
t.Fatal(err)
}
if err := ms.CreateAttachment("att-foreign", 2, "b.png", "s-b.png", "image/png", 10, nil, nil); err != nil {
t.Fatal(err)
}
result, err := svc.SendMessage(context.Background(), SendMessageParams{
ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member",
Content: "with files",
AttachmentIDs: []string{"att-own", "att-foreign", "att-missing"},
})
if err != nil {
t.Fatalf("SendMessage: %v", err)
}
if result.MessageID <= 0 {
t.Fatal("message should persist even when some attachments are skipped")
}
own, _ := ms.GetAttachmentByID("att-own")
if own.MessageID == nil || *own.MessageID != result.MessageID {
t.Error("sender's own attachment should be linked to the new message")
}
foreign, _ := ms.GetAttachmentByID("att-foreign")
if foreign.MessageID != nil {
t.Error("another user's attachment must never be linked (IDOR guard)")
}
// A retry naming the now-linked attachment must still send.
retry, err := svc.SendMessage(context.Background(), SendMessageParams{
ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member",
Content: "retry",
AttachmentIDs: []string{"att-own"},
})
if err != nil {
t.Fatalf("retry with already-linked attachment should still send: %v", err)
}
if retry.MessageID <= 0 {
t.Fatal("retry should persist a message")
}
own2, _ := ms.GetAttachmentByID("att-own")
if own2.MessageID == nil || *own2.MessageID != result.MessageID {
t.Error("already-linked attachment must stay linked to the original message")
}
}
func TestSendMessage_EmptyContent(t *testing.T) {
svc, _ := newTestMessageService()