Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #35303 -- Added async auth backends and associated functionality #18036

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bigfootjon
Copy link
Contributor

@bigfootjon bigfootjon commented Mar 31, 2024

Trac ticket number

ticket-35303

Branch description

This is the culmination of the project to asyncify the contrib.auth module. This PR contains 3 things:

  1. An update to the BaseBackend and all built-in backends (and related code) to provide async logic
  2. An update to the documented (public) auth interface to use these new backends
  3. An improvement to the RemoteUser middleware to take advantage of the new async-native logic

Each item above is a separate commit below, for easier convenience in reviewing. If requested, I'm happy to split this PR up into multiple PRs or combine commits.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • For UI changes, I have attached screenshots in both light and dark modes.

@nessita
Copy link
Contributor

nessita commented Apr 1, 2024

Hi Jon! Thank you for your contribution.

I see this is a draft PR, please remember that when this work is ready for review, you need to adjust the ticket flags in the Trac system so this PR gets added to the "branches needing review" section of the Django Developer Dahsboard. More information in these docs.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hey @bigfootjon — Thanks for this. It looks good.

I don't think there's anything we can do about it — certainly not to block this PR about it! – but, I still find the duplication over the function color a bit… erm… sad. 🙂 We've got ≈identical skeletons differing in ≈only whether the internal helper is awaited or not. It would be nice to reduce that where possible over time.

About half-way down, that caused me to start scanning a little, so I can't say I looked at every code path, but looks good at the high level. 👍

@sarahboyce
Copy link
Contributor

Hi @bigfootjon 👋 thank you for this PR
I have reviewed the list of new features with @nessita and there are some other PRs which we are already in progress reviewing and will likely struggle to review this in time for the 5.1 feature freeze on 22nd May.
Planning to pick this up after the feature freeze 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants