From a04397f59d29e75a87e87d0aa09fe7d33086a036 Mon Sep 17 00:00:00 2001 From: Francia Csaba Date: Mon, 22 Apr 2024 14:49:42 +0200 Subject: [PATCH] Use verify_permission in routes --- amarillo/models/User.py | 2 +- amarillo/routers/agency.py | 10 +++--- amarillo/routers/carpool.py | 16 +++++----- amarillo/routers/users.py | 33 ++++++-------------- amarillo/services/oauth2.py | 50 ++++++++++-------------------- amarillo/tests/test_permissions.py | 5 +-- 6 files changed, 43 insertions(+), 73 deletions(-) diff --git a/amarillo/models/User.py b/amarillo/models/User.py index d94eacf..c48d49e 100644 --- a/amarillo/models/User.py +++ b/amarillo/models/User.py @@ -21,7 +21,7 @@ class User(BaseModel): min_length=8, max_length=256, examples=["$2b$12$EixZaYVK1fsbw1ZfbX3OXePaWxn96p36WQoeG6Lruj3vjPGga31lW"]) - permissions: Optional[List[Annotated[str, Field(pattern=r'^[a-z0-9]+(:[a-z]+)?$')]]] = Field([], + permissions: Optional[List[Annotated[str, Field(pattern=r'^[a-z0-9-]+(:[a-z]+)?$')]]] = Field([], description="The permissions of this user, a list of strings in the format or ", max_length=256, # pattern=r'^[a-zA-Z0-9]+(:[a-zA-Z]+)?$', #TODO diff --git a/amarillo/routers/agency.py b/amarillo/routers/agency.py index a40d424..9047eb4 100644 --- a/amarillo/routers/agency.py +++ b/amarillo/routers/agency.py @@ -5,8 +5,8 @@ from typing import List from fastapi import APIRouter, HTTPException, status, Depends from amarillo.models.Carpool import Carpool, Agency -from amarillo.routers.users import verify_permission_for_same_agency_or_admin -from amarillo.services.oauth2 import get_current_agency +from amarillo.models.User import User +from amarillo.services.oauth2 import get_current_user, verify_permission # TODO should move this to service from amarillo.routers.carpool import store_carpool, delete_agency_carpools_older_than from amarillo.services.agencies import AgencyService @@ -33,7 +33,7 @@ router = APIRouter( status.HTTP_404_NOT_FOUND: {"description": "Agency not found"}, }, ) -async def get_agency(agency_id: str, admin_api_key: str = Depends(get_current_agency)) -> Agency: +async def get_agency(agency_id: str, requesting_user: User = Depends(get_current_user)) -> Agency: agencies: AgencyService = container['agencies'] agency = agencies.get_agency(agency_id) agency_exists = agency is not None @@ -62,8 +62,8 @@ async def get_agency(agency_id: str, admin_api_key: str = Depends(get_current_ag status.HTTP_500_INTERNAL_SERVER_ERROR: { "description": "Import error"} }) -async def sync(agency_id: str, requesting_agency_id: str = Depends(get_current_agency)) -> List[Carpool]: - await verify_permission_for_same_agency_or_admin(agency_id, requesting_agency_id) +async def sync(agency_id: str, requesting_user: User = Depends(get_current_user)) -> List[Carpool]: + verify_permission(f"{agency_id}:sync") if agency_id == "ride2go": import_function = import_ride2go diff --git a/amarillo/routers/carpool.py b/amarillo/routers/carpool.py index 8310189..a9c7b28 100644 --- a/amarillo/routers/carpool.py +++ b/amarillo/routers/carpool.py @@ -9,8 +9,8 @@ from fastapi import APIRouter, Body, HTTPException, status, Depends from datetime import datetime from amarillo.models.Carpool import Carpool -from amarillo.routers.users import verify_permission_for_same_agency_or_admin -from amarillo.services.oauth2 import get_current_agency +from amarillo.models.User import User +from amarillo.services.oauth2 import get_current_user, verify_permission from amarillo.tests.sampledata import examples @@ -33,8 +33,8 @@ router = APIRouter( }) async def post_carpool(carpool: Carpool = Body(..., examples=examples), - requesting_agency_id: str = Depends(get_current_agency)) -> Carpool: - await verify_permission_for_same_agency_or_admin(carpool.agency, requesting_agency_id) + requesting_user: User = Depends(get_current_user)) -> Carpool: + verify_permission(f"{carpool.agency}:write", requesting_user) logger.info(f"POST trip {carpool.agency}:{carpool.id}.") await assert_agency_exists(carpool.agency) @@ -54,7 +54,9 @@ async def post_carpool(carpool: Carpool = Body(..., examples=examples), status.HTTP_404_NOT_FOUND: {"description": "Carpool not found"}, }, ) -async def get_carpool(agency_id: str, carpool_id: str, api_key: str = Depends(get_current_agency)) -> Carpool: +async def get_carpool(agency_id: str, carpool_id: str, requesting_user: User = Depends(get_current_user)) -> Carpool: + verify_permission(f"{agency_id}:read", requesting_user) + logger.info(f"Get trip {agency_id}:{carpool_id}.") await assert_agency_exists(agency_id) await assert_carpool_exists(agency_id, carpool_id) @@ -73,8 +75,8 @@ async def get_carpool(agency_id: str, carpool_id: str, api_key: str = Depends(ge "description": "Carpool or agency not found"}, }, ) -async def delete_carpool(agency_id: str, carpool_id: str, requesting_agency_id: str = Depends(get_current_agency) ): - await verify_permission_for_same_agency_or_admin(agency_id, requesting_agency_id) +async def delete_carpool(agency_id: str, carpool_id: str, requesting_user: User = Depends(get_current_user)): + verify_permission(f"{agency_id}:write", requesting_user) logger.info(f"Delete trip {agency_id}:{carpool_id}.") await assert_agency_exists(agency_id) diff --git a/amarillo/routers/users.py b/amarillo/routers/users.py index ba43487..ded8e36 100644 --- a/amarillo/routers/users.py +++ b/amarillo/routers/users.py @@ -5,7 +5,7 @@ from fastapi import APIRouter, HTTPException, status, Header, Depends from amarillo.models.User import User from amarillo.services.users import UserService -from amarillo.services.oauth2 import get_current_agency, verify_admin +from amarillo.services.oauth2 import get_current_user, verify_permission from amarillo.services.config import config from amarillo.utils.container import container @@ -21,22 +21,6 @@ router = APIRouter( include_in_schema = config.env != 'PROD' -# TODO Return code 403 Unauthoized (in response_status_codes as well...) -async def verify_permission_for_same_agency_or_admin(agency_id_in_path_or_body, agency_id_from_api_key): - """Verifies that an agency is accessing something it owns or the user is admin - - The agency_id is part of some paths, or when not in the path it is in the body, e.g. in PUT /carpool. - - This function encapsulates the formula 'working with own stuff, or admin'. - """ - is_permitted = agency_id_in_path_or_body == agency_id_from_api_key or agency_id_from_api_key == "admin" - - if not is_permitted: - message = f"Working with {agency_id_in_path_or_body} resources is not permitted for {agency_id_from_api_key}." - logger.error(message) - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=message) - - @router.get("/", include_in_schema=include_in_schema, operation_id="getUserIdsWhichHaveAConfiguration", @@ -44,7 +28,7 @@ async def verify_permission_for_same_agency_or_admin(agency_id_in_path_or_body, response_model=List[str], description="Returns the user_ids but not the details.", status_code=status.HTTP_200_OK) -async def get_user_ids(admin_api_key: str = Depends(get_current_agency)) -> [str]: +async def get_user_ids(requesting_user: User = Depends(get_current_user)) -> [str]: return container['users'].get_user_ids() @@ -52,7 +36,8 @@ async def get_user_ids(admin_api_key: str = Depends(get_current_agency)) -> [str include_in_schema=include_in_schema, operation_id="postNewUserConf", summary="Post a new User") -async def post_user_conf(user_conf: User, admin_api_key: str = Depends(verify_admin)): +async def post_user_conf(user_conf: User, requesting_user: User = Depends(get_current_user)): + verify_permission("admin", requesting_user) user_service: UserService = container['users'] user_service.add(user_conf) @@ -64,13 +49,13 @@ async def post_user_conf(user_conf: User, admin_api_key: str = Depends(verify_ad summary="Delete configuration of a user. Returns true if the token for the user existed, " "false if it didn't exist." ) -async def delete_user(user_id: str, requesting_user_id: str = Depends(get_current_agency)): - agency_may_delete_own = requesting_user_id == user_id - admin_may_delete_everything = requesting_user_id == "admin" - is_permitted = agency_may_delete_own or admin_may_delete_everything +async def delete_user(user_id: str, requesting_user: User = Depends(get_current_user)): + user_may_delete_own = requesting_user.user_id == user_id + admin_may_delete_everything = "admin" in requesting_user.permissions + is_permitted = user_may_delete_own or admin_may_delete_everything if not is_permitted: - message = f"The API key for {requesting_user_id} can not delete the configuration for {user_id}" + message = f"User '{requesting_user.user_id} can not delete the configuration for {user_id}" logger.error(message) raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=message) diff --git a/amarillo/services/oauth2.py b/amarillo/services/oauth2.py index c63b458..e6491f5 100644 --- a/amarillo/services/oauth2.py +++ b/amarillo/services/oauth2.py @@ -32,23 +32,22 @@ class Token(BaseModel): token_type: str class TokenData(BaseModel): - #TODO: rename to user_id - agency_id: Union[str, None] = None + user_id: Union[str, None] = None oauth2_scheme = OAuth2PasswordBearer(tokenUrl="token", auto_error=False) async def verify_optional_api_key(X_API_Key: Optional[str] = Header(None)): if X_API_Key == None: return None return await verify_api_key(X_API_Key) -def authenticate_agency(agency_id: str, password: str): +def authenticate_user(user_id: str, password: str): user_service : UserService = container['users'] - user_conf = user_service.user_id_to_user_conf.get(agency_id, None) + user_conf = user_service.user_id_to_user_conf.get(user_id, None) if not user_conf: return False if not verify_password(password, user_conf.password): return False - return agency_id + return user_id def create_access_token(data: dict, expires_delta: Union[timedelta, None] = None): @@ -62,11 +61,7 @@ def create_access_token(data: dict, expires_delta: Union[timedelta, None] = None return encoded_jwt -#TODO: rename to get_current_user, agency_from_api_key -> user_from_api_key -async def get_current_agency(token: str = Depends(oauth2_scheme), agency_from_api_key: str = Depends(verify_optional_api_key)): - return (await get_current_user(token, agency_from_api_key)).user_id - -async def get_current_user(token: str = Depends(oauth2_scheme), agency_from_api_key: str = Depends(verify_optional_api_key)) -> User: +async def get_current_user(token: str = Depends(oauth2_scheme), user_from_api_key: str = Depends(verify_optional_api_key)) -> User: if token: credentials_exception = HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, @@ -75,22 +70,22 @@ async def get_current_user(token: str = Depends(oauth2_scheme), agency_from_api_ ) try: payload = jwt.decode(token, SECRET_KEY, algorithms=[ALGORITHM]) - agency_id: str = payload.get("sub") - if agency_id is None: + user_id: str = payload.get("sub") + if user_id is None: raise credentials_exception - token_data = TokenData(agency_id=agency_id) + token_data = TokenData(user_id=user_id) except JWTError: raise credentials_exception - user_id = token_data.agency_id + user_id = token_data.user_id if user_id is None: raise credentials_exception user_service : UserService = container['users'] return user_service.get_user(user_id) - elif agency_from_api_key: - logger.info(f"API Key provided: {agency_from_api_key}") + elif user_from_api_key: + logger.info(f"API Key provided: {user_from_api_key}") user_service : UserService = container['users'] - return user_service.get_user(agency_from_api_key) + return user_service.get_user(user_from_api_key) else: credentials_exception = HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, @@ -99,9 +94,6 @@ async def get_current_user(token: str = Depends(oauth2_scheme), agency_from_api_ ) raise credentials_exception -# TODO: use verify_permission("admin", user) - - def verify_permission(permission: str, user: User): def permissions_exception(): @@ -121,6 +113,7 @@ def verify_permission(permission: str, user: User): return + #permission is in agency:operation format def permission_matches(permission, user_permission): prescribed_agency, prescribed_operation = permission.split(":") given_agency, given_operation = user_permission.split(":") @@ -132,17 +125,6 @@ def verify_permission(permission: str, user: User): raise permissions_exception() - -async def verify_admin(agency: str = Depends(get_current_agency)): - #TODO: maybe separate error for when admin credentials are invalid vs valid but not admin? - if(agency != "admin"): - message="This operation requires admin privileges" - logger.error(message) - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=message) - - return "admin" - - # noinspection PyPep8Naming # X_API_Key is upper case for OpenAPI async def verify_api_key(X_API_Key: str = Header(...)): @@ -154,7 +136,7 @@ async def verify_api_key(X_API_Key: str = Header(...)): async def login_for_access_token( form_data: Annotated[OAuth2PasswordRequestForm, Depends()] ) -> Token: - agency = authenticate_agency(form_data.username, form_data.password) + agency = authenticate_user(form_data.username, form_data.password) if not agency: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, @@ -170,7 +152,7 @@ async def login_for_access_token( # TODO: eventually remove this @router.get("/users/me/", response_model=Agency) async def read_users_me( - current_agency: Annotated[Agency, Depends(get_current_agency)] + current_agency: Annotated[Agency, Depends(get_current_user)] ): agency_service : AgencyService = container['agencies'] return agency_service.get_agency(agency_id=current_agency) @@ -178,6 +160,6 @@ async def read_users_me( # TODO: eventually remove this @router.get("/users/me/items/") async def read_own_items( - current_agency: Annotated[str, Depends(get_current_agency)] + current_agency: Annotated[str, Depends(get_current_user)] ): return [{"item_id": "Foo", "owner": current_agency}] diff --git a/amarillo/tests/test_permissions.py b/amarillo/tests/test_permissions.py index 7ae4fed..5e2ded2 100644 --- a/amarillo/tests/test_permissions.py +++ b/amarillo/tests/test_permissions.py @@ -3,11 +3,11 @@ import pytest from amarillo.services.oauth2 import verify_permission from amarillo.models.User import User -test_user = User(user_id="test", password="testpassword", permissions=["all:read", "mfdz:write", "ride2go:all", "metrics"]) +test_user = User(user_id="test", password="testpassword", permissions=["all:read", "mfdz:write", "ride2go:all", "gtfs"]) admin_user = User(user_id="admin", password="testpassword", permissions=["admin"]) def test_operation(): - verify_permission("metrics", test_user) + verify_permission("gtfs", test_user) with pytest.raises(HTTPException): verify_permission("geojson", test_user) @@ -25,6 +25,7 @@ def test_agency_permission(): def test_admin(): verify_permission("admin", admin_user) + verify_permission("gtfs", admin_user) verify_permission("all:all", admin_user) verify_permission("mvv:all", admin_user) verify_permission("mfdz:read", admin_user)