Articles created by deleted users cause server errors

[jloveland]

I create an article with user1, then I delete user1 from the system. I log in as user2 and try to view the article created by user1. This causes a server error:

We could solve this by checking to make sure article.user exists in articles.server.controller.js
here:
https://github.com/meanjs/mean/blob/master/modules/articles/server/controllers/articles.server.controller.js#L107

    if (article.user) {
      req.article = article;
    }

[ilanbiala]

Ideally articles created by users are still accessible, and we just change the user field to something like "Deleted user".

[mleanos]

There's either an issue that discusses this, or it was asked in the Gitter room. Can't find it now..

I agree with @ilanbiala that they should still be accessible, and displaying something like "Deleted User" on the front-end.

However, I think we could come up with a way to handle it on the back-end as well. Perhaps, we'd have a task that performs an update on articles or other artifacts that belong to a user, when that user is deleted. We could set the Article.user field to an Admin account, or similar type of system user.

My worry is that we'd have an orphaned User reference in that MongoDB document. Which might bring up another interesting point.. Maybe we should never delete a User from the system completely, if they have artifacts linked to them. We could do a soft delete of their account.

This is sort of an edge case, but I think it can be solved & handled in a graceful manner.

[ilanbiala]

@mleanos privacy/security issue for that user, and it's deceiving because they technically weren't deleted.

[mleanos]

@ilanbiala You're right. If the developer wanted to have a "soft" delete feature, that would be their call. And probably would need to be a good reason for it.

Any thoughts on solving the problem of the orphan User reference in a case like this?

[ilanbiala]

Create a placeholder user with name "Deleted user" and assign articles to be owned by that user?

[mleanos]

@ilanbiala Yea, that's along the line of what I was trying to describe above.

We'd need a post delete hook (if we don't already) on the User model. We would use that as a cleanup of sorts on the User data & any related data.

[ilanbiala]

@lirantal @codydaig any other suggestions? If not, @mleanos do you want to create a PR for this?

[codydaig]

@ilanbiala @mleanos I think the placeholder user seems like the best idea.

[lirantal]

I'm +1 for handling a post delete hook.

[mleanos]

@ilanbiala Sounds like we have a consensus. I'll submit a PR, unless @jloveland has already done some work on this and wants to handle it; Jason, I wanted to throw that out there since you were the one that opened this issue.

[jloveland]

@mleanos go for it! I haven't had a lot of time lately.

[mleanos]

I'm having a bit back & forth with myself, on the implementation for this.

I like the idea of handling this in a post('remove') hook. It makes sense that we would want to perform the cleanup here, since it's something that should be done regardless of where the delete is coming from.

The issue with this is that we would need to reference other models from the User model definition file. For instance, we would need to pull in the Articles model for finding any articles that are related to this deleted User.

We can't be sure the model is available when the model is loaded; in the case of referencing the Article model from the User model it's fine because of the issue of the models loaded in alphabetical order. But the inverse will not work. This opens up a whole other issue of how we're loading the models when the app starts.

The simple implementation seems to be to add the cleanup here. But it's not DRY enough for my taste.
https://github.com/meanjs/mean/blob/master/modules/users/server/controllers/admin.server.controller.js#L48

Any thoughts?

[lirantal]

Referencing other models from the User model sounds like a bad idea to me too.
Is there another option?

If not, then in the Articles module we can show a [deleted] entry instead.

By the way, another approach is to also re-think the user deletion in general, where we don't actually remove user entries from the system, but instead have an isDeleted flag. This is usually a common practice so that in the future you can track such entries, and also easily restore when required.
This sounds a better solution in all to me. WDYT?