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.

Sign up for GitHub

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

Jump to bottom

Update outdated spec and do not restrict on 3PID medium #1039

Closed
wants to merge 2 commits into from
Closed

Update outdated spec and do not restrict on 3PID medium #1039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4de9039
Update outdated spec and do not restrict on 3PID medium
Oct 26, 2017
21d8ce0
Apply feedback from review
Oct 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions api/identity/lookup.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# Copyright 2016 OpenMarket Ltd
# Copyright 2017 Kamax.io
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -32,13 +33,13 @@ paths:
type: string
name: medium
required: true
description: The literal string "email".
description: The 3pid medium type. See `Appendices`_.
x-example: "email"
- in: query
type: string
name: address
required: true
description: The email address being looked up.
description: The 3pid address being looked up.
x-example: "louise@bobs.burgers"
responses:
200:
Expand Down Expand Up @@ -82,4 +83,4 @@ paths:
description: The unix timestamp at which the association was verified.
signatures:
type: object
description: The signatures of the verifying identity service which show that the association should be trusted, if you trust the verifying identity service.
description: The signatures of the verifying identity services which show that the association should be trusted, if you trust the verifying identity services.
34 changes: 34 additions & 0 deletions specification/appendices/3pid_mediums.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
.. Copyright 2017 Kamax.io
..
.. Licensed under the Apache License, Version 2.0 (the "License");
.. you may not use this file except in compliance with the License.
.. You may obtain a copy of the License at
..
.. http://www.apache.org/licenses/LICENSE-2.0
..
.. Unless required by applicable law or agreed to in writing, software
.. distributed under the License is distributed on an "AS IS" BASIS,
.. WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
.. See the License for the specific language governing permissions and
.. limitations under the License.

3PID Medium types
-----------------
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:
Copy link
Member

Choose a reason for hiding this comment

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

See comment below, but I would expect this to be telling me what mediums this versions of the spec is defining the behaviour of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my other comments for why the spec should not be a closed list.


========== ==================
Medium ID Network
========== ==================
``email`` E-mail
``msisdn`` PSTN Phone numbers
========== ==================

This list is not exhaustive and arbitrary values can be used throughout the protocol.
Copy link
Member

Choose a reason for hiding this comment

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

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@maxidorius maxidorius Nov 8, 2017 •
edited

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 the protocol equivalent of a
``Not Implemented`` status.
| This behaviour can be overwritten at the endpoint specification level.
5 changes: 5 additions & 0 deletions specification/identity_service_api.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
.. Copyright 2016 OpenMarket Ltd
.. Copyright 2017 Kamax.io
..
.. Licensed under the Apache License, Version 2.0 (the "License");
.. you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -52,6 +53,8 @@ necessarily provide evidence that they have validated associations, but claim to
have done so. Establishing the trustworthiness of an individual identity service
is left as an exercise for the client.

3PID Medium types are described in the `Appendices`_.

Privacy
-------

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

are you missing a '3' in the link 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.

No this is the correct link. The documentation generator does not put a 3 there for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, I thought it might be but wanted to check.


1 change: 1 addition & 0 deletions specification/targets.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ targets:
files:
- appendices.rst
- appendices/base64.rst
- appendices/3pid_mediums.rst
- appendices/signing_json.rst
- appendices/identifier_grammar.rst
- appendices/threat_model.rst
Expand Down