Merge pull request #407 from natthan-pigoux/feat_sb_dl_permissions

feat: Ensure SBAccessPolicy verifies the user owns the sandbox

Author: chrisburr

diracx-db/src/diracx/db/sql/sandbox_metadata/db.py

@@ -30,6 +30,14 @@ async def get_owner_id(self, user: UserInfo) -> int | None:
         )
         return (await self.conn.execute(stmt)).scalar_one_or_none()
 
+    async def get_sandbox_owner_id(self, pfn: str) -> int | None:
+        """Get the id of the owner of a sandbox."""
+        stmt = select(SBOwners.OwnerID).where(
+            SBOwners.OwnerID == SandBoxes.OwnerId,
+            SandBoxes.SEPFN == pfn,
+        )
+        return (await self.conn.execute(stmt)).scalar_one_or_none()
+
     async def insert_owner(self, user: UserInfo) -> int:
         stmt = insert(SBOwners).values(
             Owner=user.preferred_username,

diracx-db/tests/jobs/test_sandbox_metadata.py

@@ -186,3 +186,25 @@ async def test_assign_and_unsassign_sandbox_to_jobs(
         res = await sandbox_metadata_db.conn.execute(stmt)
     res_sb_id = [row.SBId for row in res]
     assert sb_id_1 not in res_sb_id
+
+
+async def test_get_sandbox_owner_id(sandbox_metadata_db: SandboxMetadataDB):
+    user_info = UserInfo(
+        sub="vo:sub", preferred_username="user1", dirac_group="group1", vo="vo"
+    )
+    pfn = secrets.token_hex()
+    sandbox_se = "SandboxSE"
+    # Insert the sandbox
+    async with sandbox_metadata_db:
+        owner_id = await sandbox_metadata_db.insert_owner(user_info)
+        await sandbox_metadata_db.insert_sandbox(owner_id, sandbox_se, pfn, 100)
+
+    async with sandbox_metadata_db:
+        sb_owner_id = await sandbox_metadata_db.get_sandbox_owner_id(pfn)
+
+    assert owner_id == 1
+    assert sb_owner_id == owner_id
+
+    async with sandbox_metadata_db:
+        sb_owner_id = await sandbox_metadata_db.get_sandbox_owner_id("not_found")
+    assert sb_owner_id is None

diracx-routers/src/diracx/routers/jobs/access_policies.py

@@ -7,7 +7,7 @@
 from fastapi import Depends, HTTPException, status
 
 from diracx.core.properties import JOB_ADMINISTRATOR, NORMAL_USER
-from diracx.db.sql import JobDB
+from diracx.db.sql import JobDB, SandboxMetadataDB
 from diracx.routers.access_policies import BaseAccessPolicy
 from diracx.routers.utils.users import AuthorizedUserInfo
 
@@ -108,10 +108,12 @@ async def policy(
         /,
         *,
         action: ActionType | None = None,
+        sandbox_metadata_db: SandboxMetadataDB | None = None,
         pfns: list[str] | None = None,
         required_prefix: str | None = None,
     ):
         assert action, "action is a mandatory parameter"
+        assert sandbox_metadata_db, "sandbox_metadata_db is a mandatory parameter"
 
         if action == ActionType.CREATE:
 
@@ -137,6 +139,14 @@ async def policy(
                         status_code=status.HTTP_403_FORBIDDEN,
                         detail=f"Invalid PFN. PFN must start with {required_prefix}",
                     )
+                # Checking if the user owns the sandbox
+                owner_id = await sandbox_metadata_db.get_owner_id(user_info)
+                sandbox_owner_id = await sandbox_metadata_db.get_sandbox_owner_id(pfn)
+                if not owner_id or owner_id != sandbox_owner_id:
+                    raise HTTPException(
+                        status_code=status.HTTP_403_FORBIDDEN,
+                        detail=f"{user_info.preferred_username} is not the owner of the sandbox",
+                    )
 
 
 CheckSandboxPolicyCallable = Annotated[Callable, Depends(SandboxAccessPolicy.check)]

diracx-routers/src/diracx/routers/jobs/sandboxes.py

@@ -55,7 +55,9 @@ async def initiate_sandbox_upload(
     If the sandbox does not exist in the database then the "url" and "fields"
     should be used to upload the sandbox to the storage backend.
     """
-    await check_permissions(action=ActionType.CREATE)
+    await check_permissions(
+        action=ActionType.CREATE, sandbox_metadata_db=sandbox_metadata_db
+    )
 
     try:
         sandbox_upload_response = await initiate_sandbox_upload_bl(
@@ -73,6 +75,7 @@ async def initiate_sandbox_upload(
 async def get_sandbox_file(
     pfn: Annotated[str, Query(max_length=256, pattern=SANDBOX_PFN_REGEX)],
     settings: SandboxStoreSettings,
+    sandbox_metadata_db: SandboxMetadataDB,
     user_info: Annotated[AuthorizedUserInfo, Depends(verify_dirac_access_token)],
     check_permissions: CheckSandboxPolicyCallable,
 ) -> SandboxDownloadResponse:
@@ -92,6 +95,7 @@ async def get_sandbox_file(
     )
     await check_permissions(
         action=ActionType.READ,
+        sandbox_metadata_db=sandbox_metadata_db,
         pfns=[short_pfn],
         required_prefix=required_prefix,
     )

diracx-routers/tests/jobs/test_wms_access_policy.py

@@ -23,13 +23,23 @@
 }
 
 
-class FakeDB:
+class FakeJobDB:
     async def summary(self, *args): ...
 
 
+class FakeSBMetadataDB:
+    async def get_owner_id(self, *args): ...
+    async def get_sandbox_owner_id(self, *args): ...
+
+
 @pytest.fixture
 def job_db():
-    yield FakeDB()
+    yield FakeJobDB()
+
+
+@pytest.fixture
+def sandbox_metadata_db():
+    yield FakeSBMetadataDB()
 
 
 WMS_POLICY_NAME = "WMSAccessPolicy_AlthoughItDoesNotMatter"
@@ -220,7 +230,7 @@ async def summary_other_vo(*args):
 )
 
 
-async def test_sandbox_access_policy_create():
+async def test_sandbox_access_policy_create(sandbox_metadata_db):
 
     admin_user = AuthorizedUserInfo(properties=[JOB_ADMINISTRATOR], **base_payload)
     normal_user = AuthorizedUserInfo(properties=[NORMAL_USER], **base_payload)
@@ -230,6 +240,7 @@ async def test_sandbox_access_policy_create():
         await SandboxAccessPolicy.policy(
             SANDBOX_POLICY_NAME,
             normal_user,
+            sandbox_metadata_db=sandbox_metadata_db,
         )
 
     # An admin cannot create any resource
@@ -238,6 +249,7 @@ async def test_sandbox_access_policy_create():
             SANDBOX_POLICY_NAME,
             admin_user,
             action=ActionType.CREATE,
+            sandbox_metadata_db=sandbox_metadata_db,
             pfns=[USER_SANDBOX_PFN],
         )
 
@@ -246,13 +258,14 @@ async def test_sandbox_access_policy_create():
         SANDBOX_POLICY_NAME,
         normal_user,
         action=ActionType.CREATE,
+        sandbox_metadata_db=sandbox_metadata_db,
         pfns=[USER_SANDBOX_PFN],
     )
 
     ##############
 
 
-async def test_sandbox_access_policy_read():
+async def test_sandbox_access_policy_read(sandbox_metadata_db, monkeypatch):
 
     admin_user = AuthorizedUserInfo(properties=[JOB_ADMINISTRATOR], **base_payload)
     normal_user = AuthorizedUserInfo(properties=[NORMAL_USER], **base_payload)
@@ -261,6 +274,7 @@ async def test_sandbox_access_policy_read():
         SANDBOX_POLICY_NAME,
         admin_user,
         action=ActionType.READ,
+        sandbox_metadata_db=sandbox_metadata_db,
         pfns=[USER_SANDBOX_PFN],
         required_prefix=SANDBOX_PREFIX,
     )
@@ -269,6 +283,7 @@ async def test_sandbox_access_policy_read():
         SANDBOX_POLICY_NAME,
         admin_user,
         action=ActionType.READ,
+        sandbox_metadata_db=sandbox_metadata_db,
         pfns=[OTHER_USER_SANDBOX_PFN],
         required_prefix=SANDBOX_PREFIX,
     )
@@ -279,24 +294,43 @@ async def test_sandbox_access_policy_read():
             SANDBOX_POLICY_NAME,
             normal_user,
             action=ActionType.READ,
+            sandbox_metadata_db=sandbox_metadata_db,
             pfns=[USER_SANDBOX_PFN],
         )
 
     # User can act on his own sandbox
+    async def get_owner_id(*args):
+        return 1
+
+    async def get_sandbox_owner_id(*args):
+        return 1
+
+    monkeypatch.setattr(sandbox_metadata_db, "get_owner_id", get_owner_id)
+    monkeypatch.setattr(
+        sandbox_metadata_db, "get_sandbox_owner_id", get_sandbox_owner_id
+    )
+
     await SandboxAccessPolicy.policy(
         SANDBOX_POLICY_NAME,
         normal_user,
         action=ActionType.READ,
+        sandbox_metadata_db=sandbox_metadata_db,
         pfns=[USER_SANDBOX_PFN],
         required_prefix=SANDBOX_PREFIX,
     )
 
     # User cannot act on others
+    async def get_owner_id(*args):
+        return 2
+
+    monkeypatch.setattr(sandbox_metadata_db, "get_owner_id", get_owner_id)
+
     with pytest.raises(HTTPException):
         await SandboxAccessPolicy.policy(
             SANDBOX_POLICY_NAME,
             normal_user,
             action=ActionType.READ,
+            sandbox_metadata_db=sandbox_metadata_db,
             pfns=[OTHER_USER_SANDBOX_PFN],
             required_prefix=SANDBOX_PREFIX,
         )