feat: adding various middlewares for services #64
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds multiple HTTP middlewares and related tests: auth header, bearer token, SPIFFE, OpenTelemetry, request ID, logger, Sentry recoverer; middleware builders (CreateMiddleware, CreateAuthMiddleware); a build‑tagged local test middleware; .testcoverage.yml exclusion; and a JWT doc comment. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (21)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/token_test.go (1)
1-155: Parsing robustness: consider trimming and collapsing spaces in middleware/token.goStoreWebToken uses strings.Split on a single space, which fails for headers with extra spaces (e.g., "Bearer t"). Prefer strings.Fields and check len==2.
Proposed change in middleware/token.go (outside this file):
- auth := strings.Split(request.Header.Get(headers.Authorization), " ") - if len(auth) > 1 && strings.ToUpper(auth[0]) == tokenAuthPrefix { - ctx = context.AddWebTokenToContext(ctx, auth[1], SignatureAlgorithms) + parts := strings.Fields(request.Header.Get(headers.Authorization)) + if len(parts) == 2 && strings.ToUpper(parts[0]) == tokenAuthPrefix { + ctx = context.AddWebTokenToContext(ctx, parts[1], SignatureAlgorithms) }If you want, I can add a test case for multiple spaces.
🧹 Nitpick comments (29)
.testcoverage.yml (1)
7-7: Anchor the new exclude path for consistency with existing patterns.The other entries use regex anchors (e.g., ^test/). Consider anchoring this one too to avoid partial matches.
- - middleware/test_support + - ^middleware/test_supportmiddleware/sentry.go (1)
17-24: Harden recovery path: write a body and consider reusing common recover helper.
- Prefer http.Error to ensure a response body when possible.
- Optionally reuse context.Recover(log) to avoid duplication and centralize Sentry behavior.
- w.WriteHeader(http.StatusInternalServerError) + // Respond with 500 and a minimal body (safe for most cases). + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)middleware/test_support/local_middleware.go (1)
16-23: Be explicit about JOSE alg type and issuer format.Minor clarity improvements:
- Cast "none" explicitly to jose.SignatureAlgorithm.
- Use a full URL for Issuer for better interop.
- claims := &jwt.RegisteredClaims{Issuer: "localhost:8080", Subject: userId, Audience: jwt.ClaimStrings{"testing"}} + claims := &jwt.RegisteredClaims{Issuer: "http://localhost:8080", Subject: userId, Audience: jwt.ClaimStrings{"testing"}} - ctx = context.AddWebTokenToContext(ctx, token, []jose.SignatureAlgorithm{"none"}) + ctx = context.AddWebTokenToContext(ctx, token, []jose.SignatureAlgorithm{jose.SignatureAlgorithm("none")})middleware/otel_test.go (3)
16-19: Restore the global propagator after the test to avoid cross-test leakage.propagator := propagation.TraceContext{} -otel.SetTextMapPropagator(propagator) +old := otel.GetTextMapPropagator() +otel.SetTextMapPropagator(propagator) +defer otel.SetTextMapPropagator(old)
47-50: Also restore propagator here.propagator := propagation.TraceContext{} -otel.SetTextMapPropagator(propagator) +old := otel.GetTextMapPropagator() +otel.SetTextMapPropagator(propagator) +defer otel.SetTextMapPropagator(old)
70-72: Restore propagator in the integration test as well.propagator := propagation.TraceContext{} -otel.SetTextMapPropagator(propagator) +old := otel.GetTextMapPropagator() +otel.SetTextMapPropagator(propagator) +defer otel.SetTextMapPropagator(old)middleware/requestid.go (2)
19-24: Consider accepting the first header value instead of requiring exactly one.Using Header.Get(requestIdHeader) would gracefully handle multiple values instead of discarding them.
- if ids, ok := request.Header[requestIdHeader]; ok && len(ids) == 1 { - requestId = ids[0] - } else { + if v := request.Header.Get(requestIdHeader); v != "" { + requestId = v + } else { // Generate a new request id, header was not received. requestId = uuid.New().String() }
25-27: Echo the request ID back in the response header.Helps clients correlate logs across hops.
ctx = context.WithValue(ctx, keys.RequestIdCtxKey, requestId) -next.ServeHTTP(responseWriter, request.WithContext(ctx)) +responseWriter.Header().Set(requestIdHeader, requestId) +next.ServeHTTP(responseWriter, request.WithContext(ctx))middleware/requestid_test.go (2)
31-31: Fix test name typo.-func TestSetRequestIdWitoutIncomingHeader(t *testing.T) { +func TestSetRequestIdWithoutIncomingHeader(t *testing.T) {
48-63: Strengthen the SetRequestIdInLogger test with an assertion.Assert a logger is present in context (and non-nil) to verify the middleware effect.
handlerToTest := SetRequestIdInLogger()(nextHandler) @@ req := httptest.NewRequest("GET", "http://testing", nil) -// call the handler using a mock response recorder -recorder := httptest.NewRecorder() -handlerToTest.ServeHTTP(httptest.NewRecorder(), req) +recorder := httptest.NewRecorder() +handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Assert logger exists + lg := logger.LoadLoggerFromContext(r.Context()) + assert.NotNil(t, lg) + w.WriteHeader(http.StatusOK) +}) +SetRequestIdInLogger()(handler).ServeHTTP(recorder, req) +assert.Equal(t, http.StatusOK, recorder.Code)middleware/logger_test.go (2)
21-23: Use assert.Same for identity, not assert.Equal.We want pointer identity (same instance), not structural equality.
- assert.Equal(t, testLog.Logger, logFromContext) + assert.Same(t, testLog.Logger, logFromContext)
13-13: Run tests in parallel.These tests are independent and can run with t.Parallel().
func TestStoreLoggerMiddleware(t *testing.T) { + t.Parallel()func TestStoreLoggerMiddleware_NilLogger(t *testing.T) { + t.Parallel()Also applies to: 38-38
middleware/middleware_test.go (3)
16-18: Avoid brittle length assertion.As the chain evolves, asserting exactly 5 will cause needless churn. Prefer ≥ 5 or assert presence/behavior of key middlewares.
- // Should return 5 middlewares - assert.Len(t, middlewares, 5) + // Should return at least the expected middlewares + assert.GreaterOrEqual(t, len(middlewares), 5)
29-33: Verify order-dependent behavior (logger before SetRequestIdInLogger).Add a subtest that asserts SetRequestIdInLogger sees a non-nil logger and request ID, ensuring chain order correctness.
12-12: Run test in parallel.func TestCreateMiddleware(t *testing.T) { + t.Parallel()middleware/middleware.go (1)
11-16: Consider SentryRecoverer as the outermost wrapper.To catch panics from any middleware (including OTel extraction), place SentryRecoverer first in the chain (i.e., applied outermost).
return []func(http.Handler) http.Handler{ - SetOtelTracingContext(), - SentryRecoverer, + SentryRecoverer, + SetOtelTracingContext(), StoreLoggerMiddleware(log), SetRequestId(), SetRequestIdInLogger(), }middleware/spiffe_test.go (2)
101-125: Make multi-header behavior deterministic.Assert that the context value equals jwt.GetSpiffeUrlValue(r.Header), not just that it “contains spiffe://”.
nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // With multiple headers, the behavior depends on jwt.GetSpiffeUrlValue implementation - // It should either return the first valid one or handle concatenated headers - spiffeFromContext, err := context.GetSpiffeFromContext(r.Context()) - if err == nil { - // If we get a value, it should be a valid SPIFFE ID - assert.Contains(t, spiffeFromContext, "spiffe://") - } + spiffeValue := jwt.GetSpiffeUrlValue(r.Header) + if spiffeValue != nil { + spiffeFromContext, err := context.GetSpiffeFromContext(r.Context()) + assert.NoError(t, err) + assert.Equal(t, *spiffeValue, spiffeFromContext) + } else { + _, err := context.GetSpiffeFromContext(r.Context()) + assert.Error(t, err) + }
13-13: Run tests in parallel.func TestStoreSpiffeHeader_WithValidSpiffeHeader(t *testing.T) { + t.Parallel()func TestStoreSpiffeHeader_WithoutSpiffeHeader(t *testing.T) { + t.Parallel()func TestStoreSpiffeHeader_WithEmptySpiffeHeader(t *testing.T) { + t.Parallel()func TestStoreSpiffeHeader_WithInvalidSpiffeHeader(t *testing.T) { + t.Parallel()func TestStoreSpiffeHeader_WithMultipleSpiffeHeaders(t *testing.T) { + t.Parallel()func TestStoreSpiffeHeader_Integration(t *testing.T) { + t.Parallel()Also applies to: 38-38, 59-59, 80-80, 101-101, 127-127
middleware/auth.go (1)
14-17: Don’t store empty Authorization header in context.Avoid polluting context with empty string; only store when non-empty.
- auth := request.Header.Get(headers.Authorization) - ctx := context.AddAuthHeaderToContext(request.Context(), auth) - next.ServeHTTP(responseWriter, request.WithContext(ctx)) + auth := request.Header.Get(headers.Authorization) + ctx := request.Context() + if auth != "" { + ctx = context.AddAuthHeaderToContext(ctx, auth) + } + next.ServeHTTP(responseWriter, request.WithContext(ctx))middleware/token.go (2)
21-24: More robust Authorization parsing.Handle multiple spaces and ensure non-empty token; use case-insensitive compare.
- auth := strings.Split(request.Header.Get(headers.Authorization), " ") - if len(auth) > 1 && strings.ToUpper(auth[0]) == tokenAuthPrefix { - ctx = context.AddWebTokenToContext(ctx, auth[1], SignatureAlgorithms) - } + parts := strings.Fields(request.Header.Get(headers.Authorization)) + if len(parts) == 2 && strings.EqualFold(parts[0], tokenAuthPrefix) && parts[1] != "" { + ctx = context.AddWebTokenToContext(ctx, parts[1], SignatureAlgorithms) + }
14-14: Global mutable config can be racy.SignatureAlgorithms is a package-level var; concurrent mutation at runtime could race. Prefer configuring once at init, a setter guarded by sync/atomic, or make it unexported and pass via options.
middleware/authzMiddlewares.go (2)
9-12: Fix deprecation notice phrasing.-// Deprecated: CreateMiddlewares use CreateAuthMiddleware instead. +// Deprecated: use CreateAuthMiddleware instead.
14-21: Tidy API: underscore unused param and right-size capacity.Param isn’t used; underscore it to silence linters. Preallocate capacity to 3.
-func CreateAuthMiddleware(retriever policy_services.TenantRetriever) []func(http.Handler) http.Handler { - mws := make([]func(http.Handler) http.Handler, 0, 5) +func CreateAuthMiddleware(_ policy_services.TenantRetriever) []func(http.Handler) http.Handler { + mws := make([]func(http.Handler) http.Handler, 0, 3) mws = append(mws, StoreWebToken()) mws = append(mws, StoreAuthHeader()) mws = append(mws, StoreSpiffeHeader()) return mws }middleware/spiffe.go (1)
12-21: Avoid unnecessary WithContext allocation when SPIFFE not present.Fast-path pass-through if no value found.
return http.HandlerFunc(func(responseWriter http.ResponseWriter, request *http.Request) { ctx := request.Context() uriVal := jwt.GetSpiffeUrlValue(request.Header) - if uriVal != nil { - ctx = context.AddSpiffeToContext(ctx, *uriVal) - } - next.ServeHTTP(responseWriter, request.WithContext(ctx)) + if uriVal != nil { + ctx = context.AddSpiffeToContext(ctx, *uriVal) + next.ServeHTTP(responseWriter, request.WithContext(ctx)) + return + } + next.ServeHTTP(responseWriter, request) })middleware/auth_test.go (1)
13-35: De-duplicate with table-driven tests and run in parallelMove these four similar cases to a table-driven test and call t.Parallel() to reduce boilerplate and speed up CI.
Also applies to: 37-56, 58-77, 79-101
middleware/sentry_test.go (1)
13-28: Minor: mark tests parallelEach test is isolated; add t.Parallel() at the top of each to improve runtime.
Also applies to: 30-68, 90-117, 119-141
middleware/token_test.go (1)
43-62: Consolidate to table-driven tests and add t.Parallel()These cases are ideal for a single table-driven test with inputs and expected presence/absence of a token; also call t.Parallel() inside subtests.
Also applies to: 64-83, 85-104, 135-155
middleware/authzMiddlewares_test.go (2)
22-34: Verify middleware order (it matters)CreateAuthMiddleware returns StoreWebToken, StoreAuthHeader, StoreSpiffeHeader in that order. Consider asserting order explicitly by applying them to a handler and checking context effects, since downstream behavior can depend on order.
I can sketch a small integration test that sets headers and verifies context values after passing through the pipeline.
Also applies to: 36-48, 50-60, 62-77, 79-92, 93-104
3-11: Run tests in parallelThese tests don’t share state; add t.Parallel() to speed up CI.
Also applies to: 22-34, 36-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.testcoverage.yml(1 hunks)middleware/auth.go(1 hunks)middleware/auth_test.go(1 hunks)middleware/authzMiddlewares.go(1 hunks)middleware/authzMiddlewares_test.go(1 hunks)middleware/logger.go(1 hunks)middleware/logger_test.go(1 hunks)middleware/middleware.go(1 hunks)middleware/middleware_test.go(1 hunks)middleware/otel.go(1 hunks)middleware/otel_test.go(1 hunks)middleware/requestid.go(1 hunks)middleware/requestid_test.go(1 hunks)middleware/sentry.go(1 hunks)middleware/sentry_test.go(1 hunks)middleware/spiffe.go(1 hunks)middleware/spiffe_test.go(1 hunks)middleware/test_support/local_middleware.go(1 hunks)middleware/token.go(1 hunks)middleware/token_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (18)
middleware/logger_test.go (2)
logger/logger.go (2)
LoadLoggerFromContext(151-160)Logger(59-61)middleware/logger.go (1)
StoreLoggerMiddleware(10-17)
middleware/middleware_test.go (2)
middleware/middleware.go (1)
CreateMiddleware(9-17)logger/logger.go (1)
Logger(59-61)
middleware/auth.go (1)
context/service_context.go (1)
AddAuthHeaderToContext(47-49)
middleware/spiffe_test.go (3)
context/service_context.go (1)
GetSpiffeFromContext(20-27)middleware/spiffe.go (1)
StoreSpiffeHeader(10-22)jwt/spiffe.go (1)
GetSpiffeUrlValue(10-18)
middleware/requestid.go (2)
context/keys/keys.go (1)
RequestIdCtxKey(8-8)logger/logger.go (4)
LoadLoggerFromContext(151-160)NewRequestLoggerFromZerolog(136-144)Logger(59-61)SetLoggerInContext(146-148)
middleware/middleware.go (5)
logger/logger.go (1)
Logger(59-61)middleware/otel.go (1)
SetOtelTracingContext(10-17)middleware/sentry.go (1)
SentryRecoverer(14-31)middleware/logger.go (1)
StoreLoggerMiddleware(10-17)middleware/requestid.go (2)
SetRequestId(14-29)SetRequestIdInLogger(31-41)
middleware/token_test.go (2)
context/service_context.go (1)
GetWebTokenFromContext(69-76)middleware/token.go (1)
StoreWebToken(17-29)
middleware/sentry_test.go (2)
middleware/sentry.go (1)
SentryRecoverer(14-31)logger/logger.go (2)
SetLoggerInContext(146-148)Logger(59-61)
middleware/spiffe.go (2)
jwt/spiffe.go (1)
GetSpiffeUrlValue(10-18)context/service_context.go (1)
AddSpiffeToContext(16-18)
middleware/sentry.go (2)
logger/logger.go (1)
LoadLoggerFromContext(151-160)context/sentry.go (1)
Recover(14-24)
middleware/auth_test.go (2)
context/service_context.go (1)
GetAuthHeaderFromContext(51-58)middleware/auth.go (1)
StoreAuthHeader(11-19)
middleware/logger.go (1)
logger/logger.go (2)
Logger(59-61)SetLoggerInContext(146-148)
middleware/requestid_test.go (2)
middleware/requestid.go (3)
GetRequestId(43-48)SetRequestId(14-29)SetRequestIdInLogger(31-41)context/keys/keys.go (1)
RequestIdCtxKey(8-8)
middleware/otel_test.go (1)
middleware/otel.go (1)
SetOtelTracingContext(10-17)
middleware/token.go (1)
context/service_context.go (1)
AddWebTokenToContext(60-67)
middleware/test_support/local_middleware.go (1)
context/service_context.go (2)
AddWebTokenToContext(60-67)AddTenantToContext(29-31)
middleware/authzMiddlewares_test.go (2)
middleware/authzMiddlewares.go (2)
CreateMiddlewares(10-12)CreateAuthMiddleware(14-22)policy_services/tenantRetriever.go (1)
TenantRetriever(55-57)
middleware/authzMiddlewares.go (4)
policy_services/tenantRetriever.go (1)
TenantRetriever(55-57)middleware/token.go (1)
StoreWebToken(17-29)middleware/auth.go (1)
StoreAuthHeader(11-19)middleware/spiffe.go (1)
StoreSpiffeHeader(10-22)
🪛 Gitleaks (8.27.2)
middleware/token_test.go
[high] 14-14: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 107-107: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ 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/otel.go (1)
10-16: LGTM: correct OTEL context extraction and propagation.Uses the global propagator and forwards a context-wrapped request as expected.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
Docstrings generation was requested by @nexus49. * #64 (comment) The following files were modified: * `middleware/auth.go` * `middleware/authzMiddlewares.go` * `middleware/logger.go` * `middleware/middleware.go` * `middleware/otel.go` * `middleware/requestid.go` * `middleware/sentry.go` * `middleware/spiffe.go` * `middleware/test_support/local_middleware.go` * `middleware/token.go`
|
Note Generated docstrings for this pull request at #65 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
middleware/authzMiddlewares.go (3)
9-9: Add GoDoc for exported function.Exported funcs should have comments starting with the function name.
+// CreateAuthMiddleware returns the default auth middlewares in execution order: +// StoreWebToken -> StoreAuthHeader -> StoreSpiffeHeader. The TenantRetriever +// argument is currently unused but reserved for future tenant-aware logic and may be nil. func CreateAuthMiddleware(_ policy_services.TenantRetriever) []func(http.Handler) http.Handler {
10-16: Simplify construction and remove misleading capacity.Only three middlewares are returned; use a composite literal and drop the capacity 5.
- mws := make([]func(http.Handler) http.Handler, 0, 5) - - mws = append(mws, StoreWebToken()) - mws = append(mws, StoreAuthHeader()) - mws = append(mws, StoreSpiffeHeader()) - - return mws + return []func(http.Handler) http.Handler{ + StoreWebToken(), + StoreAuthHeader(), + StoreSpiffeHeader(), + }
1-1: Align filename with contents.File is named “authz…” (authorization) but assembles authentication middlewares. Consider renaming to
authMiddlewares.gofor clarity.middleware/authzMiddlewares_test.go (2)
49-64: Make this test assert ordering/behavior, not just non-nil.Current assertions duplicate earlier checks. Consider verifying order by applying the chain to a stub handler and asserting context mutations happen in the expected sequence.
66-77: Prefer a compile-time interface assertion over a runtime test.Replace the runtime interface test with a compile-time check.
-// Test that implements policy_services.TenantRetriever interface -func TestTenantRetrieverInterface(t *testing.T) { - mockRetriever := &MockTenantRetriever{} - - // Verify our mock implements the interface - var retriever policy_services.TenantRetriever = mockRetriever - assert.NotNil(t, retriever) - - // Test that we can use it with the middleware functions - middlewares := CreateAuthMiddleware(retriever) - assert.Len(t, middlewares, 3) -} +// Compile-time assertion that MockTenantRetriever implements the interface. +var _ policy_services.TenantRetriever = (*MockTenantRetriever)(nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/authzMiddlewares.go(1 hunks)middleware/authzMiddlewares_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
middleware/authzMiddlewares.go (4)
policy_services/tenantRetriever.go (1)
TenantRetriever(55-57)middleware/token.go (1)
StoreWebToken(17-29)middleware/auth.go (1)
StoreAuthHeader(11-19)middleware/spiffe.go (1)
StoreSpiffeHeader(10-22)
middleware/authzMiddlewares_test.go (2)
middleware/authzMiddlewares.go (1)
CreateAuthMiddleware(9-17)policy_services/tenantRetriever.go (1)
TenantRetriever(55-57)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
middleware/authzMiddlewares.go (1)
12-14: Confirm middleware ordering is intentional.Order affects context population; verify this precedence is required: token parsing before raw auth header, then SPIFFE.
middleware/authzMiddlewares_test.go (1)
23-35: LGTM: basic shape and nil checks are solid.Test asserts count and non-nil functions; good baseline coverage.
github-actions
bot
added
the
documentation
Docstrings generation was requested by @nexus49. * #64 (comment) The following files were modified: * `middleware/auth.go` * `middleware/authzMiddlewares.go` * `middleware/logger.go` * `middleware/middleware.go` * `middleware/otel.go` * `middleware/requestid.go` * `middleware/sentry.go` * `middleware/spiffe.go` * `middleware/test_support/local_middleware.go` * `middleware/token.go` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Bastian Echterhölter <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
middleware/token.go (2)
7-10: Alias the custom context package to avoid confusion with stdlib contextImporting github.com/platform-mesh/golang-commons/context as context is easy to confuse with the stdlib context package. Alias for clarity.
"github.com/go-http-utils/headers" "github.com/go-jose/go-jose/v4" - "github.com/platform-mesh/golang-commons/context" + pmcontext "github.com/platform-mesh/golang-commons/context"And update usage (outside the selected range):
- ctx = context.AddWebTokenToContext(ctx, parts[1], SignatureAlgorithms) + ctx = pmcontext.AddWebTokenToContext(ctx, parts[1], SignatureAlgorithms)
14-15: Avoid exported mutable slice — document or make per-middleware configurableSignatureAlgorithms is declared at middleware/token.go:14 and only referenced (read) at middleware/token.go:29; no other assignments/appends found in the repo. Either document that it must be configured once at init and not mutated at runtime, or provide a per-middleware configuration to avoid shared mutable state.
Minimal doc addition:
-var SignatureAlgorithms = []jose.SignatureAlgorithm{jose.RS256} +// SignatureAlgorithms controls the accepted JOSE signature algorithms used when parsing +// the Authorization Bearer token. Configure once (e.g., at init); do not mutate at runtime. +var SignatureAlgorithms = []jose.SignatureAlgorithm{jose.RS256}Optional: add StoreWebTokenWithAlgs(algs []jose.SignatureAlgorithm) to accept per-middleware algs (fallback to package default when nil/empty).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
middleware/auth.go(1 hunks)middleware/authzMiddlewares.go(1 hunks)middleware/logger.go(1 hunks)middleware/middleware.go(1 hunks)middleware/otel.go(1 hunks)middleware/requestid.go(1 hunks)middleware/sentry.go(1 hunks)middleware/spiffe.go(1 hunks)middleware/test_support/local_middleware.go(1 hunks)middleware/token.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- middleware/authzMiddlewares.go
- middleware/spiffe.go
- middleware/auth.go
- middleware/test_support/local_middleware.go
- middleware/middleware.go
- middleware/sentry.go
- middleware/logger.go
- middleware/requestid.go
- middleware/otel.go
🧰 Additional context used
🧬 Code graph analysis (1)
middleware/token.go (1)
context/service_context.go (1)
AddWebTokenToContext(60-67)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
middleware/authzMiddlewares_test.go (3)
3-9: Remove unnecessary blank imports.
_ "context"and_ "github.com/stretchr/testify/mock"have no side effects here. Drop them to reduce noise.import ( - _ "context" "testing" "github.com/stretchr/testify/assert" - _ "github.com/stretchr/testify/mock" )
23-33: Rename test and fix stale comment referencing a removed parameter.There is no retriever anymore; update the name and comment.
-func TestCreateAuthMiddleware_WithNilRetriever(t *testing.T) { +func TestCreateAuthMiddleware_NoParams(t *testing.T) { middlewares := CreateAuthMiddleware() - // Should still return 3 middlewares even with nil retriever + // Should return 3 middlewares (StoreWebToken, StoreAuthHeader, StoreSpiffeHeader) assert.Len(t, middlewares, 3)
11-21: Run tests in parallel to speed up the suite.These table-free tests are independent; mark them parallel.
func TestCreateAuthMiddleware(t *testing.T) { + t.Parallel() middlewares := CreateAuthMiddleware()func TestCreateAuthMiddleware_NoParams(t *testing.T) { + t.Parallel() middlewares := CreateAuthMiddleware()func TestCreateAuthMiddleware_ReturnsCorrectMiddlewares(t *testing.T) { + t.Parallel() middlewares := CreateAuthMiddleware()Also applies to: 35-48
middleware/authzMiddlewares.go (1)
15-22: Simplify construction and avoid over-allocating capacity.Use a composite literal (or set cap to 3) since only three middlewares are appended.
-func CreateAuthMiddleware() []func(http.Handler) http.Handler { - mws := make([]func(http.Handler) http.Handler, 0, 5) - - mws = append(mws, StoreWebToken()) - mws = append(mws, StoreAuthHeader()) - mws = append(mws, StoreSpiffeHeader()) - - return mws -} +func CreateAuthMiddleware() []func(http.Handler) http.Handler { + return []func(http.Handler) http.Handler{ + StoreWebToken(), + StoreAuthHeader(), + StoreSpiffeHeader(), + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/authzMiddlewares.go(1 hunks)middleware/authzMiddlewares_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
middleware/authzMiddlewares.go (3)
middleware/token.go (1)
StoreWebToken(23-35)middleware/auth.go (1)
StoreAuthHeader(14-22)middleware/spiffe.go (1)
StoreSpiffeHeader(15-27)
middleware/authzMiddlewares_test.go (1)
middleware/authzMiddlewares.go (1)
CreateAuthMiddleware(14-22)
🔇 Additional comments (2)
middleware/authzMiddlewares.go (2)
7-14: Docs read well and the API cleanup is solid.Good move removing the unused retriever; resolves the prior compile warning from older reviews.
15-22: No stale call sites found — CreateAuthMiddleware uses the new no-arg signature.Occurrences: middleware/authzMiddlewares.go (definition) and middleware/authzMiddlewares_test.go (usages at lines 12, 24, 36). No calls pass arguments. TenantRetriever only appears as a type declaration in policy_services/tenantRetriever.go.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
middleware/authzMiddlewares.go (2)
3-5: Prefer a named Middleware type for readability and reuse.
Reduces repetition and aligns with common Go patterns.Apply:
package middleware import ( "net/http" ) +// Middleware represents an HTTP middleware constructor. +type Middleware func(http.Handler) http.Handler + -// CreateAuthMiddleware returns a slice of HTTP middleware constructors that populate +// CreateAuthMiddleware returns a slice of HTTP middleware constructors that populate // authentication-related request context and headers. // // The returned middlewares, applied in order, are: // 1. StoreWebToken() // 2. StoreAuthHeader() // 3. StoreSpiffeHeader() -func CreateAuthMiddleware() []func(http.Handler) http.Handler { - return []func(http.Handler) http.Handler{ +func CreateAuthMiddleware() []Middleware { + return []Middleware{ StoreWebToken(), StoreAuthHeader(), StoreSpiffeHeader(), } }Also applies to: 14-20
1-1: File-naming nit: prefer snake_case.
Consider renaming to authz_middleware.go for idiomatic Go file naming.middleware/authzMiddlewares_test.go (2)
21-31: Remove stale/duplicated test referencing a non-existent retriever.
The function no longer takes a retriever; this test duplicates the first and its name/comment are misleading.Apply:
func TestCreateAuthMiddleware_WithNilRetriever(t *testing.T) { - middlewares := CreateAuthMiddleware() - - // Should still return 3 middlewares even with nil retriever - assert.Len(t, middlewares, 3) - - // Each middleware should be a valid function - for _, mw := range middlewares { - assert.NotNil(t, mw) - } + // Deprecated: kept to preserve history; remove this test. }Or delete the function entirely.
3-7: Add an execution test to ensure the chain composes and calls next.
Verifies middleware constructors return wrappers that forward to the next handler.Apply:
import ( "testing" + "net/http" + "net/http/httptest" "github.com/stretchr/testify/assert" ) @@ func TestCreateAuthMiddleware_ReturnsCorrectMiddlewares(t *testing.T) { middlewares := CreateAuthMiddleware() @@ } + +func TestCreateAuthMiddleware_ComposesAndCallsNext(t *testing.T) { + mws := CreateAuthMiddleware() + called := false + final := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusTeapot) + }) + // Apply in reverse to build the chain correctly. + h := final + for i := len(mws) - 1; i >= 0; i-- { + h = mws[i](h) + } + req := httptest.NewRequest(http.MethodGet, "http://example.com", nil) + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + assert.True(t, called, "final handler should be invoked") + assert.Equal(t, http.StatusTeapot, rr.Code) +}Also applies to: 33-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
middleware/auth_test.go(1 hunks)middleware/authzMiddlewares.go(1 hunks)middleware/authzMiddlewares_test.go(1 hunks)middleware/logger.go(1 hunks)middleware/test_support/local_middleware.go(1 hunks)middleware/token.go(1 hunks)middleware/token_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- middleware/auth_test.go
- middleware/test_support/local_middleware.go
- middleware/logger.go
- middleware/token.go
- middleware/token_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
middleware/authzMiddlewares.go (3)
middleware/token.go (1)
StoreWebToken(24-36)middleware/auth.go (1)
StoreAuthHeader(14-22)middleware/spiffe.go (1)
StoreSpiffeHeader(15-27)
middleware/authzMiddlewares_test.go (1)
middleware/authzMiddlewares.go (1)
CreateAuthMiddleware(14-20)
⏰ 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 (2)
middleware/authzMiddlewares.go (1)
14-20: LGTM: middleware composition and docs are clear.
Order and intent are explicit; function returns the expected three constructors.middleware/authzMiddlewares_test.go (1)
9-19: Basic shape test looks good.
Asserts count and non-nil entries; fine as a smoke test.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jwt/model.go (1)
39-41: Do not include raw JWTs in error messages (PII/secret leakage).Leaking id_token contents is a compliance and security risk.
Apply this diff:
- err = fmt.Errorf("unable to parse id_token: [%s], %w", idToken, parseErr) + err = fmt.Errorf("unable to parse id_token: %w", parseErr)
♻️ Duplicate comments (4)
middleware/sentry_test.go (1)
70-88: Expect a panic for http.ErrAbortHandler and re‑panic in middleware.Tests should assert Panics; middleware should re‑panic. This mirrors net/http semantics.
Apply this diff:
- // The middleware should not recover from http.ErrAbortHandler - // Since the condition is `err != http.ErrAbortHandler`, it should let this panic through - // However, since the defer recover catches it but doesn't handle it, it won't re-panic - // Let's test that it doesn't crash the middleware - assert.NotPanics(t, func() { - handlerToTest.ServeHTTP(recorder, req) - }) + // The middleware should re-panic on http.ErrAbortHandler so net/http can abort the request + assert.Panics(t, func() { + handlerToTest.ServeHTTP(recorder, req) + })middleware/logger_test.go (1)
38-56: Strengthen nil-logger test to assert StdLogger fallback.This repeats prior feedback: verify the middleware falls back to logger.StdLogger when constructed with nil.
func TestStoreLoggerMiddleware_NilLogger(t *testing.T) { - nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Even with nil logger, the middleware should not panic - w.WriteHeader(http.StatusOK) - }) + nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Should fall back to StdLogger + l := logger.LoadLoggerFromContext(r.Context()) + assert.NotNil(t, l) + assert.Same(t, logger.StdLogger, l) + w.WriteHeader(http.StatusOK) + }) middleware := StoreLoggerMiddleware(nil) handlerToTest := middleware(nextHandler) req := httptest.NewRequest("GET", "http://testing", nil) recorder := httptest.NewRecorder() // Should not panic assert.NotPanics(t, func() { handlerToTest.ServeHTTP(recorder, req) }) assert.Equal(t, http.StatusOK, recorder.Code) }middleware/auth_test.go (1)
79-101: Nice fix: assert first Authorization value.This aligns the test with http.Header.Get semantics and the prior review note. Looks good.
middleware/requestid.go (1)
41-47: Nil-deref risk: guard logger from context before dereferencing.LoadLoggerFromContext can return a typed-nil; dereferencing log.Logger will panic.
Apply this diff:
return http.HandlerFunc(func(responseWriter http.ResponseWriter, request *http.Request) { ctx := request.Context() log := logger.LoadLoggerFromContext(ctx) - log = logger.NewRequestLoggerFromZerolog(ctx, log.Logger) + if log == nil || log.Logger == nil { + // Fallback to a safe default logger + log = logger.StdLogger + } + log = logger.NewRequestLoggerFromZerolog(ctx, log.Logger) ctx = logger.SetLoggerInContext(ctx, log) next.ServeHTTP(responseWriter, request.WithContext(ctx)) })
🧹 Nitpick comments (22)
.testcoverage.yml (1)
7-7: Anchor the new exclude path for consistency (and to avoid partial matches).Match existing patterns by anchoring the directory.
Apply this diff:
- - middleware/test_support + - ^middleware/test_supportPlease confirm the coverage tool treats these as regex globs; if it expects path prefixes, we may want a trailing slash instead (
^middleware/test_support/).jwt/model.go (2)
36-36: Fix grammar and strengthen the safety disclaimer.Tighten the comment and avoid “properer”.
Apply this diff:
-// JWT Claims are parsed without verification, ensure properer JWT verification before calling this function, eg. with istio +// Claims are parsed without verification. Ensure JWT verification happens before calling this function +// (e.g., via an Envoy/Istio JWT filter or explicit signature/key validation in-process).
45-46: Avoid pointer-to-pointer in claims deserialization.Pass the allocated struct pointer directly; pointer-to-pointer is unnecessary and confusing.
Apply this diff:
- desErr := token.UnsafeClaimsWithoutVerification(&rawToken) + desErr := token.UnsafeClaimsWithoutVerification(rawToken)middleware/otel.go (1)
18-25: Micro‑nit: capture the propagator once at construction.Avoid per-request global lookup; keeps the hot path a touch slimmer.
Apply this diff:
-func SetOtelTracingContext() func(http.Handler) http.Handler { - return func(next http.Handler) http.Handler { +func SetOtelTracingContext() func(http.Handler) http.Handler { + p := otel.GetTextMapPropagator() + return func(next http.Handler) http.Handler { return http.HandlerFunc(func(responseWriter http.ResponseWriter, request *http.Request) { - ctx := otel.GetTextMapPropagator().Extract(request.Context(), propagation.HeaderCarrier(request.Header)) + ctx := p.Extract(request.Context(), propagation.HeaderCarrier(request.Header)) next.ServeHTTP(responseWriter, request.WithContext(ctx)) }) } }middleware/sentry.go (2)
3-10: Import errors to preserve abort semantics.Apply this diff:
import ( + "errors" "net/http" "runtime/debug" "time"
23-25: Guard against nil logger from context.If no logger is set, logging the panic could itself panic.
Apply this diff:
- log := logger.LoadLoggerFromContext(r.Context()) + log := logger.LoadLoggerFromContext(r.Context()) + if log == nil { + log = logger.StdLogger + }middleware/test_support/local_middleware.go (1)
18-18: Idiomatic Go naming: use tenantID/userID.Follows Go’s initialism style.
Apply this diff:
-func LocalMiddleware(tenantId string, userId string) func(http.Handler) http.Handler { +func LocalMiddleware(tenantID string, userID string) func(http.Handler) http.Handler {And update references:
- claims := &jwt.RegisteredClaims{Issuer: "localhost:8080", Subject: userId, Audience: jwt.ClaimStrings{"testing"}} + claims := &jwt.RegisteredClaims{Issuer: "localhost:8080", Subject: userID, Audience: jwt.ClaimStrings{"testing"}} ... - ctx = context.AddTenantToContext(ctx, tenantId) + ctx = context.AddTenantToContext(ctx, tenantID)middleware/otel_test.go (4)
16-19: Restore the global propagator after the test.Avoid leaking global state into other tests.
Apply this diff:
// Set up a test propagator - propagator := propagation.TraceContext{} - otel.SetTextMapPropagator(propagator) + propagator := propagation.TraceContext{} + prev := otel.GetTextMapPropagator() + otel.SetTextMapPropagator(propagator) + defer otel.SetTextMapPropagator(prev)Repeat similarly in the other tests.
25-29: Strengthen assertions: verify span context, not just non‑nil ctx.Non‑nil context is trivial. Consider asserting a valid extracted SpanContext.
Example:
sc := trace.SpanContextFromContext(r.Context()) assert.True(t, sc.IsValid())
46-50: Same: restore propagator in this test as well.Apply the same prev/defer pattern here.
68-96: Optional: assert extracted span context differs from the original.You already compare contexts; consider also checking trace IDs via SpanContextFromContext for clarity.
middleware/middleware_test.go (2)
16-18: Avoid brittle length assertions for middleware chains.Asserting exact count couples the test to implementation details. Prefer capability checks (e.g., request ID present, logger present, tracing context extracted).
24-41: Operational: ensure SentryRecoverer wraps the entire chain.For maximal coverage, Sentry should be the outermost middleware (wraps all others). Today, if OTel middleware panics, it won’t be recovered.
If CreateMiddleware() currently returns [OTel, Sentry, ...], consider reordering to [Sentry, OTel, ...].
middleware/logger_test.go (1)
21-23: Use identity assertion for logger instance.Assert the same instance with assert.Same rather than structural equality.
- assert.Equal(t, testLog.Logger, logFromContext) + assert.Same(t, testLog.Logger, logFromContext)middleware/logger.go (1)
11-21: LGTM; clarify fallback behavior in comment.Implementation safely defaults nil to logger.StdLogger. Add that to the doc for readers.
-// StoreLoggerMiddleware returns an HTTP middleware that injects the provided -// -// logger into each request's context so downstream handlers can retrieve it. +// StoreLoggerMiddleware returns an HTTP middleware that injects the provided logger +// into each request's context so downstream handlers can retrieve it. +// If the provided log is nil, logger.StdLogger is used. func StoreLoggerMiddleware(log *logger.Logger) func(http.Handler) http.Handler { if log == nil { log = logger.StdLogger }middleware/authzMiddlewares_test.go (1)
9-32: Add a smoke test that each middleware wraps a handler and forwards the call.Catches nil returns or short-circuiting regressions.
func TestCreateAuthMiddleware_ReturnsCorrectMiddlewares(t *testing.T) { middlewares := CreateAuthMiddleware() // Should return exactly 3 middlewares assert.Len(t, middlewares, 3) // Each middleware should be a valid function for _, mw := range middlewares { assert.NotNil(t, mw) // Signature is implicitly tested by compilation and return type } } + +func TestCreateAuthMiddleware_WrapsAndCallsNext(t *testing.T) { + called := 0 + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called++ + w.WriteHeader(http.StatusOK) + }) + for _, mw := range CreateAuthMiddleware() { + rec := httptest.NewRecorder() + req := httptest.NewRequest("GET", "http://testing", nil) + mw(next).ServeHTTP(rec, req) + assert.Equal(t, http.StatusOK, rec.Code) + } + assert.Equal(t, 3, called) +}middleware/spiffe.go (1)
10-27: Only store non-empty SPIFFE values and alias internal context import.Avoid empty-string entries and clarify import intent.
import ( "net/http" - "github.com/platform-mesh/golang-commons/context" + appctx "github.com/platform-mesh/golang-commons/context" "github.com/platform-mesh/golang-commons/jwt" ) // StoreSpiffeHeader returns an HTTP middleware that extracts a SPIFFE URL from the request headers // and, if present, inserts it into the request context for downstream handlers. // // The middleware always calls the next handler; when a SPIFFE URL is found it updates the request's // context with that value so subsequent handlers can retrieve it. func StoreSpiffeHeader() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(responseWriter http.ResponseWriter, request *http.Request) { ctx := request.Context() uriVal := jwt.GetSpiffeUrlValue(request.Header) - if uriVal != nil { - ctx = context.AddSpiffeToContext(ctx, *uriVal) + if uriVal != nil && *uriVal != "" { + ctx = appctx.AddSpiffeToContext(ctx, *uriVal) } next.ServeHTTP(responseWriter, request.WithContext(ctx)) }) } }middleware/token_test.go (1)
85-104: Optional: add a test for extra whitespace (“Bearer ”).We parse with strings.Fields; multiple spaces should be accepted. Consider adding a test to lock this in.
If helpful, here’s a new test you can add:
func TestStoreWebToken_WithBearerTokenAndExtraSpaces(t *testing.T) { nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, err := context.GetWebTokenFromContext(r.Context()) assert.NoError(t, err) w.WriteHeader(http.StatusOK) }) handlerToTest := StoreWebToken()(nextHandler) req := httptest.NewRequest("GET", "http://testing", nil) req.Header.Set(headers.Authorization, "Bearer fake.invalid.token") rec := httptest.NewRecorder() handlerToTest.ServeHTTP(rec, req) assert.Equal(t, http.StatusOK, rec.Code) }middleware/token.go (1)
17-24: Doc nit: reference the correct package alias (pmcontext) in comments.Avoid ambiguity with the stdlib context package.
Apply this diff:
-// case-insensitive). When present, the token is added to the pmcontext via -// context.AddWebTokenToContext using the package's signatureAlgorithms. If the header is absent, +// case-insensitive). When present, the token is added to the pmcontext via +// pmcontext.AddWebTokenToContext using the package's signatureAlgorithms. If the header is absent,middleware/requestid.go (1)
19-33: Echo the request ID to the response for client correlation (optional).Helps traceability across services while remaining backward‑compatible.
Apply this diff:
ctx = context.WithValue(ctx, keys.RequestIdCtxKey, requestId) + // Propagate the request ID back to clients + responseWriter.Header().Set(requestIdHeader, requestId) next.ServeHTTP(responseWriter, request.WithContext(ctx))middleware/requestid_test.go (1)
31-46: Rename test: fix typo in function name.Small clarity win for test discovery.
Apply this diff:
-func TestSetRequestIdWitoutIncomingHeader(t *testing.T) { +func TestSetRequestIdWithoutIncomingHeader(t *testing.T) {middleware/spiffe_test.go (1)
101-125: Make the multiple-header behavior deterministic (assert the chosen value).GetSpiffeUrlValue calls header.Get(HeaderSpiffeValue) which returns the first header value; assert that the first SPIFFE is stored.
func TestStoreSpiffeHeader_WithMultipleSpiffeHeaders(t *testing.T) { nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // With multiple headers, the behavior depends on jwt.GetSpiffeUrlValue implementation - // It should either return the first valid one or handle concatenated headers - spiffeFromContext, err := context.GetSpiffeFromContext(r.Context()) - if err == nil { - // If we get a value, it should be a valid SPIFFE ID - assert.Contains(t, spiffeFromContext, "spiffe://") - } + spiffeFromContext, err := context.GetSpiffeFromContext(r.Context()) + assert.NoError(t, err) + // Expect first valid header value to win (preserves header order). + assert.Equal(t, "spiffe://example.org/workload1", spiffeFromContext) w.WriteHeader(http.StatusOK) }) middleware := StoreSpiffeHeader() handlerToTest := middleware(nextHandler) req := httptest.NewRequest("GET", "http://testing", nil) req.Header.Add("X-Forwarded-Client-Cert", "Subject=\"CN=test1\";URI=spiffe://example.org/workload1") req.Header.Add("X-Forwarded-Client-Cert", "Subject=\"CN=test2\";URI=spiffe://example.org/workload2") recorder := httptest.NewRecorder() handlerToTest.ServeHTTP(recorder, req) assert.Equal(t, http.StatusOK, recorder.Code) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.testcoverage.yml(1 hunks)jwt/model.go(1 hunks)middleware/auth.go(1 hunks)middleware/auth_test.go(1 hunks)middleware/authzMiddlewares.go(1 hunks)middleware/authzMiddlewares_test.go(1 hunks)middleware/logger.go(1 hunks)middleware/logger_test.go(1 hunks)middleware/middleware.go(1 hunks)middleware/middleware_test.go(1 hunks)middleware/otel.go(1 hunks)middleware/otel_test.go(1 hunks)middleware/requestid.go(1 hunks)middleware/requestid_test.go(1 hunks)middleware/sentry.go(1 hunks)middleware/sentry_test.go(1 hunks)middleware/spiffe.go(1 hunks)middleware/spiffe_test.go(1 hunks)middleware/test_support/local_middleware.go(1 hunks)middleware/token.go(1 hunks)middleware/token_test.go(1 hunks)
🔇 Additional comments (7)
jwt/model.go (1)
37-38: Verify support for “none” algorithm in upstream parsing path.Local test middleware stores allowed algorithms as []jose.SignatureAlgorithm{"none"}. Ensure New(...)’s ParseSigned path accepts this value (or that tests bypass New) to avoid parse failures.
If needed, gate “none” behind build tags or map it to the appropriate jose type in test-only flows.
middleware/test_support/local_middleware.go (2)
1-4: Nice: file is build‑tagged for non‑production use.If you still support Go <1.17, add the legacy tag too:
//go:build test || local +// +build test local
29-30: Confirm jose “none” allowance end‑to‑end.Ensure downstream parsing honors []jose.SignatureAlgorithm{"none"} in test/local flows only; prevent accidental acceptance in production paths.
If needed, gate the “none” value behind the same build tags or avoid passing allowed algorithms when using unsigned test tokens.
middleware/authzMiddlewares.go (1)
10-18: Confirm ordering assumptions between middlewares.If StoreWebToken relies on Authorization from context (set by StoreAuthHeader), the current order will break it; otherwise it’s fine. Please confirm and reorder if needed.
middleware/auth.go (1)
10-22: Verify Authorization header isn't logged / redact before storing
- Repo scan: no direct logger calls printing the Authorization header.
- Raw header is stored in context (context/service_context.go: AddAuthHeaderToContext) via middleware/auth.go and later added to outgoing GraphQL requests (policy_services/common.go).
- Potential sinks to double-check: directive/authorized.go logs the OpenFGA request (.Any("req", req)) and createClient assigns the app logger to machinebox/graphql (client.Log = …) — either could expose headers depending on logger / client behavior.
- Action: confirm your logger and machinebox/graphql client do NOT serialize request headers or context values; if they might, redact/mask the token before storing or avoid storing the raw header in context.
middleware/spiffe_test.go (1)
13-36: Good positive-path coverage for SPIFFE propagation.This test validates the core contract (parse header → store SPIFFE in context) cleanly.
middleware/token_test.go (1)
106-133: Lowercase bearer should still work; assert success.Case-insensitive scheme is supported; assert token presence instead of allowing both outcomes.
Apply this diff:
func TestStoreWebToken_WithFakeBearerTokenLowercase(t *testing.T) { token := "fake.invalid.token" authHeader := "bearer " + token // lowercase nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Token parsing may fail due to signature validation, which is expected in tests - _, err := context.GetWebTokenFromContext(r.Context()) - // The middleware should process lowercase bearer tokens - // but token validation may still fail due to signature issues - if err != nil { - // This is expected behavior when token validation fails - assert.Error(t, err) - } + _, err := context.GetWebTokenFromContext(r.Context()) + assert.NoError(t, err) w.WriteHeader(http.StatusOK) })Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
middleware/auth.go (1)
12-17: Good: internal context is aliased; no confusion with stdlibUsing
appctx "github.com/platform-mesh/golang-commons/context"clarifies intent and avoids clashes withcontext. This addresses the earlier review ask.
🧹 Nitpick comments (2)
middleware/auth.go (2)
26-31: Trim whitespace before deciding whether to store AuthorizationAvoid storing whitespace-only values; trim before the emptiness check. Low risk, improves robustness.
Apply this diff in the function body:
- auth := request.Header.Get(headers.Authorization) + auth := strings.TrimSpace(request.Header.Get(headers.Authorization)) ctx := request.Context() if auth != "": ctx = appctx.AddAuthHeaderToContext(ctx, auth)And add the import:
import ( "net/http" "github.com/go-http-utils/headers" appctx "github.com/platform-mesh/golang-commons/context" + "strings" )
23-34: Security hygiene: treat Authorization as sensitiveEnsure downstream logging middlewares redact this context value and the header from logs/traces to prevent credential leakage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/auth.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
middleware/auth.go (1)
context/service_context.go (1)
AddAuthHeaderToContext(47-49)
🪛 GitHub Check: pipe / lint / lint
middleware/auth.go
[failure] 12-12:
imports must appear before other declarations
[failure] 10-10:
expected declaration, found 'package'
🪛 GitHub Actions: ci
middleware/auth.go
[error] 10-10: Go test failed during coverage collection. Syntax error: auth.go:10:1: expected declaration, found 'package' (and 1 more errors). Command: go test ./... -coverprofile=./cover.out -covermode=atomic -coverpkg=./...
⏰ 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). (1)
- GitHub Check: Analyze (go)
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
• Adds middleware for storing authorization headers and tokens in request context • Introduces local middleware for testing with JWT and tenant information
• Updates coverage configuration to exclude middleware test support files
• Simplifies middleware creation by using CreateAuthMiddleware directly • Cleans up test cases by removing references to deprecated function
• Clarifies that JWT claims are parsed without verification • Advises on proper JWT verification practices before usage
Docstrings generation was requested by @nexus49. * #64 (comment) The following files were modified: * `middleware/auth.go` * `middleware/authzMiddlewares.go` * `middleware/logger.go` * `middleware/middleware.go` * `middleware/otel.go` * `middleware/requestid.go` * `middleware/sentry.go` * `middleware/spiffe.go` * `middleware/test_support/local_middleware.go` * `middleware/token.go` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Bastian Echterhölter <[email protected]>
• Remove unused retriever parameter for clarity • Clean up related test cases to reflect changes
• Replace context package with pmcontext to avoid conflict with stdlib
• Streamlines the creation of middleware by returning a slice directly
• Replaces strings.Split with strings.Fields for clarity • Enhances readability by using strings.EqualFold for case-insensitive comparison
• Defaults to standard logger if no logger is provided
…cret scanner findings
- Update multiple auth headers test to verify specific expected value - Add build constraints to limit test support middleware to test/local builds - Clarify comment about http.Header.Get behavior with multiple headers
• Introduces Middleware type for better clarity • Updates test names for improved readability
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
• Remove unnecessary imports and improve code organization • Enhance test clarity by refining error handling in token tests
nexus49
force-pushed
the
feat/middlewares
branch
from
7fabe0a to
d01fc84
Compare
Summary by CodeRabbit
New Features
Tests
Chores
Documentation