Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC2290: Separate Endpoints for Threepid Binding #2290
MSC2290: Separate Endpoints for Threepid Binding #2290
Changes from 23 commits
fdea3e3
7096092
f5b10c6
5193c31
cb7c072
5b259bf
1fc1e3c
196f27e
7b656e9
f36ed9a
f06ba49
4bc005a
af24676
2a55310
c57250b
0b67f34
3eda9f7
1e69a7f
169174e
53519f9
e50bb3d
87d641c
bd64ffc
40420d9
9311e89
219ebff
1a51a24
0332d53
ec7e795
46e7137
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relatedly: my main question is how hitting /bind on the HS differs from hitting the IS directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HS will just proxy this request. However, it will remember that the bind occurred, and when it comes time to unbind all of a user's threepids (during account deactivation or when they explicitly request it), then the homeserver is aware of what binds occurred where and can carry out the unbinds on the user's behalf.
But other than that it is simply just proxying the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this obsoletes MSC2263 too then? :/
They were made optional in 2263 for the reason of maintaining backwards compatibility with clients. Removing them does not maintain that backwards compatibility. In fact, 2263 even introduces an error code to let the HS make the decision about whether or not it cares to give power to the homeserver without harming the protocol.
This is duplicated effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave them as optional parameters but homeserver should ignore them else they are opening themselves up to malicious identity servers.
And then at that point there's not much point in keeping them as optional parameters versus removing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By removing them, it becomes impossible for
/bind
to work. The client will have no way of contacting an identity server through the homeserver, so the homeserver might end up rejecting/bind
. If there's no way to proxy the calls to the IS through the HS, there's no point in/bind
and we should just remove thebind
flag from/account/3pid
instead of splitting the endpoint.I'd rather we keep the fields as optional and put a red box saying "you should abuse M_SERVER_NOT_TRUSTED at every given opportunity" until the deprecation period has passed and they get removed (as they are deprecated by MSC2263).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, iiuc the problem is:
id_server
in synapse because we want to ditch the concept of trusted ID serversid_server
I think, as long as the requests still work in synapse with an
id_server
(modulo the fact they won't bind, but that's covered by this MSC), that's fine? The HS will send a token (not via the IS specified) but the right thing will still happen.So I would suggest maybe keeping them optional for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping them optional but returning
M_SERVER_NOT_TRUSTED
works I suppose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2263 was specifically written to allow Synapse to not support
id_server
ftr: it can just throwM_SERVER_NOT_TRUSTED
for everything.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't have homeservers throw
M_SERVER_NOT_TRUSTED
else old clients trying to sendid_server
will get confused when they get this error.But new servers can just ignore the parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still expect that servers throw the error instead of ignoring it, as ignoring it is against the spec.
It'd be no different than a 500 error in the eyes of the client (which it should be handling because
/requestToken
is certainly in the top 10 for most error-prone endpoints).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a client perspective, we'd like old Riots to still be able add 3PIDs to the HS account.
If
requestToken
begins throwing an error whenid_server
is included, those clients will no longer have any path to add a 3PID. The old Riots would have to have known to special case and ignoreM_SERVER_NOT_TRUSTED
, which did not exist in the past.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jryans sounds like the server shouldn't be throwing the error then and instead handle the
id_server
as deprecated (as proposed by #2263). If the server doesn't want to handle it, it is no different from it throwing a 500 error for a misconfigured email server.It would be intentional that adding 3PIDs be blocked - it's an error related to something the server is not comfortable with doing.
This all should have come out on MSC2263, not here, as well. I'd still have the same opinion, but it's off-topic for this MSC (now).