From 19a28fef1773b816649a2c6d8f3e69382a13e9cb Mon Sep 17 00:00:00 2001 From: Jay Brown Date: Thu, 19 Mar 2026 15:22:07 +0000 Subject: [PATCH] Merged in bugfix/auth-permit (pull request #217) permit.io fix * permit.io fix --- internal/cognitoauth/middleware.go | 57 +++--- internal/cognitoauth/middleware_test.go | 235 ++++++++++++++++++++++++ internal/cognitoauth/permitio.go | 5 + 3 files changed, 270 insertions(+), 27 deletions(-) diff --git a/internal/cognitoauth/middleware.go b/internal/cognitoauth/middleware.go index e8355cf0..172c228a 100644 --- a/internal/cognitoauth/middleware.go +++ b/internal/cognitoauth/middleware.go @@ -79,8 +79,9 @@ func isAuthOnlyPath(requestPath string) bool { return false } -// performPermitIOAuthorization handles the Permit.io authorization check -func performPermitIOAuthorization(c echo.Context, requestPath string, config auth.ConfigProvider) error { +// performPermitIOAuthorization handles the Permit.io authorization check. +// Returns *HTTPError on denial/failure so the caller can write a single JSON response. +func performPermitIOAuthorization(c echo.Context, requestPath string, config auth.ConfigProvider, checker PermitChecker) error { // Skip authorization for swagger and metrics if strings.HasPrefix(requestPath, "/swagger") || strings.HasPrefix(requestPath, "/metrics") { return nil @@ -95,32 +96,27 @@ func performPermitIOAuthorization(c echo.Context, requestPath string, config aut userSubject, ok := GetUserSubject(c) if !ok { config.GetAuthLogger().Warn("No user subject found in request") - return c.JSON(http.StatusUnauthorized, map[string]string{ - "error": "Authentication required", - }) + return &HTTPError{ + StatusCode: http.StatusUnauthorized, + Response: map[string]string{"error": "Authentication required"}, + } } - // Initialize Permit.io client - permitClient := NewPermitIOClient( - config.GetPermitIOAPIKey(), - config.GetPermitIOPDPURL(), - config.GetAuthLogger(), - ) - // Authorize with Permit.io resource := GetResourceFromRoute(requestPath) action := GetActionFromMethod(c.Request().Method) - permitted, err := permitClient.CheckPermission(userSubject, action, resource) + permitted, err := checker.CheckPermission(userSubject, action, resource) if err != nil { config.GetAuthLogger().Error("Permit.io authorization failed", "error", err, "user", userSubject, "resource", resource, "action", action) - return c.JSON(http.StatusInternalServerError, map[string]string{ - "error": "Authorization service unavailable", - }) + return &HTTPError{ + StatusCode: http.StatusInternalServerError, + Response: map[string]string{"error": "Authorization service unavailable"}, + } } if !permitted { @@ -129,17 +125,16 @@ func performPermitIOAuthorization(c echo.Context, requestPath string, config aut "resource", resource, "action", action, "path", requestPath) - - if err := c.JSON(http.StatusForbidden, map[string]interface{}{ - "error": "Insufficient permissions", - "message": "Access denied by authorization service", - "user": userSubject, - "resource": resource, - "action": action, - }); err != nil { - config.GetAuthLogger().Error("Failed to send JSON response", "error", err) + return &HTTPError{ + StatusCode: http.StatusForbidden, + Response: map[string]interface{}{ + "error": "Insufficient permissions", + "message": "Access denied by authorization service", + "user": userSubject, + "resource": resource, + "action": action, + }, } - return echo.NewHTTPError(http.StatusForbidden, "access denied") } config.GetAuthLogger().Debug("Access granted by Permit.io", @@ -407,7 +402,15 @@ func TokenValidationMiddleware(config auth.ConfigProvider, cognitoClient *cognit } // Perform Permit.io authorization - if err := performPermitIOAuthorization(c, requestPath, config); err != nil { + permitClient := NewPermitIOClient( + config.GetPermitIOAPIKey(), + config.GetPermitIOPDPURL(), + config.GetAuthLogger(), + ) + if err := performPermitIOAuthorization(c, requestPath, config, permitClient); err != nil { + if httpErr, ok := err.(*HTTPError); ok { + return c.JSON(httpErr.StatusCode, httpErr.Response) + } return err } diff --git a/internal/cognitoauth/middleware_test.go b/internal/cognitoauth/middleware_test.go index 2889644f..9f60abd0 100644 --- a/internal/cognitoauth/middleware_test.go +++ b/internal/cognitoauth/middleware_test.go @@ -2,14 +2,18 @@ package cognitoauth import ( "encoding/json" + "errors" "log/slog" "net/http" "net/http/httptest" "os" + "strings" "testing" "time" "github.com/labstack/echo/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestJWTAuthMiddleware tests the JWTAuthMiddleware function to ensure it correctly processes @@ -411,3 +415,234 @@ func TestIsAuthOnlyPath(t *testing.T) { }) } } + +// --- Test infrastructure for performPermitIOAuthorization --- + +// stubPermitChecker controls permit behavior in tests without a live PDP. +type stubPermitChecker struct { + allowed bool + err error + called bool +} + +func (s *stubPermitChecker) CheckPermission(userID, action, resourceType string) (bool, error) { + s.called = true + return s.allowed, s.err +} + +// newAuthContext creates an Echo context with optional JWT claims pre-set. +func newAuthContext(e *echo.Echo, method, path, userSubject string) (echo.Context, *httptest.ResponseRecorder) { + req := httptest.NewRequest(method, path, nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + if userSubject != "" { + c.Set("user_claims", map[string]interface{}{"sub": userSubject}) + } + return c, rec +} + +// assertSingleJSONResponse verifies the response body contains exactly one JSON object. +// This is the critical regression guard for the double-write bug. +func assertSingleJSONResponse(t *testing.T, rec *httptest.ResponseRecorder) { + t.Helper() + body := strings.TrimSpace(rec.Body.String()) + require.NotEmpty(t, body, "response body should not be empty") + + decoder := json.NewDecoder(strings.NewReader(body)) + var first json.RawMessage + require.NoError(t, decoder.Decode(&first), "response body should contain valid JSON") + require.False(t, decoder.More(), "response body contains multiple JSON objects (double-write bug)") +} + +// TestPerformPermitIOAuthorization tests the authorization function directly, +// verifying each error path returns a single *HTTPError (not c.JSON) so +// the caller writes exactly one response. +func TestPerformPermitIOAuthorization(t *testing.T) { + e := echo.New() + cfg := &mockConfigProvider{ + region: "us-east-2", + userPoolID: "us-east-2_testpool", + } + + t.Run("NoUserSubject_Returns401", func(t *testing.T) { + // No user_claims in context -- empty userSubject + stub := &stubPermitChecker{allowed: false} + c, _ := newAuthContext(e, http.MethodGet, "/document", "") + + err := performPermitIOAuthorization(c, "/document", cfg, stub) + + require.Error(t, err) + httpErr, ok := err.(*HTTPError) + require.True(t, ok, "expected *HTTPError, got %T", err) + assert.Equal(t, http.StatusUnauthorized, httpErr.StatusCode) + + resp, ok := httpErr.Response.(map[string]string) + require.True(t, ok) + assert.Equal(t, "Authentication required", resp["error"]) + assert.False(t, stub.called, "permit checker should not be called without a user subject") + }) + + t.Run("PermitServiceError_Returns500", func(t *testing.T) { + stub := &stubPermitChecker{allowed: false, err: errors.New("connection refused")} + c, _ := newAuthContext(e, http.MethodGet, "/document", "user-123") + + err := performPermitIOAuthorization(c, "/document", cfg, stub) + + require.Error(t, err) + httpErr, ok := err.(*HTTPError) + require.True(t, ok, "expected *HTTPError, got %T", err) + assert.Equal(t, http.StatusInternalServerError, httpErr.StatusCode) + + resp, ok := httpErr.Response.(map[string]string) + require.True(t, ok) + assert.Equal(t, "Authorization service unavailable", resp["error"]) + assert.True(t, stub.called) + }) + + t.Run("PermissionDenied_Returns403", func(t *testing.T) { + stub := &stubPermitChecker{allowed: false, err: nil} + c, _ := newAuthContext(e, http.MethodPost, "/document", "user-123") + + err := performPermitIOAuthorization(c, "/document", cfg, stub) + + require.Error(t, err) + httpErr, ok := err.(*HTTPError) + require.True(t, ok, "expected *HTTPError, got %T", err) + assert.Equal(t, http.StatusForbidden, httpErr.StatusCode) + + resp, ok := httpErr.Response.(map[string]interface{}) + require.True(t, ok) + assert.Equal(t, "Insufficient permissions", resp["error"]) + assert.Equal(t, "user-123", resp["user"]) + assert.Equal(t, "document", resp["resource"]) + assert.Equal(t, "post", resp["action"]) + assert.True(t, stub.called) + }) + + t.Run("PermissionGranted_ReturnsNil", func(t *testing.T) { + stub := &stubPermitChecker{allowed: true, err: nil} + c, _ := newAuthContext(e, http.MethodGet, "/document", "user-123") + + err := performPermitIOAuthorization(c, "/document", cfg, stub) + + assert.NoError(t, err) + assert.True(t, stub.called) + }) + + t.Run("AuthOnlyPath_SkipsPermitCheck", func(t *testing.T) { + stub := &stubPermitChecker{allowed: false, err: errors.New("should not be called")} + c, _ := newAuthContext(e, http.MethodGet, "/identity", "user-123") + + err := performPermitIOAuthorization(c, "/identity", cfg, stub) + + assert.NoError(t, err) + assert.False(t, stub.called, "permit checker should not be called for auth-only paths") + }) + + t.Run("SwaggerPath_SkipsPermitCheck", func(t *testing.T) { + stub := &stubPermitChecker{allowed: false, err: errors.New("should not be called")} + c, _ := newAuthContext(e, http.MethodGet, "/swagger/index.html", "user-123") + + err := performPermitIOAuthorization(c, "/swagger/index.html", cfg, stub) + + assert.NoError(t, err) + assert.False(t, stub.called, "permit checker should not be called for swagger paths") + }) +} + +// TestPermitIOAuthorization_SingleResponseIntegration tests the full middleware call site +// to confirm that *HTTPError from performPermitIOAuthorization flows through the caller +// and produces exactly one JSON response (not the pre-fix double-write). +func TestPermitIOAuthorization_SingleResponseIntegration(t *testing.T) { + e := echo.New() + cfg := &mockConfigProvider{ + region: "us-east-2", + userPoolID: "us-east-2_testpool", + noJWTValidation: true, + } + + // handlerCalled tracks whether the downstream handler runs + handlerCalled := false + testHandler := func(c echo.Context) error { + handlerCalled = true + return c.JSON(http.StatusOK, map[string]string{"status": "ok"}) + } + + // simulateMiddlewareCallSite reproduces the caller logic from TokenValidationMiddleware + // (lines 409-418) so we can test *HTTPError unwrapping without needing full JWT flow. + simulateMiddlewareCallSite := func(c echo.Context, requestPath string, checker PermitChecker) error { + if err := performPermitIOAuthorization(c, requestPath, cfg, checker); err != nil { + if httpErr, ok := err.(*HTTPError); ok { + return c.JSON(httpErr.StatusCode, httpErr.Response) + } + return err + } + return testHandler(c) + } + + t.Run("PermitDenied_SingleJSONResponse", func(t *testing.T) { + handlerCalled = false + stub := &stubPermitChecker{allowed: false, err: nil} + c, rec := newAuthContext(e, http.MethodGet, "/document", "user-123") + + err := simulateMiddlewareCallSite(c, "/document", stub) + + assert.NoError(t, err) + assert.Equal(t, http.StatusForbidden, rec.Code) + assert.False(t, handlerCalled, "handler should not run when authorization is denied") + assertSingleJSONResponse(t, rec) + }) + + t.Run("PermitServiceError_SingleJSONResponse", func(t *testing.T) { + handlerCalled = false + stub := &stubPermitChecker{allowed: false, err: errors.New("connection refused")} + c, rec := newAuthContext(e, http.MethodGet, "/document", "user-123") + + err := simulateMiddlewareCallSite(c, "/document", stub) + + assert.NoError(t, err) + assert.Equal(t, http.StatusInternalServerError, rec.Code) + assert.False(t, handlerCalled, "handler should not run when permit service fails") + assertSingleJSONResponse(t, rec) + }) + + t.Run("NoUserSubject_SingleJSONResponse", func(t *testing.T) { + handlerCalled = false + stub := &stubPermitChecker{allowed: false} + c, rec := newAuthContext(e, http.MethodGet, "/document", "") // no subject + + err := simulateMiddlewareCallSite(c, "/document", stub) + + assert.NoError(t, err) + assert.Equal(t, http.StatusUnauthorized, rec.Code) + assert.False(t, handlerCalled, "handler should not run without user subject") + assertSingleJSONResponse(t, rec) + }) + + t.Run("PermitGranted_SingleJSONResponse", func(t *testing.T) { + handlerCalled = false + stub := &stubPermitChecker{allowed: true, err: nil} + c, rec := newAuthContext(e, http.MethodGet, "/document", "user-123") + + err := simulateMiddlewareCallSite(c, "/document", stub) + + assert.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + assert.True(t, handlerCalled, "handler should run when authorization is granted") + assertSingleJSONResponse(t, rec) + }) + + t.Run("AuthOnlyPath_SingleJSONResponse", func(t *testing.T) { + handlerCalled = false + stub := &stubPermitChecker{allowed: false, err: errors.New("should not be called")} + c, rec := newAuthContext(e, http.MethodGet, "/identity", "user-123") + + err := simulateMiddlewareCallSite(c, "/identity", stub) + + assert.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + assert.True(t, handlerCalled, "handler should run for auth-only paths") + assert.False(t, stub.called, "permit checker should not be called for auth-only paths") + assertSingleJSONResponse(t, rec) + }) +} diff --git a/internal/cognitoauth/permitio.go b/internal/cognitoauth/permitio.go index 2b3c29c7..b81b9f8a 100644 --- a/internal/cognitoauth/permitio.go +++ b/internal/cognitoauth/permitio.go @@ -15,6 +15,11 @@ type clientKey struct { pdpURL string } +// PermitChecker abstracts the permit.io authorization check for testability. +type PermitChecker interface { + CheckPermission(userID, action, resourceType string) (bool, error) +} + // PermitIOClient wraps the Permit.io client with logging type PermitIOClient struct { client *permit.Client