Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Soft delete users #3726

Merged
merged 7 commits into from
Dec 15, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions contentcuration/contentcuration/constants/user_history.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from django.utils.translation import ugettext_lazy as _

DELETION = "deletion"
RECOVERY = "recovery"
RELATED_DATA_HARD_DELETION = "related-data-hard-deletion"

choices = (
(DELETION, _("User soft deletion")),
(RECOVERY, _("User soft deletion recovery")),
(RELATED_DATA_HARD_DELETION, _("User related data hard deletion")),
)
3 changes: 2 additions & 1 deletion contentcuration/contentcuration/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from django.contrib.auth.forms import UserChangeForm
from django.contrib.auth.forms import UserCreationForm
from django.core import signing
from django.db.models import Q
from django.template.loader import render_to_string

from contentcuration.models import User
Expand Down Expand Up @@ -45,7 +46,7 @@ class RegistrationForm(UserCreationForm, ExtraFormMixin):

def clean_email(self):
email = self.cleaned_data['email'].strip().lower()
if User.objects.filter(email__iexact=email, is_active=True).exists():
if User.objects.filter(Q(is_active=True) | Q(deleted_at__isnull=False), email__iexact=email).exists():
raise UserWarning
return email

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from contentcuration.utils.garbage_collect import clean_up_contentnodes
from contentcuration.utils.garbage_collect import clean_up_deleted_chefs
from contentcuration.utils.garbage_collect import clean_up_feature_flags
from contentcuration.utils.garbage_collect import clean_up_soft_deleted_users
from contentcuration.utils.garbage_collect import clean_up_stale_files
from contentcuration.utils.garbage_collect import clean_up_tasks

Expand All @@ -26,15 +27,23 @@ def handle(self, *args, **options):
Actual logic for garbage collection.
"""

# clean up contentnodes, files and file objects on storage that are associated
# with the orphan tree
# Clean up users that are soft deleted and are older than ACCOUNT_DELETION_BUFFER (90 days).
# Also clean contentnodes, files and file objects on storage that are associated
# with the orphan tree.
logging.info("Cleaning up soft deleted users older than ACCOUNT_DELETION_BUFFER (90 days)")
clean_up_soft_deleted_users()

logging.info("Cleaning up contentnodes from the orphan tree")
clean_up_contentnodes()

logging.info("Cleaning up deleted chef nodes")
clean_up_deleted_chefs()

logging.info("Cleaning up feature flags")
clean_up_feature_flags()

logging.info("Cleaning up stale file objects")
clean_up_stale_files()

logging.info("Cleaning up tasks")
clean_up_tasks()
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# Generated by Django 3.2.14 on 2022-10-19 09:41
import django.db.models.deletion
import django.utils.timezone
from django.conf import settings
from django.db import migrations
from django.db import models


class Migration(migrations.Migration):

dependencies = [
('contentcuration', '0140_delete_task'),
]

operations = [
migrations.AddField(
model_name='user',
name='deleted_at',
vkWeb marked this conversation as resolved.
Show resolved Hide resolved
field=models.DateTimeField(blank=True, null=True),
),
migrations.CreateModel(
name='UserHistory',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('action', models.CharField(choices=[('deletion', 'User soft deletion'), ('recovery', 'User soft deletion recovery'),
('related-data-hard-deletion', 'User related data hard deletion')], max_length=32)),
('performed_at', models.DateTimeField(default=django.utils.timezone.now)),
('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='history', to=settings.AUTH_USER_MODEL)),
],
),
]
73 changes: 64 additions & 9 deletions contentcuration/contentcuration/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@

from contentcuration.constants import channel_history
from contentcuration.constants import completion_criteria
from contentcuration.constants import user_history
from contentcuration.constants.contentnode import kind_activity_map
from contentcuration.db.models.expressions import Array
from contentcuration.db.models.functions import ArrayRemove
Expand Down Expand Up @@ -199,6 +200,7 @@ class User(AbstractBaseUser, PermissionsMixin):
content_defaults = JSONField(default=dict)
policies = JSONField(default=dict, null=True)
feature_flags = JSONField(default=dict, null=True)
deleted_at = models.DateTimeField(null=True, blank=True)

_field_updates = FieldTracker(fields=[
# Field to watch for changes
Expand All @@ -213,27 +215,67 @@ def __unicode__(self):
return self.email

def delete(self):
"""
Soft deletes the user account.
"""
self.deleted_at = timezone.now()
# Deactivate the user to disallow authentication and also
# to let the user verify the email again after recovery.
self.is_active = False
self.save()
self.history.create(user_id=self.pk, action=user_history.DELETION)

def recover(self):
"""
Use this method when we want to recover a user.
"""
self.deleted_at = None
self.save()
self.history.create(user_id=self.pk, action=user_history.RECOVERY)

def hard_delete_user_related_data(self):
"""
Hard delete all user related data. But keeps the user record itself intact.

User related data that gets hard deleted are:
- sole editor non-public channels.
- sole editor non-public channelsets.
- sole editor non-public channels' content nodes and its underlying files that are not
used by any other channel.
- all user invitations.
"""
from contentcuration.viewsets.common import SQCount
# Remove any invitations associated to this account

# Hard delete invitations associated to this account.
self.sent_to.all().delete()
self.sent_by.all().delete()

# Delete channels associated with this user (if user is the only editor)
user_query = (
editable_channels_user_query = (
User.objects.filter(editable_channels__id=OuterRef('id'))
.values_list('id', flat=True)
.distinct()
)
self.editable_channels.annotate(num_editors=SQCount(user_query, field="id")).filter(num_editors=1).delete()
non_public_channels_sole_editor = self.editable_channels.annotate(num_editors=SQCount(
editable_channels_user_query, field="id")).filter(num_editors=1, public=False)

# Point sole editor non-public channels' contentnodes to orphan tree to let
# our garbage collection delete the nodes and underlying files.
ContentNode._annotate_channel_id(ContentNode.objects).filter(channel_id__in=list(
non_public_channels_sole_editor)).update(parent_id=settings.ORPHANAGE_ROOT_ID)

# Delete channel collections associated with this user (if user is the only editor)
# Hard delete non-public channels associated with this user (if user is the only editor).
non_public_channels_sole_editor.delete()

# Hard delete non-public channel collections associated with this user (if user is the only editor).
user_query = (
User.objects.filter(channel_sets__id=OuterRef('id'))
.values_list('id', flat=True)
.distinct()
)
self.channel_sets.annotate(num_editors=SQCount(user_query, field="id")).filter(num_editors=1).delete()
self.channel_sets.annotate(num_editors=SQCount(user_query, field="id")).filter(num_editors=1, public=False).delete()

super(User, self).delete()
# Create history!
self.history.create(user_id=self.pk, action=user_history.RELATED_DATA_HARD_DELETION)

def can_edit(self, channel_id):
return Channel.filter_edit_queryset(Channel.objects.all(), self).filter(pk=channel_id).exists()
Expand Down Expand Up @@ -405,17 +447,20 @@ def filter_edit_queryset(cls, queryset, user):
return queryset.filter(pk=user.pk)

@classmethod
def get_for_email(cls, email, **filters):
def get_for_email(cls, email, deleted_at__isnull=True, **filters):
"""
Returns the appropriate User record given an email, ordered by:
- those with is_active=True first, which there should only ever be one
- otherwise by ID DESC so most recent inactive shoud be returned

Filters out deleted User records by default. Can be overridden with
deleted_at__isnull argument.

:param email: A string of the user's email
:param filters: Additional filters to filter the User queryset
:return: User or None
"""
return User.objects.filter(email__iexact=email.strip(), **filters)\
return User.objects.filter(email__iexact=email.strip(), deleted_at__isnull=deleted_at__isnull, **filters)\
.order_by("-is_active", "-id").first()


Expand Down Expand Up @@ -1038,6 +1083,16 @@ class Meta:
]


class UserHistory(models.Model):
"""
Model that stores the user's action history.
"""
user = models.ForeignKey(settings.AUTH_USER_MODEL, null=False, blank=False, related_name="history", on_delete=models.CASCADE)
action = models.CharField(max_length=32, choices=user_history.choices)

performed_at = models.DateTimeField(default=timezone.now)


class ChannelSet(models.Model):
# NOTE: this is referred to as "channel collections" on the front-end, but we need to call it
# something else as there is already a ChannelCollection model on the front-end
Expand Down
6 changes: 4 additions & 2 deletions contentcuration/contentcuration/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,10 @@ def gettext(s):
HELP_EMAIL = '[email protected]'
DEFAULT_FROM_EMAIL = 'Kolibri Studio <[email protected]>'
POLICY_EMAIL = '[email protected]'
ACCOUNT_DELETION_BUFFER = 5 # Used to determine how many days a user
# has to undo accidentally deleting account

# Used to determine how many days a user
# has to undo accidentally deleting account.
ACCOUNT_DELETION_BUFFER = 90

DEFAULT_LICENSE = 1

Expand Down
33 changes: 33 additions & 0 deletions contentcuration/contentcuration/tests/test_models.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import datetime
import uuid

import mock
Expand All @@ -11,6 +12,7 @@
from le_utils.constants import format_presets

from contentcuration.constants import channel_history
from contentcuration.constants import user_history
from contentcuration.models import AssessmentItem
from contentcuration.models import Channel
from contentcuration.models import ChannelHistory
Expand All @@ -22,6 +24,7 @@
from contentcuration.models import Invitation
from contentcuration.models import object_storage_name
from contentcuration.models import User
from contentcuration.models import UserHistory
from contentcuration.tests import testdata
from contentcuration.tests.base import StudioTestCase

Expand Down Expand Up @@ -787,6 +790,7 @@ def test_get_for_email(self):
user1 = self._create_user("[email protected]", is_active=False)
user2 = self._create_user("[email protected]", is_active=False)
user3 = self._create_user("[email protected]", is_active=True)
user4 = self._create_user("[email protected]", is_active=True)

# active should be returned first
self.assertEqual(user3, User.get_for_email("[email protected]"))
Expand All @@ -801,6 +805,35 @@ def test_get_for_email(self):
# ensure nothing found doesn't error
self.assertIsNone(User.get_for_email("[email protected]"))

# ensure we don't return soft-deleted users
user4.delete()
self.assertIsNone(User.get_for_email("[email protected]"))

def test_delete(self):
user = self._create_user("[email protected]")
user.delete()

# Sets deleted_at?
self.assertIsInstance(user.deleted_at, datetime.datetime)
# Sets is_active to False?
self.assertEqual(user.is_active, False)
# Creates user history?
user_delete_history = UserHistory.objects.filter(user_id=user.id, action=user_history.DELETION).first()
self.assertIsNotNone(user_delete_history)

def test_recover(self):
user = self._create_user("[email protected]")
user.delete()
user.recover()

# Sets deleted_at to None?
self.assertEqual(user.deleted_at, None)
# Keeps is_active to False?
self.assertEqual(user.is_active, False)
# Creates user history?
user_recover_history = UserHistory.objects.filter(user_id=user.id, action=user_history.RECOVERY).first()
self.assertIsNotNone(user_recover_history)


class ChannelHistoryTestCase(StudioTestCase):
def setUp(self):
Expand Down
29 changes: 18 additions & 11 deletions contentcuration/contentcuration/tests/test_settings.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import json

import mock
from django.urls import reverse_lazy

from .base import BaseAPITestCase
Expand All @@ -10,7 +11,7 @@

class SettingsTestCase(BaseAPITestCase):
def test_username_change(self):
data = json.dumps({"first_name": "New firstname", "last_name": "New lastname",})
data = json.dumps({"first_name": "New firstname", "last_name": "New lastname", })
request = self.create_post_request(
reverse_lazy("update_user_full_name"),
data=data,
Expand Down Expand Up @@ -40,13 +41,19 @@ def test_delete_account_invalid(self):
self.assertTrue(User.objects.filter(email=self.user.email).exists())

def test_delete_account(self):
# TODO: send_email causes connection errors
data = json.dumps({"email": self.user.email})
self.create_post_request(
reverse_lazy("delete_user_account"),
data=data,
content_type="application/json",
)
# response = DeleteAccountView.as_view()(request)
# self.assertEqual(response.status_code, 200)
# self.assertFalse(User.objects.filter(email=self.user.email).exists())
with mock.patch("contentcuration.views.users.djangologout") as djangologout:
self.user.delete = mock.Mock()
data = json.dumps({"email": self.user.email})
request = self.create_post_request(
reverse_lazy("delete_user_account"),
data=data,
content_type="application/json",
)
response = DeleteAccountView.as_view()(request)

# Ensure successful response.
self.assertEqual(response.status_code, 200)
# Ensure user's delete method is called.
self.user.delete.assert_called_once()
# Ensure we logout the user.
djangologout.assert_called_once_with(request)
14 changes: 14 additions & 0 deletions contentcuration/contentcuration/tests/views/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,14 @@ def test_login__whitespace(self):
self.assertIsInstance(redirect, HttpResponseRedirectBase)
self.assertIn("channels", redirect['Location'])

def test_after_delete__no_login(self):
with mock.patch("contentcuration.views.users.djangologin") as djangologin:
self.user.delete()
response = login(self.request)

self.assertIsInstance(response, HttpResponseForbidden)
djangologin.assert_not_called()


class UserRegistrationViewTestCase(StudioAPITestCase):
def setUp(self):
Expand All @@ -121,6 +129,12 @@ def test_post__no_duplicate_registration(self):
response = self.view.post(self.request)
self.assertIsInstance(response, HttpResponseForbidden)

def test_after_delete__no_registration(self):
user = testdata.user(email="[email protected]")
user.delete()
response = self.view.post(self.request)
self.assertIsInstance(response, HttpResponseForbidden)


class UserActivationViewTestCase(StudioAPITestCase):
def setUp(self):
Expand Down
Loading