Test the scope of a transaction IDs#613
Closed
sandhose wants to merge 2 commits into
Closed
Conversation
sandhose
force-pushed
the
quenting/txnid-scope
branch
3 times, most recently
from
d776cd1 to
56838e8
Compare
clokep
reviewed
Feb 16, 2023
| runtime.SkipIf(t, runtime.Dendrite) | ||
| // Dendrite and Conduit don't support refresh tokens yet. | ||
| runtime.SkipIf(t, runtime.Dendrite, runtime.Conduit) | ||
| // XXX: Synapse's implementation is broken. |
Member
There was a problem hiding this comment.
Is the plan to get this working in Synapse? It seems odd to merge a test without one homeserver passing it.
Member
There was a problem hiding this comment.
I agree it looks odd and have done two things:
- Opened a specific issue to track the bug in Synapse
- Added further comments to clarify what is going on here
Member
|
I've added a further test that checks the idempotency of requests. |
Member
hughns
reviewed
Feb 28, 2023
| // TestTxnInEvent checks that the transaction ID is present when getting the event from the /rooms/{roomID}/event/{eventID} endpoint. | ||
| func TestTxnInEvent(t *testing.T) { | ||
| // Both Synapse and Dendrite implementations are broken | ||
| runtime.SkipIf(t, runtime.Synapse) |
Member
|
As per discussion in #synapse-dev:matrix.org I've pulled the definitely good tests out in to #622 for separate review. |
Member
|
Sorry, is this still requiring review? |
hughns
force-pushed
the
quenting/txnid-scope
branch
3 times, most recently
from
18a8c81 to
3049413
Compare
hughns
force-pushed
the
quenting/txnid-scope
branch
from
3049413 to
af92b2f
Compare
Member
|
@erikjohnston I've given this a severe rebase. It doesn't require review at the moment as the test does not pass for any implementation. Please can it be made draft? |
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
This test will fail in Synapse because the behaviour does not match the spec, and it will fail in Dendrite because it does not implement refreshable access tokens. Let me know if I should at it to their respective blacklist?
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