log errors when a personal space is deleted

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
 		}

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() {