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?

[ilanbiala]

@lirantal security/privacy issue. End users think delete means "gone", not just marked as deleted and kept.

[jloveland]

I agree with @ilanbiala that user privacy is an issue here if we don't actually remove the user's account completely. However, I believe the "policy" would be up to the developer of the app and corresponding to their EULA. I would like to see the default behavior of Mean.js delete the user's data and update the articles they published to be owned by deleted user.

@mleanos given @lirantal's feedback, putting the code to delete the user and set the articles to be owned by deleted user in a admin controller instead of the model would decouple the user and article model. This would make it easier for me to remove the article module and not need to worry about repercussions?

Is there a clean way to detect existing modules and automatically discover modules? i.e. I remove the articles module, then the admin code would be able to check if it's there before it tries to query for articles?

[mleanos]

@ilanbiala @jloveland @lirantal

I still like the idea of a "soft" delete feature, for User's. I know there are concerns over privacy. However, what exactly are those concerns (negative consequences)? It seems standard to have a "soft" delete with a isDeleted field; especially when it comes to User profiles.

Currently, we don't have a feature that allows the User to delete their own account. The delete operation in question here, can only be performed by an admin. The end user doesn't have any expectations on what should happen to their data when their account is deleted. Other than what might be expressed in a EULA.

For me the issue comes down to the reasoning behind the need to delete all the User data. Is it because the policy is to remove any traces of existence of that User? If so, then that would mean we'd want to remove every artifact (data) that they created; including Articles. If it's for privacy concerns over their User profile data, then I say we remove any sensitive data; Provider data, tokens, roles, profile image, and any other details we can deem as "sensitive".

[mleanos]

One thing I have to point out is that the database would need a User that we would use to set these articles to be owned by. That might add additional complexity.

[mleanos]

On the issue of the User model references..

I feel like the current implementation of the User models could use some work, or at least discussion. It seems that the current model definition file (e.g. user.server.model.js) is doing too much. By including all the other configurations for the model (pre/post hooks, statics, and methods), we end up coupling the behavior of the models too closely with the definition of what the model represents (Schema); maybe they should be?

Are the pre/post hooks, statics, and methods of a Mongoose model never intended to interact with anything outside the scope of it's own model? I want to think this is not the case. However, in order to solve this problem (referencing other models from statics/hooks/methods) then we'd need to come up with a more effective way of loading, and defining, our models that would allow for more flexibility.

Some exploration I was doing, led me to this discussion, and response.. Automattic/mongoose#1953 (comment)

It seems like it's acceptable to reference models from other models' methods? If so, how do we solve the problem of not knowing if a model has been registered with Mongoose at the time that we need to reference said Model from a Model?

One route that I went down was to redefine the models like this, using Articles as a simple example...

// article.server.model.js
'use strict';

/**
 * Module dependencies.
 */
var mongoose = require('mongoose'),
  path = require('path'),
  User = require(path.resolve('modules/users/server/models/user.server.model.js')).init(),
  Schema = mongoose.Schema;

var model = {
  schema: getSchema,
  init: initModel
};

module.exports = model;

/**
 * Initialize Article Model
 */
function initModel (schema) {
  schema = schema || getSchema();

  return getModelOrCreate(schema);
}

function getModelOrCreate (schema) {
  try {
    return mongoose.model('Article');    
  } catch (e) {
    mongoose.model('Article', schema);
    return mongoose.model('Article');
  }
}

/**
 * Article Schema
 */
function getSchema () {

  var schema = new Schema({
    created: {
      type: Date,
      default: Date.now
    },
    title: {
      type: String,
      default: '',
      trim: true,
      required: 'Title cannot be blank'
    },
    content: {
      type: String,
      default: '',
      trim: true
    },
    user: {
      type: Schema.ObjectId,
      ref: 'User'
    }
  });

  return schema;
}

Notice my intention of being able to ensure a model has been registered with Mongoose, and loading it into a different model for use. However, I ran into weird behavior with doing this. You can see the implementation here mleanos@9d55a8e and this is where the problem comes in mleanos@9d55a8e#commitcomment-14810936

[lirantal]

I'll quickly sum up my points here:

• I don't think we should go to length of privacy/EULA/etc. Remember that MEAN.JS is a framework, it's not a website, not a service, and not a product you buy and sell to someone else. If we had cared so much about privacy then what about when deleting a user we should also begin cleaning the logs that MEAN.JS keep? it will never end. With that said, we should definitely keep in mind security and privacy so let's find a good alternative.
• There should be no tight-coupling whatsoever between the User model/controller to any other module like Articles. The reason for that is that the User module is meant to be used as a core component of MEAN.JS which users can build on, while the Articles module serves as a reference for a good example. We don't, and shouldn't be coupling these two.

[mleanos]

I agree with your points @lirantal I got a little carried away with all that model craziness that I was experimenting with :)

I think these are our main options..

1. Let's not worry about the orphaned User references. On the client-side we'll just display "DELETED USER" when we don't have a populated User field in the Article (because it couldn't be found with the populate method). And most importantly, we'll make sure to handle the missing User reference on the back-end, so that we don't throw this server error.

2. On the client-side callback, from the User delete operation, we perform the clean-up by finding Articles that are related to the User & then remove them. This way we don't have any server-side dependency between Articles & Users.

3. Soft delete of the User, and not worry about the privacy issue. The reference will still be intact, but we can remove any sensitive data. But keep the Display Name, or update it to DELETED USER. This idea doesn't seem to be gaining any traction but I think it should still be on the table.

4. Delete the User, and all Articles owned by them. I'm not a real fan of this idea, but it's an option if we care so much about privacy.

[mleanos]

I should clarify in the 2nd option above, I meant that we should remove the reference to the deleted user from the article, and not remove the Article.

[lirantal]

IMO it's either option (1) or (3).
I'm good with either, because basically this is only related to the Articles module which only serves as an example module.

[mleanos]

@codydaig @jloveland @ilanbiala Any feedback?

[codydaig]

So I am a fan of #3, however, I agree with @lirantal either (1) or (3).
If we do #3, I think that we should be handling the missing user reference on the back-end.

[jloveland]

I like option 3. When soft-deleting the user, I think we should remove sensitive data where possible. Additionally, since, we are not completely deleting the user, maybe we should update the display name to DISABLED_USER? Rationale about making the user's display name disabled is that the user can no longer register themselves again with that email address and suggesting it was deleted implies the user can re-register one day..right?

In the case that we disable the user, maybe in a future PR, we provide a way to re-enable their account?

i.e. When a user tries to re-register/sign up with that email address, we could check if the user's account has DISABLED_USER in the display name, it could display a warning at the bottom saying the user's account is disabled and they will receive an email to re-enable it, then the server could send them an email asking if they want to enable their account..they click on a url to confirm..

[jloveland]

Per my last comment, maybe we delete the users email address too?

[lirantal]

Having a soft delete complicates things a bit like that because we need to "manage" the deleted users.
if we don't delete the user's e-mail address then they won't be able to sign-up again, and will basically be blocked.

To avoid over-complicating it I suggest we take the simpler approach out of the possible options. Looks to me like it's option (1) ?

[mleanos]

Agreed. I'll start working on option 1.

Thanks guys!