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

spec is missing documentation of /_matrix/federation/v2/send_{join,leave} #2541

Closed
turt2live opened this issue May 13, 2020 · 9 comments · Fixed by #2547
Closed

spec is missing documentation of /_matrix/federation/v2/send_{join,leave} #2541

turt2live opened this issue May 13, 2020 · 9 comments · Fixed by #2547
Assignees
Labels
s2s Server-to-Server API (federation) spec-omission implemented but not currently specified

Comments

@turt2live
Copy link
Member

This is due to a legacy bug in Synapse from before the federation spec was a thing. It's correctly specified in the spec, but we need a /v2 endpoint of some things to remove the thing - similar to the invite endpoint.

This issue primarily exists as a place to point people who notice the weirdness.

@turt2live turt2live added wart A point where the protocol is inconsistent or inelegant s2s Server-to-Server API (federation) labels May 13, 2020
@aaronraimist
Copy link
Contributor

Wasn't that #1418?

@turt2live turt2live self-assigned this May 14, 2020
@turt2live
Copy link
Member Author

indeed, I knew I wasn't crazy in thinking this was fixed. Looks like it needs a spec PR - I'll keep this open as a personal reminder to write that spec before we cut a release (as clearly it being a merged PR with a label isn't good enough).

@richvdh
Copy link
Member

richvdh commented May 14, 2020

as clearly it being a merged PR with a label isn't good enough

if that's true, isn't it also true of other MSCs under spec-pr-missing ?

@iinuwa
Copy link
Contributor

iinuwa commented May 14, 2020

I think I'm a little confused on this issue. So this is a spec bug and not actually implemented in Synapse/Construct? So I should use just res instead of [200, res] as the response body in the v1 endpoint for our server library?

@richvdh
Copy link
Member

richvdh commented May 14, 2020

the v1 endpoints return [200, res]. There also exist v2 versions of these endpoints which just return res: v2/invite has existed since s2s spec r0.1.0, and MSC1802 proposes the introduction of v2/send_join and v2/send_leave, though these have not yet made it into a released spec (though this is now just a formality).

Recent versions of synapse implement all three v2 endpoints, as well as the v1 versions for compatibility with older implementations. For outbound requests, Synapse will first try the v2 endpoints and then fall back to the v1 versions for compatibility with older servers.

I can't speak to what construct does here.

To be spec-compliant, you should follow Synapse's behaviour.

If this sounds like a mess: yes, fixing bugs in federated protocols is hard and takes ages :/.

@richvdh richvdh removed the wart A point where the protocol is inconsistent or inelegant label May 14, 2020
@richvdh richvdh changed the title Some federation endpoints have an ugly [200, {}] response spec is missing documentation of /_matrix/federation/v2/send_{join,leave} May 14, 2020
@richvdh richvdh added the spec-omission implemented but not currently specified label May 14, 2020
@turt2live
Copy link
Member Author

as clearly it being a merged PR with a label isn't good enough

if that's true, isn't it also true of other MSCs under spec-pr-missing ?

Almost certainly. Usually I try to keep an eye on that label but this particular MSC missed the queue somehow.

turt2live added a commit that referenced this issue May 15, 2020
MSC: #1802

Fixes #2541

This also adds the v2 invite endpoint to the ACL protected list as that appears to be an omission.
@iinuwa
Copy link
Contributor

iinuwa commented May 25, 2020

Does the same thing apply for the send transaction endpoint?

@turt2live
Copy link
Member Author

That would be #2236

@iinuwa
Copy link
Contributor

iinuwa commented May 25, 2020

Ah, thank you. I didn't find it in the other events listed in the MSCs linked here, but a simple search probably would have worked. Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s2s Server-to-Server API (federation) spec-omission implemented but not currently specified
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants