From cf5a6e94c135d0de7df0d71bc5d2742f0a6b53c3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= <tim+github@zitadel.com>
Date: Wed, 17 Jan 2024 11:15:35 +0200
Subject: [PATCH] fix tests and be more consistent with the returned status
 code

---
 pkg/op/error.go            | 33 +++++++++++++++++++++++----------
 pkg/op/error_test.go       |  6 +++++-
 pkg/op/server_http_test.go |  6 +++---
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/pkg/op/error.go b/pkg/op/error.go
index 40480fe..e4580f6 100644
--- a/pkg/op/error.go
+++ b/pkg/op/error.go
@@ -157,16 +157,29 @@ func (e StatusError) Is(err error) bool {
 		e.statusCode == target.statusCode
 }
 
-// WriteError asserts for a StatusError containing an [oidc.Error].
-// If no StatusError is found, the status code will default to [http.StatusBadRequest].
-// If no [oidc.Error] was found in the parent, the error type defaults to [oidc.ServerError].
-// When the final oidc Error is a server error, the status code is adjusted to [http.StatusInternalServerError].
+// WriteError asserts for a [StatusError] containing an [oidc.Error].
+// If no `StatusError` is found, the status code will default to [http.StatusBadRequest].
+// If no `oidc.Error` was found in the parent, the error type defaults to [oidc.ServerError].
+// When there was no `StatusError` and the `oidc.Error` is of type `oidc.ServerError`,
+// the status code will be set to [http.StatusInternalServerError]
 func WriteError(w http.ResponseWriter, r *http.Request, err error, logger *slog.Logger) {
-	statusError := AsStatusError(err, http.StatusBadRequest)
-	e := oidc.DefaultToServerError(statusError.parent, statusError.parent.Error())
-	if e.ErrorType == oidc.ServerError {
-		statusError.statusCode = http.StatusInternalServerError
+	var statusError StatusError
+	if errors.As(err, &statusError) {
+		writeError(w, r,
+			oidc.DefaultToServerError(statusError.parent, statusError.parent.Error()),
+			statusError.statusCode, logger,
+		)
+		return
 	}
-	logger.Log(r.Context(), e.LogLevel(), "request error", "oidc_error", e, "status_code", statusError.statusCode)
-	httphelper.MarshalJSONWithStatus(w, e, statusError.statusCode)
+	statusCode := http.StatusBadRequest
+	e := oidc.DefaultToServerError(err, err.Error())
+	if e.ErrorType == oidc.ServerError {
+		statusCode = http.StatusInternalServerError
+	}
+	writeError(w, r, e, statusCode, logger)
+}
+
+func writeError(w http.ResponseWriter, r *http.Request, err *oidc.Error, statusCode int, logger *slog.Logger) {
+	logger.Log(r.Context(), err.LogLevel(), "request error", "oidc_error", err, "status_code", statusCode)
+	httphelper.MarshalJSONWithStatus(w, err, statusCode)
 }
diff --git a/pkg/op/error_test.go b/pkg/op/error_test.go
index 689ee5a..50a9cbf 100644
--- a/pkg/op/error_test.go
+++ b/pkg/op/error_test.go
@@ -579,7 +579,7 @@ func TestWriteError(t *testing.T) {
 		{
 			name:       "not a status or oidc error",
 			err:        io.ErrClosedPipe,
-			wantStatus: http.StatusBadRequest,
+			wantStatus: http.StatusInternalServerError,
 			wantBody: `{
 				"error":"server_error",
 				"error_description":"io: read/write on closed pipe"
@@ -592,6 +592,7 @@ func TestWriteError(t *testing.T) {
 					"parent":"io: read/write on closed pipe",
 					"type":"server_error"
 				},
+				"status_code":500,
 				"time":"not"
 			}`,
 		},
@@ -611,6 +612,7 @@ func TestWriteError(t *testing.T) {
 					"parent":"io: read/write on closed pipe",
 					"type":"server_error"
 				},
+				"status_code":500,
 				"time":"not"
 			}`,
 		},
@@ -629,6 +631,7 @@ func TestWriteError(t *testing.T) {
 					"description":"oops",
 					"type":"invalid_request"
 				},
+				"status_code":400,
 				"time":"not"
 			}`,
 		},
@@ -650,6 +653,7 @@ func TestWriteError(t *testing.T) {
 					"description":"oops",
 					"type":"unauthorized_client"
 				},
+				"status_code":401,
 				"time":"not"
 			}`,
 		},
diff --git a/pkg/op/server_http_test.go b/pkg/op/server_http_test.go
index 4eac4a0..6cb268f 100644
--- a/pkg/op/server_http_test.go
+++ b/pkg/op/server_http_test.go
@@ -365,14 +365,14 @@ func Test_webServer_authorizeHandler(t *testing.T) {
 			},
 		},
 		{
-			name: "authorize error",
+			name: "server error",
 			fields: fields{
 				server:  &requestVerifier{},
 				decoder: testDecoder,
 			},
 			r: httptest.NewRequest(http.MethodPost, "/authorize", strings.NewReader("foo=bar")),
 			want: webServerResult{
-				wantStatus: http.StatusBadRequest,
+				wantStatus: http.StatusInternalServerError,
 				wantBody:   `{"error":"server_error"}`,
 			},
 		},
@@ -1237,7 +1237,7 @@ func Test_webServer_simpleHandler(t *testing.T) {
 			},
 			r: httptest.NewRequest(http.MethodGet, "/", bytes.NewReader(make([]byte, 11<<20))),
 			want: webServerResult{
-				wantStatus: http.StatusBadRequest,
+				wantStatus: http.StatusInternalServerError,
 				wantBody:   `{"error":"server_error", "error_description":"io: read/write on closed pipe"}`,
 			},
 		},