-
Notifications
You must be signed in to change notification settings - Fork 9
Adding async auth #46
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
base: master
Are you sure you want to change the base?
Conversation
79a7477 to
2658e97
Compare
|
Thanks for working on this! That decorator is really cool, I'm definitely stealing that lol. I think the biggest problem in implementing hybrid sync/async is that Shinobi's auth expects a Callable, this is also a hot path so I'd like to avoid as much additional logic as possible. I think the decorator does help us keep this flow mostly intact, which is good for compatibility. I think it would be easier for users to use if they had specific functions to override in the class rather than having them use the decorator themselves. So something like class HybridAuth(AuthBase, ABC):
is_async = True # HybridAuth will always be asyncable
@abstractmethod
def check_auth(self, request: HttpRequest) -> Optional[Any]:
pass
@abstractmethod
async def check_auth_async(self, request: HttpRequest) -> Optional[Any]:
pass
# Then define __call__ using the decorator to call the two methods abovebut I'd be interested to hear your thoughts. I think the PR is going in the right direction overall so I'll go through and add some comments too. This is a bit of a ramble but just to brainstorm a bit if there's anyway to improve this, I thought about what this might look like if hybrid auth was considered from the start. I think if the logic of mode switching was pushed into the AuthBase class, we could do both hybrid and single mode from one class. I think from a user's perspective, it would make it easier to transition to using hybrid auth. class AuthBase:
# Users implement one or both of the auth functions
def check_auth(self, request: HttpRequest) -> Optional[Any]:
pass
async def check_auth_async(self, request: HttpRequest) -> Optional[Any]:
pass
def __init__(self):
# Do something to figure out which functions were subclassed
# If we're missing one, wrap the other to substitute it in
if sync_is_not_defined:
self.check_auth = sync_to_async(self.check_auth_async)
if async_is_not_defined:
self.check_auth_async = async_to_sync(self.check_auth)
...Then in def _run_authentication(self, request: HttpRequest) -> Optional[HttpResponse]:
for callback in self.auth_callbacks:
try:
if isinstance(callback, AuthBase):
result = callback.check_auth()
elif is_async_callable(callback):
result = callback(request)
if inspect.iscoroutine(result):
result = async_to_sync(callback)(request)
else:
result = callback(request)
except Exception as exc:
return self.api.on_exception(request, exc)
...I'd be interested to hear your thoughts on something like this too. This isn't doable right now but if we make a class like HybridAuth, maybe we can push towards something like this in the future. |
|
Hey @pmdevita, Thanks for all the great input, I had a look at your suggestion and wanted to point out some issues I ran into so we can discuss the pros and cons before I put more effort into this. I'm confident I can figure it out but wanted to hear your thoughts on this first. The problem with the HybridAuth class is there are many base auth classes with their own
So to make those hybrid compatible I have to update those to use this base class, I am thinking this will risk falling into multiple inheritance problems and being more buggy as the logic will be spread across multiple auth classes. I will have to make significant changes to django-shinobi that will be breaking changes from djanog-ninja. The current logic decorator has the advantage of not using inheritance, therefore not risking unintended side effects of users subclassing and breaking hybrid auth, we also we maintain backwards compatibility. So I think it comes to a toss up of what do we prefer:
Looking forward to hearing your thoughts! |
This pull request adds support for hybrid (sync and async) authentication in Django Ninja, making both the default and custom authentication classes compatible with asynchronous endpoints.
It introduces a new
@asyncabledecorator to define authentication methods that can be called in both synchronous and asynchronous contexts, updates the default session-based authentication classes to use this pattern, and ensures that synchronous authentication functions are properly handled in async contexts. The documentation and test suite are also updated to reflect and verify these changes.Hybrid Authentication Support and Decorator:
@asyncabledecorator inninja/decorators.py, I used this blog post as a base for this logic but updated the logic to make it more robust.docs/docs/guides/authentication.md) to explain hybrid authentication, the use of@asyncable, and how Django Ninja now supports async authentication by default.Default Authentication Classes:
SessionAuth,SessionAuthSuperUser, andSessionAuthIsStaffinninja/security/session.pyto use the new@asyncabledecorator, providing both sync and asyncauthenticatemethods for each class.Testing and Verification:
tests/test_auth_async.pywith new tests to validate hybrid authentication behavior for both sync and async endpoints, including custom and session-based authentication classes.