cheshire-cat-ai / core

Production ready AI agent framework
https://cheshirecat.ai
GNU General Public License v3.0
2.14k stars 282 forks source link

[WIP] Simplified auth handler #853

Closed lucapirrone closed 1 week ago

lucapirrone commented 3 weeks ago

Description

Simplified auth handler to allow custom token extraction and custom token authorization.

Related to issue #(issue)

Type of change

Checklist:

lucapirrone commented 3 weeks ago

Some info about the proposal:

Permissions Management

functions http_auth and ws_auth are factory functions that needs resource and permission to be instantiated. In that way the AuthHandler can implement granuar authorization. Both http_auth and ws_auth validate core authorization first with the local idp (ccat.core_auth_handler) and then validate the custom authorization from the implemented auth handler (ccat.custom_auth_handler). One of the two is enough to validate the request.

image

AuthResource and AuthPermission are defined enums:

image

Every protected API route needs to implement the http_auth at the route point and not at the router

image

CoreAuthHandler

The core auth handler verify token with the local idp. First of all is checked the API_KEY for legacy usage, then is validated the jwt token. If neither pass then the check is made on CustomAuthHandler

image

CustomAuthHandler

This is the AuthHandler abstraction that allows to implement custom auth handler. It contains:

class BaseAuthHandler(ABC): """ Base class to build custom Auth systems that will live alongside core auth. Methodsauthorize_user_from_token` MUST be implemented by subclasses. """

@abstractmethod
async def authorize_user_from_token(self, credential: str, auth_resource: AuthResource, auth_persmission: AuthPermission) -> AuthUserInfo | None:
    pass

async def http_extract_credential(self, request: Request) -> str | None:
    """
    Get JWT token or api key passed with the request
    """

    # Proper Authorization header
    authorization_header = request.headers.get("Authorization")
    if authorization_header and ("Bearer " in authorization_header):
        return authorization_header.replace("Bearer ", "")

    # Legacy header to pass CCAT_API_KEY
    access_token_header = request.headers.get("access_token")
    if access_token_header:
        log.warning(
            "Deprecation Warning: `access_token` header will not be supported in v2."
            "Pass your token/key using the `Authorization: Bearer <token>` format."
        )
        return access_token_header

    # no token found
    return None  

async def ws_extract_credential(self, websocket: WebSocket) -> AuthUserInfo | None:
    """
    Extract token from WebSocket query string
    """
    query_params = parse_qs(websocket.url.query)
    return query_params.get("token", [None])[0]

`

pieroit commented 3 weeks ago

Awesome @lucapirrone thanks! Tell me when can I work on it and merge, I'm entusiatic about this feature. After this I'd like to dedicate time to docs, tutorial and advanced plugins.

A few observations:

- Depends in endpoint method va dependencies in router decorator

By having the dependency in the @router decorator, we cannot have the stray available inside the endpoint code. Here the fastAPI docs say:

In some cases you don't really need the return value of a dependency inside your path operation function.
Or the dependency doesn't return a value.
But you still need it to be executed/solved.
For those cases, instead of declaring a path operation function parameter with Depends, you can add a list of dependencies to the path operation decorator.

What we want ultimately is the stray available in the actual endpoint function, something like:

@route.get("/something/")
async def read_something(
    stray: StrayCat = Depends(http_auth)
):
    # stray is needed here for actually do something related to the user and the session
    pass

Since it points to a simple function Depends cannot have external args (resources and permissions), we could rely on this class based Depends. Don't know if there is an easier way :/ I am available for the change, no stress ;)

- Custom http_extract_credential and ws_extract_credential

Strongly disagree on this one, as I see better to make all the auth pass via a specific standard:

Custom verification and user info is fine, while different standards to communicate tokens and api keys is not (leads fragmentation, confusion and client incompatibility). My bad I did not recognize this before.

- Admin auth modal

I would go for a unique login form under its specific route, able to redirect logged request back, instead of a modal into the admin. Reasons:

Waiting for your cue to merge! Thanks a lot for patience and skills :heart: :cat2:

lucapirrone commented 2 weeks ago

Hi @pieroit!

I made some updates about your ovservations:

pieroit commented 1 week ago

Can I merge? :D

lucapirrone commented 1 week ago

Yep

pieroit commented 1 week ago

Thanks a lot for your contribution @lucapirrone I merged with several additions, it's on develop. Many things are still to be refined and fixed, next dev meeting we can talk about it (or when you want)