-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Update the auth providers to be async. #7935
Conversation
We should likely also note in the upgrading changes or something that any third-party providers can upgrade to be async/await? |
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.
A couple questions regarding typing, but otherwise lgtm
""" | ||
|
||
def check_auth(self, authdict, clientip): | ||
async def check_auth(self, authdict: dict, clientip: str) -> Any: |
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.
Were you holding off specifying the possibilities here so that you wouldn't need to specify Deferred
? If so, it's worth noting that isinstance(a_deferred, Awaitable)
resolves to True
, so I think something not too cumbersome like Optional[Union[str, Tuple]]
should work here.
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.
I didn't specify the possibilities because I did the typing before reading the separate documentation and the docstring didn't specify what was returned. 😄 I can double check the return types to the documentation.
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.
Oh wait, I know why I did this. (I should have ☕ before replying to things...)
Although this method is also named check_auth
, it is NOT the same as above. It gets called twice:
synapse/synapse/handlers/auth.py
Lines 426 to 430 in 4e5ec32
result = await self.checkers[stagetype].check_auth(authdict, clientip) | |
if result: | |
await self.store.mark_ui_auth_stage_complete( | |
authdict["session"], stagetype, result | |
) |
synapse/synapse/handlers/auth.py
Lines 510 to 513 in 4e5ec32
checker = self.checkers.get(login_type) | |
if checker is not None: | |
res = await checker.check_auth(authdict, clientip=clientip) | |
return res |
While the one from a password provider gets called:
synapse/synapse/handlers/auth.py
Lines 754 to 758 in 4e5ec32
result = await provider.check_auth(username, login_type, login_dict) | |
if result: | |
if isinstance(result, str): | |
result = (result, None) | |
return result |
Anyway, the result of UserInteractiveAuthChecker.check_auth
gets saved into the database and sometimes inspected in other places.
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.
Ah, fair enough then! Thanks for the clear explanation :)
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.
You're welcome! I'm now realizing that putting these changes in the same PR was confusing...since they're not really related. 😢 Sorry about that!
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.
lgtm!
…rove_test_times * 'develop' of github.com:matrix-org/synapse: (148 commits) Add script for finding files with unix line terminators (#7965) Convert the remaining media repo code to async / await. (#7947) Convert a synapse.events to async/await. (#7949) Convert groups and visibility code to async / await. (#7951) Convert push to async/await. (#7948) update changelog 1.18.0rc1 Fix error reporting when using `opentracing.trace` (#7961) Fix typing replication not being handled on master (#7959) Remove hacky error handling for inlineDeferreds. (#7950) Convert tests/rest/admin/test_room.py to unix file endings (#7953) Support oEmbed for media previews. (#7920) Convert state resolution to async/await (#7942) Fix up types and comments that refer to Deferreds. (#7945) Do not convert async functions to Deferreds in the interactive_auth_handler (#7944) Convert more of the media code to async/await (#7873) Return an empty body for OPTIONS requests. (#7886) Downgrade warning on client disconnect to INFO (#7928) Convert presence handler helpers to async/await. (#7939) Update the auth providers to be async. (#7935) ...
* commit 'f88c48f3b': 1.18.0rc1 Fix error reporting when using `opentracing.trace` (#7961) Fix typing replication not being handled on master (#7959) Remove hacky error handling for inlineDeferreds. (#7950) Convert tests/rest/admin/test_room.py to unix file endings (#7953) Support oEmbed for media previews. (#7920) Convert state resolution to async/await (#7942) Fix up types and comments that refer to Deferreds. (#7945) Do not convert async functions to Deferreds in the interactive_auth_handler (#7944) Convert more of the media code to async/await (#7873) Return an empty body for OPTIONS requests. (#7886) Downgrade warning on client disconnect to INFO (#7928) Convert presence handler helpers to async/await. (#7939) Update the auth providers to be async. (#7935) Put a cache on `/state_ids` (#7931)
This updates the built in auth providers to use async/await.
The callers were already converted to async/await so this shouldn't cause an issue. The third-party providers should still work (since you can
await
aDeferred
).I don't find the documentation here to be great:
Awaitables
instead ofDeferred
This also fixes a bug I found by reading the documentation closer --
on_logged_out
may return anAwaitable
, but doesn't have to.