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

Update the auth providers to be async.#7935

Merged
clokep merged 5 commits intodevelopfrom
clokep/async-auth-providers
Jul 23, 2020
Merged

Update the auth providers to be async.#7935
clokep merged 5 commits intodevelopfrom
clokep/async-auth-providers

Conversation

@clokep
Copy link
Member

@clokep clokep commented Jul 22, 2020

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 clokep requested a review from a team July 22, 2020 17:34
@clokep
Copy link
Member Author

clokep commented Jul 22, 2020

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

anoadragon453
anoadragon453 reviewed Jul 22, 2020
View reviewed changes
Copy link
Member

@anoadragon453 anoadragon453 left a 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

synapse/handlers/ui_auth/checkers.py
"""

def check_auth(self, authdict, clientip):
async def check_auth(self, authdict: dict, clientip: str) -> Any:
Copy link
Member

@anoadragon453 anoadragon453 Jul 22, 2020

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.

Copy link
Member Author

@clokep clokep Jul 23, 2020

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.

Copy link
Member Author

@clokep clokep Jul 23, 2020

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.

Copy link
Member

@anoadragon453 anoadragon453 Jul 23, 2020

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 :)

Copy link
Member Author

@clokep clokep Jul 23, 2020

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!

@clokep clokep requested a review from anoadragon453 July 23, 2020 11:22
@clokep clokep self-assigned this Jul 23, 2020
anoadragon453
anoadragon453 approved these changes Jul 23, 2020
View reviewed changes
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

lgtm!

@clokep clokep merged commit 83434df into develop Jul 23, 2020
@clokep clokep deleted the clokep/async-auth-providers branch July 23, 2020 19:45
anoadragon453 added a commit that referenced this pull request Jul 28, 2020
@anoadragon453
Merge branch 'develop' of github.com:matrix-org/synapse into anoa/imp…
5d230b3 
…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)
  ...
@clokep clokep mentioned this pull request Jul 30, 2020
Closed
48 tasks
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@anoadragon453
Merge commit 'f88c48f3b' into anoa/dinsic_release_1_18_x
7f2e76b 
* 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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@anoadragon453 anoadragon453 anoadragon453 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@clokep clokep

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@clokep @anoadragon453