Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify that the event field of the send_join is only required when performing a restricted join #1834

Merged

Conversation

Kladki
Copy link
Contributor

@Kladki Kladki commented May 31, 2024

Fixes #1708

Pull Request Checklist

Preview: https://pr1834--matrix-spec-previews.netlify.app

…erforming a restricted join

Signed-off-by: Matthias Ahouansou <matthias@ahouansou.cz>
@Kladki Kladki requested a review from a team as a code owner May 31, 2024 12:38
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me given the discussion in #1708.

Thanks for bringing this one over the line!

@anoadragon453 anoadragon453 merged commit 7ff785f into matrix-org:main Jun 3, 2024
12 checks passed
@Kladki Kladki deleted the clarify-send-join-restricted-resp branch June 3, 2024 13:27
@clokep
Copy link
Contributor

clokep commented Jun 3, 2024

This looks correct to me given the discussion in #1708.

I'm unsure it is. There's two definitions at play:

  1. The issue says that the event signature is required only for a restricted join.
  2. This PR says that the event itself is returned only for a restricted join.

I think the intention was 1 to keep the API shape easier for implementations.

@Kladki
Copy link
Contributor Author

Kladki commented Jun 3, 2024 via email

@clokep
Copy link
Contributor

clokep commented Jun 4, 2024

What is the usecase of the former?

As I mentioned above, to keep the API shape simpler -- the joining server doesn't need to worry about why it was allowed to join the room, it just grabs the event from the response and it will be the exact event sent into the room.

This might be of dubious value, but I believe it was the original design.

Comment on lines +210 to +211
Required if the `content` of the event in the request contained the `join_authorised_via_users_server`
field. The signed copy of the membership event sent to other servers by the resident server,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I've just written over at #1708: this doesn't seem to match Synapse's behaviour. AFAICT, the presence of join_authorised_via_users_server is not sufficient for Synapse to return a (signed) event here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Synapse return the event if join_authorised_via_users_server is present and the room version supports restricted joins? Or what is it's behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The synapse code is at https://github.com/element-hq/synapse/blob/v1.108.0/synapse/federation/federation_server.py#L952-L956.

Currently, it returns the event for all requests to /send_join. But it only signs the event if the user is actually performing a restricted join.

@richvdh
Copy link
Member

richvdh commented Jun 5, 2024

This PR is updated by #1840

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify that join events only need to be signed by resident servers if using join_authorised_via_users_server
4 participants