Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement MSC1915 - 3PID unbind APIs #4982

Merged
merged 9 commits into from
Apr 3, 2019
Merged

Implement MSC1915 - 3PID unbind APIs #4982

merged 9 commits into from
Apr 3, 2019

Conversation

erikjohnston
Copy link
Member

The commits should make sense by themselves, but the thrust of this PR is that we now keep track of which IS a given 3PID is bound via so we can use that when we come to unbind.

Fixes #4962

@erikjohnston erikjohnston requested a review from a team April 1, 2019 12:28
@codecov
Copy link

codecov bot commented Apr 1, 2019

Codecov Report

Merging #4982 into develop will decrease coverage by 0.04%.
The diff coverage is 39.39%.

@@             Coverage Diff             @@
##           develop    #4982      +/-   ##
===========================================
- Coverage     60.6%   60.55%   -0.05%     
===========================================
  Files          332      331       -1     
  Lines        34235    34239       +4     
  Branches      5655     5659       +4     
===========================================
- Hits         20749    20735      -14     
- Misses       12011    12028      +17     
- Partials      1475     1476       +1

@erikjohnston
Copy link
Member Author

(Sytests are on their way)

@erikjohnston erikjohnston removed the request for review from a team April 1, 2019 14:18
@erikjohnston
Copy link
Member Author

(Sytest has revealed some issues)

This will then be used to know which IS to default to when unbinding the
threepid.
This changes the behaviour from using the server specified trusted
identity server to using the IS that used during the binding of the
3PID, if known.

This is the behaviour specified by MSC1915.
By default the homeserver will use the identity server used during the
binding of the 3PID to unbind the 3PID. However, we need to allow
clients to explicitly ask the homeserver to unbind via a particular
identity server, for the case where the 3PID was bound out of band from
the homeserver.

Implements MSC915.
We assume, as we did before, that users bound their threepid to one of
the trusted identity servers. So we simply fill the new table with all
threepids in `user_threepids` joined with the trusted identity servers.
@erikjohnston
Copy link
Member Author

Now with tests: matrix-org/sytest#595

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits. Can you also check the copyright declarations on the files you're making significant changes to?

synapse/storage/schema/delta/53/user_threepid_id.sql Outdated Show resolved Hide resolved
synapse/storage/schema/delta/53/user_threepid_id.sql Outdated Show resolved Hide resolved
synapse/storage/registration.py Outdated Show resolved Hide resolved
synapse/handlers/identity.py Outdated Show resolved Hide resolved
synapse/handlers/identity.py Show resolved Hide resolved
synapse/handlers/deactivate_account.py Show resolved Hide resolved
medium=threepid["medium"],
address=threepid["address"],
)
if threepid.get("id_server"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this in the dict rather than a function param? (and anyway, the docstring needs updating)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadly because this is how its handled in the bind path where the threepid dict contains the identity server. I don't mind pulling it out but then we probably want to also pull out medium and address

this.

We do this by grandfathering in existing user threepids assuming that
they used one of the server configured trusted identity servers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one of, or all of? (surely it should be one of, but the code does all of)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We assume that they used "one of", but we don't know which one so we have to include all the id servers.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@erikjohnston erikjohnston merged commit 8f549c1 into develop Apr 3, 2019
turt2live added a commit to matrix-org/matrix-spec-proposals that referenced this pull request May 28, 2019
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.
turt2live added a commit to matrix-org/matrix-spec-proposals that referenced this pull request May 28, 2019
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.
turt2live added a commit to matrix-org/matrix-spec-proposals that referenced this pull request May 28, 2019
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants