Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Bind the event TXN ID to the device ID instead of the access token ID #13064

Closed
sandhose opened this issue Jun 15, 2022 · 2 comments · Fixed by #15318
Closed

Bind the event TXN ID to the device ID instead of the access token ID #13064

sandhose opened this issue Jun 15, 2022 · 2 comments · Fixed by #15318
Assignees
@sandhose
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@sandhose
Copy link
Member

When sending an event, the client sets a locally unique txnID on it, which serves two purposes:

  • deduplicating events in case of network failures/retries
  • when the client receives an event from /sync that they created, map it correctly to the locally-created event (for proper local echo)

The problem is, this txnID is currently bound to the user ID and the access token ID. Since MSC2918 (refresh tokens), a single client might deal with multiple access tokens, meaning that the current scenario is possible:

  • client starts a /sync with its current access token
  • this token is about to expire, so it refreshes it and gets a new access token
  • the client sends a new event, with a random txnID, using the new access token
  • /sync gets back, with the new event but not the txnID, since this /sync was done with another access token than when the event was created

I think the proper way to deal with this would be to have the txnIDs bound to devices instead of access tokens.

This is also relevant for the OIDC patches, since we don't really have access token IDs, but we do have the device ID.

What I would like to do is:

  • add a column to the event_txn_id to store the device ID
  • add the device_id field in the _EventInternalMetadata (and ensure we're persisting it when saving the txn IDs)
  • when looking up existing events, consider both the token_id and the device_id
  • release Synapse like that, so current transactions don't break
  • remove the token_id from event transactions (event_txn_id table, _EventInternalMetadata) everywhere, and do another release
@erikjohnston erikjohnston added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Jun 15, 2022
@sandhose
Copy link
Member Author

I've noticed two more places where we're using the access token/access token ID where we might be better off using the device ID:

Does it make sense to also change those?

This was referenced Jun 16, 2022
Open
Closed
Closed
@sandhose sandhose mentioned this issue Feb 15, 2023
Closed
sandhose added a commit to sandhose/complement that referenced this issue Feb 15, 2023
@sandhose
Test the scope of a transaction IDs
50b038d 
This adds two tests, which check the current spec behaviour of
transaction IDs, which are that they are scoped to a series of access
tokens, and not the device ID.

The first test highlight this behaviour, by logging in with refresh
token enabled, sending an event, using the refresh token and syncing
with the new access token. On the sync, the transaction ID should be
there, but currently in Synapse it is not.

The second test highlight that the transaction ID is not scoped to the
device ID, by logging in twice with the same device ID, sending an event
with the first access token, and syncing with the second access token.
In that case, the sync should not contain the transaction ID, but
I think it's the case in HS implementations which use the device ID to
scope the transaction IDs, like Conduit.

Related: matrix-org/matrix-spec#1133, matrix-org/matrix-spec#1236, matrix-org/synapse#13064 and matrix-org/synapse#13083
sandhose added a commit to sandhose/complement that referenced this issue Feb 15, 2023
@sandhose
Test the scope of a transaction IDs
9f153c0 
This adds two tests, which check the current spec behaviour of
transaction IDs, which are that they are scoped to a series of access
tokens, and not the device ID.

The first test highlight this behaviour, by logging in with refresh
token enabled, sending an event, using the refresh token and syncing
with the new access token. On the sync, the transaction ID should be
there, but currently in Synapse it is not.

The second test highlight that the transaction ID is not scoped to the
device ID, by logging in twice with the same device ID, sending an event
with the first access token, and syncing with the second access token.
In that case, the sync should not contain the transaction ID, but
I think it's the case in HS implementations which use the device ID to
scope the transaction IDs, like Conduit.

Related: matrix-org/matrix-spec#1133, matrix-org/matrix-spec#1236, matrix-org/synapse#13064 and matrix-org/synapse#13083
sandhose added a commit to sandhose/complement that referenced this issue Feb 16, 2023
@sandhose
Test the scope of a transaction IDs
d776cd1 
This adds two tests, which check the current spec behaviour of
transaction IDs, which are that they are scoped to a series of access
tokens, and not the device ID.

The first test highlight this behaviour, by logging in with refresh
token enabled, sending an event, using the refresh token and syncing
with the new access token. On the sync, the transaction ID should be
there, but currently in Synapse it is not.

The second test highlight that the transaction ID is not scoped to the
device ID, by logging in twice with the same device ID, sending an event
with the first access token, and syncing with the second access token.
In that case, the sync should not contain the transaction ID, but
I think it's the case in HS implementations which use the device ID to
scope the transaction IDs, like Conduit.

Related: matrix-org/matrix-spec#1133, matrix-org/matrix-spec#1236, matrix-org/synapse#13064 and matrix-org/synapse#13083
sandhose added a commit to sandhose/complement that referenced this issue Feb 16, 2023
@sandhose
Test the scope of a transaction IDs
56838e8 
This adds two tests, which check the current spec behaviour of
transaction IDs, which are that they are scoped to a series of access
tokens, and not the device ID.

The first test highlight this behaviour, by logging in with refresh
token enabled, sending an event, using the refresh token and syncing
with the new access token. On the sync, the transaction ID should be
there, but currently in Synapse it is not.

The second test highlight that the transaction ID is not scoped to the
device ID, by logging in twice with the same device ID, sending an event
with the first access token, and syncing with the second access token.
In that case, the sync should not contain the transaction ID, but
I think it's the case in HS implementations which use the device ID to
scope the transaction IDs, like Conduit.

Related: matrix-org/matrix-spec#1133, matrix-org/matrix-spec#1236, matrix-org/synapse#13064 and matrix-org/synapse#13083
This was referenced Feb 23, 2023
Closed
Merged
@hughns
Copy link
Member

hughns commented Feb 24, 2023 •
edited
Loading

MSC3970 now proposes changing the transaction ID such that the present issue becomes spec compliant.

This was referenced Feb 28, 2023
Merged
Merged
@sandhose sandhose mentioned this issue Mar 17, 2023
Merged
@sandhose sandhose mentioned this issue Mar 24, 2023
Merged
hughns pushed a commit to sandhose/complement that referenced this issue Apr 18, 2023
@sandhose @hughns
Test the scope of a transaction IDs
fa67145 
This adds two tests, which check the current spec behaviour of
transaction IDs, which are that they are scoped to a series of access
tokens, and not the device ID.

The first test highlight this behaviour, by logging in with refresh
token enabled, sending an event, using the refresh token and syncing
with the new access token. On the sync, the transaction ID should be
there, but currently in Synapse it is not.

The second test highlight that the transaction ID is not scoped to the
device ID, by logging in twice with the same device ID, sending an event
with the first access token, and syncing with the second access token.
In that case, the sync should not contain the transaction ID, but
I think it's the case in HS implementations which use the device ID to
scope the transaction IDs, like Conduit.

Related: matrix-org/matrix-spec#1133, matrix-org/matrix-spec#1236, matrix-org/synapse#13064 and matrix-org/synapse#13083
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@sandhose sandhose

Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Milestone
No milestone
3 participants
@sandhose @hughns @erikjohnston