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

Feat: add async support for auth handler #1045

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat: add async support for auth handler
IA-PieroCV committed Dec 3, 2024
commit c71ba6569472ff187a97e3a296c67759230119cb
4 changes: 2 additions & 2 deletions robyn/authentication.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from abc import ABC, abstractmethod
from typing import Optional
from typing import Any, Coroutine, Optional, Union

from robyn.robyn import Headers, Identity, Request, Response
from robyn.status_codes import HTTP_401_UNAUTHORIZED
@@ -64,7 +64,7 @@ def unauthorized_response(self) -> Response:
)

@abstractmethod
def authenticate(self, request: Request) -> Optional[Identity]:
def authenticate(self, request: Request) -> Union[Optional[Identity], Coroutine[Any, Any, Optional[Identity]]]:
Copy link
Member

Choose a reason for hiding this comment

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

Hey @IA-PieroCV 👋

Thanks for the PR but why do we need this type?

Copy link
Author

@IA-PieroCV IA-PieroCV Nov 23, 2024

Choose a reason for hiding this comment

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

Hey @sansyrox !
This is for typing complying. I'm using pyright and this is making it. Guess with mypy, pyre and others will work as well.
As the child classes can override sync method to async methods, authenticate should be awared of returning both sync result (Identity | None) and async result (Coroutine[Any, Any, Identity | None]).
Edit: Adding more context

Copy link
Member

Choose a reason for hiding this comment

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

@IA-PieroCV 👋

Should we use a type alias instead? The return type is not super readable

"""
Authenticates the user.
:param request: The request object.
9 changes: 7 additions & 2 deletions robyn/router.py
Original file line number Diff line number Diff line change
@@ -290,10 +290,15 @@ def add_auth_middleware(self, endpoint: str):

def decorator(handler):
@wraps(handler)
def inner_handler(request: Request, *args):
async def inner_handler(request: Request, *args):
Copy link
Member

Choose a reason for hiding this comment

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

and what about sync support?

Copy link
Author

Choose a reason for hiding this comment

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

I tested with both sync and async and both worked pretty well...
However, I'm kind of worried not showing support for both sync and async. I'll make some tests and will back with the results

if not self.authentication_handler:
raise AuthenticationNotConfiguredError()
identity = self.authentication_handler.authenticate(request)

if inspect.iscoroutinefunction(self.authentication_handler.authenticate):
identity = await self.authentication_handler.authenticate(request)
else:
identity = self.authentication_handler.authenticate(request)

if identity is None:
return self.authentication_handler.unauthorized_response
request.identity = identity