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.

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

Merged
merged 6 commits into from Mar 24, 2019

Conversation

@erikjohnston
Copy link
Member

commented Mar 6, 2019

Rendered

An alternative proposal to #1194

@erikjohnston erikjohnston requested a review from matrix-org/spec-core-team Mar 6, 2019

@richvdh richvdh self-requested a review Mar 6, 2019

@erikjohnston erikjohnston added this to In progress in Homeserver Task Board via automation Mar 6, 2019

@turt2live turt2live self-requested a review Mar 6, 2019

Show resolved Hide resolved proposals/1915-unbind-identity-server-param.md Outdated
Show resolved Hide resolved proposals/1915-unbind-identity-server-param.md Outdated
Show resolved Hide resolved proposals/1915-unbind-identity-server-param.md Outdated

### Client-Server API

Add `POST /_matrix/client/r0/account/3pid/delete` API, which expects a JSON body

This comment has been minimized.

Copy link
@dbkr

dbkr Mar 7, 2019

Member

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?

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Mar 12, 2019

Author Member

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.

Show resolved Hide resolved proposals/1915-unbind-identity-server-param.md Outdated
Update proposals/1915-unbind-identity-server-param.md
Co-Authored-By: erikjohnston <erikj@jki.re>

@richvdh richvdh removed their request for review Mar 11, 2019

@erikjohnston erikjohnston requested a review from turt2live Mar 15, 2019

@turt2live
Copy link
Member

left a comment

This is realistically my only concern. The proposal looks great overall, and avoiding breaking changes is really appreciated.

Show resolved Hide resolved proposals/1915-unbind-identity-server-param.md Outdated

@erikjohnston erikjohnston requested a review from turt2live Mar 18, 2019

Update proposals/1915-unbind-identity-server-param.md
Co-Authored-By: erikjohnston <erikj@jki.re>
@turt2live
Copy link
Member

left a comment

Thanks for enduring my feedback

@erikjohnston

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 18, 2019

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.

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 18, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 24, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@turt2live

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

this now needs an implementation and it's good to go

@turt2live turt2live merged commit b76b7cd into master Mar 24, 2019

7 checks passed

ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

Homeserver Task Board automation moved this from In progress to Done Mar 24, 2019

@turt2live turt2live moved this from Done to In progress in Homeserver Task Board Mar 24, 2019

@erikjohnston erikjohnston moved this from In progress to Done in Homeserver Task Board Mar 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.