Merged in bugfix/auth-permit (pull request #217)

permit.io fix * permit.io fix
2026-03-19 15:22:07 +00:00
parent 2eff52877b
commit 19a28fef17
3 changed files with 270 additions and 27 deletions
@@ -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
 			}

@@ -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)
+	})
+}
@@ -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