catch delete key not exist error

gen3workflow/aws_utils.py

@@ -2,6 +2,8 @@
 
 import boto3
 from botocore.exceptions import ClientError
+from fastapi import HTTPException
+from starlette.status import HTTP_404_NOT_FOUND
 
 from gen3workflow import logger
 from gen3workflow.config import config
@@ -165,7 +167,14 @@ def list_iam_user_keys(user_id):
 
 
 def delete_iam_user_key(user_id, key_id):
-    iam_client.delete_access_key(
-        UserName=get_iam_user_name(user_id),
-        AccessKeyId=key_id,
-    )
+    try:
+        iam_client.delete_access_key(
+            UserName=get_iam_user_name(user_id),
+            AccessKeyId=key_id,
+        )
+    except ClientError as e:
+        if e.response["Error"]["Code"] == "NoSuchEntity":
+            raise HTTPException(
+                HTTP_404_NOT_FOUND,
+                f"No such key: '{key_id}'",
+            )

gen3workflow/routes/storage.py

@@ -1,7 +1,12 @@
 from datetime import datetime, timedelta, timezone
 
 from fastapi import APIRouter, Depends, HTTPException, Request
-from starlette.status import HTTP_200_OK, HTTP_400_BAD_REQUEST
+from starlette.status import (
+    HTTP_200_OK,
+    HTTP_201_CREATED,
+    HTTP_204_NO_CONTENT,
+    HTTP_400_BAD_REQUEST,
+)
 
 # from gen3workflow import logger
 from gen3workflow.auth import Auth
@@ -24,7 +29,7 @@ async def get_storage_info(request: Request, auth=Depends(Auth)):
     }
 
 
-@router.post("/credentials", status_code=HTTP_200_OK)
+@router.post("/credentials", status_code=HTTP_201_CREATED)
 async def generate_user_key(request: Request, auth=Depends(Auth)):
     token_claims = await auth.get_token_claims()
     user_id = token_claims.get("sub")
@@ -84,7 +89,7 @@ def get_key_expiration(key_status, key_creation_date):
     ]
 
 
-@router.delete("/credentials/{key_id}", status_code=HTTP_200_OK)
+@router.delete("/credentials/{key_id}", status_code=HTTP_204_NO_CONTENT)
 async def delete_user_key(request: Request, key_id: str, auth=Depends(Auth)):
     token_claims = await auth.get_token_claims()
     user_id = token_claims.get("sub")

tests/test_ga4gh_tes.py

@@ -51,7 +51,7 @@ async def test_get_task(client, access_token_patcher, view):
     url = f"/ga4gh/tes/v1/tasks/123?unsupported_param=value"
     if view:
         url += f"&view={view}"
-    res = await client.get(url, headers={"Authorization": f"bearer 123"})
+    res = await client.get(url, headers={"Authorization": "bearer 123"})
 
     # the call to the TES server always has `view=FULL` so we get the AUTHZ tag
     mock_tes_server_request.assert_called_once_with(
@@ -111,7 +111,7 @@ async def test_create_task(client, access_token_patcher):
     res = await client.post(
         "/ga4gh/tes/v1/tasks",
         json={"name": "test-task"},
-        headers={"Authorization": f"bearer 123"},
+        headers={"Authorization": "bearer 123"},
     )
     if not client.authorized:
         assert res.status_code == 403, res.text
@@ -159,7 +159,7 @@ async def test_create_task_new_user(client, access_token_patcher):
     res = await client.post(
         "/ga4gh/tes/v1/tasks",
         json={"name": "test-task"},
-        headers={"Authorization": f"bearer 123"},
+        headers={"Authorization": "bearer 123"},
     )
     assert res.status_code == 200, res.text
     assert res.json() == {"id": "123"}
@@ -228,7 +228,7 @@ async def test_list_tasks(client, access_token_patcher, view):
     url = f"/ga4gh/tes/v1/tasks?state=COMPLETE&unsupported_param=value"
     if view:
         url += f"&view={view}"
-    res = await client.get(url, headers={"Authorization": f"bearer 123"})
+    res = await client.get(url, headers={"Authorization": "bearer 123"})
 
     # the call to the TES server always has `view=FULL` so we get the AUTHZ tag
     mock_tes_server_request.assert_called_once_with(
@@ -298,7 +298,7 @@ async def test_delete_task(client, access_token_patcher):
     res = await client.post(
         "/ga4gh/tes/v1/tasks/123:cancel",
         json={"unsupported_body": "value"},
-        headers={"Authorization": f"bearer 123"},
+        headers={"Authorization": "bearer 123"},
     )
 
     # there is always a 1st call with view=FULL to get the AUTHZ tag

tests/test_storage.py

@@ -19,7 +19,7 @@ async def test_create_and_list_user_keys(client, access_token_patcher):
 
         # the user does not have keys yet
         res = await client.get(
-            "/storage/credentials", headers={"Authorization": f"bearer 123"}
+            "/storage/credentials", headers={"Authorization": "bearer 123"}
         )
         assert res.status_code == 200, res.text
         assert res.json() == []
@@ -28,16 +28,16 @@ async def test_create_and_list_user_keys(client, access_token_patcher):
         keys = []
         for _ in range(2):
             res = await client.post(
-                "/storage/credentials", headers={"Authorization": f"bearer 123"}
+                "/storage/credentials", headers={"Authorization": "bearer 123"}
             )
-            assert res.status_code == 200, res.text
+            assert res.status_code == 201, res.text
             key_data = res.json()
             assert "aws_key_id" in key_data and "aws_key_secret" in key_data
             keys.append(key_data)
 
         # both keys should be listed in the user's keys
         res = await client.get(
-            "/storage/credentials", headers={"Authorization": f"bearer 123"}
+            "/storage/credentials", headers={"Authorization": "bearer 123"}
         )
         assert res.status_code == 200, res.text
         assert res.json() == [
@@ -54,13 +54,13 @@ async def test_create_and_list_user_keys(client, access_token_patcher):
         # delete the 1st key
         res = await client.delete(
             f"/storage/credentials/{keys[0]['aws_key_id']}",
-            headers={"Authorization": f"bearer 123"},
+            headers={"Authorization": "bearer 123"},
         )
-        assert res.status_code == 200, res.text
+        assert res.status_code == 204, res.text
 
         # only the 2nd key should now be listed in the user's keys
         res = await client.get(
-            "/storage/credentials", headers={"Authorization": f"bearer 123"}
+            "/storage/credentials", headers={"Authorization": "bearer 123"}
         )
         assert res.status_code == 200, res.text
         assert res.json() == [
@@ -91,20 +91,20 @@ async def test_list_user_keys_status(client, access_token_patcher):
                     - datetime.timedelta(days=config["IAM_KEYS_LIFETIME_DAYS"] + 1)
                 ):
                     res = await client.post(
-                        "/storage/credentials", headers={"Authorization": f"bearer 123"}
+                        "/storage/credentials", headers={"Authorization": "bearer 123"}
                     )
             else:
                 res = await client.post(
-                    "/storage/credentials", headers={"Authorization": f"bearer 123"}
+                    "/storage/credentials", headers={"Authorization": "bearer 123"}
                 )
-            assert res.status_code == 200, res.text
+            assert res.status_code == 201, res.text
             key_data = res.json()
             assert "aws_key_id" in key_data and "aws_key_secret" in key_data
             keys.append(key_data)
 
         # list the user's keys; the 1st key should show as expired
         res = await client.get(
-            "/storage/credentials", headers={"Authorization": f"bearer 123"}
+            "/storage/credentials", headers={"Authorization": "bearer 123"}
         )
         assert res.status_code == 200, res.text
         assert res.json() == [
@@ -123,7 +123,7 @@ async def test_list_user_keys_status(client, access_token_patcher):
 
         # list the user's keys; both keys should now show as expired
         res = await client.get(
-            "/storage/credentials", headers={"Authorization": f"bearer 123"}
+            "/storage/credentials", headers={"Authorization": "bearer 123"}
         )
         assert res.status_code == 200, res.text
         assert res.json() == [
@@ -143,15 +143,15 @@ async def test_too_many_user_keys(client, access_token_patcher):
         # create the max number of keys
         for _ in range(config["MAX_IAM_KEYS_PER_USER"]):
             res = await client.post(
-                "/storage/credentials", headers={"Authorization": f"bearer 123"}
+                "/storage/credentials", headers={"Authorization": "bearer 123"}
             )
-            assert res.status_code == 200, res.text
+            assert res.status_code == 201, res.text
             key_data = res.json()
             assert "aws_key_id" in key_data and "aws_key_secret" in key_data
 
         # attempt to create another key; this should fail since `MAX_IAM_KEYS_PER_USER` is reached
         res = await client.post(
-            "/storage/credentials", headers={"Authorization": f"bearer 123"}
+            "/storage/credentials", headers={"Authorization": "bearer 123"}
         )
         assert res.status_code == 400, res.text
         assert res.json() == {
@@ -161,19 +161,36 @@ async def test_too_many_user_keys(client, access_token_patcher):
         # delete one of the keys
         res = await client.delete(
             f"/storage/credentials/{key_data['aws_key_id']}",
-            headers={"Authorization": f"bearer 123"},
+            headers={"Authorization": "bearer 123"},
         )
-        assert res.status_code == 200, res.text
+        assert res.status_code == 204, res.text
 
         # attempt to create another key; now it should succeed
         res = await client.post(
-            "/storage/credentials", headers={"Authorization": f"bearer 123"}
+            "/storage/credentials", headers={"Authorization": "bearer 123"}
         )
-        assert res.status_code == 200, res.text
+        assert res.status_code == 201, res.text
         key_data = res.json()
         assert "aws_key_id" in key_data and "aws_key_secret" in key_data
 
 
+@pytest.mark.asyncio
+async def test_delete_non_existent_key(client, access_token_patcher):
+    """
+    Users should not be able to create new keys after reaching MAX_IAM_KEYS_PER_USER.
+    """
+    with mock_aws():
+        aws_utils.iam_client = boto3.client("iam")
+
+        key_id = "thiskeydoesnotexist"
+        res = await client.delete(
+            f"/storage/credentials/{key_id}",
+            headers={"Authorization": "bearer 123"},
+        )
+        assert res.status_code == 404, res.text
+        assert res.json() == {"detail": f"No such key: '{key_id}'"}
+
+
 @pytest.mark.asyncio
 async def test_torage_info(client, access_token_patcher):
     """
@@ -182,9 +199,7 @@ async def test_torage_info(client, access_token_patcher):
     with mock_aws():
         aws_utils.iam_client = boto3.client("iam")
 
-        res = await client.get(
-            "/storage/info", headers={"Authorization": f"bearer 123"}
-        )
+        res = await client.get("/storage/info", headers={"Authorization": "bearer 123"})
         assert res.status_code == 200, res.text
         storage_info = res.json()
         assert storage_info == {