Merge pull request #515 from wri/gtc-2822

GTC-2822 Check for dataset ownership on asset requests

app/authentication/token.py

@@ -95,7 +95,7 @@ async def get_user(token: str = Depends(oauth2_scheme)) -> User:
 
     if response.status_code == 401:
         logger.info("Unauthorized user")
-        raise HTTPException(status_code=401, detail="Unauthorized")
+        raise HTTPException(status_code=401, detail="Unauthorized access - this operation requires user authentication via a token")
     else:
         return User(**response.json())
 
@@ -104,7 +104,7 @@ async def get_admin(user: User = Depends(get_user)) -> User:
     """Get the details for authenticated ADMIN user."""
 
     if user.role != "ADMIN":
-        raise HTTPException(status_code=401, detail="Unauthorized")
+        raise HTTPException(status_code=401, detail="Unauthorized access - this operation requires authentication as a user that is an admin")
 
     return user
 
@@ -114,6 +114,6 @@ async def get_manager(user: User = Depends(get_user)) -> User:
     ADMIN user."""
 
     if user.role != "ADMIN" and user.role != "MANAGER":
-        raise HTTPException(status_code=401, detail="Unauthorized")
+        raise HTTPException(status_code=401, detail="Unauthorized write access to a dataset/version/asset by a user who is not an admin or data manager")
 
     return user

app/routes/assets/asset.py

@@ -71,6 +71,10 @@
 from ..assets import asset_response
 from ..tasks import paginated_tasks_response, tasks_response
 
+from ...authentication.token import get_manager
+from ...models.pydantic.authentication import User
+from ..datasets.dataset import get_owner
+
 router = APIRouter()
 
 
@@ -103,13 +107,21 @@ async def update_asset(
     *,
     asset_id: UUID = Path(...),
     request: AssetUpdateIn,
-    is_authorized: bool = Depends(is_admin),
+    user: User = Depends(get_manager),
 ) -> AssetResponse:
     """Update Asset metadata.
 
     Only the dataset's owner or a user with `ADMIN` user role can do this operation.
     """
 
+    try:
+        asset_row: ORMAsset = await assets.get_asset(asset_id)
+    except RecordNotFoundError as e:
+        raise HTTPException(status_code=404, detail=str(e))
+
+    # This is the actual check that the user is either the dataset owner or an admin
+    _ = await get_owner(asset_row.dataset, user)
+
     input_data = request.dict(exclude_none=True, by_alias=True)
 
     try:
@@ -133,7 +145,7 @@ async def update_asset(
 async def delete_asset(
     *,
     asset_id: UUID = Path(...),
-    is_authorized: bool = Depends(is_admin),
+    user: User = Depends(get_manager),
     background_tasks: BackgroundTasks,
 ) -> AssetResponse:
     """Delete selected asset.
@@ -149,6 +161,9 @@ async def delete_asset(
     except RecordNotFoundError as e:
         raise HTTPException(status_code=404, detail=str(e))
 
+    # This is the actual check that the user is either the dataset owner or an admin
+    _ = await get_owner(row.dataset, user)
+
     if row.is_default:
         raise HTTPException(
             status_code=409,
@@ -346,12 +361,22 @@ async def get_field_metadata(*, asset_id: UUID = Path(...), field_name: str):
     response_model=FieldMetadataResponse,
 )
 async def update_field_metadata(
-    *, asset_id: UUID = Path(...), field_name: str, request: FieldMetadataUpdate
+    *, asset_id: UUID = Path(...), field_name: str, request: FieldMetadataUpdate,
+    user: User = Depends(get_manager),
 ):
     """Update the field metadata for an asset.
 
     Only the dataset's owner or a user with `ADMIN` user role can do this operation.
     """
+
+    try:
+        asset_row: ORMAsset = await assets.get_asset(asset_id)
+    except RecordNotFoundError as e:
+        raise HTTPException(status_code=404, detail=str(e))
+
+    # This is the actual check that the user is either the dataset owner or an admin
+    _ = await get_owner(asset_row.dataset, user)
+
     input_data = request.dict(exclude_none=True, by_alias=True)
     metadata = await metadata_crud.get_asset_metadata(asset_id)
     field_metadata: ORMFieldMetadata = await metadata_crud.update_field_metadata(
@@ -410,7 +435,8 @@ async def create_metadata(*, asset_id: UUID = Path(...), request: AssetMetadata)
     tags=["Assets"],
     response_model=AssetMetadataResponse,
 )
-async def update_metadata(*, asset_id: UUID = Path(...), request: AssetMetadataUpdate):
+async def update_metadata(*, asset_id: UUID = Path(...), request: AssetMetadataUpdate,
+                          user: User = Depends(get_manager)):
     """Update metadata record for an asset.
 
     Only the dataset's owner or a user with `ADMIN` user role can do this operation.
@@ -426,6 +452,9 @@ async def update_metadata(*, asset_id: UUID = Path(...), request: AssetMetadataU
     except RecordNotFoundError as e:
         raise HTTPException(status_code=404, detail=str(e))
 
+    # This is the actual check that the user is either the dataset owner or an admin
+    _ = await get_owner(asset.dataset, user)
+
     validated_metadata = asset_metadata_factory(asset)
 
     return Response(data=validated_metadata)
@@ -437,7 +466,8 @@ async def update_metadata(*, asset_id: UUID = Path(...), request: AssetMetadataU
     tags=["Assets"],
     response_model=AssetMetadataResponse,
 )
-async def delete_metadata(asset_id: UUID = Path(...)):
+async def delete_metadata(asset_id: UUID = Path(...),
+                          user: User = Depends(get_manager)):
     """Delete an asset's metadata record.
 
     Only the dataset's owner or a user with `ADMIN` user role can do this operation.
@@ -448,6 +478,9 @@ async def delete_metadata(asset_id: UUID = Path(...)):
     except RecordNotFoundError as e:
         raise HTTPException(status_code=404, detail=str(e))
 
+    # This is the actual check that the user is either the dataset owner or an admin
+    _ = await get_owner(asset.dataset, user)
+
     validated_metadata = asset_metadata_factory(asset)
 
     return Response(data=validated_metadata)

app/routes/datasets/asset.py

@@ -20,6 +20,7 @@
 from ...crud import assets
 from ...errors import RecordAlreadyExistsError
 from ...models.orm.assets import Asset as ORMAsset
+from ...models.pydantic.authentication import User
 from ...models.pydantic.assets import (
     AssetCreateIn,
     AssetResponse,
@@ -32,6 +33,7 @@
 from ...utils.paginate import paginate_collection
 from ...utils.path import get_asset_uri
 from ..assets import asset_response, assets_response, paginated_assets_response
+from .dataset import get_owner
 from . import (
     validate_creation_options,
     verify_asset_dependencies,
@@ -118,7 +120,7 @@ async def add_new_asset(
     dv: Tuple[str, str] = Depends(dataset_version_dependency),
     request: AssetCreateIn,
     background_tasks: BackgroundTasks,
-    is_authorized: bool = Depends(is_admin),
+    is_authorized: User = Depends(get_owner),
     response: ORJSONResponse,
 ) -> AssetResponse:
     """Add a new asset to a dataset version. Managed assets will be generated

app/routes/datasets/dataset.py

@@ -37,7 +37,7 @@ async def get_owner(
     dataset_row: ORMDataset = await datasets.get_dataset(dataset)
     owner: str = dataset_row.owner_id
     if owner != user.id:
-        raise HTTPException(status_code=401, detail="Unauthorized")
+        raise HTTPException(status_code=401, detail=f"Unauthorized write access to dataset {dataset} (or its versions/assets) by a user who is not an admin or owner of the dataset")
     return user
 
 

tests_v2/unit/app/routes/datasets/test_asset_metadata.py

@@ -2,6 +2,15 @@
 from httpx import AsyncClient
 
 from app.crud import assets as assets_crud
+from app.authentication import token
+from app.authentication.token import get_manager, get_user
+from tests_v2.utils import (
+    get_admin_mocked,
+    get_manager_mocked,
+    get_user_mocked,
+    raises_401,
+)
+from app.main import app as appmain
 
 
 @pytest.mark.asyncio
@@ -29,7 +38,6 @@ async def test_get_asset_metadata(
 @pytest.mark.asyncio
 async def test_update_asset_metadata(
     generic_vector_source_version,
-    async_client: AsyncClient,
 ):
     max_zoom_before_update = 14
     max_zoom = 12
@@ -42,12 +50,41 @@ async def test_update_asset_metadata(
     (tile_cache_asset,) = assets
     assert tile_cache_asset.metadata.max_zoom == max_zoom_before_update
 
-    resp = await async_client.patch(
-        f"asset/{tile_cache_asset.asset_id}/metadata", json={"max_zoom": max_zoom}
-    )
-
-    assert resp.json()["status"] == "success"
-    assert resp.json()["data"]["max_zoom"] == max_zoom
+    # generic_vector_source_version creates with owner of manager_mocked, so
+    # update by manager_mocked should succeed
+    appmain.dependency_overrides[get_user] = get_manager_mocked
+
+    async with AsyncClient(
+        app=appmain,
+        base_url="http://test",
+        trust_env=False,
+        headers={"Origin": "https://www.globalforestwatch.org"},
+    ) as async_client:
+        resp = await async_client.patch(
+            f"asset/{tile_cache_asset.asset_id}/metadata", json={"max_zoom": max_zoom}
+        )
+
+        assert resp.json()["status"] == "success"
+        assert resp.json()["data"]["max_zoom"] == max_zoom
+
+    # update by user_mocked should fail
+    appmain.dependency_overrides = {}
+    appmain.dependency_overrides[get_user] = get_user_mocked
+
+    async with AsyncClient(
+        app=appmain,
+        base_url="http://test",
+        trust_env=False,
+        headers={"Origin": "https://www.globalforestwatch.org"},
+    ) as async_client:
+        resp = await async_client.patch(
+            f"asset/{tile_cache_asset.asset_id}/metadata", json={"max_zoom": max_zoom}
+        )
+
+        assert resp.json()["status"] == "failed"
+        assert resp.status_code == 401
+
+    appmain.dependency_overrides = {}
 
 
 @pytest.mark.asyncio
@@ -94,7 +131,7 @@ async def test_get_field_metadata(
 
 @pytest.mark.asyncio
 async def test_update_field_metadata(
-    generic_vector_source_version, async_client: AsyncClient
+    generic_vector_source_version
 ):
     field_description = "geometry field"
     dataset, version, _ = generic_vector_source_version
@@ -107,10 +144,37 @@ async def test_update_field_metadata(
     assert tile_cache_asset.metadata.fields[1].description is None
     assert tile_cache_asset.metadata.fields[1].name == "geom"
 
-    resp = await async_client.patch(
-        f"asset/{tile_cache_asset.asset_id}/fields/geom",
-        json={"description": field_description},
-    )
-
-    assert resp.json()["status"] == "success"
-    assert resp.json()["data"]["description"] == field_description
+    appmain.dependency_overrides[get_user] = get_manager_mocked
+
+    async with AsyncClient(
+        app=appmain,
+        base_url="http://test",
+        trust_env=False,
+        headers={"Origin": "https://www.globalforestwatch.org"},
+    ) as async_client:
+        resp = await async_client.patch(
+            f"asset/{tile_cache_asset.asset_id}/fields/geom",
+            json={"description": field_description},
+        )
+
+        assert resp.json()["status"] == "success"
+        assert resp.json()["data"]["description"] == field_description
+
+    appmain.dependency_overrides = {}
+    appmain.dependency_overrides[get_user] = get_user_mocked
+
+    async with AsyncClient(
+        app=appmain,
+        base_url="http://test",
+        trust_env=False,
+        headers={"Origin": "https://www.globalforestwatch.org"},
+    ) as async_client:
+        resp = await async_client.patch(
+            f"asset/{tile_cache_asset.asset_id}/fields/geom",
+            json={"description": field_description},
+        )
+
+        assert resp.json()["status"] == "failed"
+        assert resp.status_code == 401
+
+    appmain.dependency_overrides = {}

tests_v2/unit/app/routes/datasets/test_dataset.py

@@ -45,9 +45,10 @@ async def test_get_owner_fail(db, init_db, monkeypatch) -> None:
         _ = await get_owner(dataset_name, some_user)
     except HTTPException as e:
         assert e.status_code == 401
-        assert e.detail == "Unauthorized"
+        assert e.detail == "Unauthorized write access to dataset my_first_dataset (or its versions/assets) by a user who is not an admin or owner of the dataset"
 
 
+@pytest.mark.asyncio
 async def test_get_owner_manager_success(db, init_db, monkeypatch) -> None:
     dataset_name: str = "my_first_dataset"
 
@@ -74,6 +75,7 @@ async def test_get_owner_manager_success(db, init_db, monkeypatch) -> None:
     _ = await get_owner(dataset_name, some_manager)
 
 
+@pytest.mark.asyncio
 async def test_get_owner_different_manager_fail(db, init_db, monkeypatch) -> None:
     dataset_name: str = "my_first_dataset"
 
@@ -102,16 +104,17 @@ async def test_get_owner_different_manager_fail(db, init_db, monkeypatch) -> Non
         _ = await get_owner(dataset_name, some_manager)
     except HTTPException as e:
         assert e.status_code == 401
-        assert e.detail == "Unauthorized"
+        assert e.detail == "Unauthorized write access to dataset my_first_dataset (or its versions/assets) by a user who is not an admin or owner of the dataset"
 
 
+@pytest.mark.asyncio
 async def test_get_owner_admin_success(db, init_db, monkeypatch) -> None:
     dataset_name: str = "my_first_dataset"
 
     from app.main import app
 
-    # Create a dataset
-    app.dependency_overrides[get_manager] = get_admin_mocked
+    # Create a dataset with a manager, then make sure get_owner succeeds with an admin.
+    app.dependency_overrides[get_manager] = get_manager_mocked
 
     async with AsyncClient(
         app=app,