From 01a6365dcbc64720151d8784cde3229a5e7c9ecd Mon Sep 17 00:00:00 2001 From: micbar Date: Tue, 13 Oct 2026 15:17:12 +0200 Subject: [PATCH] log errors when a personal space is deleted --- services/graph/pkg/service/v0/users.go | 15 ++- services/graph/pkg/service/v0/users_test.go | 107 ++++++++++++++++++++ 2 files changed, 120 insertions(+), 2 deletions(-) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 6a5dc2120a..5575db07b3 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -723,7 +723,7 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { // Deleting a space a two step process (1. disabling/trashing, 2. purging) // Do the "disable/trash" step only if the space is not marked as trashed yet: if _, ok := sp.Opaque.Map[_spaceStateTrashed]; !ok { - _, err := client.DeleteStorageSpace(r.Context(), &storageprovider.DeleteStorageSpaceRequest{ + resp, err := client.DeleteStorageSpace(r.Context(), &storageprovider.DeleteStorageSpaceRequest{ Id: &storageprovider.StorageSpaceId{ OpaqueId: sp.Id.OpaqueId, }, @@ -733,11 +733,16 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not disable homespace, aborting") return } + if code := resp.GetStatus().GetCode(); code != cs3rpc.Code_CODE_OK { + logger.Error().Str("message", resp.GetStatus().GetMessage()).Interface("code", code).Msg("could not disable homespace: unexpected status code") + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not disable homespace, aborting") + return + } } // the space will if the system does not have a UserSoftDeleteRetentionTime configured, e.g. SoftDelete disabled if g.config.UserSoftDeleteRetentionTime == 0 || (purgeUser && us.State == userstate.UserStateSoftDeleted) { purgeSpaceFlag := utils.AppendPlainToOpaque(nil, "purge", "") - _, err := client.DeleteStorageSpace(r.Context(), &storageprovider.DeleteStorageSpaceRequest{ + resp, err := client.DeleteStorageSpace(r.Context(), &storageprovider.DeleteStorageSpaceRequest{ Opaque: purgeSpaceFlag, Id: &storageprovider.StorageSpaceId{ OpaqueId: sp.Id.OpaqueId, @@ -749,6 +754,12 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not delete homespace, aborting") return } + // a space that is already gone needs no purging + if code := resp.GetStatus().GetCode(); code != cs3rpc.Code_CODE_OK && code != cs3rpc.Code_CODE_NOT_FOUND { + logger.Error().Str("message", resp.GetStatus().GetMessage()).Interface("code", code).Msg("could not delete homespace: unexpected status code") + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not delete homespace, aborting") + return + } } break } diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index c8d2e13c8b..55aac117db 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -1065,6 +1065,113 @@ var _ = Describe("Users", func() { Expect(rr.Code).To(Equal(http.StatusNoContent)) gatewayClient.AssertNumberOfCalls(GinkgoT(), "DeleteStorageSpace", 2) // 2 calls for the home space. first trash, then purge }) + + It("aborts when the personal space cannot be deleted", func() { + otheruser := &userv1beta1.User{ + Id: &userv1beta1.UserId{ + OpaqueId: "otheruser", + }, + } + + lu := libregraph.User{} + lu.SetId(otheruser.Id.OpaqueId) + identityBackend.On("GetUser", mock.Anything, mock.Anything, mock.Anything).Return(&lu, nil) + gatewayClient.On("DeleteStorageSpace", mock.Anything, mock.Anything).Return(&provider.DeleteStorageSpaceResponse{ + Status: status.NewInternal(ctx, "deleting the space failed"), + }, nil) + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{ + { + Opaque: &typesv1beta1.Opaque{}, + Id: &provider.StorageSpaceId{OpaqueId: "drive1"}, + Root: &provider.ResourceId{SpaceId: "space", OpaqueId: "space"}, + SpaceType: "personal", + Owner: otheruser, + }, + }, + }, nil) + + natsKeyValueMock.EXPECT().Get(mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, key string) (jetstream.KeyValueEntry, error) { + byteRep, _ := json.Marshal(userstate.UserState{ + UserId: lu.GetId(), + State: userstate.UserStateSoftDeleted, + TimeStamp: time.Now().UTC(), + RetentionPeriod: 5 * time.Minute, + Reason: "unit test", + }) + + kve := &mocks.KeyValueEntry{} + kve.On("Value").Return(byteRep) + + return kve, nil + }).Maybe() + natsKeyValueMock.EXPECT().Put(mock.Anything, mock.Anything, mock.Anything).Return(1, nil).Maybe() + + r := httptest.NewRequest(http.MethodDelete, "/graph/v1.0/users/{userid}", nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("userID", lu.GetId()) + r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx)) + svc.DeleteUser(rr, r) + + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + gatewayClient.AssertNumberOfCalls(GinkgoT(), "DeleteStorageSpace", 1) + identityBackend.AssertNotCalled(GinkgoT(), "DeleteUser", mock.Anything, mock.Anything) + }) + + It("deletes the user when the personal space is already purged", func() { + otheruser := &userv1beta1.User{ + Id: &userv1beta1.UserId{ + OpaqueId: "otheruser", + }, + } + + lu := libregraph.User{} + lu.SetId(otheruser.Id.OpaqueId) + identityBackend.On("GetUser", mock.Anything, mock.Anything, mock.Anything).Return(&lu, nil) + identityBackend.On("DeleteUser", mock.Anything, mock.Anything).Return(nil) + gatewayClient.On("DeleteStorageSpace", mock.Anything, mock.Anything).Return(&provider.DeleteStorageSpaceResponse{ + Status: status.NewNotFound(ctx, "space is gone"), + }, nil) + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{ + { + Opaque: &typesv1beta1.Opaque{Map: map[string]*typesv1beta1.OpaqueEntry{"trashed": {Decoder: "plain", Value: []byte("trashed")}}}, + Id: &provider.StorageSpaceId{OpaqueId: "drive1"}, + Root: &provider.ResourceId{SpaceId: "space", OpaqueId: "space"}, + SpaceType: "personal", + Owner: otheruser, + }, + }, + }, nil) + + natsKeyValueMock.EXPECT().Get(mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, key string) (jetstream.KeyValueEntry, error) { + byteRep, _ := json.Marshal(userstate.UserState{ + UserId: lu.GetId(), + State: userstate.UserStateSoftDeleted, + TimeStamp: time.Now().UTC(), + RetentionPeriod: 5 * time.Minute, + Reason: "unit test", + }) + + kve := &mocks.KeyValueEntry{} + kve.On("Value").Return(byteRep) + + return kve, nil + }).Maybe() + natsKeyValueMock.EXPECT().Put(mock.Anything, mock.Anything, mock.Anything).Return(1, nil).Maybe() + + r := httptest.NewRequest(http.MethodDelete, "/graph/v1.0/users/{userid}", nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("userID", lu.GetId()) + r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx)) + svc.DeleteUser(rr, r) + + Expect(rr.Code).To(Equal(http.StatusNoContent)) + gatewayClient.AssertNumberOfCalls(GinkgoT(), "DeleteStorageSpace", 1) // already trashed, so only the purge + identityBackend.AssertCalled(GinkgoT(), "DeleteUser", mock.Anything, mock.Anything) + }) }) Describe("SoftDeleteUser", func() {