Conversation
- 17 test functions with 40+ subtests covering both middleware files - gRPC: stripBearer, policyForMethod, grpcErrorFromHTTP, extractTokenFromMD, SubFromMetadata, NewGRPCAuthUnaryPolicy integration - HTTP: checkAuthorization subject construction for normal-user and application tokens, mock server integration, error handling - Documents known bug: application tokens hardcode "admin/" prefix instead of using owner claim from JWT - Add testify v1.11.1 dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add extractTenantClaims to parse tenantId/tenantSlug/owner from JWT - Propagate tenant metadata (md-tenant-id, md-tenant-slug, md-tenant-owner) in unary interceptor when MULTI_TENANT_ENABLED=true - Add NewGRPCAuthStreamPolicy streaming interceptor mirroring unary behavior - Add wrappedServerStream to override context for streaming calls - Add tests for extraction, propagation, and streaming interceptor Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace NewTracerFromContext + NewHeaderIDFromContext with the unified NewTrackingFromContext across HTTP and gRPC middleware. X-Lerian-Ref: 0x1
WalkthroughAdds multi-tenant support to gRPC auth middleware by extracting tenant claims from JWTs and injecting them into gRPC metadata; adds a streaming auth interceptor Sequence DiagramsequenceDiagram
participant Client as gRPC Client
participant Middleware as gRPC Auth Middleware
participant AuthClient as Auth Service (HTTP)
participant JWT as JWT Parser
participant Metadata as gRPC Metadata
participant Downstream as Downstream Service
Client->>Middleware: gRPC request (md-auth token)
Middleware->>Metadata: extract token
Middleware->>JWT: parse token (ParseUnverified)
JWT-->>Middleware: claims
alt MULTI_TENANT_ENABLED
Middleware->>JWT: extract tenantId/tenantSlug/owner
JWT-->>Middleware: tenant claims
Middleware->>Metadata: inject md-tenant-id/md-tenant-slug/md-tenant-owner
end
Middleware->>AuthClient: checkAuthorization(subject, policy) (HTTP)
AuthClient-->>Middleware: AuthResponse (authorized / denied / error)
alt authorized
Middleware->>Downstream: forward request with metadata (propagated context/request-id)
Downstream-->>Client: response
else denied or error
Middleware-->>Client: gRPC error (unauthenticated/permission denied/internal)
end
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@auth/middleware/middleware_test.go`:
- Line 10: The test imports the wrong major version of commons log causing type
mismatch with AuthClient.Logger; change the import from
"github.com/LerianStudio/lib-commons/v2/commons/log" to the v3 module (matching
production), and update any testLogger methods (the ones returning log.Logger)
to use the v3 log.Logger type so their return types match AuthClient.Logger.
In `@auth/middleware/middlewareGRPC_test.go`:
- Around line 60-66: The test and middleware currently treat an "Authorization:
Bearer " header as the literal "Bearer" token, which hides a parsing bug; update
the token-parsing logic (stripBearer) so that when the header is exactly the
bearer prefix with no token (e.g., "Bearer " or "bearer ") it returns an empty
string, and update the GRPC auth middleware handler (the code path that inspects
the token and maps to gRPC status) to treat an empty token as unauthenticated
and return codes.Unauthenticated rather than proceeding; also change the failing
tests in middlewareGRPC_test.go (including the case named
"bearer_prefix_with_no_token_returns_bearer_literal" and similar cases around
lines referenced) to expect an empty credential and an Unauthenticated outcome.
In `@auth/middleware/middlewareGRPC.go`:
- Around line 102-121: Extract the duplicated MULTI_TENANT_ENABLED + metadata
mutation block into a single helper function (e.g., propagateTenantClaims(ctx
context.Context, token *jwt.Token) context.Context) in middlewareGRPC.go; move
the os.Getenv("MULTI_TENANT_ENABLED") check, tenant extraction
(extractTenantClaims), metadata.Copy/Set calls for "md-tenant-id",
"md-tenant-slug", "md-tenant-owner", and metadata.NewIncomingContext into that
helper, and then replace the duplicated blocks in both the unary interceptor and
streaming interceptor with a call to propagateTenantClaims(ctx, token) (or
similar) to return the updated context. Ensure the helper handles empty claim
values and returns the original context when multi-tenant is disabled or no
tenant claims are present.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
auth/middleware/middleware.goauth/middleware/middlewareGRPC.goauth/middleware/middlewareGRPC_test.goauth/middleware/middleware_test.gogo.mod
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@auth/middleware/middleware_test.go`:
- Around line 436-449: Update the TestAuthResponse_JSONRoundTrip to include the
Timestamp field: import "time", set original.Timestamp (e.g., time.Now().UTC()),
marshal/unmarshal as before, and assert that decoded.Timestamp equals
original.Timestamp using time.Time's Equal method (or compare UnixNano) to
ensure the full struct round-trip is covered; reference the
TestAuthResponse_JSONRoundTrip function and the AuthResponse.Timestamp field
when making the change.
In `@auth/middleware/middleware.go`:
- Around line 59-64: The wrapped error is created but never emitted; update the
error branch after calling zap.InitializeLoggerWithError() so the failure is
logged before falling back to &log.NoneLogger{} (e.g., wrap the error with
fmt.Errorf("failed to initialize logger: %w", err) and write it to stderr with
fmt.Fprintln(os.Stderr, wrappedErr) or similar), and add the "os" import; ensure
you still assign l = &log.NoneLogger{} afterwards so InitializeLoggerWithError,
l, and log.NoneLogger are used correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
auth/middleware/middleware.goauth/middleware/middleware_test.gogo.mod
There was a problem hiding this comment.
♻️ Duplicate comments (1)
auth/middleware/middleware.go (1)
60-65:⚠️ Potential issue | 🟡 MinorLogging to
NoneLoggeris ineffective—error context is still lost.The error is being logged via
l.Errorf(), butlis assigned&log.NoneLogger{}before that call. By definition,NoneLoggerdiscards all log output, so the failure message is never emitted. This doesn't resolve the original concern about losing error context.Log to
os.Stderrbefore assigning the fallback logger, or use a simplefmt.Fprintfsince no working logger is available at that point.🔧 Proposed fix
} else { l, err = zap.InitializeLoggerWithError() if err != nil { + fmt.Fprintf(os.Stderr, "failed to initialize logger, falling back to NoneLogger: %v\n", err) l = &log.NoneLogger{} - - l.Errorf("failed to initialize logger, using NoneLogger: %v\n", err) } }Add
"os"to the imports:import ( "bytes" "context" "encoding/json" "errors" "fmt" "io" "net/http" + "os" "time"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/middleware/middleware.go` around lines 60 - 65, The current error handling assigns l = &log.NoneLogger{} before calling l.Errorf, so the initialization error is never emitted; change the sequence so you write the error directly to stderr (e.g., use fmt.Fprintf(os.Stderr, ...) or log.Print to os.Stderr) immediately after zap.InitializeLoggerWithError() returns an error, then assign the fallback l = &log.NoneLogger{} afterward; update the imports to include "os" (and "fmt" if using fmt.Fprintf) and replace the l.Errorf(...) call with a direct stderr write referencing the error from InitializeLoggerWithError() so the failure context is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@auth/middleware/middleware.go`:
- Around line 60-65: The current error handling assigns l = &log.NoneLogger{}
before calling l.Errorf, so the initialization error is never emitted; change
the sequence so you write the error directly to stderr (e.g., use
fmt.Fprintf(os.Stderr, ...) or log.Print to os.Stderr) immediately after
zap.InitializeLoggerWithError() returns an error, then assign the fallback l =
&log.NoneLogger{} afterward; update the imports to include "os" (and "fmt" if
using fmt.Fprintf) and replace the l.Errorf(...) call with a direct stderr write
referencing the error from InitializeLoggerWithError() so the failure context is
preserved.
|
🎉 This PR is included in version 2.5.0-beta.6 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Pull Request Checklist
Pull Request Type
Checklist
Please check each item after it's completed.
Additional Notes
Obs: Please, always remember to target your PR to develop branch instead of main.