Add 3pid unbind callback to module API by MatMaul · Pull Request #13227 · matrix-org/synapse

MatMaul · 2022-07-08T14:57:29Z

Currently we have a module interface that can react on a 3pid bind action, but it doesn't exist for unbind.

We rely on the fact that the identity server where we want to unbind should be stored in user_threepid_id_server table.
Unfortunately the info there doesn't include the scheme, hence a bind to http://localhost:7777 will fail on unbind because https://localhost:7777 is called instead.

To overcome that this PR introduces a similar callback for 3pid unbind, so we can also override the identity server URL there for example.

MatMaul · 2022-07-12T21:45:20Z

This is now tested alongside matrix-org/synapse-bind-sydent#2, and tests seems to pass outside of an unrelated (flaky ?) one I think.

docs/modules/third_party_rules_callbacks.md

changelog.d/13227.feature

synapse/events/third_party_rules.py

docs/modules/third_party_rules_callbacks.md

synapse/events/third_party_rules.py

docs/modules/third_party_rules_callbacks.md

Co-authored-by: Brendan Abolivier <babolivier@matrix.org>

babolivier

Sorry about the delay on this one!

docs/modules/third_party_rules_callbacks.md

babolivier

Mostly changes for documentation (but also a bit about the logic around the return value). I haven't looked at the test yet, since I assume it might change as a result of discussions around some of the comments I've made.

babolivier · 2022-12-13T10:40:58Z

docs/modules/third_party_rules_callbacks.md

+A module can hence do its own custom unbinding, if for example it did also registered a custom
+binding logic with `on_threepid_bind`.
+
+It should return a tuple of 2 booleans:


Suggested change

It should return a tuple of 2 booleans:

The module must return a tuple of 2 booleans, which indicate respectively:

The main thing here is replacing "should" by "must": "should" implies the module is strongly encouraged to do something but there isn't a strict obligation. "must" implies there's a strict obligation, rather than a strong encouragement (which is the case here - Synapse won't be able to process any other type of return value).

babolivier · 2022-12-13T10:50:34Z

docs/modules/third_party_rules_callbacks.md

+) -> Tuple[bool, bool]:
+```
+
+Called before a threepid association is removed.


Suggested change

Called before a threepid association is removed.

Called before the association between a local user and a third-party identifier

(email address, phone number) is removed.

I wouldn't expect users who aren't deeply familiar with Matrix's concepts to know what a "threepid" is.

babolivier · 2022-12-13T10:51:14Z

docs/modules/third_party_rules_callbacks.md

+as well as the medium (`email` or `msisdn`), address of the third-party identifier and
+the identity server where the threepid was successfully registered.
+
+A module can hence do its own custom unbinding, if for example it did also registered a custom


Suggested change

A module can hence do its own custom unbinding, if for example it did also registered a custom

A module can hence implement its own custom unbinding, if for example it also implemented a custom

babolivier · 2022-12-13T11:08:08Z

synapse/events/third_party_rules.py

+                (changed, stop) = await callback(
+                    user_id, medium, address, identity_server
+                )
+                global_changed |= changed


Shouldn't we rather indicate whether all callbacks succeeded, rather than just one of them?

babolivier · 2022-12-13T11:12:27Z

docs/modules/third_party_rules_callbacks.md

+- first one should be `True` on a success calling the identity server, otherwise `False` if
+the identity server doesn't support unbinding (or no identity server found to contact).
+- second one should be `True` if unbind needs to stop there. In this case no other module
+unbind will be called, and the default unbind made to the IS that was used on bind will also be
+skipped. In any case the mapping will be removed from the Synapse 3pid remote table,
+except if an Exception was raised at some point.


Suggested change

- first one should be `True` on a success calling the identity server, otherwise `False` if

the identity server doesn't support unbinding (or no identity server found to contact).

- second one should be `True` if unbind needs to stop there. In this case no other module

unbind will be called, and the default unbind made to the IS that was used on bind will also be

skipped. In any case the mapping will be removed from the Synapse 3pid remote table,

except if an Exception was raised at some point.

- whether the unbind was successful (`True` in this case, `False` otherwise).

- whether Synapse should stop the unbinding process there (`True` in this case, `False` otherwise). In

this case, Synapse will also remove any local record of an association between the local user and the

third-party identifier on a remote identity server.

If multiple modules implement this callback, they will be considered in order. Unless the return value of

a callback indicates to stop the unbinding process, Synapse falls through to the next one. Otherwise,

Synapse will not call any subsequent implementation of this callback.

We should also add to the last paragraph a mention of how the first boolean is processed once we have resolved this question in a previous thread.

A few things this suggestion changes:

I don't believe it is safe to assume all modules will do their binding/unbinding against an identity server, so it might not make sense to tie the definition of return values to the existence of an identity server.

The bit about exceptions don't feel right to me: first, looking at the code it doesn't look correct; second the way it's phrased makes it sound like modules should use exceptions to define whether Synapse should delete the remote record, and using exceptions as a control method is something we actively try to discourage.

Also some clarity and consistency on how Synapse handles multiple modules implementing this callback.

babolivier · 2022-12-13T11:50:12Z

synapse/handlers/identity.py

-            url_bytes=url_bytes,
-            content=content,
-            destination_is=id_server.encode("ascii"),
+        (changed, stop,) = await self.hs.get_third_party_event_rules().unbind_threepid(


Suggested change

(changed, stop,) = await self.hs.get_third_party_event_rules().unbind_threepid(

changed, stop = await self.hs.get_third_party_event_rules().unbind_threepid(

babolivier · 2022-12-13T11:50:57Z

synapse/handlers/identity.py

-            await self.blacklisting_http_client.post_json_get_json(
-                url, content, headers
+        # If a module wants to take over unbind it will return stop = True,
+        # in this case we should just purge the table from the 3pid record


Suggested change

# in this case we should just purge the table from the 3pid record

# in this case we should just remove the entry from the 3pid remote records table

babolivier · 2022-12-13T11:52:03Z

synapse/events/third_party_rules.py

+        Note that this callback is called before an association is deleted on the
+        local homeserver.


This feels a bit superfluous with the line above?

babolivier · 2022-12-13T11:54:42Z

synapse/events/third_party_rules.py

+            A tuple of 2 booleans reporting if a changed happened for the first, and if unbind
+            needs to stop there for the second (True value). In this case no other module unbind will be
+            called, and the default unbind made to the IS that was used on bind will also be skipped.
+            In any case the mapping will be removed from the Synapse 3pid remote table, except if an Exception
+            was raised at some point.


Suggested change

A tuple of 2 booleans reporting if a changed happened for the first, and if unbind

needs to stop there for the second (True value). In this case no other module unbind will be

called, and the default unbind made to the IS that was used on bind will also be skipped.

In any case the mapping will be removed from the Synapse 3pid remote table, except if an Exception

was raised at some point.

A tuple of 2 booleans reporting, respectively, if the unbind was successful, and if the unbind process

should stop there.

The bit about what happens if the second bool is True is only relevant to module callbacks, not to the interface.

anoadragon453 · 2023-01-31T17:29:51Z

Currently we have a module interface that can react on a 3pid bind action, but it doesn't exist for unbind.

Not quite I'm afraid. Currently we have a on_threepid_bind method which has nothing to do with identity servers association bindings. It is called when a local association is made on the homeserver. The method proposed in this PR would only be tangentially related to the existing method (and it would be confusing if they were given similar names). I've outlined this problem in more detail at #14955.

Additionally in the same issue, I've tweaked the design of the module callback method proposed by this PR: #14955 (comment). The tweaked design only returns a single boolean while still fitting the use case of synapse-bind-sydent.

anoadragon453 · 2023-02-13T19:34:11Z

This PR has been superseded by #15044.

MatMaul requested a review from a team as a code owner July 8, 2022 14:57

babolivier added the Z-Time-Tracked Element employees should track their time spent on this issue/PR. label Jul 8, 2022

squahtx marked this pull request as draft July 11, 2022 11:17

MatMaul force-pushed the mv/unbind-callback branch 8 times, most recently from 6e8db1e to 6cd2726 Compare July 12, 2022 15:28

babolivier requested review from babolivier and removed request for a team July 12, 2022 15:32

MatMaul force-pushed the mv/unbind-callback branch 6 times, most recently from 381e73e to 89ef487 Compare July 12, 2022 21:22

MatMaul changed the title ~~[Draft] Add 3pid unbind callback to module API~~ Add 3pid unbind callback to module API Jul 12, 2022

MatMaul marked this pull request as ready for review July 12, 2022 21:35

MatMaul force-pushed the mv/unbind-callback branch from 89ef487 to 0951ff7 Compare July 18, 2022 09:25

babolivier suggested changes Jul 18, 2022

View reviewed changes

guillaumevillemont mentioned this pull request Jul 28, 2022

unbind callback + hide discovery_room in directory element-hq/synapse-tchap#1

Merged

MatMaul and others added 3 commits August 3, 2022 14:52

Add 3pid unbind callback to module API

0274a7f

Apply suggestions from code review

97f991e

Co-authored-by: Brendan Abolivier <babolivier@matrix.org>

Adress comments

9b4c0e7

MatMaul force-pushed the mv/unbind-callback branch from 9ad1153 to 9b4c0e7 Compare August 3, 2022 14:26

MatMaul requested a review from babolivier August 9, 2022 16:01

babolivier suggested changes Aug 16, 2022

View reviewed changes

docs/modules/third_party_rules_callbacks.md Outdated Show resolved Hide resolved

babolivier mentioned this pull request Aug 26, 2022

[Draft] Add unbind callback matrix-org/synapse-bind-sydent#2

Closed

Merge remote-tracking branch 'origin/develop' into mv/unbind-callback

5ff0ba2

github-actions bot deployed to PR Documentation Preview November 28, 2022 12:01 View deployment

MatMaul self-assigned this Dec 12, 2022

Mathieu Velten added 2 commits December 12, 2022 17:48

Merge remote-tracking branch 'origin/develop' into mv/unbind-callback

4ccade6

Adress comments

6073c0e

MatMaul requested a review from babolivier December 12, 2022 22:32

github-actions bot deployed to PR Documentation Preview December 12, 2022 22:32 View deployment

Fix import order

4dc7b44

github-actions bot deployed to PR Documentation Preview December 13, 2022 09:26 View deployment

babolivier suggested changes Dec 13, 2022

View reviewed changes

anoadragon453 mentioned this pull request Jan 31, 2023

The on_threepid_bind module API is called when a 3PID is *added* (associated locally), not when it is *bound* #14955

Closed

anoadragon453 mentioned this pull request Feb 9, 2023

Add module API callbacks for adding and deleting local 3PID associations #15044

Merged

anoadragon453 closed this Feb 13, 2023

	It should return a tuple of 2 booleans:
	The module must return a tuple of 2 booleans, which indicate respectively:

	Called before a threepid association is removed.
	Called before the association between a local user and a third-party identifier
	(email address, phone number) is removed.

	A module can hence do its own custom unbinding, if for example it did also registered a custom
	A module can hence implement its own custom unbinding, if for example it also implemented a custom

-- first one should be `True` on a success calling the identity server, otherwise `False` if
-the identity server doesn't support unbinding (or no identity server found to contact).
-- second one should be `True` if unbind needs to stop there. In this case no other module
-unbind will be called, and the default unbind made to the IS that was used on bind will also be
-skipped. In any case the mapping will be removed from the Synapse 3pid remote table,
-except if an Exception was raised at some point.
+- whether the unbind was successful (`True` in this case, `False` otherwise).
+- whether Synapse should stop the unbinding process there (`True` in this case, `False` otherwise). In
+this case, Synapse will also remove any local record of an association between the local user and the
+third-party identifier on a remote identity server.
+If multiple modules implement this callback, they will be considered in order. Unless the return value of
+a callback indicates to stop the unbinding process, Synapse falls through to the next one. Otherwise,
+Synapse will not call any subsequent implementation of this callback.

	(changed, stop,) = await self.hs.get_third_party_event_rules().unbind_threepid(
	changed, stop = await self.hs.get_third_party_event_rules().unbind_threepid(

	# in this case we should just purge the table from the 3pid record
	# in this case we should just remove the entry from the 3pid remote records table

		Note that this callback is called before an association is deleted on the
		local homeserver.

Uh oh!

Conversation

MatMaul commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatMaul commented Jul 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

babolivier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

babolivier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anoadragon453 commented Jan 31, 2023

Uh oh!

anoadragon453 commented Feb 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MatMaul commented Jul 8, 2022 •

edited

Loading