Add integration and docs for Django Ninja authentication#1646
Add integration and docs for Django Ninja authentication#1646brianhelba wants to merge 3 commits intodjango-oauth:masterfrom
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
brianhelba
force-pushed
the
ninja
branch
2 times, most recently
from
e736064 to
74e105d
Compare
|
Tests have been added. This is ready for final review. |
|
@dopry Can you please take a look at this? |
There was a problem hiding this comment.
Pull request overview
This PR adds Django Ninja integration for django-oauth-toolkit, addressing issue #1373. It provides an authentication class (HttpOAuth2) that enables OAuth2 authentication for Django Ninja APIs, following the same pattern as the existing Django REST Framework integration.
Changes:
- Added
HttpOAuth2authentication class for Django Ninja with scope enforcement support - Added comprehensive test coverage for the new integration
- Added documentation with examples for basic usage, optional authentication, scope enforcement, and custom authorization
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
oauth2_provider/contrib/ninja/security.py |
Core authentication class implementing OAuth2 for Django Ninja |
oauth2_provider/contrib/ninja/__init__.py |
Module exports for the ninja integration |
tests/test_ninja.py |
Unit tests covering authentication scenarios including valid/invalid tokens and scope enforcement |
docs/ninja.rst |
Documentation with usage examples and API reference |
docs/index.rst |
Added ninja documentation to the table of contents |
tox.ini |
Added django-ninja dependency for testing |
CHANGELOG.md |
Added changelog entry for the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
brianhelba
force-pushed
the
ninja
branch
3 times, most recently
from
a716a25 to
f92c8e6
Compare
brianhelba
force-pushed
the
ninja
branch
4 times, most recently
from
2988878 to
08fa721
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@dopry Please take a look again. |
This is taken from django-oauth/django-oauth-toolkit#1646 and will be removed when that is released.
This is taken from django-oauth/django-oauth-toolkit#1646 and will be removed when that is released.
|
@dopry I've responded to all feedback from you and Copilot. Please take a look again. |
the upstream ninja dependency wasn't present in the doc build, configure sphinx to mock it.
|
@dopry Thanks for the help reviewing this! If any further feedback comes up, I'm happy to address it on March 16th (I'll be offline until then). |
|
|
||
| This consists of a ``HttpOAuth2`` class, which will determine whether the | ||
| incoming HTTP request contains a valid OAuth2 access token issued | ||
| by Django OAuth Toolkit. Optionally, ``HttpOAuth2`` can also ensure that the |
| # Ninja doesn't automatically set `request.user`: https://github.com/vitalik/django-ninja/issues/76 | ||
| # However, Django's AuthenticationMiddleware (which does set this from a session cookie) is | ||
| # ubiquitous, and even Ninja's own tutorials assume that `request.user` will somehow be set, | ||
| # so ensure that authentication via OAuth2 doesn't violate expectations. | ||
| request.user = r.user | ||
|
|
There was a problem hiding this comment.
This is an interesting consideration, but Copilot's interpretation is incorrect.
To restate my code comment, as Copilot doesn't seem to understand... At this point in the code valid is True. We should assume that our HttpOAuth2 handler is responsible for authenticating the entirety of this request (unfortunately, there's no specification to follow, but this seems like the only behavior that makes sense). It's possible that a session cookie was also passed with this request, in addition to OAuth2 Authorization: Bearer ... headers; in that case, an AuthenticationMiddleware would have pre-set request.user to whatever the session cookie resolves, but we want to always overwrite that to whatever the OAuth2 token resolves. We have to do this unconditionally, or else we'd have conflicting sources of truth for the request user.
However, r is an AbstractAccessToken, and r.user is nullable. What should we do if r.user is None? The answer should not depend on whether an AuthenticationMiddleware has already set request.user, because we now "own" the request's authentication and should unconditionally set the user.
I don't think we should set request.user = AnonymousUser(), because this isn't an anonymous request. It's an authenticated request, but the token simply doesn't have an associated user (which OAuth-Toolkit apparently allows).
However, while Request.user isn't a part of pure Django (it's not defined on the Request model), practically it's always patched on by AuthenticationMiddleware (which is typically enabled). So, most sensible projects (and django-stubs) assume that request.user is always set to something: either an actual user or AnonymousUser (it is not expected to be None, so people don't guard against calling something like request.user.is_authenticated).
So, what do we do? If you force me to resolve the compromise, I'd say perhaps we should set it to AnonymousUser(), so at least we won't break people's type expectations, plus I'm hoping that having AbstractAccessToken.user be None is pretty rare. However, I'd really appreciate some additional feedback @dopry.
There was a problem hiding this comment.
Thanks for taking the time to look into this. I'll try to find the time to consider it in more depth in the next few days.
dopry
left a comment
dopry
left a comment
There was a problem hiding this comment.
@brianhelba I'm concerned about the _call issue copilot raised. Can you look into that?
Fixes #1373.
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS