-
Notifications
You must be signed in to change notification settings - Fork 269
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
from os import getenv | ||
from typing import Type | ||
from cat.log import log | ||
from pydantic import BaseModel, ConfigDict | ||
from cat.factory.custom_authorizator import BaseAuth, AuthorizatorNoAuth, AuthorizatorApiKey | ||
from cat.mad_hatter.mad_hatter import MadHatter | ||
|
||
class AuthorizatorSettings(BaseModel): | ||
_pyclass: Type[BaseAuth] = None | ||
|
||
@classmethod | ||
def get_authorizator_from_config(cls, config): | ||
if cls._pyclass is None or issubclass(cls._pyclass.default, BaseAuth) is False: | ||
raise Exception( | ||
"Authorizator configuration class has self._pyclass==None. Should be a valid Authorizator class" | ||
) | ||
return cls._pyclass.default(**config) | ||
|
||
class AuthorizatorNoAuthConfig(AuthorizatorSettings): | ||
_pyclass: Type = AuthorizatorNoAuth | ||
|
||
model_config = ConfigDict( | ||
json_schema_extra={ | ||
"humanReadableName": "No Authorizator", | ||
"description": "No authorizator is used. All requests are allowed.", | ||
"link": "", | ||
} | ||
) | ||
|
||
class AuthorizatorApiKeyConfig(AuthorizatorSettings): | ||
api_key: str | ||
_pyclass: Type = AuthorizatorApiKey | ||
|
||
model_config = ConfigDict( | ||
json_schema_extra={ | ||
"humanReadableName": 'API Key Authorizator', | ||
"description": 'Authoriza requests based on API key', | ||
"link": "", | ||
} | ||
) | ||
|
||
def get_allowed_authorizator_strategies(): | ||
list_authorizator_default = [ | ||
AuthorizatorNoAuthConfig, | ||
AuthorizatorApiKeyConfig, | ||
] | ||
|
||
mad_hatter_instance = MadHatter() | ||
list_authorizator = mad_hatter_instance.execute_hook( | ||
"factory_allowed_authorizators", list_authorizator_default, cat=None | ||
) | ||
|
||
return list_authorizator | ||
|
||
def get_authorizators_schemas(): | ||
AUTHORIZATOR_SCHEMAS = {} | ||
for config_class in get_allowed_authorizator_strategies(): | ||
schema = config_class.model_json_schema() | ||
schema["auhrizatorName"] = schema["title"] | ||
AUTHORIZATOR_SCHEMAS[schema["title"]] = schema | ||
|
||
return AUTHORIZATOR_SCHEMAS | ||
|
||
def get_authorizator_from_name(name): | ||
list_authorizator = get_allowed_authorizator_strategies() | ||
for authorizator in list_authorizator: | ||
if authorizator.__name__ == name: | ||
return authorizator | ||
return None |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
from os import getenv | ||
from fastapi import ( | ||
WebSocket, | ||
Request, | ||
HTTPException | ||
) | ||
|
||
from cat.log import log | ||
|
||
class BaseAuth(): | ||
def __init__(self): | ||
self.master_key = getenv("API_KEY") | ||
|
||
def is_master_key(self, request): | ||
if self.is_master_key == None: | ||
return True | ||
return request.headers.get("access_token") == self.master_key | ||
|
||
def is_http_allowed(self, request): | ||
return self.is_master_key(request) | ||
|
||
def is_ws_allowed(self, websocket): | ||
return True | ||
|
||
class AuthorizatorNoAuth(BaseAuth): | ||
def __init__(self): | ||
pass | ||
|
||
def is_master_key(self, request): | ||
return True | ||
|
||
def is_http_allowed(self, request: Request): | ||
return True | ||
|
||
def is_ws_allowed(self, websocket: WebSocket): | ||
return True | ||
|
||
class AuthorizatorApiKey(BaseAuth): | ||
def __init__(self, api_key): | ||
self.api_key = api_key | ||
super().__init__() | ||
|
||
def is_master_key(self, request): | ||
if self.master_key == None: | ||
raise HTTPException( | ||
status_code=403, | ||
detail={"error": "Master key is not set"} | ||
) | ||
return super().is_master_key(request) | ||
|
||
def is_http_allowed(self, request: Request): | ||
if request.headers.get("Authorization") != self.api_key: | ||
raise HTTPException( | ||
status_code=403, | ||
detail={"error": "Invalid API Key"} | ||
) | ||
return True | ||
|
||
def is_ws_allowed(self, websocket: WebSocket): | ||
if websocket.headers.get("Authorization") != self.api_key: | ||
raise HTTPException( | ||
status_code=403, | ||
detail={"error": "Invalid API Key"} | ||
) | ||
return True |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,13 @@ | ||
import os | ||
import fnmatch | ||
|
||
from fastapi import Request | ||
from typing import Annotated | ||
from cat.log import log | ||
|
||
from fastapi import ( | ||
WebSocket, | ||
Request, | ||
) | ||
from fastapi import Security, HTTPException | ||
from fastapi.security.api_key import APIKeyHeader | ||
|
||
|
@@ -18,8 +24,32 @@ | |
|
||
api_key_header = APIKeyHeader(name="access_token", auto_error=False) | ||
|
||
def ws_auth( | ||
websocket: WebSocket, | ||
) -> None | str: | ||
"""Authenticate endpoint. | ||
|
||
Check the provided key is available in API keys list. | ||
|
||
Parameters | ||
---------- | ||
request : Request | ||
HTTP request. | ||
|
||
Returns | ||
------- | ||
api_key : str | None | ||
Returns the valid key if set in the `.env`, otherwise return None. | ||
|
||
Raises | ||
------ | ||
HTTPException | ||
Error with status code `403` if the provided key is not valid. | ||
|
||
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 commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't agree with keeping websocket chat open, this is a terrible security issue. If a dev wants to implement custom security autorizer for the websocket connection, it should not be public but the admin still needs to work. In the default authorizer "BaseAuth" the check on is_ws_allowed returns true so by default the old admin should works until the dev implement an authorizer with custom is_ws_allowed logic. To avoid having backward compatibility problems we need to extend the admin panel to somehow pass the api key in websocket connection. Little recap: implementing this websocket check -> authorizator.is_ws_master_key(websocket) or authorizator.is_ws_allowed(websocket) what do you think? @giovannialbero1992 @zAlweNy26 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we are overcomplicating because two things happen:
We currently understand what is going on but this could be really hard to get for newbies. I take it as my own fault, for insisting so much on both having no users, and having the admin included in core. Admin userThere will be an user system into core, and as a start it will only include the When admin panel opens, it can do as follows:
JWT is used for:
Machine 2 machineAs it is now, In M2M communication, as far as I know, there is no need for a JWT and keys can be used directly. Summary
Custom auth stays as @giovannialbero1992 implemented it, with minor modifications. |
||
|
||
def http_auth(request: Request) -> None | str: | ||
"""Authenticate endpoint. | ||
|
||
Check the provided key is available in API keys list. | ||
|
@@ -42,16 +72,13 @@ def check_api_key(request: Request, api_key: str = Security(api_key_header)) -> | |
Error with status code `403` if the provided key is not valid. | ||
|
||
""" | ||
if not API_KEY: | ||
return None | ||
if fnmatch.fnmatch(request.url.path, "/admin*"): | ||
authorizator = request.app.state.ccat.authorizator | ||
if authorizator.is_master_key(request) or authorizator.is_http_allowed(request): | ||
return None | ||
if api_key in API_KEY: | ||
return api_key | ||
else: | ||
raise HTTPException( | ||
status_code=403, | ||
detail={"error": "Invalid API Key"} | ||
detail={"error": "Invalid Credentials"} | ||
) | ||
|
||
|
||
|
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?