repository-service-tuf / repository-service-tuf-api

Repository Service for TUF: API
MIT License
8 stars 19 forks source link

Bug: API returns 200 on error #647

Closed lukpueh closed 1 month ago

lukpueh commented 1 month ago

What happened?

There are several cases, where the RSTUF API raises an HTTPException with status code 200, i.e. returns 2xx on error. This does not seem right wrt HTTP semantics, and also goes against the FastAPI recommendations.

cc @simi

What steps did you take?

$ grep -rnA1 "raise HTTPException(" | grep -B1 200

./repository_service_tuf_api/bootstrap.py:200:        raise HTTPException(
./repository_service_tuf_api/bootstrap.py-201-            status_code=status.HTTP_200_OK,
--
./repository_service_tuf_api/metadata.py:61:        raise HTTPException(
./repository_service_tuf_api/metadata.py-62-            status.HTTP_200_OK,
--
./repository_service_tuf_api/metadata.py:121:        raise HTTPException(
./repository_service_tuf_api/metadata.py-122-            status.HTTP_200_OK,
--
./repository_service_tuf_api/metadata.py:136:        raise HTTPException(
./repository_service_tuf_api/metadata.py-137-            status.HTTP_200_OK,
--
./repository_service_tuf_api/metadata.py:154:            raise HTTPException(
./repository_service_tuf_api/metadata.py-155-                status.HTTP_200_OK,
--
./repository_service_tuf_api/metadata.py:166:            raise HTTPException(
./repository_service_tuf_api/metadata.py-167-                status.HTTP_200_OK,
--
./repository_service_tuf_api/metadata.py:229:        raise HTTPException(
./repository_service_tuf_api/metadata.py-230-            status.HTTP_200_OK,
--
./repository_service_tuf_api/metadata.py:321:        raise HTTPException(
./repository_service_tuf_api/metadata.py-322-            status.HTTP_200_OK,
--
./repository_service_tuf_api/metadata.py:379:        raise HTTPException(
./repository_service_tuf_api/metadata.py-380-            status.HTTP_200_OK,
--
./repository_service_tuf_api/artifacts.py:161:        raise HTTPException(
./repository_service_tuf_api/artifacts.py-162-            status.HTTP_200_OK,
--
./repository_service_tuf_api/artifacts.py:219:        raise HTTPException(
./repository_service_tuf_api/artifacts.py-220-            status.HTTP_200_OK,

What behavior did you expect?

A status code from the 4xx range.

Relevant log output

No response

Code of Conduct

simi commented 1 month ago
response_headers={"date"=>"Fri, 21 Jun 2024 20:08:49 GMT", "server"=>"uvicorn", "content-length"=>"100", "content-type"=>"
status=200,
reason_phrase="OK",
response_body={"detail"=>{"message"=>"Task not accepted.", "error"=>"It requires bootstrap finished. State: signing"}}>,

task failed to submit, but returned 200

kairoaraujo commented 1 month ago

It was intentionally added as status code 200 (OK), as the submitted task returns 202 (Accepted).

It is a little difficult to choose an error 4XX for this kind of request. As the error 4XX is a client error but the client request is correct and there is no error in the server (5XX).

Unless we define a custom 4XX for it, or use 400 that doesn't sounds the best IMO.

Any suggestion?

lukpueh commented 1 month ago

It was intentionally added as status code 200 (OK), as the submitted task returns 202 (Accepted).

Where? I think these all get returned before the task is even submitted.

It is a little difficult to choose an error 4XX for this kind of request. As the error 4XX is a client error but the client request is correct and there is no error in the server (5XX).

Yeah, it's tricky. But I would argue that the client request is actually not correct, because it requests a resource, which does not exist at that time (because the server is not yet bootstrapped).

So 404 should be fine and you are actually returning that in the config api, if the server is not bootstrapped yet.

Unless we define a custom 4XX for it, or use 400 that doesn't sounds the best IMO.

Defining a custom code is probably not worth it. 400 seems okay, and 403 might also work. I don't have strong feelings as long as it is not 2xx.

simi commented 1 month ago

Maybe 503? Is Task API unavailable until bootstrap ceremony happens? :thinking:

lukpueh commented 1 month ago

Maybe 503? Is Task API unavailable until bootstrap ceremony happens? 🤔

Yeah, that might work too. But I wouldn't use it, if the server waits for client input (a bootstrap payload) to become available.

kairoaraujo commented 1 month ago

Between 404 and 503 my +1 for 404 as the resource Bootstrap is unavailable is a reasonable justification.