[WIP] Authenticators Factory

[giovannialbero1992]

Description

Based on the discussion #690

Related to issue #(issue)

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas

[sambarza]

Just a note about of the doc. The check_ws_with_authorizator doesn't know how the authenticator will do the authorization check, do you agree?

[lucagobbi]

Hi @giovannialbero1992 I think you are doing a great job, this is really flexible and easy to implement. I propose also to have a generic abstract class from which custom authorizators should inherit, since is_allowed and is_allowed_ws methods should be the same for every implementation:

class Auth(ABC):
    @abstractmethod
    def is_allowed(self, request: Request) -> bool:
        """
        Abstract method to validate if the HTTP request is allowed based on the implemented
        authentication and authorization mechanism.
        This method should return True if the request is allowed, or False otherwise.
        """
        pass

    @abstractmethod
    def is_allowed_ws(self, websocket: WebSocket) -> bool:
        """
        Abstract method to validate if the WebSocket connection is allowed based on the
        implemented authentication and authorization mechanism.
        This method should return True if the connection is allowed, or False otherwise.
        """
        pass

then the AuthSettings model should look something like this:

class AuthSettings(BaseModel):
    _pyclass: Type[Auth] = None

    @classmethod
    def get_authorizator_from_config(cls, config: Dict):
        if cls._pyclass is None or not issubclass(cls._pyclass, Auth):
            raise ValueError(
                "Invalid configuration: _pyclass must be a subclass of Auth."
            )
        return cls._pyclass(**config)

I think this enforcement ensures consistency across different auth strategies, provides type safety on settings, reducing type errors and gives an easier way to devs to implement this.

[pieroit]

Hi @giovannialbero1992 I think you are doing a great job, this is really flexible and easy to implement. I propose also to have a generic abstract class from which custom authorizators should inherit, since is_allowed and is_allowed_ws methods should be the same for every implementation:

class Auth(ABC):
    @abstractmethod
    def is_allowed(self, request: Request) -> bool:
        """
        Abstract method to validate if the HTTP request is allowed based on the implemented
        authentication and authorization mechanism.
        This method should return True if the request is allowed, or False otherwise.
        """
        pass

    @abstractmethod
    def is_allowed_ws(self, websocket: WebSocket) -> bool:
        """
        Abstract method to validate if the WebSocket connection is allowed based on the
        implemented authentication and authorization mechanism.
        This method should return True if the connection is allowed, or False otherwise.
        """
        pass

then the AuthSettings model should look something like this:

class AuthSettings(BaseModel):
    _pyclass: Type[Auth] = None

    @classmethod
    def get_authorizator_from_config(cls, config: Dict):
        if cls._pyclass is None or not issubclass(cls._pyclass, Auth):
            raise ValueError(
                "Invalid configuration: _pyclass must be a subclass of Auth."
            )
        return cls._pyclass(**config)

I think this enforcement ensures consistency across different auth strategies, provides type safety on settings, reducing type errors and gives an easier way to devs to implement this.

Agree on the parent class, also allows for more methods we may add later that may be overrideen or not from a subclass

P.S.: why ABS and not directly pydantic BaseModel as langchain does?
The latest covers having configs saved as dictionaries and calling _pyclass(**config), I don't how it works for ABC - maybe BaseModel is not needed

[lucagobbi]

Hi @giovannialbero1992 I think you are doing a great job, this is really flexible and easy to implement. I propose also to have a generic abstract class from which custom authorizators should inherit, since is_allowed and is_allowed_ws methods should be the same for every implementation:

class Auth(ABC):
    @abstractmethod
    def is_allowed(self, request: Request) -> bool:
        """
        Abstract method to validate if the HTTP request is allowed based on the implemented
        authentication and authorization mechanism.
        This method should return True if the request is allowed, or False otherwise.
        """
        pass

    @abstractmethod
    def is_allowed_ws(self, websocket: WebSocket) -> bool:
        """
        Abstract method to validate if the WebSocket connection is allowed based on the
        implemented authentication and authorization mechanism.
        This method should return True if the connection is allowed, or False otherwise.
        """
        pass

then the AuthSettings model should look something like this:

class AuthSettings(BaseModel):
    _pyclass: Type[Auth] = None

    @classmethod
    def get_authorizator_from_config(cls, config: Dict):
        if cls._pyclass is None or not issubclass(cls._pyclass, Auth):
            raise ValueError(
                "Invalid configuration: _pyclass must be a subclass of Auth."
            )
        return cls._pyclass(**config)

I think this enforcement ensures consistency across different auth strategies, provides type safety on settings, reducing type errors and gives an easier way to devs to implement this.

Agree on the parent class, also allows for more methods we may add later that may be overrideen or not from a subclass

P.S.: why ABS and not directly pydantic BaseModel as langchain does? The latest covers having configs saved as dictionaries and calling _pyclass(**config), I don't how it works for ABC - maybe BaseModel is not needed

If you are referring to AuthSettings I'd still keep the pydantic model, since it needs data validation on the auth class you choose, but the auth class imho should be a simple interface (i.e. an abstract class in python) with focus on behaviour rather than data validatio. Having a pydantic model here seems quite an overhead to me. Isn't it?

[pieroit]

If you are referring to AuthSettings I'd still keep the pydantic model, since it needs data validation on the auth class you choose, but the auth class imho should be a simple interface (i.e. an abstract class in python) with focus on behaviour rather than data validatio. Having a pydantic model here seems quite an overhead to me. Isn't it?

@lucagobbi fair enough!! Asking also to @Pingdred if you want to chip in

[Pingdred]

Hi @giovannialbero1992 I think you are doing a great job, this is really flexible and easy to implement. I propose also to have a generic abstract class from which custom authorizators should inherit, since is_allowed and is_allowed_ws methods should be the same for every implementation:

class Auth(ABC):
    @abstractmethod
    def is_allowed(self, request: Request) -> bool:
        """
        Abstract method to validate if the HTTP request is allowed based on the implemented
        authentication and authorization mechanism.
        This method should return True if the request is allowed, or False otherwise.
        """
        pass

    @abstractmethod
    def is_allowed_ws(self, websocket: WebSocket) -> bool:
        """
        Abstract method to validate if the WebSocket connection is allowed based on the
        implemented authentication and authorization mechanism.
        This method should return True if the connection is allowed, or False otherwise.
        """
        pass

then the AuthSettings model should look something like this:

class AuthSettings(BaseModel):
    _pyclass: Type[Auth] = None

    @classmethod
    def get_authorizator_from_config(cls, config: Dict):
        if cls._pyclass is None or not issubclass(cls._pyclass, Auth):
            raise ValueError(
                "Invalid configuration: _pyclass must be a subclass of Auth."
            )
        return cls._pyclass(**config)

I think this enforcement ensures consistency across different auth strategies, provides type safety on settings, reducing type errors and gives an easier way to devs to implement this.

Agree on the parent class, also allows for more methods we may add later that may be overrideen or not from a subclass

P.S.: why ABS and not directly pydantic BaseModel as langchain does? The latest covers having configs saved as dictionaries and calling _pyclass(**config), I don't how it works for ABC - maybe BaseModel is not needed

If you are referring to AuthSettings I'd still keep the pydantic model, since it needs data validation on the auth class you choose, but the auth class imho should be a simple interface (i.e. an abstract class in python) with focus on behaviour rather than data validatio. Having a pydantic model here seems quite an overhead to me. Isn't it?

Totally agree