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

Mark `home_server` field deprecated #1097

Merged
merged 3 commits into from Dec 18, 2017

Conversation

Projects
None yet
6 participants
@richvdh
Member

richvdh commented Dec 18, 2017

This is spelt wrong, and is redundant to user_id, so let's stop people using it.

richvdh added some commits Dec 18, 2017

Mark `home_server` field deprecated
This is spelt wrong, and is redundant to user_id, so let's stop people using
it.
@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Dec 18, 2017

I really don't want clients having to parse user IDs to get the server name, it keeps getting implemented wrong.

@CobaltCake

This comment has been minimized.

CobaltCake commented Dec 18, 2017

This is probably a breaking change, but could I suggest changing user_id to contain just the username and homeserver to contain just the homeserver address? (Then clients would do something like "@" + user_id + ":" + homeserver to get their fully qualified user ID)

@maxidor

This comment has been minimized.

Contributor

maxidor commented Dec 18, 2017

I would refrain from removing that field as it's a very convenient mechanism to:

  • Redirect a user somewhere else after login
  • Avoid that the client parse its own Matrix ID and connect to the WRONG HS, leaking credentials.

It might not be used in any public projects yet, but I can see it happening pretty soon.

@richvdh

This comment has been minimized.

Member

richvdh commented Dec 18, 2017

This is probably a breaking change, but could I suggest changing user_id to contain just the username and homeserver to contain just the homeserver address? (Then clients would do something like "@" + user_id + ":" + homeserver to get their fully qualified user ID)

Possibly, but in that case it would be a user_id_localpart rather than user_id. It's not obviously an improvement as far as I'm concerned.

@richvdh

This comment has been minimized.

Member

richvdh commented Dec 18, 2017

For context, this change was mostly inspired by the fact that home_server is spelt wrong (so needs to be deprecated anyway, whether it is then replaced by homeserver or not), but also by the confusion being caused by the redundancy:

wait okay, if the user_id it returns is fully qualified, why does /login also return a home_server key/value?

it just seems kinda redundant to have what seems like the same thing twice

maybe there's a situation where they'd be different but idk

oh i have a guess:
when logins become decentralized, home_server gives the current homeserver, where user_id still has the account origin's homeserver?

so the user_id was a more useful thing which was added later
and home_server kept around for compatability
which i suspect nothing has ever used

so as a client, i can just pick one and ignore it's value?

i guess it's /just/ possible that kegan was thinking that @foo:matrix.org might actually be accessed via a HS called hs1.matrix.org or something
but this seems a very weird place to put it

okay, so now returning a fully qualified user_id and the homeserver makes sense
the user_id is for, well, the user_id (useful when alternative login types are used, eg email) and the home_server part is where the client should start making requests to

if it's not supposed to be used by the client to know where to go next, why is it there currently? what was the use case?

and much other confused and circular discussion

@richvdh

This comment has been minimized.

Member

richvdh commented Dec 18, 2017

I would refrain from removing that field as it's a very convenient mechanism to:

  • Redirect a user somewhere else after login
  • Avoid that the client parse its own Matrix ID and connect to the WRONG HS, leaking credentials.

Both these ideas appear to be founded on the incorrect belief that this field returns the address of the client-server API. The existence of this incorrect belief demonstrates the need for its removal imho.

@richvdh richvdh merged commit 6d0eb2e into master Dec 18, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ara4n

This comment has been minimized.

Member

ara4n commented Dec 24, 2017

Just for the record, @Kegsay dropped by and gave his understanding of the original code:

screen shot 2017-12-24 at 11 40 05

Again, it seems that the idea was to provide a way of forming CS APIs URLs - but the implementation itself doesn't reflect this, and clients didn't acknowledge it anyway. Plus I think we need a much more generic way of forming CS API URLs anyway (#433 or whatever).

@IT-Sumpfling

This comment has been minimized.

IT-Sumpfling commented Feb 8, 2018

Regarding: "no one uses it" - I did (and do!). AFAIK Riot implements it (that is, I can enter a UserId of @ user : example.org and a homeserver of matrix.example.org and it works ...

And I see very much large corporations using it, too: They probably do not want their "normal" web-server infrastructure (perhaps distributed via a CDN) to handle their matrix-workload?

So, they would have a (home-)server matrix.example.org
BUT they also would like to have IDs like @ user : example.org

@richvdh

This comment has been minimized.

Member

richvdh commented Feb 8, 2018

@IT-Sumpfling I think you are misunderstanding what the behaviour of this field was.

@IT-Sumpfling

This comment has been minimized.

IT-Sumpfling commented Feb 8, 2018

@richvdh : this may very well be the case - I "stumbled" upon this pull-request via
#433
After having a look at the complete files (and not just the diffs of the merge request) I tend to agree - probably I misunderstood the usage of the field.

Nevertheless I see potential use for such a field (maybe named differently) - and if I interpret the chat with Kegan above correctly, this was also the original intent:
Loadbalancing/redirecting beyond the "normal" means of the (DNS-only) server in the matrix-Id. I.e. a central login which then redirects users to "their" server (without the user knowing in the first place).

E.g. Users @A ... @c : example.org (normally) go to hs1.example .org, @d .. @f : example.org go to hs2.example.org and so on.
All without the users knowing - they all just login via "@ user : example.org "

@maxidor

This comment has been minimized.

Contributor

maxidor commented Feb 8, 2018

FYI this will be used in a mxisd & mxhsd combo until another equivalent mechanism make it into the spec.

@richvdh

This comment has been minimized.

Member

richvdh commented Feb 8, 2018

FYI this will be used in a mxisd & mxhsd combo until another equivalent mechanism make it into the spec.

The spec says "figure it out from the user_id". What are you going to use home_server for, ooi?

@maxidor

This comment has been minimized.

Contributor

maxidor commented Feb 8, 2018

mxisd will perform the authentication, but the client API will be under several subdomains depending on the authenticated user (yes I'll need a custom client).

@richvdh

This comment has been minimized.

Member

richvdh commented Feb 8, 2018

again: given (at least per the spec) this field doesn't tell you where the client API is, how does it help you?

I can see the argument for having a field in the login response which tells you were the client api is - but as I've said before this field was never it. If you'd like to introduce one, I would strongly advise you to call it something else.

@maxidor

This comment has been minimized.

Contributor

maxidor commented Feb 8, 2018

@richvdh maybe some context would help here.
I'm building a solution that needs

  • to auto-discover HS & IS URLs, which would be done via .well-known (see #443)
  • after login, tell the client which HS domain to use if it needs to be different (because of different account types), which would use the auto-discovery above again if needed.

So I believe the field match my needs as I need a domain and not a URL?

@richvdh

This comment has been minimized.

Member

richvdh commented Feb 8, 2018

you haven't said exactly what you mean by "HS domain" or why you need it, but I'm assuming you mean the domain part of the user_id, which, as per the recommendation here, the client should get from ... the user_id.

@maxidor

This comment has been minimized.

Contributor

maxidor commented Feb 8, 2018

The hostname used in the C2S URL doesn't have to match the matrix domain of the homeserver. I need a mechanism to automate this discovery, ideally given by the endpoint used for login.
Is there any other mechanism which could be used to do so? This is the closest I found, even if it doesn't match the spec but since it is deprecated, it would be acceptable to use for private purposes.

@richvdh

This comment has been minimized.

Member

richvdh commented Feb 9, 2018

out-of-band conversation with @maxidor shows he is using home_server as a way of communicating the location of the (post-login) CS API to the client, with some additional indirection.

I suggested that he could use a different property name to (a) avoid confusion/conflict with synapse (b) avoid the stupid underscore, but since he has an existing, working configuration, the change is hard to justify.

I still believe it's correct to deprecate this field in the spec.

@richvdh

This comment has been minimized.

Member

richvdh commented Nov 23, 2018

MSC1730 proposes a new field to be used for the CS API.

@richvdh richvdh deleted the rav/deprecate_home_server branch Nov 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment