diff --git a/api/comms/chat.go b/api/comms/chat.go index fe493782..4533cdf2 100644 --- a/api/comms/chat.go +++ b/api/comms/chat.go @@ -255,7 +255,7 @@ func updatePermissions(db dbv1.DBTX, ctx context.Context, userId int32, permit C insert into chat_permissions (user_id, permits, allowed, updated_at) values ($1, $2, $3, $4) on conflict (user_id, permits) - do update set allowed = $3 where chat_permissions.updated_at < $4 + do update set allowed = $3, updated_at = $4 where chat_permissions.updated_at < $4 `, userId, permit, permitAllowed, messageTimestamp.UTC()) return err } diff --git a/api/comms/chat_inbox_closed_test.go b/api/comms/chat_inbox_closed_test.go new file mode 100644 index 00000000..6c9251e6 --- /dev/null +++ b/api/comms/chat_inbox_closed_test.go @@ -0,0 +1,135 @@ +package comms + +import ( + "context" + "fmt" + "testing" + "time" + + "api.audius.co/database" + "api.audius.co/trashid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// An artist who blasts their followers and later closes their inbox must stop +// receiving new DMs from blast recipients. Threads that hold real messages keep +// working, and a fresh blast sent after closing re-opens replies to it. +func TestChatBlastThenCloseInbox(t *testing.T) { + t0 := time.Now().Add(time.Second * -100).UTC() + t1 := time.Now().Add(time.Second * -90).UTC() + t2 := time.Now().Add(time.Second * -80).UTC() + t3 := time.Now().Add(time.Second * -70).UTC() + t4 := time.Now().Add(time.Second * -60).UTC() + t5 := time.Now().Add(time.Second * -50).UTC() + + pool := database.CreateTestDatabase(t, "test_comms") + defer pool.Close() + ctx := context.Background() + + // user 1 is the artist. 201, 202, 203 follow them before the blast. + database.Seed(pool, database.FixtureMap{ + "users": { + {"user_id": 1, "wallet": "wallet1", "handle": "user1"}, + {"user_id": 201, "wallet": "wallet201", "handle": "user201"}, + {"user_id": 202, "wallet": "wallet202", "handle": "user202"}, + {"user_id": 203, "wallet": "wallet203", "handle": "user203"}, + }, + "follows": { + {"follower_user_id": 201, "followee_user_id": 1, "created_at": t0}, + {"follower_user_id": 202, "followee_user_id": 1, "created_at": t0}, + {"follower_user_id": 203, "followee_user_id": 1, "created_at": t0}, + }, + }) + validator := CreateTestValidator(t, pool, DefaultRateLimitConfig, DefaultTestValidatorConfig) + + chatAllowed := func(from, to int32) bool { + var ok bool + require.NoError(t, pool.QueryRow(ctx, `select chat_allowed($1, $2)`, from, to).Scan(&ok)) + return ok + } + assertMessageAllowed := func(sender int32, chatId string, shouldWork bool) { + rpc := RawRPC{ + Params: []byte(fmt.Sprintf(`{"chat_id": "%s", "message_id": "m-%s-%d", "message": "hi"}`, chatId, chatId, sender)), + } + err := validator.validateChatMessage(ctx, sender, rpc) + if shouldWork { + assert.NoError(t, err) + } else { + assert.ErrorContains(t, err, "Not permitted to send messages to this user") + } + } + upgrade := func(follower int32, ts time.Time) string { + chatId := trashid.ChatID(int(follower), 1) + err := chatCreate(pool, ctx, follower, ts, ChatCreateRPCParams{ + ChatID: chatId, + Invites: []PurpleInvite{ + {UserID: trashid.MustEncodeHashID(int(follower)), InviteCode: "x"}, + {UserID: trashid.MustEncodeHashID(1), InviteCode: "x"}, + }, + }) + require.NoError(t, err) + return chatId + } + + // artist blasts followers with an open inbox + _, err := chatBlast(pool, ctx, 1, t1, ChatBlastRPCParams{ + BlastID: "b_open", + Audience: FollowerAudience, + Message: "hello followers", + }) + require.NoError(t, err) + + // 202 opens the blast into a thread but never replies + chatId_202 := upgrade(202, t2) + + // 203 opens the blast into a thread and replies, so a real conversation exists + chatId_203 := upgrade(203, t2) + require.NoError(t, chatSendMessage(pool, ctx, 203, chatId_203, "reply_203", t2, "203 replying")) + + // while the inbox is open, everyone in the audience can reach the artist + assertChatCreateAllowed(t, ctx, validator, 201, 1, true) + assertMessageAllowed(202, chatId_202, true) + assertMessageAllowed(203, chatId_203, true) + + // artist closes their inbox + require.NoError(t, chatSetPermissions(pool, ctx, 1, ChatPermissionAll, []ChatPermission{ChatPermissionNone}, boolPtr(true), t3)) + + // 201 can no longer start a thread off the old blast + assertChatCreateAllowed(t, ctx, validator, 201, 1, false) + assert.False(t, chatAllowed(201, 1)) + + // 202's thread holds nothing but the blast seed, so it grants no reply rights + assertMessageAllowed(202, chatId_202, false) + assert.False(t, chatAllowed(202, 1)) + + // 203's thread is a real conversation and keeps working + assertMessageAllowed(203, chatId_203, true) + assert.True(t, chatAllowed(203, 1)) + + // a follower's own view of the seed-only thread asks the client to recheck permissions + { + chat, err := getUserChat(pool, ctx, chatMembershipParams{UserID: 202, ChatID: chatId_202}) + require.NoError(t, err) + assert.True(t, chat.LastMessageIsPlaintext) + } + + // artist blasts again while closed: recipients of the new blast may reply to it + _, err = chatBlast(pool, ctx, 1, t4, ChatBlastRPCParams{ + BlastID: "b_closed", + Audience: FollowerAudience, + Message: "hello again", + }) + require.NoError(t, err) + + assertChatCreateAllowed(t, ctx, validator, 201, 1, true) + assert.True(t, chatAllowed(202, 1), "new blast fanned into 202's thread re-opens replies") + + // closing the inbox once more after that blast shuts the door again + require.NoError(t, chatSetPermissions(pool, ctx, 1, ChatPermissionAll, []ChatPermission{ChatPermissionNone}, boolPtr(true), t5)) + assertChatCreateAllowed(t, ctx, validator, 201, 1, false) + assert.False(t, chatAllowed(202, 1)) + assert.True(t, chatAllowed(203, 1)) +} + +func boolPtr(b bool) *bool { return &b } diff --git a/api/comms/validator.go b/api/comms/validator.go index af8d5260..a0331cfa 100644 --- a/api/comms/validator.go +++ b/api/comms/validator.go @@ -576,6 +576,13 @@ func hasNewBlastFromUser(pool *dbv1.DBPools, ctx context.Context, userID int32, where blast.from_user_id = $2 and blast.created_at > (select t from last_permission_change) + -- the blaster's own inbox settings: a blast grants reply rights only + -- while it is newer than the blaster's most recent settings change + and blast.created_at > ( + select coalesce(max(updated_at), to_timestamp(0)) + from chat_permissions + where user_id = $2 + ) and chat_allowed(blast.from_user_id, $1) and not exists ( select 1 from chat_member cm diff --git a/api/dbv1/user_chat_row.go b/api/dbv1/user_chat_row.go index 5afe732b..8962cc5e 100644 --- a/api/dbv1/user_chat_row.go +++ b/api/dbv1/user_chat_row.go @@ -48,7 +48,10 @@ func (row UserChatRow) MarshalJSON() ([]byte, error) { clearedHistoryAt = row.ClearedHistoryAt.Time.UTC().Format(time.RFC3339Nano) } - recheckPermissions := false + // A thread whose latest message is a blast may hold nothing but blast + // seeds; whether the viewer may reply then depends on the other member's + // current inbox settings rather than on the thread existing. + recheckPermissions := row.LastMessageIsPlaintext for _, member := range row.ChatMembers { if member.ClearedHistoryAt.Valid && (row.LastMessageAt == nil || member.ClearedHistoryAt.Time.After(*row.LastMessageAt)) { recheckPermissions = true diff --git a/ddl/functions/chat_allowed.sql b/ddl/functions/chat_allowed.sql index 2164c4f9..40b01108 100644 --- a/ddl/functions/chat_allowed.sql +++ b/ddl/functions/chat_allowed.sql @@ -26,7 +26,10 @@ BEGIN RETURN TRUE; END IF; - -- existing chat takes priority over permissions + -- existing chat takes priority over permissions. + -- A blast message only counts if the blast is newer than to_user's most + -- recent inbox settings change: otherwise an artist who blasts and then + -- closes their inbox would still be reachable by every blast recipient. SELECT COUNT(*) > 0 INTO can_message FROM chat_member member_a JOIN chat_member member_b USING (chat_id) @@ -34,6 +37,14 @@ BEGIN WHERE member_a.user_id = from_user_id AND member_b.user_id = to_user_id AND (member_b.cleared_history_at IS NULL OR chat_message.created_at > member_b.cleared_history_at) + AND ( + chat_message.blast_id IS NULL + OR chat_message.created_at > ( + SELECT COALESCE(MAX(updated_at), to_timestamp(0)) + FROM chat_permissions + WHERE user_id = to_user_id + ) + ) ; IF can_message THEN diff --git a/sql/01_schema.sql b/sql/01_schema.sql index 93751524..9dbaa638 100644 --- a/sql/01_schema.sql +++ b/sql/01_schema.sql @@ -1203,7 +1203,10 @@ BEGIN RETURN TRUE; END IF; - -- existing chat takes priority over permissions + -- existing chat takes priority over permissions. + -- A blast message only counts if the blast is newer than to_user's most + -- recent inbox settings change: otherwise an artist who blasts and then + -- closes their inbox would still be reachable by every blast recipient. SELECT COUNT(*) > 0 INTO can_message FROM chat_member member_a JOIN chat_member member_b USING (chat_id) @@ -1211,6 +1214,14 @@ BEGIN WHERE member_a.user_id = from_user_id AND member_b.user_id = to_user_id AND (member_b.cleared_history_at IS NULL OR chat_message.created_at > member_b.cleared_history_at) + AND ( + chat_message.blast_id IS NULL + OR chat_message.created_at > ( + SELECT COALESCE(MAX(updated_at), to_timestamp(0)) + FROM chat_permissions + WHERE user_id = to_user_id + ) + ) ; IF can_message THEN