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

MSC2061: make the trailing slash on GET /_matrix/key/v2/server/ optional #2061

Closed

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented May 31, 2019

I think there's a strong argument that this could be called a spec clarification, but let's have an MSC for it to be on the safe side.

Rendered: https://github.com/matrix-org/matrix-doc/blob/rav/proposal/no_trailing_slash_on_key_request/proposals/2061-drop-trailing-slash-from-key-request.md

@richvdh richvdh added the proposal A matrix spec change proposal label May 31, 2019
@richvdh
Copy link
Member Author

richvdh commented May 31, 2019

Oh, this needs extending to cover https://matrix.org/docs/spec/server_server/r0.1.1.html#get-matrix-key-v2-query-servername-keyid, following the same logic.

@richvdh
Copy link
Member Author

richvdh commented May 31, 2019

This gets more complicated:

  • The specced behaviour for GET /_matrix/key/v2/query/{serverName}/{keyId} (ignore the keyId) is different from that for the POST equivalent.

  • It's worth noting that synapse does (attempt to) filter the results for both /query endpoints by the given keyId(s) (modulo sometimes the key notary endpoint doesn't give back the keys you asked for synapse#5305), unlike the /server version.

  • For the /query endpoint, the use of keyId is quite important, since it is the only clue to the notary server that its list of keys for a given server may be incomplete.

I therefore think that the deprecation of the keyId param for GET /_matrix/key/v2/query/{serverName}/{keyId} and the suggestion that the server ignore it was misguided.

However, in many ways, those concerns are orthogonal to the proposal here: I remain of the opinion that, as long as the keyId param is optional, so should the trailing slash be.

@turt2live
Copy link
Member

I honestly don't think this needs to be a MSC: we already don't specify what trailing slashes are required (outside of push rules), so the whole spec could do with a clarification.

@richvdh
Copy link
Member Author

richvdh commented May 31, 2019

I honestly don't think this needs to be a MSC: we already don't specify what trailing slashes are required (outside of push rules), so the whole spec could do with a clarification.

I'm very happy with that, especially if you'd like to take the lead on adding that clarification...

@turt2live
Copy link
Member

Will do. I'll leave this open as a reminder, but I think the best status for it is.. uhh... taking the proposal label off of it?

@turt2live turt2live self-assigned this May 31, 2019
@richvdh richvdh removed the proposal A matrix spec change proposal label May 31, 2019
turt2live added a commit that referenced this pull request Jun 6, 2019
See #2061
Fixes #1468
Fixes #1528

The section is not referenced by the specifications yet - they do a fairly good job of explaining it over and over. In future, it would be good to point all the references to the index.
@turt2live
Copy link
Member

closing in favour of #2097

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.

2 participants