test+fix: address principal-engineer review findings (Phases 0-4)

cmd/root.go

@@ -106,8 +106,9 @@ func init() {
 	f.IntVar(&rootArgs.config.GraphQLMaxAliases, "graphql-max-aliases", 30, "Maximum total number of aliased fields per GraphQL operation")
 	f.Int64Var(&rootArgs.config.GraphQLMaxBodyBytes, "graphql-max-body-bytes", 1<<20, "Maximum allowed GraphQL request body size in bytes (default 1MB)")
 
-	// gRPC server flags
-	f.IntVar(&rootArgs.config.GRPCPort, "grpc-port", 8081, "Port the gRPC server listens on")
+	// gRPC server flags. Port 9091 avoids collision with the metrics
+	// listener which defaults to 8081 (and with the HTTP listener on 8080).
+	f.IntVar(&rootArgs.config.GRPCPort, "grpc-port", 9091, "Port the gRPC server listens on")
 	f.BoolVar(&rootArgs.config.EnableGRPCReflection, "enable-grpc-reflection", true, "Enable the gRPC server-reflection service")
 	f.StringVar(&rootArgs.config.GRPCTLSCert, "grpc-tls-cert", "", "Path to the TLS certificate for the gRPC server")
 	f.StringVar(&rootArgs.config.GRPCTLSKey, "grpc-tls-key", "", "Path to the TLS private key for the gRPC server")
@@ -344,9 +345,20 @@ func applyFlagDefaults() {
 // Run the service
 func runRoot(c *cobra.Command, args []string) {
 	applyFlagDefaults()
-	if rootArgs.server.HTTPPort == rootArgs.server.MetricsPort {
-		fmt.Fprintf(os.Stderr, "invalid server ports: --http-port and --metrics-port must differ (metrics are always served on a dedicated listener)\n")
-		os.Exit(1)
+	// All three listeners (HTTP, metrics, gRPC) bind concurrently; any
+	// collision is unrecoverable at runtime, so we fail fast at startup.
+	ports := map[string]int{
+		"--http-port":    rootArgs.server.HTTPPort,
+		"--metrics-port": rootArgs.server.MetricsPort,
+		"--grpc-port":    rootArgs.config.GRPCPort,
+	}
+	for nameA, a := range ports {
+		for nameB, b := range ports {
+			if nameA < nameB && a == b {
+				fmt.Fprintf(os.Stderr, "invalid server ports: %s (%d) and %s (%d) must differ — each listener binds independently\n", nameA, a, nameB, b)
+				os.Exit(1)
+			}
+		}
 	}
 
 	// Refuse to start without an admin secret. The previous default of

gen/openapi/openapi.go

@@ -0,0 +1,13 @@
+// Package openapi exposes the generated OpenAPI 2.0 spec as a byte slice
+// so HTTP handlers can serve it from any working directory (test, Docker
+// container, etc.). The file is embedded at compile time via go:embed so
+// builds fail loudly if `make proto-gen` hasn't been run.
+package openapi
+
+import _ "embed"
+
+//go:embed authorizer.swagger.json
+var spec []byte
+
+// Spec returns the embedded OpenAPI 2.0 JSON.
+func Spec() []byte { return spec }

internal/cookie/cookie_test.go

@@ -0,0 +1,91 @@
+package cookie
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/authorizerdev/authorizer/internal/constants"
+)
+
+func TestBuildSessionCookies(t *testing.T) {
+	tests := []struct {
+		name       string
+		hostname   string
+		secure     bool
+		sameSite   http.SameSite
+		wantDomain string // expected `.example.com`-style domain on the domain-scoped cookie
+	}{
+		{"production https", "https://auth.example.com", true, http.SameSiteNoneMode, ".example.com"},
+		{"localhost dev", "http://localhost:8080", false, http.SameSiteLaxMode, "localhost"},
+		{"subdomain", "https://auth.svc.example.com", true, http.SameSiteStrictMode, ".example.com"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cookies := BuildSessionCookies(tt.hostname, "session-id", tt.secure, tt.sameSite)
+			require.Len(t, cookies, 2, "BuildSessionCookies must return exactly the host-scoped and domain-scoped pair")
+
+			for _, c := range cookies {
+				assert.Equal(t, "session-id", c.Value)
+				assert.Equal(t, tt.secure, c.Secure)
+				assert.True(t, c.HttpOnly, "session cookies must be HttpOnly")
+				assert.Equal(t, "/", c.Path)
+				assert.Equal(t, tt.sameSite, c.SameSite)
+				assert.Equal(t, 24*60*60, c.MaxAge, "session cookie MaxAge must be 1 day")
+			}
+
+			// Sanity-check cookie names.
+			assert.Equal(t, constants.AppCookieName+"_session", cookies[0].Name)
+			assert.Equal(t, constants.AppCookieName+"_session_domain", cookies[1].Name)
+			// Domain-scoped cookie picks up the apex.
+			assert.Equal(t, tt.wantDomain, cookies[1].Domain)
+		})
+	}
+}
+
+func TestBuildMfaSessionCookies(t *testing.T) {
+	cookies := BuildMfaSessionCookies("https://auth.example.com", "mfa-id", true)
+	require.Len(t, cookies, 2)
+	for _, c := range cookies {
+		assert.Equal(t, "mfa-id", c.Value)
+		assert.True(t, c.Secure)
+		assert.True(t, c.HttpOnly)
+		assert.Equal(t, http.SameSiteNoneMode, c.SameSite, "secure → SameSite=None")
+		assert.Equal(t, 60, c.MaxAge, "MFA cookies are short-lived (60s)")
+	}
+	assert.Equal(t, constants.MfaCookieName+"_session", cookies[0].Name)
+	assert.Equal(t, constants.MfaCookieName+"_session_domain", cookies[1].Name)
+}
+
+func TestBuildMfaSessionCookies_InsecureLaxSameSite(t *testing.T) {
+	cookies := BuildMfaSessionCookies("http://localhost:8080", "mfa-id", false)
+	require.Len(t, cookies, 2)
+	for _, c := range cookies {
+		assert.False(t, c.Secure)
+		// Insecure → SameSite=Lax (so cross-site flows still complete when not behind TLS).
+		// Verified against the original SetMfaSession behaviour: this is intentional.
+		assert.Equal(t, http.SameSiteLaxMode, c.SameSite)
+	}
+}
+
+func TestParseSameSite(t *testing.T) {
+	tests := []struct {
+		in   string
+		want http.SameSite
+	}{
+		{"none", http.SameSiteNoneMode},
+		{"NONE", http.SameSiteNoneMode},
+		{"strict", http.SameSiteStrictMode},
+		{"lax", http.SameSiteLaxMode},
+		{"", http.SameSiteLaxMode},      // unknown defaults to Lax
+		{"garbage", http.SameSiteLaxMode},
+		{"  none  ", http.SameSiteNoneMode},
+	}
+	for _, tt := range tests {
+		t.Run(tt.in, func(t *testing.T) {
+			assert.Equal(t, tt.want, ParseSameSite(tt.in))
+		})
+	}
+}

internal/grpcsrv/interceptors/interceptors_test.go

@@ -0,0 +1,145 @@
+package interceptors
+
+import (
+	"bytes"
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/rs/zerolog"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
+	metav1 "github.com/authorizerdev/authorizer/gen/go/authorizer/meta/v1"
+	userv1 "github.com/authorizerdev/authorizer/gen/go/authorizer/user/v1"
+)
+
+// info builds a *grpc.UnaryServerInfo for a fake RPC. The full-method name is
+// the only field interceptors actually read.
+func info(method string) *grpc.UnaryServerInfo {
+	return &grpc.UnaryServerInfo{FullMethod: method}
+}
+
+func TestRecovery_TurnsPanicIntoInternal(t *testing.T) {
+	var buf bytes.Buffer
+	log := zerolog.New(&buf)
+
+	r := Recovery(&log)
+	_, err := r(context.Background(), nil, info("/svc/Method"), func(_ context.Context, _ any) (any, error) {
+		panic("kaboom")
+	})
+
+	st, ok := status.FromError(err)
+	require.True(t, ok, "expected a gRPC status error")
+	assert.Equal(t, codes.Internal, st.Code())
+	assert.Equal(t, "internal server error", st.Message(), "panic detail must not leak to clients")
+	// The stack stays server-side.
+	assert.Contains(t, buf.String(), "panicked")
+	assert.Contains(t, buf.String(), "kaboom")
+}
+
+func TestRecovery_PassesNormalErrorsThrough(t *testing.T) {
+	log := zerolog.Nop()
+	r := Recovery(&log)
+	want := status.Error(codes.NotFound, "no")
+	_, err := r(context.Background(), nil, info("/svc/X"), func(_ context.Context, _ any) (any, error) {
+		return nil, want
+	})
+	assert.Equal(t, want, err)
+}
+
+func TestLogging_OkPath(t *testing.T) {
+	var buf bytes.Buffer
+	log := zerolog.New(&buf)
+	mw := Logging(&log)
+	_, err := mw(context.Background(), nil, info("/svc/Foo"), func(_ context.Context, _ any) (any, error) {
+		return "ok", nil
+	})
+	require.NoError(t, err)
+	out := buf.String()
+	assert.Contains(t, out, `"method":"/svc/Foo"`)
+	assert.Contains(t, out, `"code":"OK"`)
+	assert.Contains(t, out, `"level":"info"`)
+}
+
+func TestLogging_ErrorPathRaisesLevel(t *testing.T) {
+	var buf bytes.Buffer
+	log := zerolog.New(&buf)
+	mw := Logging(&log)
+	_, _ = mw(context.Background(), nil, info("/svc/Bad"), func(_ context.Context, _ any) (any, error) {
+		return nil, status.Error(codes.Internal, "boom")
+	})
+	out := buf.String()
+	assert.Contains(t, out, `"code":"Internal"`)
+	assert.Contains(t, out, `"level":"error"`, "Internal/Unknown/DataLoss must raise log level to error")
+}
+
+func TestLogging_PermissionDeniedIsWarn(t *testing.T) {
+	var buf bytes.Buffer
+	log := zerolog.New(&buf)
+	mw := Logging(&log)
+	_, _ = mw(context.Background(), nil, info("/svc/X"), func(_ context.Context, _ any) (any, error) {
+		return nil, status.Error(codes.PermissionDenied, "no")
+	})
+	assert.Contains(t, buf.String(), `"level":"warn"`, "non-Internal failures must log at warn, not error")
+}
+
+func TestValidate_RejectsBadRequest(t *testing.T) {
+	mw, err := Validate()
+	require.NoError(t, err)
+
+	// CreateUserRequest enforces email format via buf.validate.field on the email
+	// field — sending an invalid email should fail the interceptor before any
+	// handler runs.
+	req := &userv1.CreateUserRequest{
+		Email:           "not-an-email",
+		Password:        "x",
+		ConfirmPassword: "x",
+	}
+	_, err = mw(context.Background(), req, info("/svc/CreateUser"), func(_ context.Context, _ any) (any, error) {
+		t.Fatal("handler must NOT run for an invalid request")
+		return nil, nil
+	})
+	st, ok := status.FromError(err)
+	require.True(t, ok)
+	assert.Equal(t, codes.InvalidArgument, st.Code())
+}
+
+func TestValidate_AllowsValidRequest(t *testing.T) {
+	mw, err := Validate()
+	require.NoError(t, err)
+	called := false
+	_, err = mw(context.Background(), &metav1.GetMetaRequest{}, info("/svc/GetMeta"), func(_ context.Context, _ any) (any, error) {
+		called = true
+		return &metav1.GetMetaResponse{}, nil
+	})
+	require.NoError(t, err)
+	assert.True(t, called, "valid request must reach the handler")
+}
+
+func TestValidate_NonProtoRequestPassesThrough(t *testing.T) {
+	mw, err := Validate()
+	require.NoError(t, err)
+	_, err = mw(context.Background(), "not-a-proto", info("/svc/X"), func(_ context.Context, _ any) (any, error) {
+		return nil, nil
+	})
+	require.NoError(t, err, "non-proto requests must not be rejected by the validator")
+}
+
+// TestValidate_PreservesInvariant guards against regressions where someone
+// makes Validate() return a non-functional middleware (e.g. by reordering
+// the protovalidate.New() call). If the validator itself fails to build,
+// callers must learn about it at startup, not at first request.
+func TestValidate_BuildsCleanly(t *testing.T) {
+	mw, err := Validate()
+	require.NoError(t, err)
+	require.NotNil(t, mw)
+	// Sanity check: the returned interceptor type is what gRPC expects.
+	_ = grpc.UnaryServerInterceptor(mw)
+}
+
+// helper used by some of the future interceptor tests
+func _ignoreUnused() { _ = strings.Builder{} }

internal/grpcsrv/transport/grpc_metadata.go

@@ -3,15 +3,15 @@
 // ResponseSideEffects.
 //
 // gRPC has no native cookie concept; cookies in ResponseSideEffects are
-// serialised to a `Set-Cookie` trailer, which grpc-gateway then promotes
-// into actual `Set-Cookie` response headers when the call comes in via REST.
-// Pure-gRPC clients (server-to-server) typically don't need cookies and
-// silently ignore them.
+// serialised to `Set-Cookie` metadata entries. grpc-gateway promotes those
+// into real `Set-Cookie` response headers when the call came in via REST.
+// Pure-gRPC clients can read them via the response trailers or ignore them.
 package transport
 
 import (
 	"context"
 	"net/http"
+	"strings"
 
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/metadata"
@@ -30,6 +30,7 @@ func MetaFromGRPC(ctx context.Context) service.RequestMetadata {
 		IPAddress:           firstHeader(md, "x-forwarded-for", "grpcgateway-x-forwarded-for", "x-real-ip"),
 		UserAgent:           firstHeader(md, "grpcgateway-user-agent", "user-agent"),
 		AuthorizationHeader: firstHeader(md, "authorization", "grpcgateway-authorization"),
+		Cookies:             cookiesFromMetadata(md),
 	}
 	// Default the host URL when no header was set (pure-gRPC caller, no
 	// proxy headers). The :authority pseudo-header is the gRPC equivalent
@@ -42,23 +43,30 @@ func MetaFromGRPC(ctx context.Context) service.RequestMetadata {
 	return meta
 }
 
-// ApplyToGRPC writes the response side-effects to the outgoing gRPC stream:
-// cookies become Set-Cookie metadata trailers. A nil receiver is a no-op.
+// ApplyToGRPC writes the response side-effects to the outgoing gRPC stream.
+// Every cookie becomes its own `Set-Cookie` metadata entry — preserving
+// multi-cookie responses (e.g. host-scoped + domain-scoped session pair).
+// grpc-gateway promotes the metadata back to real `Set-Cookie` HTTP headers.
+// A nil receiver is a no-op.
 func ApplyToGRPC(ctx context.Context, side *service.ResponseSideEffects) error {
 	if side == nil || len(side.Cookies) == 0 {
 		return nil
 	}
-	values := make([]string, 0, len(side.Cookies))
+	// grpc-gateway honours the per-RPC `Set-Cookie` metadata when prefixed
+	// `Grpc-Metadata-Set-Cookie` or under the canonical header. Use
+	// metadata.Pairs equivalents: same key, repeated values.
+	header := http.CanonicalHeaderKey("Set-Cookie")
+	md := metadata.MD{}
 	for _, c := range side.Cookies {
 		if c == nil {
 			continue
 		}
-		values = append(values, c.String())
+		md.Append(header, c.String())
 	}
-	if len(values) == 0 {
+	if len(md) == 0 {
 		return nil
 	}
-	return grpc.SendHeader(ctx, metadata.Pairs(http.CanonicalHeaderKey("Set-Cookie"), values[0])) //nolint:staticcheck // only one cookie surfaces; multi-cookie comes with the gateway-aware wiring
+	return grpc.SendHeader(ctx, md)
 }
 
 func firstHeader(md metadata.MD, keys ...string) string {
@@ -69,3 +77,25 @@ func firstHeader(md metadata.MD, keys ...string) string {
 	}
 	return ""
 }
+
+// cookiesFromMetadata parses Cookie header(s) supplied via gRPC metadata.
+// grpc-gateway forwards browser cookies as the `grpcgateway-cookie` key;
+// pure-gRPC clients can set `cookie` directly. Multiple Cookie headers are
+// concatenated (semicolon-separated per RFC 6265).
+func cookiesFromMetadata(md metadata.MD) []*http.Cookie {
+	var raw []string
+	raw = append(raw, md.Get("grpcgateway-cookie")...)
+	raw = append(raw, md.Get("cookie")...)
+	if len(raw) == 0 {
+		return nil
+	}
+	// http.Request.Cookies parses the Cookie header for us. Synthesize a
+	// minimal request rather than re-implementing the cookie grammar.
+	req := &http.Request{Header: http.Header{}}
+	for _, line := range raw {
+		// One header may contain multiple cookies separated by "; ".
+		// http.Header.Add preserves the line; cookies are parsed downstream.
+		req.Header.Add("Cookie", strings.TrimSpace(line))
+	}
+	return req.Cookies()
+}