From 57de6256627bfa776d0c73113be80f4dfd31fa6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arturo=20Filast=C3=B2?= Date: Tue, 9 Apr 2024 15:31:31 +0200 Subject: [PATCH] Fix/validation (#834) Pydantic has an unusual default behaviour when you set a `default` value for `Field`: ``` validate_default bool | None If True, apply validation to the default value every time you create an instance. Otherwise, for performance reasons, the default value of the field is trusted and not validated. ``` https://docs.pydantic.dev/latest/api/fields/ This was leading to a validation bypass when the fields that specified an invalid default value were not being set. We fix that bug, but also add extra validation that checks if the `author` matches the email address and refactoring of the authentication logic. --- ooniapi/common/src/common/dependencies.py | 6 ++- ooniapi/common/src/common/utils.py | 29 ++++--------- .../ooniauth/src/ooniauth/routers/v1.py | 11 ++--- .../ooniauth/src/ooniauth/routers/v2.py | 9 ++-- .../services/oonirun/src/oonirun/__about__.py | 2 +- ooniapi/services/oonirun/src/oonirun/main.py | 4 +- .../src/oonirun/routers/{oonirun.py => v2.py} | 42 +++++++++---------- ooniapi/services/oonirun/tests/conftest.py | 1 + .../services/oonirun/tests/test_database.py | 2 +- .../services/oonirun/tests/test_oonirun.py | 35 +++++++++++++++- 10 files changed, 80 insertions(+), 61 deletions(-) rename ooniapi/services/oonirun/src/oonirun/routers/{oonirun.py => v2.py} (95%) diff --git a/ooniapi/common/src/common/dependencies.py b/ooniapi/common/src/common/dependencies.py index 5b4e70af..476a6910 100644 --- a/ooniapi/common/src/common/dependencies.py +++ b/ooniapi/common/src/common/dependencies.py @@ -25,12 +25,14 @@ async def verify_jwt( settings: Annotated[Settings, Depends(get_settings)], authorization: str = Header("authorization"), ): - tok = get_client_token(authorization, settings.jwt_encryption_key) - if tok is None: + try: + tok = get_client_token(authorization, settings.jwt_encryption_key) + except: raise HTTPException(detail="Authentication required", status_code=401) if tok["role"] not in roles: raise HTTPException(detail="Role not authorized", status_code=401) + return tok # TODO(art): we don't check for the session_expunge table yet. It's empty so the impact is none # query = """SELECT threshold # FROM session_expunge diff --git a/ooniapi/common/src/common/utils.py b/ooniapi/common/src/common/utils.py index 4768b508..63bca052 100644 --- a/ooniapi/common/src/common/utils.py +++ b/ooniapi/common/src/common/utils.py @@ -73,13 +73,10 @@ def create_jwt(payload: dict, key: str) -> str: return token -def get_client_token(authorization: str, jwt_encryption_key: str): - try: - assert authorization.startswith("Bearer ") - token = authorization[7:] - return decode_jwt(token, audience="user_auth", key=jwt_encryption_key) - except: - return None +def get_client_token(authorization: str, jwt_encryption_key: str) -> Dict[str, Any]: + assert authorization.startswith("Bearer ") + token = authorization[7:] + return decode_jwt(token, audience="user_auth", key=jwt_encryption_key) def get_client_role(authorization: str, jwt_encryption_key: str) -> str: @@ -93,24 +90,14 @@ def get_account_id_or_none( authorization: str, jwt_encryption_key: str ) -> Optional[str]: """Returns None for unlogged users""" - tok = get_client_token(authorization, jwt_encryption_key) - if tok: + try: + tok = get_client_token(authorization, jwt_encryption_key) return tok["account_id"] - return None + except: + return None def get_account_id_or_raise(authorization: str, jwt_encryption_key: str) -> str: """Raise exception for unlogged users""" tok = get_client_token(authorization, jwt_encryption_key) - if tok: - return tok["account_id"] - raise Exception - - -def get_account_id(authorization: str, jwt_encryption_key: str): - # TODO: switch to get_account_id_or_none - tok = get_client_token(authorization, jwt_encryption_key) - if not tok: - return jerror("Authentication required", 401) - return tok["account_id"] diff --git a/ooniapi/services/ooniauth/src/ooniauth/routers/v1.py b/ooniapi/services/ooniauth/src/ooniauth/routers/v1.py index bf0b2332..f6557296 100644 --- a/ooniapi/services/ooniauth/src/ooniauth/routers/v1.py +++ b/ooniapi/services/ooniauth/src/ooniauth/routers/v1.py @@ -196,9 +196,10 @@ async def get_account_metadata( authorization: str = Header("authorization"), ): """Get account metadata for logged-in users""" - tok = get_client_token( - authorization=authorization, jwt_encryption_key=settings.jwt_encryption_key - ) - if not tok: + try: + tok = get_client_token( + authorization=authorization, jwt_encryption_key=settings.jwt_encryption_key + ) + return AccountMetadata(logged_in=True, role=tok["role"]) + except: return AccountMetadata(logged_in=False, role="") - return AccountMetadata(logged_in=True, role=tok["role"]) diff --git a/ooniapi/services/ooniauth/src/ooniauth/routers/v2.py b/ooniapi/services/ooniauth/src/ooniauth/routers/v2.py index 0811830c..d5493e81 100644 --- a/ooniapi/services/ooniauth/src/ooniauth/routers/v2.py +++ b/ooniapi/services/ooniauth/src/ooniauth/routers/v2.py @@ -115,10 +115,11 @@ class UserSession(BaseModel): def maybe_get_user_session_from_header( authorization_header: str, jwt_encryption_key: str, admin_emails: List[str] ) -> Optional[UserSession]: - token = get_client_token( - authorization=authorization_header, jwt_encryption_key=jwt_encryption_key - ) - if token is None: + try: + token = get_client_token( + authorization=authorization_header, jwt_encryption_key=jwt_encryption_key + ) + except: return None email_address = token["email_address"] diff --git a/ooniapi/services/oonirun/src/oonirun/__about__.py b/ooniapi/services/oonirun/src/oonirun/__about__.py index 0536852e..f4061091 100644 --- a/ooniapi/services/oonirun/src/oonirun/__about__.py +++ b/ooniapi/services/oonirun/src/oonirun/__about__.py @@ -1 +1 @@ -VERSION = "0.6.0rc0" +VERSION = "0.7.0" diff --git a/ooniapi/services/oonirun/src/oonirun/main.py b/ooniapi/services/oonirun/src/oonirun/main.py index aeccf355..50b32214 100644 --- a/ooniapi/services/oonirun/src/oonirun/main.py +++ b/ooniapi/services/oonirun/src/oonirun/main.py @@ -9,7 +9,7 @@ from prometheus_fastapi_instrumentator import Instrumentator from . import models -from .routers import oonirun +from .routers import v2 from .dependencies import get_postgresql_session from .common.dependencies import get_settings @@ -48,7 +48,7 @@ async def lifespan(app: FastAPI): allow_headers=["*"], ) -app.include_router(oonirun.router, prefix="/api") +app.include_router(v2.router, prefix="/api") @app.get("/version") diff --git a/ooniapi/services/oonirun/src/oonirun/routers/oonirun.py b/ooniapi/services/oonirun/src/oonirun/routers/v2.py similarity index 95% rename from ooniapi/services/oonirun/src/oonirun/routers/oonirun.py rename to ooniapi/services/oonirun/src/oonirun/routers/v2.py index a81a046f..d6b2e3f1 100644 --- a/ooniapi/services/oonirun/src/oonirun/routers/oonirun.py +++ b/ooniapi/services/oonirun/src/oonirun/routers/v2.py @@ -19,8 +19,6 @@ from ..common.routers import BaseModel from ..common.dependencies import get_settings, role_required from ..common.utils import ( - get_client_role, - get_account_id_or_raise, get_account_id_or_none, ) from ..dependencies import get_postgresql_session @@ -60,21 +58,17 @@ class OONIRunLinkEngineDescriptor(BaseModel): class OONIRunLinkBase(BaseModel): - name: str = Field( - default="", title="name of the ooni run link", min_length=2, max_length=50 - ) + name: str = Field(title="name of the ooni run link", min_length=2, max_length=50) short_description: str = Field( - default="", title="short description of the ooni run link", min_length=2, max_length=200, ) description: str = Field( - default="", title="full description of the ooni run link", min_length=2 + title="full description of the ooni run link", min_length=2 ) author: str = Field( - default="", title="public email address of the author name of the ooni run link", min_length=2, max_length=100, @@ -151,22 +145,24 @@ class OONIRunLinkCreateEdit(OONIRunLinkBase): @router.post( "/v2/oonirun/links", tags=["oonirun"], - dependencies=[Depends(role_required(["admin", "user"]))], response_model=OONIRunLink, ) def create_oonirun_link( create_request: OONIRunLinkCreateEdit, - authorization: str = Header("authorization"), + token=Depends(role_required(["admin", "user"])), db=Depends(get_postgresql_session), - settings=Depends(get_settings), ): """Create a new oonirun link or a new version for an existing one.""" log.debug("creating oonirun") - account_id = get_account_id_or_raise( - authorization, jwt_encryption_key=settings.jwt_encryption_key - ) + account_id = token["account_id"] assert create_request + if create_request.author != token["email_address"]: + raise HTTPException( + status_code=400, + detail="email_address must match the email address of the user who created the oonirun link", + ) + now = utcnow_seconds() revision = 1 @@ -229,34 +225,34 @@ def create_oonirun_link( @router.put( "/v2/oonirun/links/{oonirun_link_id}", - dependencies=[Depends(role_required(["admin", "user"]))], tags=["oonirun"], response_model=OONIRunLink, ) def edit_oonirun_link( oonirun_link_id: str, edit_request: OONIRunLinkCreateEdit, - authorization: str = Header("authorization"), + token=Depends(role_required(["admin", "user"])), db=Depends(get_postgresql_session), - settings=Depends(get_settings), ): """Edit an existing OONI Run link""" log.debug(f"edit oonirun {oonirun_link_id}") - account_id = get_account_id_or_raise( - authorization, jwt_encryption_key=settings.jwt_encryption_key - ) + account_id = token["account_id"] now = utcnow_seconds() q = db.query(models.OONIRunLink).filter( models.OONIRunLink.oonirun_link_id == oonirun_link_id ) - client_role = get_client_role(authorization, settings.jwt_encryption_key) - if client_role == "user": + if token["role"] == "user": q = q.filter(models.OONIRunLink.creator_account_id == account_id) + if token["email_address"] != edit_request.author: + raise HTTPException( + status_code=403, + detail="You are not allowed to set the email_address to something other than your email address", + ) else: # When you are an admin we can do everything and there are no other roles - assert client_role == "admin" + assert token["role"] == "admin" try: oonirun_link = q.one() diff --git a/ooniapi/services/oonirun/tests/conftest.py b/ooniapi/services/oonirun/tests/conftest.py index aab2193f..4c6f716e 100644 --- a/ooniapi/services/oonirun/tests/conftest.py +++ b/ooniapi/services/oonirun/tests/conftest.py @@ -71,6 +71,7 @@ def create_session_token(account_id: str, role: str) -> str: "exp": now + 10 * 86400, "aud": "user_auth", "account_id": account_id, + "email_address": "oonitarian@example.com", "login_time": None, "role": role, } diff --git a/ooniapi/services/oonirun/tests/test_database.py b/ooniapi/services/oonirun/tests/test_database.py index 722f41a2..79969c58 100644 --- a/ooniapi/services/oonirun/tests/test_database.py +++ b/ooniapi/services/oonirun/tests/test_database.py @@ -1,7 +1,7 @@ from copy import deepcopy from datetime import datetime import pathlib -from oonirun.routers.oonirun import utcnow_seconds +from oonirun.routers.v2 import utcnow_seconds import pytest import sqlalchemy as sa diff --git a/ooniapi/services/oonirun/tests/test_oonirun.py b/ooniapi/services/oonirun/tests/test_oonirun.py index 9d4b8fe9..737fb4cc 100644 --- a/ooniapi/services/oonirun/tests/test_oonirun.py +++ b/ooniapi/services/oonirun/tests/test_oonirun.py @@ -7,7 +7,7 @@ import time from oonirun import models -from oonirun.routers.oonirun import utcnow_seconds +from oonirun.routers.v2 import utcnow_seconds import pytest import sqlalchemy as sa @@ -26,7 +26,7 @@ "it": "integ-test descrizione breve in italiano", }, "icon": "myicon", - "author": "integ-test author", + "author": "oonitarian@example.com", "nettests": [ { "inputs": [ @@ -85,7 +85,31 @@ def test_get_root(client): assert r.status_code == 200 +def test_oonirun_author_validation(client, client_with_user_role): + z = deepcopy(SAMPLE_OONIRUN) + z["name"] = "integ-test name in English" + del z["author"] + r = client_with_user_role.post("/api/v2/oonirun/links", json=z) + assert r.status_code == 422, "empty author should be rejected" + + z["author"] = "not an author" + r = client_with_user_role.post("/api/v2/oonirun/links", json=z) + assert r.status_code != 200, "invalid author is rejected" + + z["author"] = "nome@example.com" + r = client_with_user_role.post("/api/v2/oonirun/links", json=z) + assert r.status_code != 200, "invalid author is rejected" + + z["author"] = "oonitarian@example.com" + r = client_with_user_role.post("/api/v2/oonirun/links", json=z) + assert r.status_code == 200, "valid author is OK" + + def test_oonirun_validation(client, client_with_user_role): + z = deepcopy(SAMPLE_OONIRUN) + r = client.post("/api/v2/oonirun/links", json=z) + assert r.status_code != 200, "unauthenticated requests are rejected" + z = deepcopy(SAMPLE_OONIRUN) r = client_with_user_role.post("/api/v2/oonirun/links", json=z) assert r.status_code == 422, "empty name should be rejected" @@ -116,6 +140,13 @@ def test_oonirun_not_found(client, client_with_user_role): assert str(j["oonirun_link_id"]).startswith("10") oonirun_link_id = r.json()["oonirun_link_id"] + # try to change the email to a different value + j["author"] = "notme@example.com" + r = client_with_user_role.put(f"/api/v2/oonirun/links/{oonirun_link_id}", json=j) + assert r.status_code != 200, r.json() + + # Expire the link + j["author"] = "oonitarian@example.com" j["expiration_date"] = (utcnow_seconds() + timedelta(minutes=-1)).strftime( "%Y-%m-%dT%H:%M:%S.%fZ" )