fix(httphelpers): a wrapped classified error keeps its status

CHANGELOG.md

@@ -65,6 +65,7 @@
 
 ### Fixed
 
+- **A wrapped classified error now keeps its HTTP status** (#841): `httphelpers.StatusCode` decided the status with a type switch that does not unwrap, while the response body (`NewErrorFromError`) does unwrap. A `*helpers.Error` returned through `%w` therefore carried the right code in the body and a status taken from `inferStatusFromErrorString`'s keyword match over the concatenated message - so a 400 refusal read as 401 when the wrapper said "token" and 404 when it said "missing". No endpoint's status changes today: every existing producer returns its classified error bare, so the defect was latent. It bites the first path that wraps one.
 - **W3C presentation verification picked a proof by accident.** A verifiable
   presentation holds two Data Integrity proofs - the holder's, over the
   presentation, and the embedded credential's, from the issuer - and both

pkg/httphelpers/status.go

@@ -61,6 +61,27 @@ func StatusCode(ctx context.Context, err error) int {
 		}
 	}
 
+	// A *helpers.Error reached through %w, which the type switch above
+	// cannot see.
+	//
+	// NewErrorFromError already unwraps - it uses errors.AsType - so the
+	// RESPONSE BODY of a wrapped classified error is correct today while
+	// the status is decided down in inferStatusFromErrorString, from
+	// whatever words happen to appear in the wrapper's text. That is not
+	// a 500 by default so much as a status chosen by prose: a 400 refusal
+	// reads as 401 when the wrapper says "token" and as 404 when it says
+	// "missing". A client acting on 401 re-authenticates in response to a
+	// request it should simply not repeat.
+	//
+	// Placed after the sentinel checks below it would be equivalent, and
+	// before them is clearer: this is the general case those four
+	// errors.Is lines are specific instances of. They were each added
+	// when a particular wrapped sentinel surfaced wrong, one at a time;
+	// this is the rule they were approximating.
+	if classified, ok := errors.AsType[*helpers.Error](err); ok {
+		return StatusCode(ctx, classified)
+	}
+
 	// Check if it's wrapped in one of our known errors
 	if errors.Is(err, helpers.ErrNoDocumentFound) || errors.Is(err, helpers.ErrNoIdentityFound) {
 		return http.StatusNotFound

pkg/httphelpers/status_unwrap_test.go

@@ -0,0 +1,93 @@
+package httphelpers
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"net/http"
+	"testing"
+
+	"github.com/SUNET/vc/pkg/helpers"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// A classified error reached through %w must keep its status.
+//
+// The wrapper's WORDING is the thing under test. Before this unwrapped,
+// a wrapped *helpers.Error fell past the type switch and all the way to
+// inferStatusFromErrorString, which keyword-matches the concatenated
+// message - so the status was decided by whatever prose the wrapper
+// happened to contain, not by the error. A 400 refusal read as 401 when
+// the wrapper said "token", and a client acting on 401 re-authenticates
+// in response to a request it should simply not repeat.
+func TestStatusCodeUnwrapsAClassifiedError(t *testing.T) {
+	refusal := helpers.NewErrorDetailsWithStatus("zkp_req_issuer_key_set_size", "two keys, one expected", http.StatusBadRequest)
+
+	for _, tc := range []struct {
+		name string
+		err  error
+	}{
+		{"returned directly", refusal},
+		{"wrapped with no keyword in the wrapper", fmt.Errorf("scope nym: %w", refusal)},
+		{"wrapped where the wrapper says invalid", fmt.Errorf("scope nym: invalid presentation: %w", refusal)},
+		{"wrapped where the wrapper says token", fmt.Errorf("scope nym: bad token: %w", refusal)},
+		{"wrapped where the wrapper says missing", fmt.Errorf("scope nym: missing field: %w", refusal)},
+		{"wrapped twice", fmt.Errorf("outer: %w", fmt.Errorf("inner: %w", refusal))},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, http.StatusBadRequest, StatusCode(context.Background(), tc.err),
+				"the status must come from the error, not from the wrapper's wording")
+		})
+	}
+}
+
+// Not only 400: whatever status the classified error carries survives.
+func TestStatusCodeUnwrapsEveryClassifiedStatus(t *testing.T) {
+	for _, status := range []int{
+		http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden,
+		http.StatusNotFound, http.StatusConflict, http.StatusInternalServerError,
+	} {
+		classified := helpers.NewErrorDetailsWithStatus("some_code", "detail", status)
+		assert.Equal(t, status, StatusCode(context.Background(), fmt.Errorf("context: %w", classified)))
+	}
+}
+
+// A classified error with no explicit status still infers from its TITLE
+// when wrapped, the same way it does when returned directly - the title
+// is a shape somebody chose, unlike the wrapper's prose.
+func TestStatusCodeInfersFromTheTitleWhenWrapped(t *testing.T) {
+	noStatus := helpers.NewErrorDetails("document_not_found", "nope")
+	assert.Equal(t,
+		StatusCode(context.Background(), noStatus),
+		StatusCode(context.Background(), fmt.Errorf("loading the document: %w", noStatus)),
+		"wrapping must not change the answer")
+}
+
+// An error nothing recognises is still a 500. The fix must not turn
+// every unexpected failure into a client error.
+func TestStatusCodeLeavesUnclassifiedErrorsAlone(t *testing.T) {
+	assert.Equal(t, http.StatusInternalServerError,
+		StatusCode(context.Background(), errors.New("something went wrong in a way nobody modelled")))
+	assert.Equal(t, http.StatusInternalServerError,
+		StatusCode(context.Background(), fmt.Errorf("while doing a thing: %w", errors.New("nothing recognisable"))))
+}
+
+// The four sentinel errors.Is checks this generalises are instances of
+// the same rule, and must keep answering as they did.
+func TestStatusCodeKeepsTheSentinelAnswers(t *testing.T) {
+	for _, tc := range []struct {
+		err  error
+		want int
+	}{
+		{helpers.ErrNoDocumentFound, http.StatusNotFound},
+		{helpers.ErrNoIdentityFound, http.StatusNotFound},
+		{helpers.ErrDocumentAlreadyExists, http.StatusConflict},
+		{helpers.ErrDuplicateKey, http.StatusConflict},
+		{helpers.ErrInternalServerError, http.StatusInternalServerError},
+	} {
+		require.NotNil(t, tc.err)
+		assert.Equal(t, tc.want, StatusCode(context.Background(), tc.err), "%v direct", tc.err)
+		assert.Equal(t, tc.want, StatusCode(context.Background(), fmt.Errorf("context: %w", tc.err)), "%v wrapped", tc.err)
+	}
+}