Fix PR comments

app/routes/datasets/dataset.py

@@ -29,17 +29,21 @@
 async def get_owner(
     dataset: str = Depends(dataset_dependency), user: User = Depends(get_manager)
 ) -> User:
-    """Retrieves the user object that owns the dataset, or ADMIN user, if that
-    user is the one making the request, otherwise raises a 401."""
+    """Retrieves the user object of the one making the request if that user
+    either owns the dataset or is an ADMIN, otherwise raises a 401."""
 
     if user.role == "ADMIN":
         return user
 
     dataset_row: ORMDataset = await datasets.get_dataset(dataset)
-    owner: str = dataset_row.owner_id
+    owner_id: str = dataset_row.owner_id
 
-    if owner != user.id:
-        raise HTTPException(status_code=401, detail="Unauthorized")
+    if owner_id != user.id:
+        owner = await get_rw_user(owner_id)
+        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. Please contact the dataset owner ({owner.email}) or an admin to modify the dataset.",
+        )
     return user
 
 

app/utils/rw_api.py

@@ -104,7 +104,7 @@ async def get_rw_user(user_id: str) -> User:
     if response.status_code == 404:
         raise HTTPException(status_code=401, detail=f"User ID invalid: {user_id}")
 
-    if response.status_code != 200 and response.status_code != 401:
+    if response.status_code != 200:
         logger.warning(
             f"Failed to authorize user. Server responded with response code: {response.status_code} and message: {response.text}"
         )

tests/__init__.py

@@ -318,17 +318,16 @@ async def get_api_key_mocked() -> Tuple[Optional[str], Optional[str]]:
     return str(uuid.uuid4()), "localhost"
 
 
-async def get_manager_mocked() -> User:
-    return User(
-        id="mr_manager123",
-        name="Mr. Manager",
-        email="[email protected]",
-        createdAt="2021-06-13T03:18:23.000Z",
-        role="MANAGER",
-        provider="local",
-        providerId="123",
-        extraUserData={},
-    )
+MANAGER = User(
+    id="mr_manager123",
+    name="Mr. Manager",
+    email="[email protected]",
+    createdAt="2021-06-13T03:18:23.000Z",
+    role="MANAGER",
+    provider="local",
+    providerId="123",
+    extraUserData={},
+)
 
 
 NEW_OWNER = User(
@@ -343,6 +342,10 @@ async def get_manager_mocked() -> User:
 )
 
 
+async def get_manager_mocked() -> User:
+    return MANAGER
+
+
 async def get_new_owner_mocked() -> User:
     return NEW_OWNER
 

tests/routes/datasets/test_datasets.py

@@ -5,7 +5,7 @@
 from app.application import ContextEngine, db
 from app.authentication.token import get_manager
 from app.models.pydantic.authentication import User
-from tests import NEW_OWNER, get_manager_mocked, get_new_owner_mocked
+from tests import MANAGER, NEW_OWNER, get_manager_mocked, get_new_owner_mocked
 from tests.utils import create_default_asset, dataset_metadata
 
 payload = {"metadata": dataset_metadata}
@@ -95,7 +95,8 @@ async def test_datasets(async_client):
         assert len(rows) == 1
         assert rows[0][0] == "new_owner_id123"
 
-        # assign original owner back
+    # assign original owner back
+    with patch("app.routes.datasets.dataset.get_rw_user", return_value=MANAGER):
         from app.main import app
 
         app.dependency_overrides[get_manager] = get_new_owner_mocked