-
Notifications
You must be signed in to change notification settings - Fork 262
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
Authenticators Factory #794
base: develop
Are you sure you want to change the base?
Authenticators Factory #794
Conversation
) -> None | str: | ||
"""Authenticate endpoint. | ||
|
||
Check the provided key is available in API keys list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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 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? |
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 |
Totally agree |
3341194
to
6062ef2
Compare
|
||
def check_api_key(request: Request, api_key: str = Security(api_key_header)) -> None | str: | ||
""" | ||
return websocket.app.state.ccat.authorizator.is_ws_allowed(websocket) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case you still need to check the master_key as in the http request, otherwise the admin panel will break if the dev implements a custom rule in is_allowed_ws
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so sure about this, I try to expand a little bit my consideration.
If the idea is to keep backward compatible the actual behavior, with this solution the websocket remains open.
So setting an API_KEY any endpoint will be protected and the websocket not.
What do you think?
I involve even @zAlweNy26 that knows how the admin panel works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho the websocket by default should remain open, also because in browser environment it's not possible to set headers to the WebSocket instance.
Description
Based on the discussion #690
Related to issue #(issue)
Type of change
Checklist: