diff --git a/backend/app/rest/api/admin.go b/backend/app/rest/api/admin.go index e61924b2..34e32387 100644 --- a/backend/app/rest/api/admin.go +++ b/backend/app/rest/api/admin.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "path" + "strings" "time" "github.com/go-pkgz/auth/v2" @@ -122,10 +123,14 @@ func (a *admin) deleteMeRequestCtrl(w http.ResponseWriter, r *http.Request) { } if claims.User.Picture != "" && a.authenticator.AvatarProxy() != nil { - avatarStore := a.authenticator.AvatarProxy().Store - if err = avatarStore.Remove(path.Base(claims.User.Picture)); err != nil { - rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't delete user's avatar", rest.ErrInternal) - return + if avatarID := avatarIDFromPicture(claims.User.Picture); avatarID != "" { + // best-effort removal: the user's data is already gone and the avatar store gives no way to tell + // an already-removed avatar from a real failure, so a missing avatar must not fail the deletion + if err = a.authenticator.AvatarProxy().Store.Remove(avatarID); err != nil { + log.Printf("[WARN] can't delete avatar for user %s on site %s: %v", claims.User.ID, audience, err) + } + } else { + log.Printf("[WARN] unexpected avatar picture %q for user %s on site %s, skipping removal", claims.User.Picture, claims.User.ID, audience) } } @@ -133,6 +138,17 @@ func (a *admin) deleteMeRequestCtrl(w http.ResponseWriter, r *http.Request) { R.RenderJSON(w, R.JSON{"user_id": claims.User.ID, "site_id": claims.Audience}) } +// avatarIDFromPicture returns the avatar-store object id for a user picture, or "" if the picture +// does not resolve to a well-formed id (the store names its objects ".image"). Guarding on the +// id shape keeps a malformed picture, e.g. a path sentinel, from making a filesystem-backed store +// target an unexpected path. +func avatarIDFromPicture(picture string) string { + if id := path.Base(picture); strings.HasSuffix(id, ".image") { + return id + } + return "" +} + // PUT /user/{userid}?site=side-id&block=1&ttl=7d - block or unblock user func (a *admin) setBlockCtrl(w http.ResponseWriter, r *http.Request) { userID := r.PathValue("userid") diff --git a/backend/app/rest/api/admin_test.go b/backend/app/rest/api/admin_test.go index 4e48f2e1..ec733e4b 100644 --- a/backend/app/rest/api/admin_test.go +++ b/backend/app/rest/api/admin_test.go @@ -717,7 +717,7 @@ func TestAdmin_DeleteMeRequest(t *testing.T) { }, User: &token.User{ ID: "user1", - Picture: "pic.image", + Picture: "https://demo.remark42.com/api/v1/avatar/pic.image", // production-shaped URL: removal must path.Base it to the avatar id Attributes: map[string]any{ "delete_me": true, }, @@ -747,6 +747,76 @@ func TestAdmin_DeleteMeRequest(t *testing.T) { email, err = srv.DataService.GetUserEmail("remark42", "user1") assert.NoError(t, err) assert.Empty(t, email, "user1 email was deleted") + + assert.NoFileExists(t, os.TempDir()+"/ava-remark42/42/pic.image", "user's avatar should be removed on deleteme") +} + +// a delete_me request whose token carries a picture must still succeed when the avatar is +// already gone from the store: the user data is deleted and a missing avatar is tolerated +func TestAdmin_DeleteMeRequestMissingAvatar(t *testing.T) { + ts, srv, teardown := startupT(t) + defer teardown() + + c1 := store.Comment{Text: "test test #1", Locator: store.Locator{SiteID: "remark42", + URL: "https://radio-t.com/blah"}, User: store.User{Name: "user3 name", ID: "user3"}} + _, err := srv.DataService.Create(c1) + require.NoError(t, err) + + claims := token.Claims{ + SessionOnly: true, + RegisteredClaims: jwt.RegisteredClaims{ + Audience: jwt.ClaimStrings{"remark42"}, + ID: "2345678", + Issuer: "remark42", + NotBefore: jwt.NewNumericDate(time.Now().Add(-1 * time.Minute)), + ExpiresAt: jwt.NewNumericDate(time.Now().Add(30 * time.Minute)), + }, + User: &token.User{ + ID: "user3", + Picture: "missing.image", // no avatar file exists for this picture in the store + Attributes: map[string]any{ + "delete_me": true, + }, + }, + } + + tkn, err := srv.Authenticator.TokenService().Token(claims) + require.NoError(t, err) + + client := http.Client{} + defer client.CloseIdleConnections() + req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/api/v1/admin/deleteme?token=%s", ts.URL, tkn), http.NoBody) + require.NoError(t, err) + req.SetBasicAuth("admin", "password") + resp, err := client.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusOK, resp.StatusCode, "a missing avatar must not fail the deletion") + + _, err = srv.DataService.User("remark42", "user3", 0, 0, store.User{}) + assert.EqualError(t, err, "no comments for user user3 in store", "user3 comments should be deleted") +} + +func TestAvatarIDFromPicture(t *testing.T) { + tbl := []struct { + name string + picture string + want string + }{ + {"local avatar url", "https://demo.remark42.com/api/v1/avatar/cb42ff493ade696d88a3a590f136ae9e34de7c1b.image", "cb42ff493ade696d88a3a590f136ae9e34de7c1b.image"}, + {"bare avatar id", "pic.image", "pic.image"}, + {"parent sentinel", "https://demo.remark42.com/api/v1/avatar/..", ""}, + {"trailing slash", "https://demo.remark42.com/api/v1/avatar/", ""}, + {"root", "/", ""}, + {"dotdot", "..", ""}, + {"empty", "", ""}, + {"provider url without image suffix", "https://example.com/pic.png", ""}, + } + for _, tc := range tbl { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, avatarIDFromPicture(tc.picture)) + }) + } } func TestAdmin_DeleteMeRequestFailed(t *testing.T) { diff --git a/backend/app/rest/api/rest_private.go b/backend/app/rest/api/rest_private.go index 60569bc6..3f511195 100644 --- a/backend/app/rest/api/rest_private.go +++ b/backend/app/rest/api/rest_private.go @@ -708,8 +708,9 @@ func (s *private) deleteMeCtrl(w http.ResponseWriter, r *http.Request) { NotBefore: jwt.NewNumericDate(time.Now().Add(-1 * time.Minute)), }, User: &token.User{ - ID: user.ID, - Name: user.Name, + ID: user.ID, + Name: user.Name, + Picture: user.Picture, // carried so the avatar can be removed when the request is processed Attributes: map[string]any{ "delete_me": true, // prevents this token from being used for login }, diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index 307bfdb1..fc6276d1 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -1512,6 +1512,8 @@ func TestRest_DeleteMe(t *testing.T) { claims, err := srv.Authenticator.TokenService().Parse(tkn) assert.NoError(t, err) assert.Equal(t, "provider1_dev", claims.User.ID) + assert.Equal(t, "http://example.com/pic.png", claims.User.Picture, + "delete_me token must carry the user's picture so the avatar can be removed when the request is processed") assert.Equal(t, "https://demo.remark42.com/web/deleteme.html?token="+tkn, m["link"]) req, err = http.NewRequest(http.MethodPost, fmt.Sprintf("%s/api/v1/deleteme?site=remark42", ts.URL), http.NoBody)