From 91aa2e8a38fbf11f6548ee06dc9913fd09c37ea3 Mon Sep 17 00:00:00 2001 From: Wenyao Date: Wed, 22 Oct 2025 07:26:04 -0700 Subject: [PATCH] fix: Address code review comments for Task #2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- clientapi/routing/admin.go | 16 +++++++++------- clientapi/routing/admin_test.go | 24 +++++++++++++++++------- clientapi/routing/routing.go | 2 +- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/clientapi/routing/admin.go b/clientapi/routing/admin.go index fb5aa6cc..d6c386c4 100644 --- a/clientapi/routing/admin.go +++ b/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, }, } } diff --git a/clientapi/routing/admin_test.go b/clientapi/routing/admin_test.go index cb63c663..7bca713b 100644 --- a/clientapi/routing/admin_test.go +++ b/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) diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go index d202ee70..d5ef7b04 100644 --- a/clientapi/routing/routing.go +++ b/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, )