diff --git a/backend/app/rest/api/admin_test.go b/backend/app/rest/api/admin_test.go index 5afe2bb8..4e48f2e1 100644 --- a/backend/app/rest/api/admin_test.go +++ b/backend/app/rest/api/admin_test.go @@ -802,7 +802,8 @@ func TestAdmin_DeleteMeRequestFailed(t *testing.T) { assert.NoError(t, resp.Body.Close()) assert.Equal(t, http.StatusForbidden, resp.StatusCode) - // try bad user + // unknown user: deletion is idempotent, so a valid (signed) delete_me token for a user with + // no stored data is a no-op success rather than an error badClaimsUser := claims badClaimsUser.User.ID = "no-such-id" tkn, err = srv.Authenticator.TokenService().Token(badClaimsUser) @@ -813,7 +814,7 @@ func TestAdmin_DeleteMeRequestFailed(t *testing.T) { resp, err = client.Do(req) assert.NoError(t, err) assert.NoError(t, resp.Body.Close()) - assert.Equal(t, http.StatusBadRequest, resp.StatusCode, resp.Status) + assert.Equal(t, http.StatusOK, resp.StatusCode, resp.Status) badClaimsUser.User.ID = "provider1_user1" // try without deleteme flag diff --git a/backend/app/store/engine/bolt.go b/backend/app/store/engine/bolt.go index 4244719e..c84094b1 100644 --- a/backend/app/store/engine/bolt.go +++ b/backend/app/store/engine/bolt.go @@ -3,6 +3,7 @@ package engine import ( "bytes" "encoding/json" + "errors" "fmt" "strings" "time" @@ -10,6 +11,7 @@ import ( log "github.com/go-pkgz/lgr" "github.com/hashicorp/go-multierror" bolt "go.etcd.io/bbolt" + berrors "go.etcd.io/bbolt/errors" "github.com/umputun/remark42/backend/app/store" ) @@ -890,29 +892,26 @@ func (b *BoltDB) deleteUser(bdb *bolt.DB, siteID, userID string, mode store.Dele log.Printf("[DEBUG] comments for removal=%d", len(comments)) - if len(comments) > 0 { - // delete collected comments - for _, ci := range comments { - if e := b.deleteComment(bdb, ci.locator, ci.commentID, mode); e != nil { - return fmt.Errorf("failed to delete comment %+v: %w", ci, err) - } + // delete collected comments + for _, ci := range comments { + if e := b.deleteComment(bdb, ci.locator, ci.commentID, mode); e != nil { + return fmt.Errorf("failed to delete comment %+v: %w", ci, e) } + } - // delete user bucket in hard mode - if mode == store.HardDelete { - err = bdb.Update(func(tx *bolt.Tx) error { - usersBkt := tx.Bucket([]byte(userBucketName)) - if usersBkt != nil { - if e := usersBkt.DeleteBucket([]byte(userID)); e != nil { - return fmt.Errorf("failed to delete user bucket for %s: %w", userID, e) - } - } - return nil - }) - - if err != nil { - return fmt.Errorf("can't delete user meta: %w", err) + // delete the user's bucket in hard mode. A user who only logged in but never commented has + // no per-user bucket, so tolerate ErrBucketNotFound; the top-level users bucket is created + // by NewBoltDB and is always present. + if mode == store.HardDelete { + err = bdb.Update(func(tx *bolt.Tx) error { + usersBkt := tx.Bucket([]byte(userBucketName)) + if e := usersBkt.DeleteBucket([]byte(userID)); e != nil && !errors.Is(e, berrors.ErrBucketNotFound) { + return fmt.Errorf("failed to delete user bucket for %s: %w", userID, e) } + return nil + }) + if err != nil { + return fmt.Errorf("can't delete user meta: %w", err) } } diff --git a/backend/app/store/engine/bolt_test.go b/backend/app/store/engine/bolt_test.go index 63e7b78e..1b9fb06a 100644 --- a/backend/app/store/engine/bolt_test.go +++ b/backend/app/store/engine/bolt_test.go @@ -843,6 +843,63 @@ func TestBoltAdmin_DeleteUserHard(t *testing.T) { assert.EqualError(t, err, `site "radio-t-bad" not found`) } +// TestBoltAdmin_DeleteUserHard_NoComments covers hard-deleting a user who has no comments +// (and therefore no user bucket) — e.g. one who only logged in. This must succeed rather than +// fail on the missing bucket, and must still remove any stored user details. +func TestBoltAdmin_DeleteUserHard_NoComments(t *testing.T) { + b, teardown := prep(t) + defer teardown() + + t.Run("login-only user with a detail but no comments", func(t *testing.T) { + const userID = "login-only-user" + loc := store.Locator{SiteID: "radio-t"} + + // user logged in and has a stored detail, but never commented (no user bucket) + _, err := b.UserDetail(UserDetailRequest{Locator: loc, UserID: userID, Detail: UserEmail, Update: "user@example.com"}) + require.NoError(t, err) + + err = b.Delete(DeleteRequest{Locator: loc, UserID: userID, DeleteMode: store.HardDelete}) + require.NoError(t, err, "hard delete must not fail on a missing user bucket") + + details, err := b.UserDetail(UserDetailRequest{Locator: loc, UserID: userID, Detail: UserEmail}) + require.NoError(t, err) + assert.Empty(t, details, "stored user detail must be removed on hard delete") + }) + + t.Run("unknown user is a no-op", func(t *testing.T) { + err := b.Delete(DeleteRequest{Locator: store.Locator{SiteID: "radio-t"}, UserID: "never-seen-user", DeleteMode: store.HardDelete}) + assert.NoError(t, err, "hard-deleting an unknown user must not error") + }) +} + +// TestBoltAdmin_DeleteUserSoft_NoComments covers soft-deleting a user with no comments. As with the +// hard path (and the existing soft path for users who do have comments) it cleans stored user +// details and is a no-op for an unknown user. +func TestBoltAdmin_DeleteUserSoft_NoComments(t *testing.T) { + b, teardown := prep(t) + defer teardown() + + t.Run("login-only user with a detail but no comments", func(t *testing.T) { + const userID = "login-only-soft" + loc := store.Locator{SiteID: "radio-t"} + + _, err := b.UserDetail(UserDetailRequest{Locator: loc, UserID: userID, Detail: UserEmail, Update: "user@example.com"}) + require.NoError(t, err) + + err = b.Delete(DeleteRequest{Locator: loc, UserID: userID, DeleteMode: store.SoftDelete}) + require.NoError(t, err) + + details, err := b.UserDetail(UserDetailRequest{Locator: loc, UserID: userID, Detail: UserEmail}) + require.NoError(t, err) + assert.Empty(t, details, "soft delete cleans stored user details, consistent with the has-comments path") + }) + + t.Run("unknown user is a no-op", func(t *testing.T) { + err := b.Delete(DeleteRequest{Locator: store.Locator{SiteID: "radio-t"}, UserID: "never-seen-soft", DeleteMode: store.SoftDelete}) + assert.NoError(t, err, "soft-deleting an unknown user must not error") + }) +} + func TestBoltAdmin_DeleteUserSoft(t *testing.T) { b, teardown := prep(t) defer teardown() diff --git a/site/src/docs/contributing/api/index.md b/site/src/docs/contributing/api/index.md index 1dc09725..f1108310 100644 --- a/site/src/docs/contributing/api/index.md +++ b/site/src/docs/contributing/api/index.md @@ -228,9 +228,9 @@ http://oldsite.com/from-old-page/1 https://newsite.com/to-new-page/1 - `GET /api/v1/admin/wait?site=site-id` - wait for completion for any async migration ops (import or remap) - `PUT /api/v1/admin/pin/{id}?site=site-id&url=post-url&pin=1` - pin or unpin comment - `GET /api/v1/admin/user/{userid}?site=site-id` - get user's info -- `DELETE /api/v1/admin/user/{userid}?site=site-id` - delete all user's comments +- `DELETE /api/v1/admin/user/{userid}?site=site-id` - delete the user's comments and stored details; succeeds even if the user has no comments or is already absent - `PUT /api/v1/admin/readonly?site=site-id&url=post-url&ro=1` - set read-only status - `PUT /api/v1/admin/verify/{userid}?site=site-id&verified=1` - set verified status -- `GET /api/v1/admin/deleteme?token=token` - process deleteme user's request +- `GET /api/v1/admin/deleteme?token=token` - process a user's deleteme request; already-deleted or dataless users return success (idempotent) _all admin calls require auth and admin privilege_