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

Conversation

IA-PieroCV
Copy link

@IA-PieroCV IA-PieroCV commented Nov 22, 2024

Description

This PR add the possibility to run async authenticate on AuthenticationHandler.

Summary

This PR changes adds the typing hints for Authentication handler in order to return an Identity that comes from a Sync process or a Coroutine (Async). Also, it modifies the add_auth_middleware function to run this authenticate using async.

FYI: This allows AuthenticationHandler to support async sessions on cache (on memory db like redis).

class RedisAuthHandler(AuthenticationHandler):
    """Redis Robyn auth handler using async Redis."""

    # Inheritance on abstract method allows both async and sync child methods.
    async def authenticate(self, request: Request) -> Identity | None: # Python 3.10 typing syntax
        """Authenticate Handler in Redis."""
        token = self.token_getter.get_token(request)
        if not token:
            return None

        user_id = await RedisClient.get_user_from_token(token) # <-- Important part. To the user this is just straightforward
        if user_id:
            return Identity(claims={"user_id": str(user_id)})
        return None

PR Checklist

Please ensure that:

  • The PR contains a descriptive title
  • The PR contains a descriptive summary of the changes
  • You build and test your changes before submitting a PR.
  • You have added relevant documentation Minor feature, so current API Reference explains the usage.
  • You have added relevant tests. We prefer integration tests wherever possible Minor feature, so current auth testing worked for both scenarios (sync and async).

Pre-Commit Instructions:

EDIT: Add some code context

Copy link

vercel bot commented Nov 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
robyn ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2024 4:59am

@@ -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

robyn/router.py Outdated
@@ -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

@sansyrox
Copy link
Member

Hey @IA-PieroCV 👋

I will have a proper review over the weekend 😄

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

Hey @IA-PieroCV 👋

I really like the implementation. Do we need to add some docs for this?

Also, would love some integration tests

@IA-PieroCV
Copy link
Author

Hey @sansyrox !
I guess I can add a tab to the mdx documentation with "sync"/"async" if this is okey. I don't think is needed to be explicit, but to mention it briefly and show the capability. Are you okey with this approach?

About integration test, of course! I'll implement it in the next couple of hours, it doesn't seem like a problem.

@IA-PieroCV
Copy link
Author

Hey @sansyrox!
As the middlewares are added on runtime the evaluation of coroutine cannot be made at first hand.
I know that we were discussing how endpoints could be registered at the start method and part of the PR I made on #1056 takes this approach too. This would solve this as the configuration should be implemented before registering the auth middleware.

I'm trying to look for an approach that solves this without the change on the registration approach, but this could take me some time. By now I will convert this PR to draft until I found a solution or having the endpoint registration on the start method (if it's done).

@dave42w
Copy link
Contributor

dave42w commented Dec 3, 2024

Hey @sansyrox! As the middlewares are added on runtime the evaluation of coroutine cannot be made at first hand. I know that we were discussing how endpoints could be registered at the start method and part of the PR I made on #1056 takes this approach too. This would solve this as the configuration should be implemented before registering the auth middleware.

I like the approach that we basically register routes, middleware, auth, routers after app = Robyn(...) and before app.start(...)

Then in app.start(), before we call run_processes()...) we prepare everything that has been registered. This makes the registration much easier to test and it avoids any restrictions on the order developers register all the parts of their application. Once applications grow it makes it easier for whole departments with their own combinations of routes, middleware etc to be plugged into the whole with one include.

I think it is going to make the whole prepare step much easier. We choose when to process authentication, subrouters, routes etc rather than the order the code got put together.

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

Successfully merging this pull request may close these issues.

3 participants