server: anonymized the user data before soft deletion #434
Conversation
WalkthroughAdds an admin DELETE endpoint to soft-delete a user: parses Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin Client
participant Router as Admin Router
participant Handler as DeleteUserHandler
participant DB as Database (Tx)
participant Keto as Keto API
participant Kratos as Kratos Admin API
Admin->>Router: DELETE /admin/user/{user_id}
Router->>Handler: route to delete(user_id)
Handler->>Handler: validate user_id
Handler->>DB: BEGIN TRANSACTION
Handler->>DB: fetch user, fetch OrganisationUser records
loop per organisation
Handler->>DB: remove OrganisationUser (in-TX) and collect org ID
end
Handler->>DB: anonymize user, set is_active=false, soft-delete (in-TX)
DB-->>Handler: OK
Handler->>DB: COMMIT
alt commit success
par remove org roles
Handler->>Keto: remove user from org roles (per org)
Keto-->>Handler: ok / errors (logged)
and
Handler->>Keto: delete relation tuples for user
Keto-->>Handler: ok / errors (logged)
end
alt KID exists
Handler->>Kratos: DELETE /identities/{kid}
Kratos-->>Handler: 204 / other (logged)
end
Handler-->>Router: 200 OK
Router-->>Admin: 200 OK
else commit fails
Handler-->>Router: error (rollback)
Router-->>Admin: error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying kavach-docs with
|
| Latest commit: |
4f054d1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c66e9a29.kavach-docs.pages.dev |
| Branch Preview URL: | https://delete-user-8yy4.kavach-docs.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
server/action/admin/user/delete.go (2)
168-168: Consider using a shared, configured HTTP client.Creating a new
http.Clientfor each request bypasses connection pooling and requires repeated timeout configuration. A package-level or singleton client would improve performance and maintainability.Example approach:
// At package level or in a shared util var defaultHTTPClient = &http.Client{ Timeout: 30 * time.Second, Transport: &http.Transport{ MaxIdleConns: 100, MaxIdleConnsPerHost: 10, IdleConnTimeout: 90 * time.Second, }, } // In deleteKratosIdentity resp, err := defaultHTTPClient.Do(req)
97-102: Extract magic strings to package constants.Hardcoded anonymization strings reduce maintainability and make it harder to ensure consistency if deletion logic is extended.
+const ( + deletedUserDisplayName = "Deleted User" + deletedEmailPrefix = "deleted-" +) + func delete(w http.ResponseWriter, r *http.Request) { // ... existing code ... // Anonymize user data - anonymizedEmail := "deleted-" + userToUpdate.Email + anonymizedEmail := deletedEmailPrefix + userToUpdate.Email // Update user with anonymized data updates := map[string]interface{}{ - "display_name": "Deleted User", - "first_name": "Deleted User", + "display_name": deletedUserDisplayName, + "first_name": deletedUserDisplayName, "email": anonymizedEmail, "is_active": false, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/action/admin/user/delete.go(1 hunks)server/action/admin/user/route.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/action/admin/user/delete.go (4)
server/model/setup.go (1)
DB(17-17)server/util/user/organisationUser.go (1)
DeleteUserFromOrganisationRoles(15-92)server/model/keto.go (2)
KetoRelationTupleWithSubjectID(50-53)KetoSubjectSet(44-48)server/util/keto/relationTuple/delete.go (1)
DeleteRelationTupleWithSubjectID(14-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
server/action/admin/user/route.go (1)
22-22: LGTM!The DELETE route addition follows the existing routing patterns and correctly binds to the delete handler.
server/action/admin/user/delete.go (3)
29-37: LGTM!URL parameter extraction and validation follows the standard pattern with appropriate error handling.
107-122: LGTM!The anonymization-then-soft-delete sequence correctly ensures data is sanitized before the soft delete marker is applied.
44-78: LGTM!Error handling follows consistent patterns with proper rollbacks, logging, and error responses throughout the deletion workflow.
server/action/admin/user/delete.go
Outdated
| 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, | ||
| } | ||
|
|
||
| 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() |
There was a problem hiding this comment.
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:
- Keto relations deleted successfully → later DB error → rollback → Keto state orphaned
- Kratos identity deleted → commit fails → identity lost but user record remains
- 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:
-
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
-
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
-
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.
| 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, | ||
| } |
There was a problem hiding this comment.
Email uniqueness constraint violation risk.
The anonymization strategy prefixes email with "deleted-", which can cause conflicts:
- Deleting the same user twice will attempt to create duplicate "[email protected]" entries
- 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.
| 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) | ||
| } |
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
server/action/admin/user/delete.go (3)
81-81: Email uniqueness constraint violation risk persists.The anonymization strategy still uses a simple "deleted-" prefix, which will cause database uniqueness constraint violations when the same user is deleted multiple times (e.g., if the user is restored and deleted again, or in test scenarios).
Consider using a unique identifier in the anonymized email to guarantee uniqueness:
- anonymizedEmail := "deleted-" + userToUpdate.Email + // Use timestamp or UUID to ensure uniqueness on repeated deletions + anonymizedEmail := fmt.Sprintf("deleted-%d-%s", uID, userToUpdate.Email)This ensures that each deletion creates a unique anonymized email even if the same user is deleted multiple times.
179-183: Add timeout to HTTP client.The HTTP client lacks a timeout configuration, which can cause the request to hang indefinitely if Kratos is unresponsive or network issues occur. This is especially problematic in a delete operation that should complete quickly.
Apply this diff to add a timeout:
// Send request - client := &http.Client{} + client := &http.Client{ + Timeout: 30 * time.Second, + } resp, err := client.Do(req)Don't forget to add the
timeimport:import ( "encoding/json" "fmt" "net/http" "net/url" "strconv" + "time" "github.com/factly/kavach-server/model"
187-193: Improve error handling for non-JSON responses.The error handling assumes Kratos returns a JSON response on failure (line 189), which may not always be the case. If the response is not valid JSON, the actual error is masked and you get a generic JSON decode error instead.
Apply this diff to handle both JSON and non-JSON error responses:
// 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) }
🧹 Nitpick comments (1)
server/action/admin/user/delete.go (1)
118-145: Good improvement: external operations after DB commit.Moving external system operations (Keto, Kratos) after the database transaction commit is the correct approach and addresses the critical transaction atomicity concern from previous reviews. This ensures the database remains consistent even if external operations fail.
Enhancement opportunity:
The inline comments mention implementing a retry mechanism or queue for failed operations (line 127). Consider implementing an asynchronous job queue or dead-letter queue pattern to handle failed external operations, which would improve reliability and allow automatic retry of transient failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/action/admin/user/delete.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/action/admin/user/delete.go (4)
server/model/setup.go (1)
DB(17-17)server/util/user/organisationUser.go (1)
DeleteUserFromOrganisationRoles(15-92)server/model/keto.go (2)
KetoRelationTupleWithSubjectID(50-53)KetoSubjectSet(44-48)server/util/keto/relationTuple/delete.go (1)
DeleteRelationTupleWithSubjectID(14-63)
Summary by CodeRabbit