fix: Add more consistent HTTP errors

This commit fixes some status codes that can cause misunderstanding. Some errors were returning Bad Request (400) even when the user sends a correct request. The errors originating from the infrastructure or error during operations should raise Internal Server Error (500) as they are not expected. Unit tests are included to avoid regression Signed-off-by: Kairo Araujo <[email protected]>
in-toto · Jan 24, 2024 · 8e3f114 · 8e3f114
1 parent aa5d56f
commit 8e3f114
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 3 deletions.
diff --git a/internal/server/server.go b/internal/server/server.go
@@ -143,7 +143,7 @@ func (s *Server) UploadHandler(w http.ResponseWriter, r *http.Request) {
 	defer r.Body.Close()
 	resp, err := s.Upload(r.Context(), r.Body)
 	if err != nil {
-		http.Error(w, err.Error(), http.StatusBadRequest)
+		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	}
 
@@ -193,16 +193,21 @@ func (s *Server) DownloadHandler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	vars := mux.Vars(r)
+	if vars == nil {
+		http.Error(w, "gitoid parameter is required", http.StatusBadRequest)
+		return
+	}
+
 	attestationReader, err := s.Download(r.Context(), vars["gitoid"])
 	if err != nil {
-		http.Error(w, err.Error(), http.StatusBadRequest)
+		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	}
 
 	defer attestationReader.Close()
 	if _, err := io.Copy(w, attestationReader); err != nil {
 		logrus.Errorf("failed to copy attestation to response: %+v", err)
-		w.WriteHeader(http.StatusBadRequest)
+		w.WriteHeader(http.StatusInternalServerError)
 		return
 	}
 

diff --git a/internal/server/server_test.go b/internal/server/server_test.go
@@ -15,9 +15,12 @@
 package server
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"io"
+	"net/http"
+	"net/http/httptest"
 	"strings"
 	"testing"
 
@@ -218,6 +221,47 @@ func (ut *UTServerSuite) Test_Upload_FailedMetadatStprage() {
 	ut.Equal(api.UploadResponse{}, resp)
 }
 
+func (ut *UTServerSuite) Test_UploadHandler() {
+
+	w := httptest.NewRecorder()
+	requestBody := []byte("fakePayload")
+	request := httptest.NewRequest(http.MethodPost, "/v1/upload", bytes.NewBuffer(requestBody))
+
+	ut.mockedStorerGetter.On("Store").Return(nil) // mock Get() to return nil
+	ut.mockedStorer.On("Store").Return(nil)       // mock Store() to return nil
+
+	ut.testServer.UploadHandler(w, request)
+	ut.Equal(http.StatusOK, w.Code)
+}
+
+func (ut *UTServerSuite) Test_UploadHandler_WrongMethod() {
+
+	w := httptest.NewRecorder()
+	requestBody := []byte("fakePayload")
+	request := httptest.NewRequest(http.MethodGet, "/upload", bytes.NewBuffer(requestBody))
+
+	ut.mockedStorerGetter.On("Store").Return(nil) // mock Get() to return nil
+	ut.mockedStorer.On("Store").Return(nil)       // mock Store() to return nil
+
+	ut.testServer.UploadHandler(w, request)
+	ut.Equal(http.StatusBadRequest, w.Code)
+	ut.Contains(w.Body.String(), "is an unsupported method")
+}
+
+func (ut *UTServerSuite) Test_UploadHandler_FailureUpload() {
+
+	w := httptest.NewRecorder()
+	requestBody := []byte("fakePayload")
+	request := httptest.NewRequest(http.MethodPost, "/upload", bytes.NewBuffer(requestBody))
+
+	ut.mockedStorerGetter.On("Store").Return(errors.New("BAD S3")) // mock Get() to return nil
+	ut.mockedStorer.On("Store").Return(nil)                        // mock Store() to return nil
+
+	ut.testServer.UploadHandler(w, request)
+	ut.Equal(http.StatusInternalServerError, w.Code)
+	ut.Contains(w.Body.String(), "BAD S3")
+}
+
 func (ut *UTServerSuite) Test_Download() {
 	ctx := context.TODO()
 	ut.mockedStorerGetter.On("Get").Return(nil) // mock Get() to return nil
@@ -259,3 +303,50 @@ func (ut *UTServerSuite) Test_Download_ObjectStorageError() {
 	_, err := ut.testServer.Download(ctx, "fakeGitoid")
 	ut.ErrorContains(err, "BAD S3")
 }
+
+func (ut *UTServerSuite) Test_DownloadHandler() {
+	w := httptest.NewRecorder()
+	request := httptest.NewRequest(http.MethodGet, "/v1/download", nil)
+	request = mux.SetURLVars(request, map[string]string{"gitoid": "fakeGitoid"})
+
+	ut.mockedStorerGetter.On("Get").Return(nil) // mock Get() to return nil
+
+	ut.testServer.DownloadHandler(w, request)
+	ut.Equal(http.StatusOK, w.Code)
+	ut.Equal("testData", w.Body.String())
+}
+
+func (ut *UTServerSuite) Test_DownloadHandler_BadMethod() {
+	w := httptest.NewRecorder()
+	request := httptest.NewRequest(http.MethodPost, "/v1/download", nil)
+	request = mux.SetURLVars(request, map[string]string{"gitoid": "fakeGitoid"})
+
+	ut.mockedStorerGetter.On("Get").Return(nil) // mock Get() to return nil
+
+	ut.testServer.DownloadHandler(w, request)
+	ut.Equal(http.StatusBadRequest, w.Code)
+	ut.Contains(w.Body.String(), "POST is an unsupported method")
+}
+
+func (ut *UTServerSuite) Test_DownloadHandler_MissingGitOID() {
+	w := httptest.NewRecorder()
+	request := httptest.NewRequest(http.MethodGet, "/v1/download", nil)
+
+	ut.mockedStorerGetter.On("Get").Return(nil) // mock Get() to return nil
+
+	ut.testServer.DownloadHandler(w, request)
+	ut.Equal(http.StatusBadRequest, w.Code)
+	ut.Contains(w.Body.String(), "gitoid parameter is required")
+}
+
+func (ut *UTServerSuite) Test_DownloadHandler_ObjectStorageFailed() {
+	w := httptest.NewRecorder()
+	request := httptest.NewRequest(http.MethodGet, "/v1/download", nil)
+	request = mux.SetURLVars(request, map[string]string{"gitoid": "fakeGitoid"})
+
+	ut.mockedStorerGetter.On("Get").Return(errors.New("BAD S3")) // mock Get() to return nil
+
+	ut.testServer.DownloadHandler(w, request)
+	ut.Equal(http.StatusInternalServerError, w.Code)
+	ut.Contains(w.Body.String(), "BAD S3")
+}