Update outdated spec and do not restrict on 3PID medium #1039
Conversation
|
Can one of the admins verify this patch? |
|
@matrixbot: test this please |
There was a problem hiding this comment.
Also don't forget to do signoff: https://github.com/matrix-org/matrix-doc/blob/master/CONTRIBUTING.rst
| ``msisdn`` PSTN Phone numbers | ||
| ========== ================== | ||
|
|
||
| This list is not exhaustive and arbitrary values can be used throughout the protocol. |
There was a problem hiding this comment.
But what would it mean to use a value other than the ones above? How would the IS know how to validate it? I think this should be more like, "More identifiers may be added in future versions of this spec."
There was a problem hiding this comment.
But what would it mean to use a value other than the ones above?
My intention is to have an open list which is not restricted by values listed in the spec for the following reasons:
- 3PID can represent identifiers on any network, of any kind, including those who do not know about. Trying to write up an exhaustive list will always cause grief as it will make clients/servers invalids when using custom medium types
- The definition of remote/foreign network identifier might not suit everyone's definition, again causing grief
- There might be closed source or secret networks we'll never hear about on purpose. That doesn't mean they must be invalid in Matrix because we're not aware of them.
- The Identity server might not need to validate a 3PID itself, it might be connected to an authoritative source (LDAP, Database) and the validation done out-of-band. So it's only a matter of storing a key-value pair.
So it would mean it's implementation depend. If the server supports it (or only need to store it).
How would the IS know how to validate it?
That would depend on the implementation. Either it does, or it doesn't and returns a specific message.
I think this should be more like, "More identifiers may be added in future versions of this spec."
I disagree. This sounds like the list is exhaustive or that other values need to be validated before being used.
My intend is to be very specific on the arbitrary values part.
There was a problem hiding this comment.
OK - all valid points. How about we namespace it then? Anything without a '.' character is reserved, as in anything starting m., otherwise use reverse DNS style like event types? I think that would cater for all of the above whilst still having a core of mediums whose semantics are well defined by the spec and that all clients and servers can implement.
That said, this is becoming a material change to the spec which would normally go through discussion, although perhaps we can just point people here and see if folks agree.
There was a problem hiding this comment.
As you point out, namespacing would be a material/functional change which I wanted to avoid.
We already have email and msisdn in implementations and those would break quite badly, Databases would need to be updated, etc.
This PR approach doesn't break anything, clarifies on msisdn and allows other future types to exists - no downsides.
If we want to namespace, it would involve complex changes. That would be best suited for a new approach in the IS altogether, don't you think?
There was a problem hiding this comment.
My suggestion was that we do it in a backwards compatible way, allowing for us to transition from email to m.email in the future but leaving the current email still valid.
I think both allowing arbitrary values is still a material change to the spec since it rules out namespacing or anything else like this down the line.
There was a problem hiding this comment.
There are a couple of things I don't understand in the alternative text above, but I don't want to make this discussion even longer. I really think at this point we should limit the scope of this PR to just adding msisdn to the list. You're right - sydent is absolutely designed to handle arbitrary mediums and I'm all for allowing other mediums, but it I do think it warrants its own discussion so that people who don't watch the matrix-doc PRs can take part.
There was a problem hiding this comment.
Apologies, the following is a bit off-topic for this PR specifically.
@dbkr where should proposals from the community like this go? The matrix
core team has their google docs folders where y'all keep this kind of
proposal, but the community can't really contribute to that. Where can we
put spec proposals from the community so that they have visibility and are
collected in a single place?
If there isn't such a place yet, github PRs against matrix-doc seems like as
good of a place as any.
CONTRIBUTING.rst says the following:
Create a discussion document outlining the proposed change. The document should include details such as the HTTP endpoint being changed (or the suggested URL for a new endpoint), any new or changed parameters and response fields, and generally as much detail about edge-cases and error handling as is practical at this stage.
The Matrix Core Team's preferred tool for such discussion documents is Google Docs thanks to its support for comment threads. Works in progress are kept in the Matrix Design drafts folder.
so to follow this, would a PR against the drafts folder be enough? Just
creating a single google doc that isn't part of any broader structure (like the
folder the matrix core team has) has very low discoverability.
There was a problem hiding this comment.
CONTRIBUTING.rst spells it out pretty clearly: rather than sprawling PR discussions like this one, the intention is to work through drafts via Google Docs. Github's code review simply doesn't cut it for this, and in practice Google Docs's threaded comment system works really well. The point is that anyone can use Google Docs - it's categorically not just for the core team, and we'll happily add drafts from the community into the shared folders there. Please let's not go back to PRing against matrix-doc/drafts.
There was a problem hiding this comment.
rather than sprawling PR discussions like this one, the intention is to work through drafts via Google Docs
@ara4n just to clarify the point: this PR is about documenting existing behavior of sydent. This PR does not include changes to the current protocol. It follow verbatim the current code. I'll be very happy to work in Google Docs for the next iteration, taking into account what Dave mentioned. This PR was meant to help others in the community understand what is currently happening, and give documented context for the draft I wanted to write.
Please let me know what can I do better so PRs documenting the current behavior of implementations are accepted?
There was a problem hiding this comment.
It does propose a change to the current protocol - the concept of the identifier being namespaced rather than an arbitrary string literal (or one of a set). I agree that a googledoc proposal for this is probably overkill though. I'd suggest just accepting(!!!) @dbkr's review (the suggestion of "More identifiers may be added in future versions of this spec.") to unblock the PR, and then quickly opening a separate spec issue or PR to propose changing the format to be namespaced. This then unblocks the rest of the perfectly valid stuff in this PR, and gives a chance to discuss the namespacing change properly elsewhere rather than buried in here. @non-Jedi, hopefully this answers your question too.
Sorry for missing this - I was travelling at the end of last week. I am unclear on you keep closing the PR rather than actually accept the review and split out the contentious item. The idiom for this in English is 'throwing the baby out with the bathwater...'
|
|
||
| | In case a server is not capable of handing a request due to a 3PID medium type, like | ||
| sending a notification, it should return a 501 HTTP Status code (Not Implemented). | ||
| | The status code can be overwritten at the endpoint specification level. |
There was a problem hiding this comment.
We generally use error codes like this: https://matrix.org/docs/spec/client_server/r0.2.0.html#api-standards
although since this part of the spec is just defining what mediums a client could use, I would be tempted to keep this section more general with something like, "Clients should not offer to validate address types they do not recognise. Servers should refuse to validate mediums they do not recognise, although may return associations with mediums they do not recognise"? I'd then give more specific error codes when talking about the actual API endpoints.
There was a problem hiding this comment.
We generally use error codes like this
This does not prevent that. Happy to write a specific error code for this case as well if you want me to?
Clients should not offer to validate address types they do not recognise
I do not agree with that statement for the same reasons I disagree with having a closed list. There could be anything there. My believe is that it's not up to the client to decide what is acceptable or not.
My long term vision is to have an endpoint to list supported 3PID medium types in the HS and the IS, but that's too much for this PR.
This PR is about making the list of medium types open, as it was limited to "email" before.
Servers should refuse to validate mediums they do not recognise.
I'd then give more specific error codes when talking about the actual API endpoints.
This would cover all the endpoints which are currently not in the spec but implemented, or any future endpoint like so. If you don't feel this is appropriate, I'm happy to remove it, but then we'll have undefined behavior for undocumented endpoints.
There was a problem hiding this comment.
OK - mostly I was worried about making this section specific to HTTP when it's not actually speccing an HTTP API and would apply equally to, say, a websockets API.
| 3PID medium types are identifiers refering to other networks can connect to. | ||
| They are always lowercase. | ||
|
|
||
| The following list are the official identifiers for most used networks: |
There was a problem hiding this comment.
See comment below, but I would expect this to be telling me what mediums this versions of the spec is defining the behaviour of.
There was a problem hiding this comment.
See my other comments for why the spec should not be a closed list.
| @@ -291,3 +294,5 @@ It will look up ``token`` which was stored in a call to ``store-invite``, and fe | |||
| } | |||
|
|
|||
| .. _`Unpadded Base64`: ../appendices.html#unpadded-base64 | |||
| .. _`Appendices`: ../appendices.html#pid-medium-types | |||
There was a problem hiding this comment.
are you missing a '3' in the link here?
There was a problem hiding this comment.
No this is the correct link. The documentation generator does not put a 3 there for some reason.
There was a problem hiding this comment.
Fair enough, I thought it might be but wanted to check.
|
Reopening to consider sydent's current behaviour within the discussion |
|
Can one of the admins verify this patch? |
Adds the /msisdn' 3pid type and generally fleshes out what a 3pid is and how they work. This merges most of the work from Max Dor in #1039 with some tweaks and additions.
Following discussion with Dave
Signed-off-by: Max Dor @ Kamax.io (no email to avoid spam)