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

Missing transaction_id in syncs for remote-echo of messages for guests #8521

Closed
t3chguy opened this issue Oct 12, 2020 · 7 comments · Fixed by #15174
Closed

Missing transaction_id in syncs for remote-echo of messages for guests #8521

t3chguy opened this issue Oct 12, 2020 · 7 comments · Fixed by #15174
Labels
A-Guests O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@t3chguy
Copy link
Member

t3chguy commented Oct 12, 2020

See element-hq/element-web#13706 for details

@clokep
Copy link
Member

clokep commented Oct 13, 2020

I've unfortunately been unable to reproduce this. I've been unable to get Element Web into a room as a guest. Can you provide more specific steps to reproduce?

@t3chguy
Copy link
Member Author

t3chguy commented Oct 13, 2020

It is only possible if Synapse joins them into that room, e.g using auto_join

@clokep
Copy link
Member

clokep commented Oct 13, 2020

I was able to confirm this:

  1. Configure synapse to allow guests and to auto-join rooms (not sure this is all necessary), this uses the room #public:localhost:8480, which I created with a separate user:
allow_guest_access: true
auto_join_rooms:
  - "#public:localhost:8480"
auto_join_rooms_for_guests: true
  1. Setup an Element Web that points to localhost:8480 by default

    1. Download the latest Element Web release
    2. Unpack it.
    3. cp config.sample.json config.json
    4. Modify the default_server_config key
  2. Serve Element Web locally: python -m http.server from the directory you unpacked Element Web in

  3. Go to http://localhost:8000/#/room/#public:localhost:8480

  4. Type something and you'll see copies of your own messages stuck at the bottom.

@clokep clokep added z-bug (Deprecated Label) z-p2 (Deprecated Label) z-p3 (Deprecated Label) and removed info-needed z-p2 (Deprecated Label) labels Oct 13, 2020
@clokep
Copy link
Member

clokep commented Oct 15, 2020

This is a bit of an edge-case and it is unclear how much impact this is having on end-users. At the moment we're not going to prioritize this, but if anyone is keen to investigate further they can join #synapse-dev:matrix.org for help! On the surface it seems to be something with the serialize_event method.

@grinapo
Copy link

grinapo commented Oct 26, 2020

I've unfortunately been unable to reproduce this. I've been unable to get Element Web into a room as a guest. Can you provide more specific steps to reproduce?

See element-hq/element-web#15567 and try the link there.

@grinapo
Copy link

grinapo commented Oct 26, 2020

This is a bit of an edge-case and it is unclear how much impact this is having on end-users.

Basically it affects everyone coming to a specific matrix room from a click on a webpage, as a guest user, to simply chat, without even being aware that it's "something called matrix". As it is now it makes plainly impossible to use since own messages accumulate at the bottom and new messages appear from the middle to top, possibly without even noticed and the user simply gives up and leaves.

@erikjohnston
Copy link
Member

This looks to be because guests use macaroons, which don't have an associated token_id:

synapse/synapse/api/auth.py

Lines 357 to 364 in 34a5696

ret = {
"user": user,
"is_guest": True,
"shadow_banned": False,
"token_id": None,
# all guests get the same device id
"device_id": GUEST_DEVICE_ID,
}

So when we serialize the event we don't include the transaction ID:

if token_id is not None:
if token_id == getattr(e.internal_metadata, "token_id", None):
txn_id = getattr(e.internal_metadata, "txn_id", None)
if txn_id is not None:
d["unsigned"]["transaction_id"] = txn_id

To fix this we may want to pass in both a user_id and token_id and changing the guard to be if user_id is not None: .... I think only a couple of places call serialize_event while specifying a token_id so it shouldn't be too invasive

@DMRobertson DMRobertson added T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. S-Major Major functionality / product severely impaired, no satisfactory workaround. O-Uncommon Most users are unlikely to come across this or unexpected workflow and removed z-bug (Deprecated Label) z-p3 (Deprecated Label) labels Jan 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Guests O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants