Tests for transaction ID semantics (matrix-org#622)

* Test for transaction ID semantics * Update internal/client/client.go Co-authored-by: kegsay <kegsay@gmail.com> * Incorporate review feedback * Refactor for legibility * Clarify that conduit doesn't pass idempotency tests * Apply suggestions from code review Co-authored-by: David Robertson <david.m.robertson1@gmail.com> * Incorporate review feedback --------- Co-authored-by: kegsay <kegsay@gmail.com> Co-authored-by: David Robertson <david.m.robertson1@gmail.com>
sandhose · Apr 18, 2023 · added55 · added55
1 parent bcc2bce
commit added55
Show file tree

Hide file tree

Showing 2 changed files with 204 additions and 9 deletions.
diff --git a/internal/client/client.go b/internal/client/client.go
@@ -266,7 +266,15 @@ func (c *CSAPI) SetPushRule(t *testing.T, scope string, kind string, ruleID stri
 func (c *CSAPI) SendEventUnsynced(t *testing.T, roomID string, e b.Event) string {
 	t.Helper()
 	txnID := int(atomic.AddInt64(&c.txnID, 1))
-	paths := []string{"_matrix", "client", "v3", "rooms", roomID, "send", e.Type, strconv.Itoa(txnID)}
+	return c.SendEventUnsyncedWithTxnID(t, roomID, e, strconv.Itoa(txnID))
+}
+
+// SendEventUnsyncedWithTxnID sends `e` into the room with a prescribed transaction ID.
+// This is useful for writing tests that interrogate transaction semantics.
+// Returns the event ID of the sent event.
+func (c *CSAPI) SendEventUnsyncedWithTxnID(t *testing.T, roomID string, e b.Event, txnID string) string {
+	t.Helper()
+	paths := []string{"_matrix", "client", "v3", "rooms", roomID, "send", e.Type, txnID}
 	if e.StateKey != nil {
 		paths = []string{"_matrix", "client", "v3", "rooms", roomID, "state", e.Type, *e.StateKey}
 	}
@@ -414,8 +422,16 @@ func (c *CSAPI) MustSyncUntil(t *testing.T, syncReq SyncReq, checks ...SyncCheck
 	}
 }
 
+type LoginOpt func(map[string]interface{})
+
+func WithDeviceID(deviceID string) LoginOpt {
+    return func(loginBody map[string]interface{}) {
+        loginBody["device_id"] = deviceID
+    }
+}
+
 // LoginUser will log in to a homeserver and create a new device on an existing user.
-func (c *CSAPI) LoginUser(t *testing.T, localpart, password string) (userID, accessToken, deviceID string) {
+func (c *CSAPI) LoginUser(t *testing.T, localpart, password string, opts ...LoginOpt) (userID, accessToken, deviceID string) {
 	t.Helper()
 	reqBody := map[string]interface{}{
 		"identifier": map[string]interface{}{
@@ -425,6 +441,11 @@ func (c *CSAPI) LoginUser(t *testing.T, localpart, password string) (userID, acc
 		"password": password,
 		"type":     "m.login.password",
 	}
+
+	for _, opt := range opts {
+		opt(reqBody)
+	}
+
 	res := c.MustDoFunc(t, "POST", []string{"_matrix", "client", "v3", "login"}, WithJSONBody(t, reqBody))
 
 	body, err := ioutil.ReadAll(res.Body)
@@ -438,8 +459,8 @@ func (c *CSAPI) LoginUser(t *testing.T, localpart, password string) (userID, acc
 	return userID, accessToken, deviceID
 }
 
-//RegisterUser will register the user with given parameters and
-// return user ID & access token, and fail the test on network error
+// RegisterUser will register the user with given parameters and
+// return user ID, access token and device ID. It fails the test on network error.
 func (c *CSAPI) RegisterUser(t *testing.T, localpart, password string) (userID, accessToken, deviceID string) {
 	t.Helper()
 	reqBody := map[string]interface{}{

diff --git a/tests/csapi/txnid_test.go b/tests/csapi/txnid_test.go
@@ -1,11 +1,14 @@
 package csapi_tests
 
 import (
+	"fmt"
+	"testing"
+
 	"github.com/matrix-org/complement/internal/b"
 	"github.com/matrix-org/complement/internal/client"
+	"github.com/matrix-org/complement/internal/must"
 	"github.com/matrix-org/complement/runtime"
 	"github.com/tidwall/gjson"
-	"testing"
 )
 
 // TestTxnInEvent checks that the transaction ID is present when getting the event from the /rooms/{roomID}/event/{eventID} endpoint.
@@ -22,20 +25,191 @@ func TestTxnInEvent(t *testing.T) {
 	// Create a room where we can send events.
 	roomID := c.CreateRoom(t, map[string]interface{}{})
 
+	txnId := "abcdefg"
 	// Let's send an event, and wait for it to appear in the timeline.
-	eventID := c.SendEventSynced(t, roomID, b.Event{
+	eventID := c.SendEventUnsyncedWithTxnID(t, roomID, b.Event{
 		Type: "m.room.message",
 		Content: map[string]interface{}{
 			"msgtype": "m.text",
 			"body":    "first",
 		},
-	})
+	}, txnId)
 
 	// The transaction ID should be present on the GET /rooms/{roomID}/event/{eventID} response.
 	res := c.MustDoFunc(t, "GET", []string{"_matrix", "client", "v3", "rooms", roomID, "event", eventID})
 	body := client.ParseJSON(t, res)
 	result := gjson.ParseBytes(body)
-	if !result.Get("unsigned.transaction_id").Exists() {
-		t.Fatalf("Event did not have a 'transaction_id' on the GET /rooms/%s/event/%s response", roomID, eventID)
+	unsignedTxnId := result.Get("unsigned.transaction_id")
+	if !unsignedTxnId.Exists() {
+		t.Fatalf("Event did not have a 'unsigned.transaction_id' on the GET /rooms/%s/event/%s response", roomID, eventID)
+	}
+
+	must.EqualStr(t, unsignedTxnId.Str, txnId, fmt.Sprintf("Event had an incorrect 'unsigned.transaction_id' on GET /rooms/%s/event/%s response", eventID, roomID))
+}
+
+
+func mustHaveTransactionIDForEvent(t *testing.T, roomID, eventID, expectedTxnId string) client.SyncCheckOpt {
+	return client.SyncTimelineHas(roomID, func(r gjson.Result) bool {
+		if r.Get("event_id").Str == eventID {
+			unsignedTxnId := r.Get("unsigned.transaction_id")
+			if !unsignedTxnId.Exists() {
+				t.Fatalf("Event %s in room %s should have a 'unsigned.transaction_id', but it did not", eventID, roomID)
+			}
+
+			must.EqualStr(t, unsignedTxnId.Str, expectedTxnId, fmt.Sprintf("Event %s in room %s had an incorrect 'unsigned.transaction_id'", eventID, roomID))
+
+			return true
+		}
+
+		return false
+	})
+}
+
+func mustNotHaveTransactionIDForEvent(t *testing.T, roomID, eventID string) client.SyncCheckOpt {
+	return client.SyncTimelineHas(roomID, func(r gjson.Result) bool {
+		if r.Get("event_id").Str == eventID {
+			unsignedTxnId := r.Get("unsigned.transaction_id")
+			if unsignedTxnId.Exists() {
+				t.Fatalf("Event %s in room %s should NOT have a 'unsigned.transaction_id', but it did (%s)", eventID, roomID, unsignedTxnId.Str)
+			}
+
+			return true
+		}
+
+		return false
+	})
+}
+
+// TestTxnScopeOnLocalEcho tests that transaction IDs in the sync response are scoped to the "client session", not the device
+func TestTxnScopeOnLocalEcho(t *testing.T) {
+	// Conduit scope transaction IDs to the device ID, not the access token.
+	runtime.SkipIf(t, runtime.Conduit)
+
+	deployment := Deploy(t, b.BlueprintCleanHS)
+	defer deployment.Destroy(t)
+
+	deployment.RegisterUser(t, "hs1", "alice", "password", false)
+
+	// Create a first client, which allocates a device ID.
+	c1 := deployment.Client(t, "hs1", "")
+	c1.UserID, c1.AccessToken, c1.DeviceID = c1.LoginUser(t, "alice", "password")
+
+	// Create a room where we can send events.
+	roomID := c1.CreateRoom(t, map[string]interface{}{})
+
+	txnId := "abdefgh"
+	// Let's send an event, and wait for it to appear in the timeline.
+	eventID := c1.SendEventUnsyncedWithTxnID(t, roomID, b.Event{
+		Type: "m.room.message",
+		Content: map[string]interface{}{
+			"msgtype": "m.text",
+			"body":    "first",
+		},
+	}, txnId)
+
+	// When syncing, we should find the event and it should have a transaction ID on the first client.
+	c1.MustSyncUntil(t, client.SyncReq{}, mustHaveTransactionIDForEvent(t, roomID, eventID, txnId))
+
+	// Create a second client, inheriting the first device ID.
+	c2 := deployment.Client(t, "hs1", "")
+	c2.UserID, c2.AccessToken, c2.DeviceID = c2.LoginUser(t, "alice", "password", client.WithDeviceID(c1.DeviceID))
+	must.EqualStr(t, c1.DeviceID, c2.DeviceID, "Device ID should be the same")
+
+	// When syncing, we should find the event and it should *not* have a transaction ID on the second client.
+	c2.MustSyncUntil(t, client.SyncReq{}, mustNotHaveTransactionIDForEvent(t, roomID, eventID))
+}
+
+// TestTxnIdempotencyScopedToClientSession tests that transaction IDs are scoped to a "client session"
+// and behave as expected across multiple clients even if they use the same device ID
+func TestTxnIdempotencyScopedToClientSession(t *testing.T) {
+	// Conduit scope transaction IDs to the device ID, not the client session.
+	runtime.SkipIf(t, runtime.Conduit)
+
+	deployment := Deploy(t, b.BlueprintCleanHS)
+	defer deployment.Destroy(t)
+
+	deployment.RegisterUser(t, "hs1", "alice", "password", false)
+
+	// Create a first client, which allocates a device ID.
+	c1 := deployment.Client(t, "hs1", "")
+	c1.UserID, c1.AccessToken, c1.DeviceID = c1.LoginUser(t, "alice", "password")
+
+	// Create a room where we can send events.
+	roomID := c1.CreateRoom(t, map[string]interface{}{})
+
+	txnId := "abcdef"
+	event := b.Event{
+		Type: "m.room.message",
+		Content: map[string]interface{}{
+			"msgtype": "m.text",
+			"body":    "foo",
+		},
 	}
+	// send an event with set txnId
+	eventID1 := c1.SendEventUnsyncedWithTxnID(t, roomID, event, txnId)
+
+	// Create a second client, inheriting the first device ID.
+	c2 := deployment.Client(t, "hs1", "")
+	c2.UserID, c2.AccessToken, c2.DeviceID = c2.LoginUser(t, "alice", "password", client.WithDeviceID(c1.DeviceID))
+	must.EqualStr(t, c1.DeviceID, c2.DeviceID, "Device ID should be the same")
+
+	// send another event with the same txnId via the second client
+	eventID2 := c2.SendEventUnsyncedWithTxnID(t, roomID, event, txnId)
+
+	// the two events should have different event IDs as they came from different clients
+	must.NotEqualStr(t, eventID2, eventID1, "Expected eventID1 and eventID2 to be different from two clients sharing the same device ID")
+}
+
+// TestTxnIdempotency tests that PUT requests idempotency follows required semantics
+func TestTxnIdempotency(t *testing.T) {
+	// Conduit appears to be tracking transaction IDs individually rather than combined with the request URI/room ID
+	runtime.SkipIf(t, runtime.Conduit)
+
+	deployment := Deploy(t, b.BlueprintCleanHS)
+	defer deployment.Destroy(t)
+
+	deployment.RegisterUser(t, "hs1", "alice", "password", false)
+
+	// Create a first client, which allocates a device ID.
+	c1 := deployment.Client(t, "hs1", "")
+	c1.UserID, c1.AccessToken, c1.DeviceID = c1.LoginUser(t, "alice", "password")
+
+	// Create a room where we can send events.
+	roomID1 := c1.CreateRoom(t, map[string]interface{}{})
+	roomID2 := c1.CreateRoom(t, map[string]interface{}{})
+
+	// choose a transaction ID
+	txnId := "abc"
+	event1 := b.Event{
+		Type: "m.room.message",
+		Content: map[string]interface{}{
+			"msgtype": "m.text",
+			"body":    "first",
+		},
+	}
+	event2 := b.Event{
+		Type: "m.room.message",
+		Content: map[string]interface{}{
+			"msgtype": "m.text",
+			"body":    "second",
+		},
+	}
+
+	// we send the event and get an event ID back
+	eventID1 := c1.SendEventUnsyncedWithTxnID(t, roomID1, event1, txnId)
+
+	// we send the identical event again and should get back the same event ID
+	eventID2 := c1.SendEventUnsyncedWithTxnID(t, roomID1, event1, txnId)
+
+	must.EqualStr(t, eventID2, eventID1, "Expected eventID1 and eventID2 to be the same, but they were not")
+
+	// even if we change the content we should still get back the same event ID as transaction ID is the same
+	eventID3 := c1.SendEventUnsyncedWithTxnID(t, roomID1, event2, txnId)
+
+	must.EqualStr(t, eventID3, eventID1, "Expected eventID3 and eventID2 to be the same even with different content, but they were not")
+
+	// if we change the room ID we should be able to use the same transaction ID
+	eventID4 := c1.SendEventUnsyncedWithTxnID(t, roomID2, event1, txnId)
+
+	must.NotEqualStr(t, eventID4, eventID3, "Expected eventID4 and eventID3 to be different, but they were not")
 }