Skip to content

server: anonymized the user data before soft deletion #434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shreeharsha-factly merged 4 commits into from
Oct 15, 2025
Merged

server: anonymized the user data before soft deletion #434

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
71b5c2a
server: anonymized the user data before soft deletion
shreeharsha-factly Oct 15, 2025
94d34c6
server: fix claude suggestions
shreeharsha-factly Oct 15, 2025
7b101ad
server: remove reduntant rollback
shreeharsha-factly Oct 15, 2025
4f054d1
server: add logs
shreeharsha-factly Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 185 additions & 0 deletions server/action/admin/user/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
package user

import (
"encoding/json"
"fmt"
"net/http"
"net/url"
"strconv"

"github.com/factly/kavach-server/model"
keto "github.com/factly/kavach-server/util/keto/relationTuple"
userUtil "github.com/factly/kavach-server/util/user"
"github.com/factly/x/errorx"
"github.com/factly/x/loggerx"
"github.com/factly/x/renderx"
"github.com/go-chi/chi"
"github.com/spf13/viper"
)

// delete - Soft delete user by id
// @Summary Soft delete a user
// @Description Soft delete user by ID and anonymize their data
// @Tags User
// @ID delete-user-by-id
// @Param X-User header string true "Admin User ID"
// @Param user_id path string true "User ID"
// @Success 200
// @Router /admin/users/{user_id} [delete]
func delete(w http.ResponseWriter, r *http.Request) {
// Get user ID from path
userID := chi.URLParam(r, "user_id")
uID, err := strconv.Atoi(userID)
if err != nil {
loggerx.Error(err)
errorx.Render(w, errorx.Parser(errorx.InvalidID()))
return
}

// Start a transaction
tx := model.DB.Begin()

// 1. Delete all user's relation tuples from Keto
err = deleteUserRelations(uint(uID))
if err != nil {
tx.Rollback()
loggerx.Error(err)
errorx.Render(w, errorx.Parser(errorx.InternalServerError()))
return
}

// 2. Remove user from all organizations and roles
// First get all organizations the user is part of
var orgUsers []model.OrganisationUser
err = tx.Model(&model.OrganisationUser{}).Where("user_id = ?", uID).Find(&orgUsers).Error
if err != nil {
tx.Rollback()
loggerx.Error(err)
errorx.Render(w, errorx.Parser(errorx.DBError()))
return
}

// Remove user from each organization
for _, orgUser := range orgUsers {
err = userUtil.DeleteUserFromOrganisationRoles(orgUser.OrganisationID, uint(uID))
if err != nil {
tx.Rollback()
loggerx.Error(err)
errorx.Render(w, errorx.Parser(errorx.InternalServerError()))
return
}

err = tx.Delete(&orgUser).Error
if err != nil {
tx.Rollback()
loggerx.Error(err)
errorx.Render(w, errorx.Parser(errorx.DBError()))
return
}
}

// 3. Soft delete user record and anonymize their data
userToUpdate := &model.User{}

// First get the user details before updating
err = tx.Model(&model.User{}).Where("id = ?", uID).First(userToUpdate).Error
if err != nil {
tx.Rollback()
loggerx.Error(err)
errorx.Render(w, errorx.Parser(errorx.DBError()))
return
}

// Store the KID for later use
kid := userToUpdate.KID

// Anonymize user data
anonymizedEmail := "deleted-" + userToUpdate.Email

// Update user with anonymized data
updates := map[string]interface{}{
"display_name": "Deleted User",
"first_name": "Deleted User",
"email": anonymizedEmail,
"is_active": false,
}
Comment on lines 81 to 90
Copy link

@coderabbitai coderabbitai bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Email uniqueness constraint violation risk.

The anonymization strategy prefixes email with "deleted-", which can cause conflicts:

  1. Deleting the same user twice will attempt to create duplicate "[email protected]" entries
  2. An actual user with email "[email protected]" would conflict

Additionally, last_name is not anonymized despite being present in the user model, potentially leaving sensitive data.

Apply this diff to use a unique anonymized email and include last_name:

-    // Anonymize user data
-    anonymizedEmail := "deleted-" + userToUpdate.Email
+    // Anonymize user data with unique identifier
+    anonymizedEmail := fmt.Sprintf("deleted-%d-%s", uID, userToUpdate.Email)
     
     // Update user with anonymized data
     updates := map[string]interface{}{
         "display_name": "Deleted User",
         "first_name":   "Deleted User",
+        "last_name":    "Deleted User",
         "email":        anonymizedEmail,
         "is_active":    false,
     }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In server/action/admin/user/delete.go around lines 97 to 105, the anonymized
email currently uses a static "deleted-"+email which can collide and the
last_name field is not anonymized; change the anonymization to produce a
guaranteed-unique email (for example include the user ID and a timestamp or
UUID, e.g. fmt.Sprintf("deleted-%s-%d@redacted", user.ID, time.Now().Unix()) or
use a UUID) and add last_name to the updates map (e.g. "last_name": "Deleted
User") so all identifiable name fields are cleared; ensure the generated email
conforms to your email schema/validation and is deterministic enough to avoid
uniqueness constraint violations on repeated deletes.


err = tx.Model(&model.User{}).Where("id = ?", uID).Updates(updates).Error
if err != nil {
tx.Rollback()
loggerx.Error(err)
errorx.Render(w, errorx.Parser(errorx.DBError()))
return
}

// Soft delete the user
err = tx.Delete(&model.User{}, uID).Error
if err != nil {
tx.Rollback()
loggerx.Error(err)
errorx.Render(w, errorx.Parser(errorx.DBError()))
return
}

// 4. Delete user's identity from Kratos
if kid != "" {
err = deleteKratosIdentity(kid)
if err != nil {
tx.Rollback()
loggerx.Error(err)
errorx.Render(w, errorx.Parser(errorx.InternalServerError()))
return
}
}

// Commit the transaction
tx.Commit()
Copy link

@coderabbitai coderabbitai bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Transaction cannot guarantee atomicity across external HTTP calls.

The transaction wraps database operations but external HTTP calls to Keto (lines 43, 64) and Kratos (line 126) are not transactional. This creates race conditions and potential inconsistencies:

Failure scenarios:

  1. Keto relations deleted successfully → later DB error → rollback → Keto state orphaned
  2. Kratos identity deleted → commit fails → identity lost but user record remains
  3. Organization roles removed from Keto → transaction rolled back → roles deleted but user associations intact

Impact: Data corruption across services, difficult-to-recover inconsistent states, potential security issues (orphaned permissions in Keto).

Consider implementing one of these patterns:

  1. Saga pattern with compensating transactions:

    • Track which external operations succeeded
    • On rollback, call compensating APIs to reverse external changes
    • Example: Store deleted relation tuples to re-create them on rollback

  2. Idempotent operations with retry logic:

    • Make external deletions idempotent
    • Use a separate cleanup job to reconcile state post-commit
    • Store deletion intent in DB first, then process asynchronously

  3. Reorder operations (partial mitigation):

    • Perform all DB operations in transaction first
    • Only call external services after successful commit
    • Accept that external call failures after commit require manual intervention or async retry

Example saga-style rollback:

+// Track operations for potential rollback
+type deletionState struct {
+    ketoRelationsDeleted bool
+    orgRolesDeleted []uint
+    kratosDeleted bool
+}

 func delete(w http.ResponseWriter, r *http.Request) {
     // ... validation ...
     tx := model.DB.Begin()
+    state := &deletionState{}
+    defer func() {
+        if r := recover(); r != nil || err != nil {
+            // Compensate external operations
+            compensateDeleteOperation(state, uint(uID))
+            tx.Rollback()
+        }
+    }()
     
-    err = deleteUserRelations(uint(uID))
+    err = deleteUserRelations(uint(uID), state)
     if err != nil {
-        tx.Rollback()
         return
     }
     // ... rest of operations tracking state ...
     tx.Commit()
 }
+
+func compensateDeleteOperation(state *deletionState, userID uint) {
+    if state.ketoRelationsDeleted {
+        // Re-create essential relations from backup
+    }
+    // ... other compensations ...
+}

Committable suggestion skipped: line range outside the PR's diff.


renderx.JSON(w, http.StatusOK, nil)
}

// deleteUserRelations deletes all relation tuples for a user as part of the soft deletion process
func deleteUserRelations(userID uint) error {
// Delete all relation tuples where the user is the subject
tuple := &model.KetoRelationTupleWithSubjectID{
KetoSubjectSet: model.KetoSubjectSet{},
SubjectID: fmt.Sprintf("%d", userID),
}

return keto.DeleteRelationTupleWithSubjectID(tuple)
}

// deleteKratosIdentity deletes a user's identity from Kratos as part of the soft deletion process
func deleteKratosIdentity(kid string) error {
// Build the Kratos admin API URL
kratosURL, err := url.Parse(viper.GetString("kratos_admin_url"))
if err != nil {
return err
}
kratosURL.Path = fmt.Sprintf("/admin/identities/%s", kid)

// Create DELETE request
req, err := http.NewRequest(http.MethodDelete, kratosURL.String(), nil)
if err != nil {
return err
}

// Send request
client := &http.Client{}
resp, err := client.Do(req)
if err != nil {
return err
}
defer resp.Body.Close()

// Check response status
if resp.StatusCode != http.StatusNoContent {
var respBody map[string]interface{}
if err := json.NewDecoder(resp.Body).Decode(&respBody); err != nil {
return fmt.Errorf("failed to decode Kratos error response: %v", err)
}
return fmt.Errorf("failed to delete Kratos identity: %v", respBody)
}
Comment on lines +181 to +195
Copy link

@coderabbitai coderabbitai bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add timeout to HTTP client and improve error handling.

The HTTP client lacks a timeout, risking indefinite hangs. Additionally, assuming the error response is JSON can mask the actual error if Kratos returns non-JSON.

Apply this diff:

-    // Send request
-    client := &http.Client{}
+    // Send request with timeout
+    client := &http.Client{
+        Timeout: 30 * time.Second,
+    }
     resp, err := client.Do(req)
     if err != nil {
         return err
     }
     defer resp.Body.Close()
 
     // Check response status
     if resp.StatusCode != http.StatusNoContent {
         var respBody map[string]interface{}
-        if err := json.NewDecoder(resp.Body).Decode(&respBody); err != nil {
-            return fmt.Errorf("failed to decode Kratos error response: %v", err)
+        decodeErr := json.NewDecoder(resp.Body).Decode(&respBody)
+        if decodeErr != nil {
+            return fmt.Errorf("failed to delete Kratos identity, status %d: %w", resp.StatusCode, decodeErr)
         }
         return fmt.Errorf("failed to delete Kratos identity: %v", respBody)
     }

Don't forget to add the time import:

 import (
     "encoding/json"
     "fmt"
     "net/http"
     "net/url"
     "strconv"
+    "time"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
client := &http.Client{}
resp, err := client.Do(req)
if err != nil {
return err
}
defer resp.Body.Close()
// Check response status
if resp.StatusCode != http.StatusNoContent {
var respBody map[string]interface{}
if err := json.NewDecoder(resp.Body).Decode(&respBody); err != nil {
return fmt.Errorf("failed to decode Kratos error response: %v", err)
}
return fmt.Errorf("failed to delete Kratos identity: %v", respBody)
}
// in the import block, add:
import (
"encoding/json"
"fmt"
"net/http"
"net/url"
"strconv"
"time"
)
// … later in deleteKratosIdentity …
// Send request with timeout
client := &http.Client{
Timeout: 30 * time.Second,
}
resp, err := client.Do(req)
if err != nil {
return err
}
defer resp.Body.Close()
// Check response status
if resp.StatusCode != http.StatusNoContent {
var respBody map[string]interface{}
decodeErr := json.NewDecoder(resp.Body).Decode(&respBody)
if decodeErr != nil {
return fmt.Errorf(
"failed to delete Kratos identity, status %d: %w",
resp.StatusCode,
decodeErr,
)
}
return fmt.Errorf("failed to delete Kratos identity: %v", respBody)
}
🤖 Prompt for AI Agents 
In server/action/admin/user/delete.go around lines 168 to 182, the HTTP client
is created without a timeout and the error-path assumes a JSON response which
can fail or mask the real error; fix by creating the client with a reasonable
timeout (e.g., http.Client{Timeout: time.Second * 10}) and on non-204 responses
read the full response body bytes, attempt to json.Decode into a map but if that
fails fall back to using the raw body string, then return an error that includes
the HTTP status code and the decoded or raw body; also add the time import.


return nil
}
1 change: 1 addition & 0 deletions server/action/admin/user/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func Router() chi.Router {
r.Post("/checker", checker)
r.Post("/", create)
r.Get("/", list)
r.Delete("/{user_id}", delete)

return r
}
Loading