-
Notifications
You must be signed in to change notification settings - Fork 382
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
MSC 1915 - Add a 3PID unbind API #1915
Conversation
|
||
### Client-Server API | ||
|
||
Add `POST /_matrix/client/r0/account/3pid/delete` API, which expects a JSON body |
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.
This API already exists: https://matrix.org/docs/spec/client_server/r0.4.0.html#post-matrix-client-r0-account-3pid-delete (you're just adding the id_server
param).
Also, the account deactivate API deletes threepids too and therefore might try to unbind them, so if you have an id_server
param on this API it implies there out to be one on there too.
Also also, the API to add threepids has a bind
boolean parameter controlling whether the HS should also bind the threepid on the given IS. It could make sense for the delete API to have this param too. On the other hand, does it make sense to delete a 3pid from the server but not want to unbind it from the IS?
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.
Good points.
Also also, the API to add threepids has a bind boolean parameter controlling whether the HS should also bind the threepid on the given IS. It could make sense for the delete API to have this param too. On the other hand, does it make sense to delete a 3pid from the server but not want to unbind it from the IS?
Honestly, I see this as mostly a problem with the APIs and functionality as they stand. I think to fix this we need to have a big rethink of it all.
Co-Authored-By: erikjohnston <erikj@jki.re>
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.
This is realistically my only concern. The proposal looks great overall, and avoiding breaking changes is really appreciated.
Co-Authored-By: erikjohnston <erikj@jki.re>
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.
Thanks for enduring my feedback
@mscbot fcp merge |
Team member @erikjohnston has proposed to merge this. The next step is review by the rest of the tagged people: No concerns currently listed. Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
this now needs an implementation and it's good to go |
Implementation happened at matrix-org/synapse#4982 so this now just needs spec |
FYI this MSC needs to be updated to reflect that the path should actually be |
it was: 2eb9708 |
Ah ok, cool thanks. Edit: have updated the link in the OP as people usually click that. |
As per [MSC1915](#1915) Implementation proof: matrix-org/synapse#4982 The only alteration made which differs from the proposal is clarity on how to handle homeservers not knowing the `id_server`. All other differences are unintentional.
As per [MSC1915](#1915) Implementation proof: matrix-org/synapse#4982 The only alteration made which differs from the proposal is clarity on how to handle homeservers not knowing the `id_server`. All other differences are unintentional.
As per [MSC1915](#1915) Implementation proof: * matrix-org/synapse#4982 * matrix-org/sydent#160 The only alteration made which differs from the proposal is clarity on how to handle homeservers not knowing the `id_server`. All other differences are unintentional.
Spec PR: #2046 |
merged 🎉 |
Rendered
An alternative proposal to #1194