feat(middleware): enhance middleware configuration for auth #67
Conversation
• Introduces optional authentication middleware in CreateMiddleware • Updates tests to validate middleware behavior with and without auth
WalkthroughRemoves the exported Middleware type alias, updates CreateAuthMiddleware to return a slice of concrete middleware functions, modifies CreateMiddleware to accept a boolean to optionally include auth middlewares, and updates tests to cover both with-auth and without-auth paths. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/authzMiddlewares.go (1)
7-15: Avoid breaking change: reintroduce a deprecated alias for compatibilityRemoving the exported middleware type breaks any downstream code that referenced
middleware.Middlewareor[]Middleware. If the intent isn’t a major release, keep a type alias to preserve source compatibility while still using the concrete function type internally.Apply this diff in this package (e.g., here or a small types.go):
package middleware import ( "net/http" ) +// Deprecated: Use func(http.Handler) http.Handler directly. Kept for source compatibility; will be removed in the next major version. +type Middleware = func(http.Handler) http.Handler + // CreateAuthMiddleware returns a slice of middleware functions for authentication and authorization. // The returned middlewares are: StoreWebToken, StoreAuthHeader, and StoreSpiffeHeader. func CreateAuthMiddleware() []func(http.Handler) http.Handler { return []func(http.Handler) http.Handler{ StoreWebToken(), StoreAuthHeader(), StoreSpiffeHeader(), } }Also consider renaming to
CreateAuthMiddlewaresfor clarity (optional; would be breaking).
🧹 Nitpick comments (6)
middleware/middleware_test.go (3)
12-18: Parallelize test and keep count assertion stable over timeRun in parallel and avoid hard-coding the base count to reduce churn if base middlewares change later.
Apply this diff:
-func TestCreateMiddleware_WithoutAuth(t *testing.T) { - log := testlogger.New() - middlewares := CreateMiddleware(log.Logger, false) +func TestCreateMiddleware_WithoutAuth(t *testing.T) { + t.Parallel() + log := testlogger.New() + middlewares := CreateMiddleware(log.Logger, false) - // Should return 5 middlewares when auth is false - assert.Len(t, middlewares, 5) + // Should return only base middlewares when auth is false + assert.GreaterOrEqual(t, len(middlewares), 1)
43-53: Derive expected length instead of hard-coding 8Compute expected count from the base chain +
CreateAuthMiddleware()so the test won’t break when the list evolves.Apply this diff:
-func TestCreateMiddleware_WithAuth(t *testing.T) { - log := testlogger.New() - middlewares := CreateMiddleware(log.Logger, true) +func TestCreateMiddleware_WithAuth(t *testing.T) { + t.Parallel() + log := testlogger.New() + middlewares := CreateMiddleware(log.Logger, true) - // Should return 8 middlewares when auth is true (5 base + 3 auth) - assert.Len(t, middlewares, 8) + // Should return base + auth middlewares + base := CreateMiddleware(log.Logger, false) + expected := len(base) + len(CreateAuthMiddleware()) + assert.Len(t, middlewares, expected) // Each middleware should be a valid function for _, mw := range middlewares { assert.NotNil(t, mw) }
55-72: Add a panic‑recovery assertion to verifySentryRecovererworks in the composed chainSmall, focused test ensures recovery behavior stays intact if order changes.
Add this new test (outside this hunk):
func TestCreateMiddleware_RecoversFromPanic(t *testing.T) { t.Parallel() log := testlogger.New() middlewares := CreateMiddleware(log.Logger, false) panicHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { panic("boom") }) var final http.Handler = panicHandler for i := len(middlewares) - 1; i >= 0; i-- { final = middlewares[i](final) } req := httptest.NewRequest("GET", "http://testing", nil) rec := httptest.NewRecorder() final.ServeHTTP(rec, req) assert.Equal(t, http.StatusInternalServerError, rec.Code) }middleware/middleware.go (3)
9-12: Document ordering contractCall out that the first element becomes the outermost wrapper when applying right-to-left, to prevent accidental misuse by consumers composing differently.
Proposed tweak:
-// CreateMiddleware creates a middleware chain with logging, tracing, and optional authentication. +// CreateMiddleware creates a middleware chain with logging, tracing, and optional authentication. +// Order matters: the first function in the returned slice is intended to be applied last +// so it becomes the outermost wrapper when composing from right to left.
13-19: Make SentryRecoverer the outermost middlewarePlace
SentryRecovererfirst so it wraps everything and reliably catches panics in all subsequent middleware, including tracing.Apply this diff:
func CreateMiddleware(log *logger.Logger, auth bool) []func(http.Handler) http.Handler { mws := []func(http.Handler) http.Handler{ - SetOtelTracingContext(), - SentryRecoverer, + SentryRecoverer, + SetOtelTracingContext(), StoreLoggerMiddleware(log), SetRequestId(), SetRequestIdInLogger(), }
12-24: Consider an options pattern instead of a boolean flagA boolean
authdoesn’t scale. Options allow future toggles (e.g., custom auth set, request ID strategy) without signature churn.Example sketch:
type Options struct { Auth bool } func CreateMiddlewareWithOpts(log *logger.Logger, opts Options) []func(http.Handler) http.Handler { /* ... */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
middleware/authzMiddlewares.go(1 hunks)middleware/middleware.go(1 hunks)middleware/middleware_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
middleware/middleware.go (5)
middleware/otel.go (1)
SetOtelTracingContext(18-25)middleware/sentry.go (1)
SentryRecoverer(19-36)middleware/logger.go (1)
StoreLoggerMiddleware(11-21)middleware/requestid.go (2)
SetRequestId(18-33)SetRequestIdInLogger(40-50)middleware/authzMiddlewares.go (1)
CreateAuthMiddleware(9-15)
middleware/middleware_test.go (1)
middleware/middleware.go (1)
CreateMiddleware(12-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / lint / lint
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
middleware/middleware.go (1)
9-25: Verify repo-wide call sites updated to new signatureDefinition found at middleware/middleware.go:12 and two callers in middleware/middleware_test.go:14 and 45 — both use CreateMiddleware(log.Logger, ...). No matches for
type Middlewareor[]Middleware.
Summary by CodeRabbit