From 3d3ba84df0febf97420dfdabc2c2412cb2e590a9 Mon Sep 17 00:00:00 2001 From: Trey t Date: Mon, 18 May 2026 21:55:33 -0500 Subject: [PATCH] fix(auth): delete the Kratos identity on account deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Account deletion removed all local data but left the Ory Kratos identity intact — an orphaned identity that can still authenticate. Close the gap: - kratos.Client gains the admin API: NewClient(publicURL, adminURL) and DeleteIdentity (DELETE /admin/identities/{id}; a 404 is treated as success so a retry after a partial failure is idempotent). - AuthService.DeleteAccount deletes the Kratos identity FIRST; if that call fails it aborts before touching local data, so the operation is retryable rather than partially applied. - KRATOS_ADMIN_URL config (default http://kratos:4434) + router wiring. - kratos NetworkPolicy split: the api pods may now reach the admin API :4434 (Traefik still reaches only the public API :4433). - kratos CORS: allow_credentials + OPTIONS so the web browser flows (ory_kratos_session cookie) work; origins stay an explicit allowlist. - Regression tests: identity teardown happens, and a Kratos failure aborts the deletion instead of orphaning local data. Co-Authored-By: Claude Opus 4.7 (1M context) --- deploy-k3s/manifests/kratos/configmap.yaml | 6 ++- deploy-k3s/manifests/kratos/kratos.yaml | 15 ++++-- internal/config/config.go | 6 +++ internal/kratos/client.go | 50 ++++++++++++++++--- internal/router/router.go | 3 +- internal/services/auth_service.go | 25 +++++++++- internal/services/auth_service_test.go | 57 ++++++++++++++++++++++ 7 files changed, 150 insertions(+), 12 deletions(-) diff --git a/deploy-k3s/manifests/kratos/configmap.yaml b/deploy-k3s/manifests/kratos/configmap.yaml index 53a4fec..f50a37a 100644 --- a/deploy-k3s/manifests/kratos/configmap.yaml +++ b/deploy-k3s/manifests/kratos/configmap.yaml @@ -31,9 +31,13 @@ data: - https://myhoneydue.com - https://app.myhoneydue.com - https://admin.myhoneydue.com - allowed_methods: [GET, POST, PUT, PATCH, DELETE] + allowed_methods: [GET, POST, PUT, PATCH, DELETE, OPTIONS] allowed_headers: [Authorization, Content-Type, X-Session-Token, Cookie] exposed_headers: [Content-Type, Set-Cookie] + # Required: the web clients call Kratos browser flows with + # credentials (the ory_kratos_session cookie). Safe here because + # allowed_origins is an explicit list, never a wildcard. + allow_credentials: true admin: base_url: http://kratos.honeydue.svc.cluster.local:4434/ diff --git a/deploy-k3s/manifests/kratos/kratos.yaml b/deploy-k3s/manifests/kratos/kratos.yaml index 52f0235..d1621a7 100644 --- a/deploy-k3s/manifests/kratos/kratos.yaml +++ b/deploy-k3s/manifests/kratos/kratos.yaml @@ -127,9 +127,10 @@ spec: port: 4434 targetPort: 4434 --- -# Ingress to Kratos: Traefik (the auth.myhoneydue.com IngressRoute) and the -# honeyDue api pods (session whoami) may reach the public API :4433. The -# admin API :4434 takes no cluster ingress — it is reachable only in-pod. +# Ingress to Kratos. Traefik (the auth.myhoneydue.com IngressRoute) reaches +# only the public API :4433. The honeyDue api pods reach the public API :4433 +# (session whoami) AND the admin API :4434 (identity deletion on account +# close). The admin API :4434 takes no other cluster ingress. apiVersion: networking.k8s.io/v1 kind: NetworkPolicy metadata: @@ -142,16 +143,24 @@ spec: policyTypes: - Ingress ingress: + # Traefik ingress controller -> public API only. - from: - namespaceSelector: matchLabels: kubernetes.io/metadata.name: kube-system + ports: + - port: 4433 + protocol: TCP + # honeyDue api pods -> public API (whoami) + admin API (identity deletion). + - from: - podSelector: matchLabels: app.kubernetes.io/name: api ports: - port: 4433 protocol: TCP + - port: 4434 + protocol: TCP --- # Kratos egress: DNS, the Neon Postgres database, SMTP, and HTTPS to the # OIDC providers (Apple/Google token + JWKS endpoints). diff --git a/internal/config/config.go b/internal/config/config.go index 8f8a8b9..39f7815 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -145,6 +145,10 @@ type SecurityConfig struct { // KratosPublicURL is the Ory Kratos public API base URL. The auth // middleware validates sessions against {KratosPublicURL}/sessions/whoami. KratosPublicURL string + // KratosAdminURL is the Ory Kratos admin API base URL. Account deletion + // removes the user's Kratos identity via + // {KratosAdminURL}/admin/identities/{id}. + KratosAdminURL string } // StorageConfig holds file storage settings. @@ -308,6 +312,7 @@ func Load() (*Config, error) { TokenExpiryDays: viper.GetInt("TOKEN_EXPIRY_DAYS"), TokenRefreshDays: viper.GetInt("TOKEN_REFRESH_DAYS"), KratosPublicURL: viper.GetString("KRATOS_PUBLIC_URL"), + KratosAdminURL: viper.GetString("KRATOS_ADMIN_URL"), }, Storage: StorageConfig{ UploadDir: viper.GetString("STORAGE_UPLOAD_DIR"), @@ -416,6 +421,7 @@ func setDefaults() { // Token expiry defaults viper.SetDefault("TOKEN_EXPIRY_DAYS", 90) // Tokens expire after 90 days viper.SetDefault("KRATOS_PUBLIC_URL", "http://kratos:4433") // Ory Kratos public API + viper.SetDefault("KRATOS_ADMIN_URL", "http://kratos:4434") // Ory Kratos admin API viper.SetDefault("TOKEN_REFRESH_DAYS", 60) // Tokens can be refreshed after 60 days // Storage defaults diff --git a/internal/kratos/client.go b/internal/kratos/client.go index aacfb19..30d383f 100644 --- a/internal/kratos/client.go +++ b/internal/kratos/client.go @@ -1,6 +1,7 @@ -// Package kratos is a thin client for the Ory Kratos public API. honeyDue +// Package kratos is a thin client for the Ory Kratos APIs. honeyDue // delegates all identity concerns (credentials, sessions, verification, -// recovery, social sign-in) to Kratos; this client only validates sessions. +// recovery, social sign-in) to Kratos; this client validates sessions +// against the public API and deletes identities via the admin API. package kratos import ( @@ -9,6 +10,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strings" "time" ) @@ -17,17 +19,21 @@ import ( // or inactive — the caller should respond 401. var ErrUnauthorized = errors.New("kratos: session invalid or inactive") -// Client talks to the Ory Kratos public API. +// Client talks to the Ory Kratos public and admin APIs. type Client struct { publicURL string + adminURL string http *http.Client } -// NewClient builds a Kratos client for the given public-API base URL -// (e.g. http://kratos:4433 in-cluster). -func NewClient(publicURL string) *Client { +// NewClient builds a Kratos client. publicURL is the public-API base used for +// session validation (e.g. http://kratos:4433 in-cluster); adminURL is the +// admin-API base used for identity management (e.g. http://kratos:4434). +// Either may be empty when the corresponding API is unused. +func NewClient(publicURL, adminURL string) *Client { return &Client{ publicURL: strings.TrimRight(publicURL, "/"), + adminURL: strings.TrimRight(adminURL, "/"), http: &http.Client{Timeout: 5 * time.Second}, } } @@ -105,3 +111,35 @@ func (c *Client) Whoami(ctx context.Context, sessionToken, cookie string) (*Sess } return &s, nil } + +// DeleteIdentity permanently removes a Kratos identity by its UUID via the +// admin API (DELETE /admin/identities/{id}). A 404 is treated as success — +// the identity is already gone, which is the desired end state, so the call +// is idempotent across retries. Called when a honeyDue account is deleted so +// no orphaned, still-loginable identity is left behind. +func (c *Client) DeleteIdentity(ctx context.Context, identityID string) error { + if c.adminURL == "" { + return errors.New("kratos: admin URL not configured") + } + if identityID == "" { + return errors.New("kratos: empty identity id") + } + endpoint := c.adminURL + "/admin/identities/" + url.PathEscape(identityID) + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, endpoint, nil) + if err != nil { + return err + } + + resp, err := c.http.Do(req) + if err != nil { + return fmt.Errorf("kratos delete identity: %w", err) + } + defer resp.Body.Close() + + switch resp.StatusCode { + case http.StatusNoContent, http.StatusNotFound: + return nil + default: + return fmt.Errorf("kratos delete identity: unexpected status %d", resp.StatusCode) + } +} diff --git a/internal/router/router.go b/internal/router/router.go index 5539e1d..3d57002 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -246,8 +246,9 @@ func SetupRouter(deps *Dependencies) *echo.Echo { subscriptionWebhookHandler.SetCacheService(deps.Cache) // Initialize Kratos auth middleware (replaces hand-rolled token auth). - kratosClient := kratos.NewClient(cfg.Security.KratosPublicURL) + kratosClient := kratos.NewClient(cfg.Security.KratosPublicURL, cfg.Security.KratosAdminURL) authMiddleware := custommiddleware.NewKratosAuth(kratosClient, deps.Cache, deps.DB) + authService.SetKratosClient(kratosClient) // account deletion removes the Kratos identity // Initialize audit service for security event logging auditService := services.NewAuditService(deps.DB) diff --git a/internal/services/auth_service.go b/internal/services/auth_service.go index 9ee6f2c..9d0ec0a 100644 --- a/internal/services/auth_service.go +++ b/internal/services/auth_service.go @@ -3,12 +3,14 @@ package services import ( "context" "errors" + "fmt" "github.com/rs/zerolog/log" "github.com/treytartt/honeydue-api/internal/apperrors" "github.com/treytartt/honeydue-api/internal/dto/requests" "github.com/treytartt/honeydue-api/internal/dto/responses" + "github.com/treytartt/honeydue-api/internal/kratos" "github.com/treytartt/honeydue-api/internal/repositories" ) @@ -18,6 +20,7 @@ type AuthService struct { userRepo *repositories.UserRepository notificationRepo *repositories.NotificationRepository cache *CacheService + kratos *kratos.Client } // NewAuthService creates a new auth service. @@ -38,6 +41,13 @@ func (s *AuthService) SetCacheService(cache *CacheService) { s.cache = cache } +// SetKratosClient wires the Ory Kratos client so account deletion also +// removes the user's Kratos identity. When nil (e.g. in tests) the Kratos +// teardown is skipped. +func (s *AuthService) SetKratosClient(k *kratos.Client) { + s.kratos = k +} + // GetCurrentUser returns the current authenticated user with profile. func (s *AuthService) GetCurrentUser(ctx context.Context, userID uint) (*responses.CurrentUserResponse, error) { user, err := s.userRepo.WithContext(ctx).FindByIDWithProfile(userID) @@ -102,12 +112,17 @@ func (s *AuthService) UpdateProfile(ctx context.Context, userID uint, req *reque // DeleteAccount deletes a user's account and all associated data. // Kratos owns credentials; confirmation string "DELETE" is required from the // caller since we can no longer verify a password here. +// +// The Kratos identity is deleted FIRST: Kratos owns the credentials, so if +// that call fails the local data is left intact and an error is returned for +// the caller to retry. Deleting local data first would risk an orphaned +// Kratos identity that can still authenticate against a half-deleted account. func (s *AuthService) DeleteAccount(ctx context.Context, userID uint, _ *string, confirmation *string) ([]string, error) { if confirmation == nil || *confirmation != "DELETE" { return nil, apperrors.BadRequest("error.confirmation_required") } - _, err := s.userRepo.WithContext(ctx).FindByID(userID) + user, err := s.userRepo.WithContext(ctx).FindByID(userID) if err != nil { if errors.Is(err, repositories.ErrUserNotFound) { return nil, apperrors.NotFound("error.user_not_found") @@ -115,6 +130,14 @@ func (s *AuthService) DeleteAccount(ctx context.Context, userID uint, _ *string, return nil, apperrors.Internal(err) } + // Tear down the Kratos identity before touching local data. DeleteIdentity + // treats a 404 as success, so a retry after a partial failure is safe. + if s.kratos != nil && user.KratosID != "" { + if err := s.kratos.DeleteIdentity(ctx, user.KratosID); err != nil { + return nil, apperrors.Internal(fmt.Errorf("delete kratos identity: %w", err)) + } + } + var fileURLs []string txErr := s.userRepo.WithContext(ctx).Transaction(func(txRepo *repositories.UserRepository) error { urls, err := txRepo.DeleteUserCascade(userID) diff --git a/internal/services/auth_service_test.go b/internal/services/auth_service_test.go index bf27597..7406d02 100644 --- a/internal/services/auth_service_test.go +++ b/internal/services/auth_service_test.go @@ -3,6 +3,7 @@ package services import ( "context" "net/http" + "net/http/httptest" "testing" "github.com/stretchr/testify/assert" @@ -10,6 +11,8 @@ import ( "github.com/treytartt/honeydue-api/internal/config" "github.com/treytartt/honeydue-api/internal/dto/requests" + "github.com/treytartt/honeydue-api/internal/kratos" + "github.com/treytartt/honeydue-api/internal/models" "github.com/treytartt/honeydue-api/internal/repositories" "github.com/treytartt/honeydue-api/internal/testutil" ) @@ -165,6 +168,60 @@ func TestAuthService_DeleteAccount_UserNotFound(t *testing.T) { testutil.AssertAppError(t, err, http.StatusNotFound, "error.user_not_found") } +// Regression: deleting an account must also delete the user's Kratos +// identity, or an orphaned, still-loginable identity is left behind. +func TestAuthService_DeleteAccount_DeletesKratosIdentity(t *testing.T) { + service, userRepo := setupAuthService(t) + db := userRepo.DB() + + user := testutil.CreateTestUser(t, db, "kuser", "kuser@test.com", "") + require.NoError(t, db.Model(&models.User{}).Where("id = ?", user.ID). + Update("kratos_id", "kratos-uuid-123").Error) + + var gotMethod, gotPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotMethod, gotPath = r.Method, r.URL.Path + w.WriteHeader(http.StatusNoContent) + })) + defer srv.Close() + service.SetKratosClient(kratos.NewClient("", srv.URL)) + + confirmation := "DELETE" + _, err := service.DeleteAccount(context.Background(), user.ID, nil, &confirmation) + require.NoError(t, err) + assert.Equal(t, http.MethodDelete, gotMethod) + assert.Equal(t, "/admin/identities/kratos-uuid-123", gotPath) + + // Local user row is gone. + _, err = userRepo.WithContext(context.Background()).FindByID(user.ID) + assert.Error(t, err) +} + +// Regression: if the Kratos identity delete fails, local data must NOT be +// deleted — the operation must be retryable, not partially applied. +func TestAuthService_DeleteAccount_KratosFailureAbortsDeletion(t *testing.T) { + service, userRepo := setupAuthService(t) + db := userRepo.DB() + + user := testutil.CreateTestUser(t, db, "kuser2", "kuser2@test.com", "") + require.NoError(t, db.Model(&models.User{}).Where("id = ?", user.ID). + Update("kratos_id", "kratos-uuid-456").Error) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + service.SetKratosClient(kratos.NewClient("", srv.URL)) + + confirmation := "DELETE" + _, err := service.DeleteAccount(context.Background(), user.ID, nil, &confirmation) + require.Error(t, err) + + // Local user row must still exist. + _, ferr := userRepo.WithContext(context.Background()).FindByID(user.ID) + assert.NoError(t, ferr) +} + // === SetNotificationRepository === func TestAuthService_SetNotificationRepository(t *testing.T) {