feat: implement authentication token source configuration and middlew…#105
feat: implement authentication token source configuration and middlew…#105Suhaibinator merged 6 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
==========================================
+ Coverage 97.39% 97.48% +0.09%
==========================================
Files 18 18
Lines 1802 1867 +65
==========================================
+ Hits 1755 1820 +65
Misses 35 35
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for configurable authentication token sources, allowing tokens to be extracted from either HTTP headers or cookies. The implementation introduces a new AuthTokenConfig type that can be overridden at route, sub-router, and global levels, following the same precedence pattern as other route configuration options.
Key Changes
- Introduced
AuthTokenSourceenum andAuthTokenConfigstructure to support both header-based and cookie-based token extraction - Extended authentication middleware to accept and use configurable token sources
- Updated the configuration hierarchy to support auth token overrides at route and sub-router levels
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/common/config.go | Added AuthTokenSource enum, AuthTokenConfig struct, and integrated it into RouteOverrides with a helper method |
| pkg/router/router.go | Updated authentication logic to support multiple token sources, added helper functions for config resolution, and refactored middleware to accept auth token configuration |
| pkg/router/route.go | Updated route registration functions to extract and pass auth token config to handler wrappers |
| pkg/router/auth_middleware_test.go | Added test for cookie-based authentication with authRequiredMiddleware |
Comments suppressed due to low confidence (2)
pkg/router/router.go:1257
- The code unconditionally strips the "Bearer " prefix from all header-based tokens (line 1257), regardless of the custom header name. This assumes all header-based authentication uses the Bearer scheme, which may not be true for custom headers. Consider making the prefix stripping optional or configurable, or only apply it when using the default "Authorization" header.
StatusCode: statusCode,
pkg/router/router.go:1266
- The cookie value is used directly without checking if it's empty. An empty cookie value would be passed to the auth function, which might not be the intended behavior. Consider adding a check to ensure the cookie value is not empty before proceeding with authentication.
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If empty, defaults to "Authorization". | ||
| HeaderName string | ||
|
|
||
| // CookieName is used when Source is AuthTokenSourceCookie. |
There was a problem hiding this comment.
The documentation for CookieName should clarify that it's required when Source is AuthTokenSourceCookie. Consider updating the comment to: "CookieName is used when Source is AuthTokenSourceCookie. Required when using cookie-based authentication."
| // CookieName is used when Source is AuthTokenSourceCookie. | |
| // CookieName is used when Source is AuthTokenSourceCookie. | |
| // Required when using cookie-based authentication. |
| func TestAuthRequiredMiddlewareWithCookieSource(t *testing.T) { | ||
| logger := zap.NewNop() | ||
| r := NewRouter(RouterConfig{Logger: logger}, mocks.MockAuthFunction, mocks.MockUserIDFromUser) | ||
|
|
||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| userID, ok := scontext.GetUserIDFromRequest[string, string](r) | ||
| if !ok { | ||
| t.Error("Expected user ID to be in context") | ||
| } | ||
| if userID != "user123" { | ||
| t.Errorf("Expected user ID %q, got %q", "user123", userID) | ||
| } | ||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write([]byte("OK")) | ||
| }) | ||
|
|
||
| authTokenConfig := common.AuthTokenConfig{ | ||
| Source: common.AuthTokenSourceCookie, | ||
| CookieName: "auth_token", | ||
| } | ||
| wrappedHandler := r.authRequiredMiddlewareWithConfig(authTokenConfig)(handler) | ||
|
|
||
| // Test with valid auth cookie | ||
| req, _ := http.NewRequest("GET", "/test", nil) | ||
| req.AddCookie(&http.Cookie{Name: "auth_token", Value: "valid-token"}) | ||
| rr := httptest.NewRecorder() | ||
| wrappedHandler.ServeHTTP(rr, req) | ||
| if rr.Code != http.StatusOK { | ||
| t.Errorf("Expected status code %d, got %d", http.StatusOK, rr.Code) | ||
| } | ||
|
|
||
| // Test with valid Authorization header but no cookie (should not fallback) | ||
| req, _ = http.NewRequest("GET", "/test", nil) | ||
| req.Header.Set("Authorization", "Bearer valid-token") | ||
| rr = httptest.NewRecorder() | ||
| wrappedHandler.ServeHTTP(rr, req) | ||
| if rr.Code != http.StatusUnauthorized { | ||
| t.Errorf("Expected status code %d, got %d", http.StatusUnauthorized, rr.Code) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test coverage for cookie-based authentication is only provided for authRequiredMiddleware but not for authOptionalMiddleware. Consider adding a similar test for authOptionalMiddleware with cookie source to ensure both authentication modes work correctly with cookies.
1 participant
…are support