Update the auth providers to be async. #7935

clokep · 2020-07-22T17:07:32Z

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 a Deferred).

I don't find the documentation here to be great:

I've updated it to refer to Awaitables instead of Deferred
I reformatted it so that it isn't all inside of block quotes.

This also fixes a bug I found by reading the documentation closer -- on_logged_out may return an Awaitable, but doesn't have to.

clokep · 2020-07-22T17:34:59Z

We should likely also note in the upgrading changes or something that any third-party providers can upgrade to be async/await?

anoadragon453

A couple questions regarding typing, but otherwise lgtm

synapse/handlers/auth.py

anoadragon453 · 2020-07-22T18:39:41Z

synapse/handlers/ui_auth/checkers.py

        """

-    def check_auth(self, authdict, clientip):
+    async def check_auth(self, authdict: dict, clientip: str) -> Any:


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.

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.

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.

Ah, fair enough then! Thanks for the clear explanation :)

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!

anoadragon453

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)

clokep added 4 commits July 22, 2020 12:37

Update the auth providers to be async.

27aafa1

Update / clarify documentation.

4e5ec32

Clarify class vs. static method.

865bb8a

Add a newsfragment.

acfbfd8

clokep requested a review from a team July 22, 2020 17:34

anoadragon453 reviewed Jul 22, 2020

View reviewed changes

Only await an awaitable.

e99f914

clokep requested a review from anoadragon453 July 23, 2020 11:22

clokep self-assigned this Jul 23, 2020

anoadragon453 approved these changes Jul 23, 2020

View reviewed changes

clokep merged commit 83434df into develop Jul 23, 2020

clokep deleted the clokep/async-auth-providers branch July 23, 2020 19:45

clokep mentioned this pull request Jul 30, 2020

Convert inlineCallbacks to async/await #7988

Closed

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the auth providers to be async. #7935

Update the auth providers to be async. #7935

clokep commented Jul 22, 2020

clokep commented Jul 22, 2020

anoadragon453 left a comment

anoadragon453 Jul 22, 2020

clokep Jul 23, 2020

clokep Jul 23, 2020

anoadragon453 Jul 23, 2020

clokep Jul 23, 2020

anoadragon453 left a comment

	result = await self.checkers[stagetype].check_auth(authdict, clientip)
	if result:
	await self.store.mark_ui_auth_stage_complete(
	authdict["session"], stagetype, result
	)

	checker = self.checkers.get(login_type)
	if checker is not None:
	res = await checker.check_auth(authdict, clientip=clientip)
	return res

	result = await provider.check_auth(username, login_type, login_dict)
	if result:
	if isinstance(result, str):
	result = (result, None)
	return result

Update the auth providers to be async. #7935

Update the auth providers to be async. #7935

Conversation

clokep commented Jul 22, 2020

clokep commented Jul 22, 2020

anoadragon453 left a comment

Choose a reason for hiding this comment

anoadragon453 Jul 22, 2020

Choose a reason for hiding this comment

clokep Jul 23, 2020

Choose a reason for hiding this comment

clokep Jul 23, 2020

Choose a reason for hiding this comment

anoadragon453 Jul 23, 2020

Choose a reason for hiding this comment

clokep Jul 23, 2020

Choose a reason for hiding this comment

anoadragon453 left a comment

Choose a reason for hiding this comment