fix: Address code review comments for Task #2

Fixes critical and high-priority issues identified in code review:

**Critical Issues Fixed:**

1. **Added missing GDPR compliance response fields** (admin.go:756-757)
   - Added `redaction_queued` boolean to response
   - Added `redaction_job_id` int64 to response
   - Critical for tracking GDPR "right to erasure" job completion
   - Allows admins to verify message redaction was queued

2. **Implemented admin context tracking** (admin.go:696, 734, routing.go:196)
   - Changed AdminDeactivateUser signature to accept device parameter
   - Populated RequestedBy field with device.UserID (authenticated admin)
   - Fixes audit trail compliance gap - now logs which admin performed action
   - Updated route registration to pass device to handler

**High Priority Issues Fixed:**

3. **Added test assertions for redaction fields** (admin_test.go:82-85, 91)
   - Tests now verify redaction_queued and redaction_job_id present
   - Tests verify RequestedBy is correctly populated from device
   - Prevents regression if fields removed in future

**Medium Priority Issues Fixed:**

4. **Improved error message security** (admin.go:727)
   - Changed from exposing internal error details to generic message
   - JSON decode errors now return "invalid request body" only
   - Prevents information disclosure via error messages

**Testing:**
- ✅ All clientapi/routing tests passing (7/7)
- ✅ All userapi/internal tests passing (SQLite)
- ⚠️  PostgreSQL tests skipped (no DB - expected)

**Security:**
- Audit trail now complete with admin user ID
- Error messages no longer expose internal details
- GDPR compliance tracking now verifiable

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

clientapi/routing/admin.go

@@ -693,7 +693,7 @@ func parseUint64OrDefault(input string, defaultValue uint64) uint64 {
 }
 
 // AdminDeactivateUser deactivates a user account as an admin action
-func AdminDeactivateUser(req *http.Request, cfg *config.ClientAPI, userAPI userapi.ClientUserAPI) util.JSONResponse {
+func AdminDeactivateUser(req *http.Request, cfg *config.ClientAPI, device *userapi.Device, userAPI userapi.ClientUserAPI) util.JSONResponse {
 	vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
 	if err != nil {
 		return util.ErrorResponse(err)
@@ -724,14 +724,14 @@ func AdminDeactivateUser(req *http.Request, cfg *config.ClientAPI, userAPI usera
 	if err := json.NewDecoder(req.Body).Decode(&reqBody); err != nil {
 		return util.JSONResponse{
 			Code: http.StatusBadRequest,
-			JSON: spec.BadJSON(fmt.Sprintf("failed to decode request body: %s", err.Error())),
+			JSON: spec.BadJSON("invalid request body"),
 		}
 	}
 
 	// Call userAPI to perform deactivation
 	deactivateReq := &userapi.PerformUserDeactivationRequest{
 		UserID:         userID,
-		RequestedBy:    "", // TODO: Get from auth context
+		RequestedBy:    device.UserID,
 		LeaveRooms:     reqBody.LeaveRooms,
 		RedactMessages: reqBody.RedactMessages,
 	}
@@ -749,10 +749,12 @@ func AdminDeactivateUser(req *http.Request, cfg *config.ClientAPI, userAPI usera
 	return util.JSONResponse{
 		Code: http.StatusOK,
 		JSON: map[string]interface{}{
-			"user_id":        deactivateRes.UserID,
-			"deactivated":    deactivateRes.Deactivated,
-			"tokens_revoked": deactivateRes.TokensRevoked,
-			"rooms_left":     deactivateRes.RoomsLeft,
+			"user_id":          deactivateRes.UserID,
+			"deactivated":      deactivateRes.Deactivated,
+			"tokens_revoked":   deactivateRes.TokensRevoked,
+			"rooms_left":       deactivateRes.RoomsLeft,
+			"redaction_queued": deactivateRes.RedactionQueued,
+			"redaction_job_id": deactivateRes.RedactionJobID,
 		},
 	}
 }

clientapi/routing/admin_test.go

@@ -49,6 +49,9 @@ func (m *mockUserAPI) PerformUserDeactivation(ctx context.Context, req *userapi.
 func TestAdminDeactivateUser_Success(t *testing.T) {
 	mock := &mockUserAPI{}
 	cfg := makeTestConfig()
+	device := &userapi.Device{
+		UserID: "@admin:test",
+	}
 
 	reqBody := map[string]interface{}{
 		"leave_rooms":     true,
@@ -61,7 +64,7 @@ func TestAdminDeactivateUser_Success(t *testing.T) {
 	req = mux.SetURLVars(req, map[string]string{"userID": "@alice:test"})
 
 	w := httptest.NewRecorder()
-	resp := routing.AdminDeactivateUser(req, cfg, mock)
+	resp := routing.AdminDeactivateUser(req, cfg, device, mock)
 	w.WriteHeader(resp.Code); json.NewEncoder(w).Encode(resp.JSON)
 
 	assert.Equal(t, http.StatusOK, w.Code)
@@ -75,10 +78,17 @@ func TestAdminDeactivateUser_Success(t *testing.T) {
 	assert.Equal(t, float64(3), response["tokens_revoked"])
 	assert.Equal(t, float64(5), response["rooms_left"])
 
+	// Verify redaction fields are present (GDPR compliance)
+	assert.Contains(t, response, "redaction_queued", "response missing redaction_queued field")
+	assert.Contains(t, response, "redaction_job_id", "response missing redaction_job_id field")
+	assert.Equal(t, true, response["redaction_queued"], "redaction should be queued")
+	assert.Equal(t, float64(123), response["redaction_job_id"], "redaction job ID should match mock")
+
 	// Verify the userAPI was called correctly
 	require.Len(t, mock.deactivationCalls, 1)
 	call := mock.deactivationCalls[0]
 	assert.Equal(t, "@alice:test", call.UserID)
+	assert.Equal(t, "@admin:test", call.RequestedBy, "RequestedBy should be admin user ID")
 	assert.True(t, call.LeaveRooms)
 	assert.True(t, call.RedactMessages)
 }
@@ -98,7 +108,7 @@ func TestAdminDeactivateUser_OptionsOff(t *testing.T) {
 	req = mux.SetURLVars(req, map[string]string{"userID": "@bob:test"})
 
 	w := httptest.NewRecorder()
-	resp := routing.AdminDeactivateUser(req, cfg, mock)
+	resp := routing.AdminDeactivateUser(req, cfg, &userapi.Device{UserID: "@admin:test"}, mock)
 	w.WriteHeader(resp.Code); json.NewEncoder(w).Encode(resp.JSON)
 
 	assert.Equal(t, http.StatusOK, w.Code)
@@ -124,7 +134,7 @@ func TestAdminDeactivateUser_InvalidUserID(t *testing.T) {
 	req = mux.SetURLVars(req, map[string]string{"userID": "invalid-user-id"})
 
 	w := httptest.NewRecorder()
-	resp := routing.AdminDeactivateUser(req, cfg, mock)
+	resp := routing.AdminDeactivateUser(req, cfg, &userapi.Device{UserID: "@admin:test"}, mock)
 	w.WriteHeader(resp.Code); json.NewEncoder(w).Encode(resp.JSON)
 
 	assert.Equal(t, http.StatusBadRequest, w.Code)
@@ -149,7 +159,7 @@ func TestAdminDeactivateUser_MissingUserID(t *testing.T) {
 	// No URL vars set - simulating missing userID
 
 	w := httptest.NewRecorder()
-	resp := routing.AdminDeactivateUser(req, cfg, mock)
+	resp := routing.AdminDeactivateUser(req, cfg, &userapi.Device{UserID: "@admin:test"}, mock)
 	w.WriteHeader(resp.Code); json.NewEncoder(w).Encode(resp.JSON)
 
 	assert.Equal(t, http.StatusBadRequest, w.Code)
@@ -163,7 +173,7 @@ func TestAdminDeactivateUser_InvalidRequestBody(t *testing.T) {
 	req = mux.SetURLVars(req, map[string]string{"userID": "@alice:test"})
 
 	w := httptest.NewRecorder()
-	resp := routing.AdminDeactivateUser(req, cfg, mock)
+	resp := routing.AdminDeactivateUser(req, cfg, &userapi.Device{UserID: "@admin:test"}, mock)
 	w.WriteHeader(resp.Code); json.NewEncoder(w).Encode(resp.JSON)
 
 	assert.Equal(t, http.StatusBadRequest, w.Code)
@@ -185,7 +195,7 @@ func TestAdminDeactivateUser_DeactivationError(t *testing.T) {
 	req = mux.SetURLVars(req, map[string]string{"userID": "@alice:test"})
 
 	w := httptest.NewRecorder()
-	resp := routing.AdminDeactivateUser(req, cfg, mock)
+	resp := routing.AdminDeactivateUser(req, cfg, &userapi.Device{UserID: "@admin:test"}, mock)
 	w.WriteHeader(resp.Code); json.NewEncoder(w).Encode(resp.JSON)
 
 	assert.Equal(t, http.StatusInternalServerError, w.Code)
@@ -204,7 +214,7 @@ func TestAdminDeactivateUser_DefaultValues(t *testing.T) {
 	req = mux.SetURLVars(req, map[string]string{"userID": "@carol:test"})
 
 	w := httptest.NewRecorder()
-	resp := routing.AdminDeactivateUser(req, cfg, mock)
+	resp := routing.AdminDeactivateUser(req, cfg, &userapi.Device{UserID: "@admin:test"}, mock)
 	w.WriteHeader(resp.Code); json.NewEncoder(w).Encode(resp.JSON)
 
 	assert.Equal(t, http.StatusOK, w.Code)

clientapi/routing/routing.go

@@ -193,7 +193,7 @@ func registerAdminRoutes(
 		"/admin/deactivate/{userID}",
 		"/deactivate/{userID}",
 		func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			return AdminDeactivateUser(req, cfg, userAPI)
+			return AdminDeactivateUser(req, cfg, device, userAPI)
 		},
 		http.MethodPost, http.MethodOptions,
 	)