From 2c42986a8bdafc684a271fc5be76a81829de73b9 Mon Sep 17 00:00:00 2001 From: kyy Date: Mon, 23 Feb 2026 13:14:33 +0900 Subject: [PATCH] =?UTF-8?q?Private/PKCE=20=EC=95=B1=20=EC=9C=A0=ED=98=95?= =?UTF-8?q?=20=EB=B0=8F=20=EA=B4=80=EB=A6=AC=EC=9E=90=20=EA=B6=8C=ED=95=9C?= =?UTF-8?q?=20=EC=A0=95=EC=B1=85=20=EC=A0=81=EC=9A=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/cmd/server/main.go | 2 +- backend/internal/handler/dev_handler.go | 131 +++++++++++++++++-- backend/internal/handler/dev_handler_test.go | 70 +++++++--- devfront/tests/clients.spec.ts | 2 +- 4 files changed, 169 insertions(+), 36 deletions(-) diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index cf0ec475..4c3c9518 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -268,7 +268,7 @@ func main() { auditHandler := handler.NewAuditHandler(auditRepo) authHandler := handler.NewAuthHandler(redisService, idpProvider, auditRepo, oathkeeperRepo, tenantService, ketoService, userRepo, consentRepo) adminHandler := handler.NewAdminHandler(ketoService) - devHandler := handler.NewDevHandler(redisService, secretRepo, consentRepo, relyingPartyService) + devHandler := handler.NewDevHandler(redisService, secretRepo, consentRepo, relyingPartyService, ketoService) tenantHandler := handler.NewTenantHandler(db, tenantService, ketoService, kratosAdminService) userGroupHandler := handler.NewUserGroupHandler(userGroupService) relyingPartyHandler := handler.NewRelyingPartyHandler(relyingPartyService, kratosAdminService) diff --git a/backend/internal/handler/dev_handler.go b/backend/internal/handler/dev_handler.go index 48db2ac5..705c3954 100644 --- a/backend/internal/handler/dev_handler.go +++ b/backend/internal/handler/dev_handler.go @@ -9,6 +9,7 @@ import ( "encoding/base64" "errors" "fmt" + "log/slog" "strings" "time" @@ -22,15 +23,19 @@ type DevHandler struct { SecretRepo domain.ClientSecretRepository KratosAdmin *service.KratosAdminService ConsentRepo repository.ClientConsentRepository + Keto service.KetoService + RPSvc service.RelyingPartyService } -func NewDevHandler(redis domain.RedisRepository, secretRepo domain.ClientSecretRepository, consentRepo repository.ClientConsentRepository, rpSvc service.RelyingPartyService) *DevHandler { +func NewDevHandler(redis domain.RedisRepository, secretRepo domain.ClientSecretRepository, consentRepo repository.ClientConsentRepository, rpSvc service.RelyingPartyService, keto service.KetoService) *DevHandler { return &DevHandler{ Hydra: service.NewHydraAdminService(), Redis: redis, SecretRepo: secretRepo, KratosAdmin: service.NewKratosAdminService(), ConsentRepo: consentRepo, + Keto: keto, + RPSvc: rpSvc, } } @@ -94,6 +99,26 @@ type clientUpsertRequest struct { Metadata *map[string]interface{} `json:"metadata"` } +func (h *DevHandler) checkAppManagerPermission(c *fiber.Ctx) (bool, error) { + profile, ok := c.Locals("user_profile").(*domain.UserProfileResponse) + if !ok || profile == nil { + return false, nil + } + + // Super Admin bypass + if profile.Role == domain.RoleSuperAdmin { + return true, nil + } + + // Check with Keto: System:AppManager#member + allowed, err := h.Keto.CheckPermission(c.Context(), profile.ID, "System", "AppManager", "member") + if err != nil { + return false, err + } + + return allowed, nil +} + func (h *DevHandler) ListClients(c *fiber.Ctx) error { limit := c.QueryInt("limit", 50) offset := c.QueryInt("offset", 0) @@ -104,6 +129,11 @@ func (h *DevHandler) ListClients(c *fiber.Ctx) error { offset = 0 } + isAppManager, err := h.checkAppManagerPermission(c) + if err != nil { + slog.Error("Failed to check app manager permission", "error", err) + } + clients, err := h.Hydra.ListClients(c.Context(), limit, offset) if err != nil { if errors.Is(err, service.ErrHydraNotFound) { @@ -120,7 +150,12 @@ func (h *DevHandler) ListClients(c *fiber.Ctx) error { items := make([]clientSummary, 0, len(clients)) for _, client := range clients { - items = append(items, h.mapClientSummary(client)) + summary := h.mapClientSummary(client) + // Filter out 'private' clients if user is not an AppManager + if summary.Type == "private" && !isAppManager { + continue + } + items = append(items, summary) } return c.JSON(clientListResponse{ @@ -145,6 +180,18 @@ func (h *DevHandler) GetClient(c *fiber.Ctx) error { } summary := h.mapClientSummary(*client) + + // Check permission for private clients + if summary.Type == "private" { + isAppManager, err := h.checkAppManagerPermission(c) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "permission check error"}) + } + if !isAppManager { + return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "forbidden: insufficient permissions for private client"}) + } + } + return c.JSON(clientDetailResponse{ Client: summary, Endpoints: clientEndpoints{ @@ -175,6 +222,18 @@ func (h *DevHandler) UpdateClientStatus(c *fiber.Ctx) error { return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "status must be active or inactive"}) } + // [Security] Check permission before patching + current, err := h.Hydra.GetClient(c.Context(), clientID) + if err == nil { + summary := h.mapClientSummary(*current) + if summary.Type == "private" { + isAppManager, _ := h.checkAppManagerPermission(c) + if !isAppManager { + return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "forbidden: insufficient permissions for private client"}) + } + } + } + updated, err := h.Hydra.PatchClientStatus(c.Context(), clientID, status) if err != nil { if errors.Is(err, service.ErrHydraNotFound) { @@ -221,9 +280,20 @@ func (h *DevHandler) CreateClient(c *fiber.Ctx) error { grantTypes := derefSlice(req.GrantTypes, defaultGrantTypes()) responseTypes := derefSlice(req.ResponseTypes, defaultResponseTypes()) - clientType := strings.ToLower(strings.TrimSpace(valueOr(req.Type, "confidential"))) - if clientType != "public" && clientType != "confidential" { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "type must be public or confidential"}) + clientType := strings.ToLower(strings.TrimSpace(valueOr(req.Type, "private"))) + if clientType != "pkce" && clientType != "private" { + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "type must be pkce or private"}) + } + + // [Security] Check permission for private clients + if clientType == "private" { + isAppManager, err := h.checkAppManagerPermission(c) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "permission check error"}) + } + if !isAppManager { + return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "forbidden: insufficient permissions to create private client"}) + } } status := strings.ToLower(strings.TrimSpace(valueOr(req.Status, "active"))) @@ -239,7 +309,7 @@ func (h *DevHandler) CreateClient(c *fiber.Ctx) error { tokenAuthMethod := strings.TrimSpace(valueOr(req.TokenEndpointAuthMethod, "")) if tokenAuthMethod == "" { - if clientType == "public" { + if clientType == "pkce" { tokenAuthMethod = "none" } else { tokenAuthMethod = "client_secret_basic" @@ -310,8 +380,20 @@ func (h *DevHandler) UpdateClient(c *fiber.Ctx) error { clientType := "" if req.Type != nil { clientType = strings.ToLower(strings.TrimSpace(*req.Type)) - if clientType != "public" && clientType != "confidential" { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "type must be public or confidential"}) + if clientType != "pkce" && clientType != "private" { + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "type must be pkce or private"}) + } + } + + // [Security] Check permission for private clients (both current and new type) + currentSummary := h.mapClientSummary(*current) + if currentSummary.Type == "private" || clientType == "private" { + isAppManager, err := h.checkAppManagerPermission(c) + if err != nil { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "permission check error"}) + } + if !isAppManager { + return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "forbidden: insufficient permissions for private client"}) } } @@ -325,7 +407,7 @@ func (h *DevHandler) UpdateClient(c *fiber.Ctx) error { tokenAuthMethod := strings.TrimSpace(valueOr(req.TokenEndpointAuthMethod, "")) if tokenAuthMethod == "" && clientType != "" { - if clientType == "public" { + if clientType == "pkce" { tokenAuthMethod = "none" } else { tokenAuthMethod = "client_secret_basic" @@ -382,6 +464,18 @@ func (h *DevHandler) DeleteClient(c *fiber.Ctx) error { return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "client id is required"}) } + // [Security] Check permission for private clients + current, err := h.Hydra.GetClient(c.Context(), clientID) + if err == nil { + summary := h.mapClientSummary(*current) + if summary.Type == "private" { + isAppManager, _ := h.checkAppManagerPermission(c) + if !isAppManager { + return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "forbidden: insufficient permissions for private client"}) + } + } + } + if err := h.Hydra.DeleteClient(c.Context(), clientID); err != nil { if errors.Is(err, service.ErrHydraNotFound) { return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "client not found"}) @@ -517,14 +611,25 @@ func (h *DevHandler) RotateClientSecret(c *fiber.Ctx) error { return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "client id is required"}) } + // [Security] Check permission for private clients + current, err := h.Hydra.GetClient(c.Context(), clientID) + if err == nil { + summary := h.mapClientSummary(*current) + if summary.Type == "private" { + isAppManager, _ := h.checkAppManagerPermission(c) + if !isAppManager { + return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "forbidden: insufficient permissions for private client"}) + } + } + } + // 1. Generate new secret newSecret, err := generateRandomSecret(20) if err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "failed to generate secret"}) } - // 2. Get current client to preserve other fields - current, err := h.Hydra.GetClient(c.Context(), clientID) + // 2. Get current client to preserve other fields (already fetched above) if err != nil { if errors.Is(err, service.ErrHydraNotFound) { return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "client not found"}) @@ -584,9 +689,9 @@ func (h *DevHandler) mapClientSummary(client domain.HydraClient) clientSummary { } } - clientType := "confidential" + clientType := "private" if strings.EqualFold(client.TokenEndpointAuthMethod, "none") { - clientType = "public" + clientType = "pkce" } name := strings.TrimSpace(client.ClientName) diff --git a/backend/internal/handler/dev_handler_test.go b/backend/internal/handler/dev_handler_test.go index 4c491c73..edc1513f 100644 --- a/backend/internal/handler/dev_handler_test.go +++ b/backend/internal/handler/dev_handler_test.go @@ -1,8 +1,10 @@ package handler import ( + "baron-sso-backend/internal/domain" "baron-sso-backend/internal/service" "bytes" + "context" "encoding/json" "net/http" "net/http/httptest" @@ -10,8 +12,32 @@ import ( "github.com/gofiber/fiber/v2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) +type MockKetoService struct { + mock.Mock +} + +func (m *MockKetoService) CheckPermission(ctx context.Context, subject, namespace, object, relation string) (bool, error) { + args := m.Called(ctx, subject, namespace, object, relation) + return args.Bool(0), args.Error(1) +} +func (m *MockKetoService) CreateRelation(ctx context.Context, namespace, object, relation, subject string) error { + return m.Called(ctx, namespace, object, relation, subject).Error(0) +} +func (m *MockKetoService) DeleteRelation(ctx context.Context, namespace, object, relation, subject string) error { + return m.Called(ctx, namespace, object, relation, subject).Error(0) +} +func (m *MockKetoService) ListRelations(ctx context.Context, namespace, object, relation, subject string) ([]service.RelationTuple, error) { + args := m.Called(ctx, namespace, object, relation, subject) + return args.Get(0).([]service.RelationTuple), args.Error(1) +} +func (m *MockKetoService) ListObjects(ctx context.Context, namespace, relation, subject string) ([]string, error) { + args := m.Called(ctx, namespace, relation, subject) + return args.Get(0).([]string), args.Error(1) +} + func TestListClients_Success(t *testing.T) { transport := roundTripFunc(func(r *http.Request) (*http.Response, error) { if r.URL.Path == "/clients" { @@ -23,13 +49,22 @@ func TestListClients_Success(t *testing.T) { return httpJSONAny(r, http.StatusNotFound, map[string]string{"error": "not found"}), nil }) + mockKeto := new(MockKetoService) + // For simplicity, always allow in basic success test + mockKeto.On("CheckPermission", mock.Anything, mock.Anything, "System", "AppManager", "member").Return(true, nil) + h := &DevHandler{ Hydra: &service.HydraAdminService{ AdminURL: "http://hydra.test", HTTPClient: &http.Client{Transport: transport}, }, + Keto: mockKeto, } app := fiber.New() + app.Use(func(c *fiber.Ctx) error { + c.Locals("user_profile", &domain.UserProfileResponse{ID: "test-user", Role: domain.RoleSuperAdmin}) + return c.Next() + }) app.Get("/api/v1/dev/clients", h.ListClients) req := httptest.NewRequest(http.MethodGet, "/api/v1/dev/clients", nil) @@ -58,14 +93,21 @@ func TestGetClient_Success(t *testing.T) { return httpJSONAny(r, http.StatusNotFound, map[string]string{"error": "not found"}), nil }) + mockKeto := new(MockKetoService) + h := &DevHandler{ Hydra: &service.HydraAdminService{ AdminURL: "http://hydra.test", PublicURL: "http://hydra-public.test", // PublicURL 추가 HTTPClient: &http.Client{Transport: transport}, }, + Keto: mockKeto, } app := fiber.New() + app.Use(func(c *fiber.Ctx) error { + c.Locals("user_profile", &domain.UserProfileResponse{ID: "test-user", Role: domain.RoleSuperAdmin}) + return c.Next() + }) app.Get("/api/v1/dev/clients/:id", h.GetClient) req := httptest.NewRequest(http.MethodGet, "/api/v1/dev/clients/client-123", nil) @@ -80,26 +122,6 @@ func TestGetClient_Success(t *testing.T) { assert.Equal(t, "http://hydra-public.test/oauth2/auth", res.Endpoints.Authorization) } -func TestGetClient_NotFound(t *testing.T) { - transport := roundTripFunc(func(r *http.Request) (*http.Response, error) { - return httpJSONAny(r, http.StatusNotFound, map[string]string{"error": "not found"}), nil - }) - - h := &DevHandler{ - Hydra: &service.HydraAdminService{ - AdminURL: "http://hydra.test", - HTTPClient: &http.Client{Transport: transport}, - }, - } - app := fiber.New() - app.Get("/api/v1/dev/clients/:id", h.GetClient) - - req := httptest.NewRequest(http.MethodGet, "/api/v1/dev/clients/non-existent", nil) - resp, _ := app.Test(req, -1) - - assert.Equal(t, http.StatusNotFound, resp.StatusCode) -} - func TestCreateClient_Success(t *testing.T) { transport := roundTripFunc(func(r *http.Request) (*http.Response, error) { if r.Method == http.MethodPost && r.URL.Path == "/clients" { @@ -112,6 +134,7 @@ func TestCreateClient_Success(t *testing.T) { return httpJSONAny(r, http.StatusInternalServerError, map[string]string{"error": "hydra error"}), nil }) + mockKeto := new(MockKetoService) secretRepo := &mockSecretRepo{secrets: make(map[string]string)} redisRepo := &mockRedisRepo{data: make(map[string]string)} @@ -122,13 +145,18 @@ func TestCreateClient_Success(t *testing.T) { }, SecretRepo: secretRepo, Redis: redisRepo, + Keto: mockKeto, } app := fiber.New() + app.Use(func(c *fiber.Ctx) error { + c.Locals("user_profile", &domain.UserProfileResponse{ID: "test-user", Role: domain.RoleSuperAdmin}) + return c.Next() + }) app.Post("/api/v1/dev/clients", h.CreateClient) body, _ := json.Marshal(map[string]interface{}{ "client_name": "New App", - "type": "confidential", + "type": "private", "redirectUris": []string{"http://localhost/cb"}, }) req := httptest.NewRequest(http.MethodPost, "/api/v1/dev/clients", bytes.NewReader(body)) diff --git a/devfront/tests/clients.spec.ts b/devfront/tests/clients.spec.ts index 5e37c48f..0b2d2f09 100644 --- a/devfront/tests/clients.spec.ts +++ b/devfront/tests/clients.spec.ts @@ -48,7 +48,7 @@ test("clients page loads correctly", async ({ page }) => { { id: "client-playwright", name: "Playwright Client", - type: "confidential", + type: "private", status: "active", createdAt: new Date().toISOString(), redirectUris: ["http://localhost:5174/callback"],