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

Password provider does not work properly since Synapse v1.19.0 due to "user_get_threepids" is now async #8108

Closed
menturion opened this issue Aug 18, 2020 · 7 comments
Assignees
Labels
z-bug (Deprecated Label)

Comments

@menturion
Copy link

menturion commented Aug 18, 2020

After upgrading to Synapse v1.19.0 the following exception is thrown within a password provider :

...for threepid in threepids:
TypeError: 'coroutine' object is not iterable

The affected piece of code of the password provider looks like this:

@defer.inlineCallbacks
def check_3pid_auth(self, medium, address, password):

   store = yield self.account_handler._hs.get_profile_handler().store

   threepids = yield store.user_get_threepids(user_id)
   for threepid in threepids:
      ...

This is obviously due to "user_get_threepids" is defined async since Synapse v1.19.0, see in "registration.py" # 526,

async def user_get_threepids(self, user_id):

So my question is:
How to request the user's threepids (asynchronously) within a password provider?

@reivilibre
Copy link
Contributor

reivilibre commented Aug 18, 2020

I don't have intimate knowledge of this area of the code but I can offer some suggestions.

You may need to do

yield ensureDeferred(store.user_get_threepids(user_id))

which converts the coroutine to a Deferred. (My understanding, which may be wrong, is that yield will only deal with Deferreds, whilst other values (including coroutines) will get passed straight through as though there was no yield).

Docs for ensureDeferred.

Or, ideally, if/when possible, switch from

@defer.inlineCallbacks
def check_3pid_auth(self, medium, address, password):

to

async def check_3pid_auth(self, medium, address, password):

(and use await instead of yield in this function)

You may need other code changes to make this work.

@reivilibre
Copy link
Contributor

I also tried switching from

@defer.inlineCallbacks
def check_3pid_auth(self, medium, address, password):

to
async def check_3pid_auth(self, medium, address, password):
in use await store.user_get_threepids(user_id) . This, however, throws an exception.

In this case you would need to switch all yields to awaits and you might have to end up touching other functions in your code as well. I only suggested it as this is the modern, clean style going forward (inlineCallbacks might even be getting deprecated in recognition of it being a relic from days gone by). If what you have works, I don't think there is need to worry, though :)

@clokep clokep self-assigned this Aug 18, 2020
@clokep clokep added the z-bug (Deprecated Label) label Aug 18, 2020
@clokep
Copy link
Member

clokep commented Aug 18, 2020

I'll take a look, but I suspect this is because private properties are being used which aren't expected to be exposed as part of the password provider API.

@clokep
Copy link
Member

clokep commented Aug 18, 2020

I'm going to close this since password providers are really only expected to use the public methods on the config and ModuleApi passed into the constructor.

There's not really anything to stop you from doing that in Python, but converting the code to be async is the right solution here IMO! 👍

If there is additional functionality that the ModuleApi class could offer, please file a separate issue with the rationale of why that should be public. It will help us not break your code in the future! 😄

I'd also be curious if your password provider is open source / what functionality it provides as I was recently looking for examples of these. 😄

@clokep clokep closed this as completed Aug 18, 2020
@clokep
Copy link
Member

clokep commented Aug 18, 2020

It looks like #7734 might be related to what you'd like?

@anoadragon453
Copy link
Member

@menturion heads up there's another modified version of that password provider here: https://github.com/ma1uta/matrix-synapse-rest-password-provider

which may or may not be useful to you :)

@anoadragon453
Copy link
Member

@menturion Ah, no, that repo may not be aware of this yet either. I'll create an issue to warn them and point to your above comment with the required changes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
z-bug (Deprecated Label)
Projects
None yet
Development

No branches or pull requests

4 participants