From ce31ee22729b941f446c57691fb9ce4379347d34 Mon Sep 17 00:00:00 2001 From: Mridang Agarwalla Date: Fri, 10 Apr 2026 15:22:56 +0700 Subject: [PATCH] fix: propagate non-NotFound errors from instance interceptor (#12019) --- .../instance_interceptor.go | 10 +- .../instance_interceptor_test.go | 103 ++++++++++++++++++ .../server/middleware/instance_interceptor.go | 13 ++- .../middleware/instance_interceptor_test.go | 84 ++++++++++++++ .../http/middleware/instance_interceptor.go | 8 +- .../middleware/instance_interceptor_test.go | 43 ++++++++ 6 files changed, 250 insertions(+), 11 deletions(-) create mode 100644 internal/api/grpc/server/connect_middleware/instance_interceptor_test.go diff --git a/internal/api/grpc/server/connect_middleware/instance_interceptor.go b/internal/api/grpc/server/connect_middleware/instance_interceptor.go index 4f340df08..806d93bad 100644 --- a/internal/api/grpc/server/connect_middleware/instance_interceptor.go +++ b/internal/api/grpc/server/connect_middleware/instance_interceptor.go @@ -10,6 +10,7 @@ import ( "github.com/zitadel/logging" "github.com/zitadel/zitadel/internal/api/authz" + "github.com/zitadel/zitadel/internal/api/grpc/gerrors" zitadel_http "github.com/zitadel/zitadel/internal/api/http" "github.com/zitadel/zitadel/internal/i18n" "github.com/zitadel/zitadel/internal/telemetry/tracing" @@ -81,7 +82,8 @@ func addInstanceByDomain(ctx context.Context, req connect.AnyRequest, handler co if errors.As(err, ¬FoundErr) && notFoundErr.Kind == zerrors.KindNotFound { notFoundErr.Message = translator.LocalizeFromCtx(ctx, notFoundErr.GetMessage(), nil) } - return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("unable to set instance using domain %s: %w", domain, notFoundErr)) + code, _, _ := gerrors.ExtractZITADELError(err) + return nil, connect.NewError(connect.Code(code), fmt.Errorf("unable to set instance using domain %s: %w", domain, err)) } return handler(authz.WithInstance(ctx, instance), req) } @@ -97,12 +99,12 @@ func addInstanceByRequestedHost(ctx context.Context, req connect.AnyRequest, han origin := zitadel_http.DomainContext(ctx) logging.WithFields("origin", requestContext.Origin(), "externalDomain", externalDomain).WithError(err).Error("unable to set instance") zErr := new(zerrors.ZitadelError) + code, _, _ := gerrors.ExtractZITADELError(err) if errors.As(err, &zErr) { zErr.SetMessage(translator.LocalizeFromCtx(ctx, zErr.GetMessage(), nil)) - zErr.Parent = err - return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("unable to set instance using origin %s (ExternalDomain is %s): %s", origin, externalDomain, zErr.Error())) + return nil, connect.NewError(connect.Code(code), fmt.Errorf("unable to set instance using origin %s (ExternalDomain is %s): %s", origin, externalDomain, zErr.Error())) } - return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("unable to set instance using origin %s (ExternalDomain is %s)", origin, externalDomain)) + return nil, connect.NewError(connect.Code(code), fmt.Errorf("unable to set instance using origin %s (ExternalDomain is %s)", origin, externalDomain)) } return handler(authz.WithInstance(ctx, instance), req) } diff --git a/internal/api/grpc/server/connect_middleware/instance_interceptor_test.go b/internal/api/grpc/server/connect_middleware/instance_interceptor_test.go new file mode 100644 index 000000000..8765c3601 --- /dev/null +++ b/internal/api/grpc/server/connect_middleware/instance_interceptor_test.go @@ -0,0 +1,103 @@ +package connect_middleware + +import ( + "context" + "errors" + "testing" + + "connectrpc.com/connect" + "golang.org/x/text/language" + + "github.com/zitadel/zitadel/internal/api/authz" + http_util "github.com/zitadel/zitadel/internal/api/http" + "github.com/zitadel/zitadel/internal/i18n" + "github.com/zitadel/zitadel/internal/zerrors" + object_v3 "github.com/zitadel/zitadel/pkg/grpc/object/v3alpha" +) + +func Test_setInstance_errorCodes(t *testing.T) { + i18n.SupportLanguages(language.English) + translator := i18n.NewZitadelTranslator(language.English) + + cases := []struct { + name string + err error + wantCode connect.Code + }{ + { + name: "not found from verifier propagates as NotFound", + err: zerrors.ThrowNotFound(nil, "TEST-001", "Errors.Instance.NotFound"), + wantCode: connect.CodeNotFound, + }, + { + name: "internal error from verifier propagates as Internal", + err: zerrors.ThrowInternal(errors.New("FATAL: the database system is shutting down (SQLSTATE 57P03)"), "TEST-002", "Errors.Internal"), + wantCode: connect.CodeInternal, + }, + { + name: "unavailable error from verifier propagates as Unavailable", + err: zerrors.ThrowUnavailable(nil, "TEST-003", "Errors.Unavailable"), + wantCode: connect.CodeUnavailable, + }, + } + + for _, tc := range cases { + verifier := &mockInstanceVerifier{err: tc.err} + + t.Run("byRequestedHost/"+tc.name, func(t *testing.T) { + ctx := http_util.WithDomainContext(context.Background(), &http_util.DomainCtx{InstanceHost: "host"}) + _, err := setInstance(ctx, &mockReq[struct{}]{}, nil, verifier, "", translator) + if err == nil { + t.Fatal("expected error, got nil") + } + var connectErr *connect.Error + if !errors.As(err, &connectErr) { + t.Fatalf("expected *connect.Error, got %T", err) + } + if got := connectErr.Code(); got != tc.wantCode { + t.Errorf("got code %v, want %v", got, tc.wantCode) + } + }) + + t.Run("byDomain/"+tc.name, func(t *testing.T) { + req := &mockReq[mockInstanceMsg]{ + Request: connect.Request[mockInstanceMsg]{ + Msg: &mockInstanceMsg{instance: object_v3.Instance{ + Property: &object_v3.Instance_Domain{Domain: "host"}, + }}, + }, + } + _, err := setInstance(context.Background(), req, nil, verifier, "", translator) + if err == nil { + t.Fatal("expected error, got nil") + } + var connectErr *connect.Error + if !errors.As(err, &connectErr) { + t.Fatalf("expected *connect.Error, got %T", err) + } + if got := connectErr.Code(); got != tc.wantCode { + t.Errorf("got code %v, want %v", got, tc.wantCode) + } + }) + } +} + +type mockInstanceMsg struct { + instance object_v3.Instance +} + +func (m *mockInstanceMsg) GetInstance() *object_v3.Instance { + return &m.instance +} + +type mockInstanceVerifier struct { + err error +} + +func (m *mockInstanceVerifier) InstanceByHost(_ context.Context, _, _ string) (authz.Instance, error) { + return nil, m.err +} + +func (m *mockInstanceVerifier) InstanceByID(_ context.Context, _ string) (authz.Instance, error) { + return nil, m.err +} diff --git a/internal/api/grpc/server/middleware/instance_interceptor.go b/internal/api/grpc/server/middleware/instance_interceptor.go index bd584709f..0379eca87 100644 --- a/internal/api/grpc/server/middleware/instance_interceptor.go +++ b/internal/api/grpc/server/middleware/instance_interceptor.go @@ -12,6 +12,7 @@ import ( "google.golang.org/grpc/status" "github.com/zitadel/zitadel/internal/api/authz" + "github.com/zitadel/zitadel/internal/api/grpc/gerrors" zitadel_http "github.com/zitadel/zitadel/internal/api/http" "github.com/zitadel/zitadel/internal/i18n" "github.com/zitadel/zitadel/internal/telemetry/tracing" @@ -69,7 +70,8 @@ func addInstanceByID(ctx context.Context, req interface{}, handler grpc.UnaryHan if errors.As(err, ¬FoundErr) { notFoundErr.Message = translator.LocalizeFromCtx(ctx, notFoundErr.GetMessage(), nil) } - return nil, status.Error(codes.NotFound, fmt.Errorf("unable to set instance using id %s: %w", id, notFoundErr).Error()) + code, _, _ := gerrors.ExtractZITADELError(err) + return nil, status.Error(code, fmt.Errorf("unable to set instance using id %s: %w", id, err).Error()) } return handler(authz.WithInstance(ctx, instance), req) } @@ -81,7 +83,8 @@ func addInstanceByDomain(ctx context.Context, req interface{}, handler grpc.Unar if errors.As(err, ¬FoundErr) && notFoundErr.Kind == zerrors.KindNotFound { notFoundErr.Message = translator.LocalizeFromCtx(ctx, notFoundErr.GetMessage(), nil) } - return nil, status.Error(codes.NotFound, fmt.Errorf("unable to set instance using domain %s: %w", domain, notFoundErr).Error()) + code, _, _ := gerrors.ExtractZITADELError(err) + return nil, status.Error(code, fmt.Errorf("unable to set instance using domain %s: %w", domain, err).Error()) } return handler(authz.WithInstance(ctx, instance), req) } @@ -96,13 +99,13 @@ func addInstanceByRequestedHost(ctx context.Context, req interface{}, handler gr if err != nil { origin := zitadel_http.DomainContext(ctx) logging.WithFields("origin", requestContext.Origin(), "externalDomain", externalDomain).WithError(err).Error("unable to set instance") + code, _, _ := gerrors.ExtractZITADELError(err) zErr := new(zerrors.ZitadelError) if errors.As(err, &zErr) { zErr.SetMessage(translator.LocalizeFromCtx(ctx, zErr.GetMessage(), nil)) - zErr.Parent = err - return nil, status.Error(codes.NotFound, fmt.Sprintf("unable to set instance using origin %s (ExternalDomain is %s): %s", origin, externalDomain, zErr)) + return nil, status.Error(code, fmt.Sprintf("unable to set instance using origin %s (ExternalDomain is %s): %s", origin, externalDomain, zErr.Error())) } - return nil, status.Error(codes.NotFound, fmt.Sprintf("unable to set instance using origin %s (ExternalDomain is %s)", origin, externalDomain)) + return nil, status.Error(code, fmt.Sprintf("unable to set instance using origin %s (ExternalDomain is %s)", origin, externalDomain)) } return handler(authz.WithInstance(ctx, instance), req) } diff --git a/internal/api/grpc/server/middleware/instance_interceptor_test.go b/internal/api/grpc/server/middleware/instance_interceptor_test.go index a57baada6..0731e91f4 100644 --- a/internal/api/grpc/server/middleware/instance_interceptor_test.go +++ b/internal/api/grpc/server/middleware/instance_interceptor_test.go @@ -2,6 +2,7 @@ package middleware import ( "context" + "errors" "fmt" "reflect" "testing" @@ -9,11 +10,15 @@ import ( "golang.org/x/text/language" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/zitadel/zitadel/internal/api/authz" http_util "github.com/zitadel/zitadel/internal/api/http" "github.com/zitadel/zitadel/internal/execution/target" "github.com/zitadel/zitadel/internal/feature" + "github.com/zitadel/zitadel/internal/i18n" + "github.com/zitadel/zitadel/internal/zerrors" object_v3 "github.com/zitadel/zitadel/pkg/grpc/object/v3alpha" ) @@ -229,9 +234,13 @@ type mockInstanceVerifier struct { id string instanceHost string publicHost string + err error } func (m *mockInstanceVerifier) InstanceByHost(_ context.Context, instanceHost, publicHost string) (authz.Instance, error) { + if m.err != nil { + return nil, m.err + } if instanceHost != m.instanceHost { return nil, fmt.Errorf("invalid host") } @@ -245,6 +254,9 @@ func (m *mockInstanceVerifier) InstanceByHost(_ context.Context, instanceHost, p } func (m *mockInstanceVerifier) InstanceByID(_ context.Context, id string) (authz.Instance, error) { + if m.err != nil { + return nil, m.err + } if id != m.id { return nil, fmt.Errorf("not found") } @@ -304,3 +316,75 @@ func (m *mockInstance) Features() feature.Features { func (m *mockInstance) ExecutionRouter() target.Router { return target.NewRouter(nil) } + +func Test_setInstance_errorCodes(t *testing.T) { + i18n.SupportLanguages(language.English) + translator := i18n.NewZitadelTranslator(language.English) + + cases := []struct { + name string + err error + wantCode codes.Code + }{ + { + name: "not found from verifier propagates as NotFound", + err: zerrors.ThrowNotFound(nil, "TEST-001", "Errors.Instance.NotFound"), + wantCode: codes.NotFound, + }, + { + name: "internal error from verifier propagates as Internal", + err: zerrors.ThrowInternal(errors.New("FATAL: the database system is shutting down (SQLSTATE 57P03)"), "TEST-002", "Errors.Internal"), + wantCode: codes.Internal, + }, + { + name: "unavailable error from verifier propagates as Unavailable", + err: zerrors.ThrowUnavailable(nil, "TEST-003", "Errors.Unavailable"), + wantCode: codes.Unavailable, + }, + } + + for _, tc := range cases { + verifier := &mockInstanceVerifier{err: tc.err} + + t.Run("byRequestedHost/"+tc.name, func(t *testing.T) { + ctx := http_util.WithDomainContext(context.Background(), &http_util.DomainCtx{InstanceHost: "host"}) + _, err := setInstance(ctx, &mockRequest{}, &grpc.UnaryServerInfo{}, nil, verifier, "", translator) + if err == nil { + t.Fatal("expected error, got nil") + } + if got := status.Code(err); got != tc.wantCode { + t.Errorf("got code %v, want %v", got, tc.wantCode) + } + }) + + t.Run("byID/"+tc.name, func(t *testing.T) { + req := &mockRequestWithExplicitInstance{ + instance: object_v3.Instance{ + Property: &object_v3.Instance_Id{Id: "any"}, + }, + } + _, err := setInstance(context.Background(), req, &grpc.UnaryServerInfo{}, nil, verifier, "", translator) + if err == nil { + t.Fatal("expected error, got nil") + } + if got := status.Code(err); got != tc.wantCode { + t.Errorf("got code %v, want %v", got, tc.wantCode) + } + }) + + t.Run("byDomain/"+tc.name, func(t *testing.T) { + req := &mockRequestWithExplicitInstance{ + instance: object_v3.Instance{ + Property: &object_v3.Instance_Domain{Domain: "any"}, + }, + } + _, err := setInstance(context.Background(), req, &grpc.UnaryServerInfo{}, nil, verifier, "", translator) + if err == nil { + t.Fatal("expected error, got nil") + } + if got := status.Code(err); got != tc.wantCode { + t.Errorf("got code %v, want %v", got, tc.wantCode) + } + }) + } +} diff --git a/internal/api/http/middleware/instance_interceptor.go b/internal/api/http/middleware/instance_interceptor.go index f43bf8b6f..a4fd1f262 100644 --- a/internal/api/http/middleware/instance_interceptor.go +++ b/internal/api/http/middleware/instance_interceptor.go @@ -48,14 +48,18 @@ func (a *instanceInterceptor) HandlerFunc(next http.Handler) http.HandlerFunc { origin := zitadel_http.DomainContext(r.Context()) logging.WithFields("origin", origin.Origin(), "externalDomain", a.externalDomain).WithError(err).Error("unable to set instance") + statusCode := http.StatusInternalServerError + if zerrors.IsNotFound(err) { + statusCode = http.StatusNotFound + } zErr := new(zerrors.ZitadelError) if errors.As(err, &zErr) { zErr.SetMessage(a.translator.LocalizeFromRequest(r, zErr.GetMessage(), nil)) - http.Error(w, fmt.Sprintf("unable to set instance using origin %s (ExternalDomain is %s): %s", origin, a.externalDomain, zErr), http.StatusNotFound) + http.Error(w, fmt.Sprintf("unable to set instance using origin %s (ExternalDomain is %s): %s", origin, a.externalDomain, zErr), statusCode) return } - http.Error(w, fmt.Sprintf("unable to set instance using origin %s (ExternalDomain is %s)", origin, a.externalDomain), http.StatusNotFound) + http.Error(w, fmt.Sprintf("unable to set instance using origin %s (ExternalDomain is %s)", origin, a.externalDomain), statusCode) } } diff --git a/internal/api/http/middleware/instance_interceptor_test.go b/internal/api/http/middleware/instance_interceptor_test.go index a3174e4b7..a9eb70eb1 100644 --- a/internal/api/http/middleware/instance_interceptor_test.go +++ b/internal/api/http/middleware/instance_interceptor_test.go @@ -2,6 +2,7 @@ package middleware import ( "context" + "errors" "fmt" "net/http" "net/http/httptest" @@ -17,6 +18,7 @@ import ( "github.com/zitadel/zitadel/internal/execution/target" "github.com/zitadel/zitadel/internal/feature" "github.com/zitadel/zitadel/internal/i18n" + "github.com/zitadel/zitadel/internal/zerrors" ) func Test_instanceInterceptor_Handler(t *testing.T) { @@ -278,6 +280,43 @@ func Test_setInstance(t *testing.T) { } } +func Test_instanceInterceptor_HandlerFunc_statusCodes(t *testing.T) { + cases := []struct { + name string + err error + wantStatusCode int + }{ + { + name: "not found from verifier propagates as 404", + err: zerrors.ThrowNotFound(nil, "TEST-001", "Errors.Instance.NotFound"), + wantStatusCode: http.StatusNotFound, + }, + { + name: "internal error from verifier propagates as 500", + err: zerrors.ThrowInternal(errors.New("FATAL: the database system is shutting down (SQLSTATE 57P03)"), "TEST-002", "Errors.Internal"), + wantStatusCode: http.StatusInternalServerError, + }, + { + name: "unavailable error from verifier propagates as 500", + err: zerrors.ThrowUnavailable(nil, "TEST-003", "Errors.Unavailable"), + wantStatusCode: http.StatusInternalServerError, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + a := &instanceInterceptor{ + verifier: &mockInstanceVerifier{err: tc.err}, + translator: i18n.NewZitadelTranslator(language.English), + } + r := httptest.NewRequest("", "/url", nil) + r = r.WithContext(zitadel_http.WithDomainContext(r.Context(), &zitadel_http.DomainCtx{InstanceHost: "host"})) + rr := httptest.NewRecorder() + a.HandlerFunc(&testHandler{}).ServeHTTP(rr, r) + assert.Equal(t, tc.wantStatusCode, rr.Code) + }) + } +} + type testHandler struct { context context.Context } @@ -289,9 +328,13 @@ func (t *testHandler) ServeHTTP(_ http.ResponseWriter, r *http.Request) { type mockInstanceVerifier struct { instanceHost string publicHost string + err error } func (m *mockInstanceVerifier) InstanceByHost(_ context.Context, instanceHost, publicHost string) (authz.Instance, error) { + if m.err != nil { + return nil, m.err + } if instanceHost != m.instanceHost { return nil, fmt.Errorf("invalid host") }