From 9351ec1d2c2f230409d6efa22d07ed6e61775ea7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 6 Nov 2026 16:08:51 -0500 Subject: [PATCH] feat(client-go): typed Unauthenticated vs PermissionDenied errors (#198) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every RPC failure surfaced as a GatewayError with no way to tell a rejected key from a missing scope, though docs/ClientLibrariesDesign.md requires the two be treated distinctly. GatewayError now matches the new sentinels ErrUnauthenticated and ErrPermissionDenied via errors.Is (by gRPC status code only, so it works through the secret-redaction wrapper) and exposes Code(). Tests: errors_auth_test.go — sentinel matching per code, Code(), and a bufconn AcknowledgeAlarm round trip proving the match survives the wire and the password stays redacted (both behavioural tests fail without GatewayError.Is). --- clients/go/GoClientDesign.md | 5 +- clients/go/README.md | 8 ++- clients/go/mxgateway/errors.go | 43 +++++++++++- clients/go/mxgateway/errors_auth_test.go | 85 ++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 clients/go/mxgateway/errors_auth_test.go diff --git a/clients/go/GoClientDesign.md b/clients/go/GoClientDesign.md index dd0d51b..e488df4 100644 --- a/clients/go/GoClientDesign.md +++ b/clients/go/GoClientDesign.md @@ -146,7 +146,10 @@ type MxAccessError struct { ... } ``` Use `errors.Is` / `errors.As` support. Preserve raw protobuf replies on command -errors. +errors. `GatewayError` matches the sentinels `ErrUnauthenticated` and +`ErrPermissionDenied` through `errors.Is` by gRPC status code, so callers treat +the two authorization failures distinctly (`docs/ClientLibrariesDesign.md`) +without parsing messages. ## Test CLI diff --git a/clients/go/README.md b/clients/go/README.md index 01f45a1..b196c16 100644 --- a/clients/go/README.md +++ b/clients/go/README.md @@ -92,7 +92,13 @@ returned subscription owns cancellation and exposes `Close` for deterministic goroutine cleanup. Raw protobuf messages remain available through the `mxgateway` package aliases and the `Raw` helper methods. Typed errors support `errors.As` for `GatewayError`, `CommandError`, and `MxAccessError`; command -errors preserve the raw reply. +errors preserve the raw reply. Authorization failures are distinct: +`errors.Is(err, mxgateway.ErrUnauthenticated)` matches a rejected API key +(gRPC `Unauthenticated` — fix the key) and `errors.Is(err, +mxgateway.ErrPermissionDenied)` matches a valid key missing a scope or another +key's session (gRPC `PermissionDenied` — grant the scope). `GatewayError.Code()` +returns the raw gRPC status code. The match reads only the status code, so it +works through the credential-redaction wrapper. `EnsureMxAccessSuccess` follows COM semantics: only a **negative** HRESULT is a failure, so positive success codes such as `S_FALSE` (1) pass. `StatusSucceeded` diff --git a/clients/go/mxgateway/errors.go b/clients/go/mxgateway/errors.go index 318cd5d..6b1a533 100644 --- a/clients/go/mxgateway/errors.go +++ b/clients/go/mxgateway/errors.go @@ -6,6 +6,8 @@ import ( "strings" pb "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -180,7 +182,20 @@ func (e *MalformedReplyError) Error() string { return fmt.Sprintf("mxgateway: %s returned a malformed reply: %s", e.Op, e.Detail) } -// GatewayError wraps transport-level gRPC failures. +// ErrUnauthenticated matches, via errors.Is, a GatewayError whose gRPC status is +// Unauthenticated: the gateway did not accept the API key (missing, malformed, +// unknown, or revoked). Retrying with the same key will not help. +var ErrUnauthenticated = errors.New("mxgateway: unauthenticated") + +// ErrPermissionDenied matches, via errors.Is, a GatewayError whose gRPC status +// is PermissionDenied: the API key is valid but lacks the scope the RPC needs, or +// the session belongs to another key. It is distinct from ErrUnauthenticated so a +// caller can tell "fix the key" from "grant the key a scope". +var ErrPermissionDenied = errors.New("mxgateway: permission denied") + +// GatewayError wraps transport-level gRPC failures. errors.Is(err, +// ErrUnauthenticated) and errors.Is(err, ErrPermissionDenied) classify the two +// authorization failures; Code returns the raw gRPC status code. type GatewayError struct { // Op names the operation that failed (for example "dial" or "invoke"). Op string @@ -207,6 +222,32 @@ func (e *GatewayError) Unwrap() error { return e.Err } +// Code returns the gRPC status code of the wrapped error, codes.Unknown for a +// non-gRPC error, and codes.OK for a nil receiver or nil wrapped error. +func (e *GatewayError) Code() codes.Code { + if e == nil || e.Err == nil { + return codes.OK + } + return status.Code(e.Err) +} + +// Is reports whether target is the sentinel matching this error's gRPC status +// code (ErrUnauthenticated or ErrPermissionDenied). The check reads only the +// status code, never the message, so credential redaction is unaffected. +func (e *GatewayError) Is(target error) bool { + if e == nil { + return false + } + switch target { + case ErrUnauthenticated: + return e.Code() == codes.Unauthenticated + case ErrPermissionDenied: + return e.Code() == codes.PermissionDenied + default: + return false + } +} + // CommandError reports a non-OK gateway protocol status and keeps the raw // command reply when one exists. type CommandError struct { diff --git a/clients/go/mxgateway/errors_auth_test.go b/clients/go/mxgateway/errors_auth_test.go new file mode 100644 index 0000000..d18c5c6 --- /dev/null +++ b/clients/go/mxgateway/errors_auth_test.go @@ -0,0 +1,85 @@ +package mxgateway + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + + pb "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// #198 — the gateway answers a bad key with Unauthenticated and a missing scope +// with PermissionDenied; callers must be able to tell them apart with errors.Is. + +func TestGatewayErrorMatchesAuthSentinelsByStatusCode(t *testing.T) { + cases := []struct { + name string + err error + unauthenticated bool + permissionDenied bool + }{ + {"unauthenticated", status.Error(codes.Unauthenticated, "bad key"), true, false}, + {"permission denied", status.Error(codes.PermissionDenied, "scope required"), false, true}, + {"unavailable", status.Error(codes.Unavailable, "down"), false, false}, + {"non-grpc", errors.New("plain"), false, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := fmt.Errorf("outer: %w", &GatewayError{Op: "invoke", Err: tc.err}) + if got := errors.Is(err, ErrUnauthenticated); got != tc.unauthenticated { + t.Fatalf("errors.Is(ErrUnauthenticated) = %v, want %v", got, tc.unauthenticated) + } + if got := errors.Is(err, ErrPermissionDenied); got != tc.permissionDenied { + t.Fatalf("errors.Is(ErrPermissionDenied) = %v, want %v", got, tc.permissionDenied) + } + }) + } +} + +func TestGatewayErrorCode(t *testing.T) { + if got := (&GatewayError{Err: status.Error(codes.PermissionDenied, "x")}).Code(); got != codes.PermissionDenied { + t.Fatalf("Code() = %v", got) + } + if got := (*GatewayError)(nil).Code(); got != codes.OK { + t.Fatalf("nil Code() = %v", got) + } +} + +func TestAuthSentinelsSurviveTheWireAndRedaction(t *testing.T) { + for _, tc := range []struct { + code codes.Code + sentinel error + other error + }{ + {codes.Unauthenticated, ErrUnauthenticated, ErrPermissionDenied}, + {codes.PermissionDenied, ErrPermissionDenied, ErrUnauthenticated}, + } { + t.Run(tc.code.String(), func(t *testing.T) { + fake := &fakeGatewayWithAlarms{acknowledgeError: status.Error(tc.code, "denied pw-secret")} + client, cleanup := newBufconnClientWithAlarms(t, fake) + defer cleanup() + + _, err := client.AcknowledgeAlarm(context.Background(), &pb.AcknowledgeAlarmRequest{ + AlarmFullReference: "Tank01.Level.HiHi", + OperatorUser: "alice", + OperatorPassword: "pw-secret", + }) + if err == nil { + t.Fatal("AcknowledgeAlarm() returned no error") + } + if !errors.Is(err, tc.sentinel) { + t.Fatalf("errors.Is(%v) = false for %v", tc.sentinel, err) + } + if errors.Is(err, tc.other) { + t.Fatalf("errors.Is(%v) = true for %v", tc.other, err) + } + if strings.Contains(err.Error(), "pw-secret") { + t.Fatalf("error leaked the operator password: %v", err) + } + }) + } +}