Robyn icon indicating copy to clipboard operation
Robyn copied to clipboard

Feat: add async support for auth handler

Open IA-PieroCV opened this issue 1 year ago • 6 comments

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:

  • [X] The PR contains a descriptive title
  • [X] The PR contains a descriptive summary of the changes
  • [X] 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

IA-PieroCV avatar Nov 22 '24 17:11 IA-PieroCV

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

vercel[bot] avatar Nov 22 '24 17:11 vercel[bot]

Hey @IA-PieroCV 👋

I will have a proper review over the weekend 😄

sansyrox avatar Nov 26 '24 23:11 sansyrox

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 avatar Dec 03 '24 01:12 IA-PieroCV

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).

IA-PieroCV avatar Dec 03 '24 17:12 IA-PieroCV

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.

dave42w avatar Dec 03 '24 18:12 dave42w

😱 Found 1 issue. Time to roll up your sleeves! 😱

recurseml[bot] avatar May 27 '25 19:05 recurseml[bot]