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

@@ -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

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"]

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"])

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"]

ooniapi/services/oonirun/src/oonirun/__about__.py

@@ -1 +1 @@
-VERSION = "0.6.0rc0"
+VERSION = "0.7.0"

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")

ooniapi/services/oonirun/src/oonirun/routers/oonirun.py → 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()

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,
     }

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

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"
     )