.well-known discovery #1359

uhoreg · 2018-07-03T19:07:23Z

Rendered version: see 'docs' commit status

There are a few (minor) differences between the proposal and the spec:

start off by saying that clients may do auto-discovery (this was never
discussed, AFAIK, but it seems reasonable to not require it, since some bot clients
will just want the HS to be configured)
- edit: this was changed to should
removed some explanatory text from some of the definitions, that I didn't think were necessary for the spec
add more explanation to IGNORE
in steps 1 and 2, use terms that are used by the s2s server discovery section
make step 3f more explicit

maxidorius

Not an accurate PR of the proposal it seems

maxidorius · 2018-07-03T19:09:38Z

api/client-server/definitions/wellknown/homeserver.yaml~

@@ -0,0 +1,23 @@
+# Copyright 2018 New Vector Ltd


(Putting here as you can't comment on a file per say) This seems like a staging file from your editor and shouldn't have been checked in?

maxidorius · 2018-07-03T19:12:32Z

api/client-server/wellknown.yaml

+      summary: Gets Matrix server discovery information about the domain.
+      description: |-
+        Gets discovery information about the domain.  The file may include
+        additional keys, which SHOULD follow the Java package naming convention,


The proposal makes Java package naming mandatory (MUST). Even tho MUST was not clearly stated, it was the intent in my original wording.

I copied that sentence from another section of the spec. I personally don't have a problem with changing it to a MUST, but am interested in what others think.

Given that this is kinda common-space, namespacing is essential and has to be uniform across applications. So yes, I agree it should be MUST.

I agree with the MUST, but I thought this was convention everywhere in Matrixland that involves keys, so I just find it a little odd that it's part of a description.

On a different note, is 'reverse DNS' a more widely used term for this naming scheme?

maxidorius · 2018-07-03T19:16:27Z

api/client-server/wellknown.yaml

+
+        **FIXME:** do we need to add a note that this endpoint is not
+        necessarily handled by the homeserver, but by another webserver?  Or
+        does the context make this clear enough?


It should be obvious enough, but it wouldn't hurt to be explicit about it. you might want to add that info at the top tho, not just in the GET section.

maxidorius · 2018-07-03T19:18:31Z

api/client-server/wellknown.yaml

+              }
+          schema:
+            type: object
+            properties:


Documentation of which one are required is missing.

It's directly below this :)

There is no indication at the OpenAPI level which of those two is required as far as I can see?

It's in the description, rather we describe what is optional. ftr I think we should be using the required field on these though.

ftr I think we should be using the required field on these though.

That's the OpenAPI level and is exactly what I'm saying :p

I know. Most of the spec uses one or the other, or both.

This was fixed by ce1e2c0#diff-ce8a14e8aa224add5c39f2ed29230014R62

maxidorius · 2018-07-03T19:21:42Z

specification/client_server_api.rst

+~~~~~~~~~~~~~~~~
+
+In order to allow users to connect to a Matrix server without needing to
+explicitly specify the homeserver's URL or other parameters, clients may use an


SHOULD might be a better idea here, to avoid any kind of silly guessing which is particular bad as the domain part of a Matrix ID is only used for federation.

maxidorius · 2018-07-03T19:23:19Z

specification/client_server_api.rst

+explicitly specify the homeserver's URL or other parameters, clients may use an
+auto-discovery mechanism to determine the server's URL based on a user's
+Matrix ID. Auto-discovery should only be done at login time, with the
+discovered values retained for the duration of the user's session.


Not necessarily. They can be retained for as long as the client wants to. The proposal never talks about how long this should be kept (if even), only when the mechanism should take place and how the resulting data should be used in the login process.

I agree this is out of the changeset scope.

The question here is whether an access token is scoped to a server_name or an HS base URL, ie. if you re-do autodiscovery and get a different HS URL, should you keep the old access token and use it for the new URL?

One possible scenario here is that a domain transitions from a third party server hosting their HS to them self-hosting it. Should it be assumed that a users access token will still be valid? On one hand, the .well-known endpoint has vouched for the new server, so we can send the access token and in the worst case it will be rejected. On the other hand, this means you just have to compromise the .well-known endpoint to steal people's access tokens.

I don't think the answer is obvious here, but I think it's a subtle one that we should decide on one way or the other.

@dbkr The proposal is very explicit that auto-discovery only happens at login time and if no URL was provided by the user. There is no auto-discovery if you already have an access token.

Right, so I've misunderstood what you're challenging here. You're suggesting the phrase, "with the discovered values retained for the duration of the user's session" be removed? I think this is implicit from the previous clause.

You're suggesting the phrase, "with the discovered values retained for the duration of the user's session" be removed?

Yes

OK. Personally I'd say this is probably a helpful expansion on the previous clause, but I don't really mind, I certainly wouldn't block the proposal on it either way.

maxidorius · 2018-07-03T19:24:08Z

specification/client_server_api.rst

+
+``PROMPT``
+  Retrieve the specific piece of information from the user in a way which
+  fits within the existing client UX, if the client is inclined to do so.


UX should be spelled out explicitly to avoid any confusion on its meaning

I still strongly disagree with having UX in the spec. The wording seems fine enough to let the client be creative. If we start saying "it must be a dialog" then people are put into boxes instead of offering the opportunity for a better solution. A better solution would be found eventually, however there's no reason to spec how UX works at a protocol level imho.

At most, I'd recommend this be rewritten as a "should" statement to encourage prompting of users, however that's relatively minor to me.

so.... after some quick discussion with Max oob, I misread his original comment. He'd like UX to be spelled as "user experience" instead of having it be an acronym. My bad, and I agree.

maxidorius · 2018-07-03T19:28:17Z

specification/client_server_api.rst

+
+1. Extract the server name from the user's Matrix ID by splitting the Matrix ID
+   at the first colon.
+2. Extract the DNS name from the server name.


DNS name is not accurate, as it can be understood in several ways. hostname is the actual value (like in the proposal).
server name is also confusing as that doesn't mean anything in URIs. We mean matrix domain here (which is allowed to contain the port info)

I'm not fond of "DNS name" either, but I used it for consistency because it's what's used in the server-server spec (https://matrix.org/docs/spec/server_server/unstable.html#resolving-server-names). Again, opinions from others is welcome.

(And I thought the point of this step was to drop the port info.)

Yes, but this is not the server-server spec, which is the point, so we need to rely on other parts of the spec, either C2S or Intro/Appendices.

In the User Identifiers section of the Appendices, domain is explicitly stated.
After double checking, DNS Name is indeed the only technical name for hostname in the spec, so that could remain even if not optimal. I think there should definitely be a link to DNS Name so people are not confused though.

Reading properly, host is the actual technical name:

dns_name = host
where host is as defined by RFC3986, section 3.2.2.

If DNS name is used everywhere else, we should keep doing that. A later PR can go through and fix the word usage.

For consistency, I'm going to keep "DNS name" for now, but I've also opened an issue (#1377) to rename it everywhere.

Since #1382 has been merged, I'll update this to say "hostname" instead of "DNS name"

maxidorius · 2018-07-03T19:30:49Z

specification/client_server_api.rst

+
+      i. Parse it as a URL.  If it is not a URL, then ``FAIL_ERROR``.
+      ii. Clients should validate that the URL points to a valid homeserver
+          before accepting it.  Currently, the suggested way of validating is


I think we should have an authoritative way here, not just a suggestion.

This was what I had put in the proposal. Would you be happier with "Clients should validate that the URL points to a valid homeserver before accepting it by connecting to the /_matrix/client/versions endpoint, and parsing and validating the data. If any step in the validation fails, then FAIL_ERROR."?

Yes, I saw you used the proposal which is fine. I was thinking, since this is the spec, "suggested way" seems inappropriate - after all, the spec is supposed to be authoritative. You should check with the proposal if the intent was SHOULD or MUST (my own proposal was MUST, but I remember being a big contention point)

Certainly not MUST; there can be other ways to validate the server. SHOULD might probably be worthwhile but tbh I don't quite care. libQMatrixClient will use /versions for the purpose.

My counter-argument is: if we leave the option for clients to choose whatever way they see fit, you can end up with various results on a given HS. One client might validate successfully while another one might not, leading to one working with auto-discovery via well-known, while the other doesn't. This is a typical problem of spec/RFCs and shouldn't be underrated I think.

Having one unambiguous and explicit way to validate is critical to ensure a common UX through the clients. UX was the original point of the proposal, so ignoring UX now would be silly.

SHOULD does not imply "whatever way they see fit"; it's a recommendation of doing things in a certain way, while allowing diversions if there are reasons for them. In case of diversion, the client is responsible to somehow align the UX with that of its peers or rather with user expectations. Consider that some clients would want to apply more stringent validations because they focus on security or privacy, and that those clients would be expected to FAIL_ERROR in case when other clients happily continue.

I think the fact that there's a disagreement on something that's taken word-for-word from the proposal doc means we don't have agreement on this, so let's go back and get aligned on it. Personally I'd very much like to see a strong reason given for this recommendation if we're making it. It's entirely non-obvious to me why it's here. If we can't give a good reason, please let's remove it.

maxidorius · 2018-07-03T19:31:52Z

specification/client_server_api.rst

+          and validate the data.  If any step in the validation fails, then
+          ``FAIL_ERROR``.
+
+   f. If the ``m.identity_server`` property is present, extract the


This section is incomplete compared to the proposal.

Ah, I forgot the FAIL_ERROR if there's no base_url. That seems to be the only thing missing from this step, right?

it seems like, yes.

KitsuneRal · 2018-07-03T23:18:00Z

api/client-server/wellknown.yaml

+      summary: Gets Matrix server discovery information about the domain.
+      description: |-
+        Gets discovery information about the domain.  The file may include
+        additional keys, which SHOULD follow the Java package naming convention,


Given that this is kinda common-space, namespacing is essential and has to be uniform across applications. So yes, I agree it should be MUST.

KitsuneRal · 2018-07-03T23:23:15Z

api/client-server/wellknown.yaml

+              m.identity_server:
+                description: Information about the identity server to connect to.
+                "$ref": "definitions/wellknown/identity_server.yaml"
+        404:


Can we add additionalProperties here to allow implementation-specific keys? They will need to follow the Java naming convention, of course (I don't think you can sensibly enforce it via a regexp but maybe something like ^([^m.]|[^.]{2,})\.?)

KitsuneRal · 2018-07-03T23:25:22Z

specification/client_server_api.rst

+explicitly specify the homeserver's URL or other parameters, clients may use an
+auto-discovery mechanism to determine the server's URL based on a user's
+Matrix ID. Auto-discovery should only be done at login time, with the
+discovered values retained for the duration of the user's session.


I agree this is out of the changeset scope.

KitsuneRal · 2018-07-03T23:33:46Z

specification/client_server_api.rst

+
+      i. Parse it as a URL.  If it is not a URL, then ``FAIL_ERROR``.
+      ii. Clients should validate that the URL points to a valid homeserver
+          before accepting it.  Currently, the suggested way of validating is


Certainly not MUST; there can be other ways to validate the server. SHOULD might probably be worthwhile but tbh I don't quite care. libQMatrixClient will use /versions for the purpose.

uhoreg · 2018-07-04T22:05:32Z

Pushed some changes, hopefully addressing all the comments. http://matrix.uhoreg.ca/well-known/client_server/unstable.html#server-discovery has been updated to this new version.

@KitsuneRal I'm not entirely sure about how to handle your additionalProperties comment, and couldn't find a good example/documentation to follow. I added something there, but let me know if I should add or change something.

KitsuneRal · 2018-07-05T02:21:17Z

I thought of enforcing the property name pattern; will try to find a good example but most likely tomorrow (out of town atm). Ftr, the way you've put it explicates what OpenAPI 2 spec already implies - that any object can have additional properties of arbitrary types on top of those specified.

dbkr · 2018-07-05T10:53:11Z

specification/client_server_api.rst

+          endpoint, and parsing and validating the data.  If any step in the
+          validation fails, then ``FAIL_ERROR``. Validation is done as a simple
+          check against configuration errors, before sending sensitive
+          information such as a user's password to the server.


#1361 happens before a user's password is sent, so this doesn't really make much sense.

While it probably makes sense for GET /login to happen before POST /login, the spec doesn't specify that it is necessarily the case. I think the point here is that clients should do something to check that it's something that looks like a homeserver behind the URL, and GET /versions is something that most clients are likely to do anyways (though I guess you would know better about that). I would be fine with changing it to recommend either GET /versions or GET /login, if others are fine with that too.

GET /login is not defined in any C2S API releases, so you can't and is why I always suggested GET /versions, which is basically the only reliable endpoint across the board.

Because this would be merged into unstable anways, and GET /login is specced (https://matrix.org/docs/spec/client_server/unstable.html#get-matrix-client-r0-login) it'd be fine. However, versions feels more useful.

In a situation when a simplistic client only knows of a single authentication flow (and such is a vast majority of the ecosystem, n'est-ce pas?) calling GET /login just for the sake of server validation doesn't look enticing. Using GET /versions not only for validation but also to figure that you can even start talking the server's language is at least worthwhile.

This has already been discussed in the proposal room and in some comments in both GDocs. Why are we re-discussing these elements? The proposal has been up for long enough surely....

@dbkr Still would like a clarification on this... There has been many occasions during the proposal to discuss this, so why is it an issue now?

This continues from the validation issues I raised at the very start in my summary of the discussion back on the 1st of March. Please don't use this thread for meta-discussion criticising the process, let's just get this solved.

@dbkr Not really. Your initial feedback on validation was that it shouldn't be there in the first place, then you were wondering how it was supposed to work, then you thought it existed only because /login wasn't speced. Each time we repeated why it was used. And finally, now you would like for it to be in its own section and not in this proposal/PR.

I'm having a hard time following where this is going and what is the issue here? The call to /versions has been in the proposal since the start, we documented why it was put to which no stronger counter-argument was given, there was extensive talk and then there was no more talk about it until this PR was made.

If you would like to add a global way of validating the existence of a HS to the spec which is a totally different matter, go ahead, but for now there is none that this PR can build on top of.

@dbkr thanks for your detailed feedback. I think I understand your concerns about having validation only being in this section. For fear of scope-creeping this proposal, I've created https://github.com/matrix-org/matrix-doc/issues/1529 to track pulling it out of here and putting it in its own (sub-)section that can be referenced elsewhere. I'll tweak the wording a bit here for now.

I don't mind if we add a global validation section or not, I'm just proposing it as way to resolve the impasse. The intention with suggesting validation be moved out was to simplify the proposal rather than scope-creep it though.

KitsuneRal · 2018-07-08T09:29:44Z

api/client-server/wellknown.yaml

                "$ref": "definitions/wellknown/identity_server.yaml"
+            additionalProperties:
+              description: Application-dependent keys using Java package naming convention.


Sorry, turns out I wanted to have patternProperties here so that property names could be further restricted but patternProperties is only a thing in JSON Schema but not in OpenAPI 2. I'm fine to go with or without additionalProperties.

richvdh · 2018-07-24T15:07:10Z

sorry, this is a PR to document the existing proposal #433

turt2live

Looks good to me - I've left some comments about random formatting things, and a request for examples.

turt2live · 2018-08-14T14:51:34Z

api/client-server/definitions/wellknown/homeserver.yaml

+properties:
+    base_url:
+        type: string
+        description: The base URL for the homeserver for client-server connections.


would be nice to have an example on this and the identity server counterpart

turt2live · 2018-08-14T14:52:09Z

api/client-server/wellknown.yaml

+    get:
+      summary: Gets Matrix server discovery information about the domain.
+      description: |-
+        Gets discovery information about the domain.  The file may include


nit: double space should be single. This is a theme throughout the PR - would be nice to use the convention of single spaces throughout.

turt2live · 2018-08-14T14:54:21Z

changelogs/client_server.rst

+  - Server discovery:
+    - Add ``.well-known`` discovery method
+      (`#1359 <https://github.com/matrix-org/matrix-doc/pull/1359>`_).
+


Updating to the latest master means using newsfragments - the changelog should not be modified on it's own anymore.

turt2live · 2018-08-14T14:56:02Z

api/client-server/wellknown.yaml

+# limitations under the License.
+swagger: '2.0'
+info:
+  title: "Matrix Client-Server server discovery API"


Convention says "Server Discovery" should be (mostly) AP style title-cased.

turt2live · 2018-08-14T15:11:22Z

api/client-server/wellknown.yaml

+      operationId: getWellknown
+      responses:
+        200:
+          description: Server discovery information


Descriptions should end in full stops

turt2live · 2018-08-14T15:14:27Z

api/client-server/wellknown.yaml

+            required:
+              - m.homeserver
+        404:
+          description: No server discovery information available


Descriptions should end in full stops.

- formatting fixes - add examples to homeserver/identity server discovery schema - replace DNS name with hostname

uhoreg · 2018-08-17T15:58:51Z

New version pushed, which should hopefully address all the concerns so far.

uhoreg · 2018-08-28T13:34:50Z

@maxidor I think I've addressed all your concerns. Can you update your review to a ✔️ ?

maxidorius · 2018-08-28T20:55:29Z

@uhoreg I'm not in agreement with the new wording related to Dave's comments, so I'm afraid not.
Also, I am still waiting for direct answers to my questions.

turt2live · 2018-08-28T21:59:37Z

@maxidor can you clarify what precisely you're in disagreement with? I've re-read the entire discussion thread and am thoroughly confused as to what you're objecting to. The updated wording says that clients should call /versions, which is well within what you were arguing for. The created issue, matrix-org/matrix-spec#347, appears to cover a larger scope of validating homeservers outside of this process, despite this PR having a mention of it. matrix-org/matrix-spec#347 sounds more like improving the documentation and moving up in the spec so it can apply to .well-known, hand-written URLs, and whatever other methods people acquire a homeserver name with for the client-server API.

maxidorius · 2018-08-28T22:42:40Z

6612dbe
#1359 (comment)
#1359 (comment)

uhoreg · 2018-08-29T14:40:57Z

After discussion with the spec core team, we've decided to merge this. The main concerns raised seem to be mostly about the spec process and not the wording itself. There were some concerns about new terminology introduced in the final sentence of the validation item: this sentence provides rationale and does not define behaviour, and as it is one point in a list, I don't want to make it too big by defining everything. We can improve the wording later if needed (possibly through matrix-org/matrix-spec#347), but I think that it's in a good enough state to merge.

initial draft of .well-known discovery

0dd3309

matrixbot mentioned this pull request Jul 3, 2018

Support for discovering API endpoints via .well-known URIs (SPEC-121) #433

Closed

uhoreg mentioned this pull request Jul 3, 2018

[draft] server discovery for client connections #1132

Closed

maxidorius suggested changes Jul 3, 2018

View reviewed changes

KitsuneRal reviewed Jul 3, 2018

View reviewed changes

remove accidentally-committed backup file

5bc29eb

uhoreg mentioned this pull request Jul 4, 2018

"DNS name" is should be renamed #1377

Closed

incorporate feedback from reviewers

ce1e2c0

dbkr reviewed Jul 5, 2018

View reviewed changes

KitsuneRal approved these changes Jul 8, 2018

View reviewed changes

richvdh assigned uhoreg Jul 24, 2018

richvdh added proposal-in-review proposal A matrix spec change proposal and removed proposal A matrix spec change proposal proposal-in-review labels Jul 24, 2018

turt2live mentioned this pull request Aug 6, 2018

Easier login with non-default homeserver element-hq/element-web#7126

Closed

turt2live approved these changes Aug 14, 2018

View reviewed changes

uhoreg added 4 commits August 14, 2018 17:00

Merge remote-tracking branch 'origin/master' into well-known

6a3cf10

various minor fixes

fcca80d

- formatting fixes - add examples to homeserver/identity server discovery schema - replace DNS name with hostname

put server discovery as its own section

a264120

tweak wording for validation

6612dbe

uhoreg merged commit 5019fb7 into matrix-org:master Aug 29, 2018

turt2live mentioned this pull request Jul 9, 2018

How do you choose what Identity Server to use as a client? (SPEC-22) matrix-org/matrix-spec#45

Closed

turt2live mentioned this pull request Aug 9, 2024

GET /.well-known/matrix/client only allows additional properties of type object matrix-org/matrix-spec#1913

Closed

.well-known discovery #1359

.well-known discovery #1359

Conversation

uhoreg commented Jul 3, 2018 • edited Loading

maxidorius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxidorius Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turt2live Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxidorius Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxidorius Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Jul 4, 2018

KitsuneRal commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxidorius Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxidorius Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Jul 24, 2018

turt2live left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Jul 3, 2018 •

edited

Loading

maxidorius Jul 3, 2018 •

edited

Loading

turt2live Jul 3, 2018 •

edited

Loading

maxidorius Jul 3, 2018 •

edited

Loading

maxidorius Jul 3, 2018 •

edited

Loading

maxidorius Jul 9, 2018 •

edited

Loading

maxidorius Aug 17, 2018 •

edited

Loading

maxidorius commented Aug 28, 2018 •

edited

Loading

maxidorius commented Aug 28, 2018 •

edited

Loading