From 930d9429efa0138da7625a5270b4eba3ead890ec Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Wed, 10 Feb 2021 12:43:53 -0800 Subject: [PATCH 1/3] User save() override instead of signal to set user fields this gets gnarly because of transaction.atomic, so it bears further testing --- bookwyrm/models/user.py | 80 ++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index da717d2e1..7b0f9a912 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -6,7 +6,6 @@ from django.apps import apps from django.contrib.auth.models import AbstractUser from django.core.validators import MinValueValidator from django.db import models -from django.dispatch import receiver from django.utils import timezone from bookwyrm import activitypub @@ -172,15 +171,23 @@ class User(OrderedCollectionPageMixin, AbstractUser): def save(self, *args, **kwargs): ''' populate fields for new local users ''' - # this user already exists, no need to populate fields if not self.local and not re.match(regex.full_username, self.username): # generate a username that uses the domain (webfinger format) actor_parts = urlparse(self.remote_id) self.username = '%s@%s' % (self.username, actor_parts.netloc) - return super().save(*args, **kwargs) + super().save(*args, **kwargs) + return - if self.id or not self.local: - return super().save(*args, **kwargs) + # this user already exists, no need to populate fields + if self.id: + super().save(*args, **kwargs) + return + + # this is a new remote user, we need to set their remote server field + if not self.local: + super().save(*args, **kwargs) + set_remote_server.delay(self.id) + return # populate fields for local users self.remote_id = 'https://%s/user/%s' % (DOMAIN, self.localname) @@ -188,7 +195,32 @@ class User(OrderedCollectionPageMixin, AbstractUser): self.shared_inbox = 'https://%s/inbox' % DOMAIN self.outbox = '%s/outbox' % self.remote_id - return super().save(*args, **kwargs) + # an id needs to be set before we can proceed with related models + super().save(*args, **kwargs) + + # create keys and shelves for new local users + self.key_pair = KeyPair.objects.create( + remote_id='%s/#main-key' % self.remote_id) + self.save(broadcast=False) + + shelves = [{ + 'name': 'To Read', + 'identifier': 'to-read', + }, { + 'name': 'Currently Reading', + 'identifier': 'reading', + }, { + 'name': 'Read', + 'identifier': 'read', + }] + + for shelf in shelves: + Shelf( + name=shelf['name'], + identifier=shelf['identifier'], + user=self, + editable=False + ).save(broadcast=False) @property def local_path(self): @@ -280,42 +312,6 @@ class AnnualGoal(BookWyrmModel): finish_date__year__gte=self.year).count() - -@receiver(models.signals.post_save, sender=User) -#pylint: disable=unused-argument -def execute_after_save(sender, instance, created, *args, **kwargs): - ''' create shelves for new users ''' - if not created: - return - - if not instance.local: - set_remote_server.delay(instance.id) - return - - instance.key_pair = KeyPair.objects.create( - remote_id='%s/#main-key' % instance.remote_id) - instance.save(broadcast=False) - - shelves = [{ - 'name': 'To Read', - 'identifier': 'to-read', - }, { - 'name': 'Currently Reading', - 'identifier': 'reading', - }, { - 'name': 'Read', - 'identifier': 'read', - }] - - for shelf in shelves: - Shelf( - name=shelf['name'], - identifier=shelf['identifier'], - user=instance, - editable=False - ).save(broadcast=False) - - @app.task def set_remote_server(user_id): ''' figure out the user's remote server in the background ''' From 65f81bd5f0fb5c6075ba5bb39b008066c6ae11c3 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 15 Feb 2021 12:21:48 -0800 Subject: [PATCH 2/3] Moves blocking to save function I just like these better than signals?? --- bookwyrm/incoming.py | 2 +- bookwyrm/models/relationship.py | 28 +++++++++++----------------- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/bookwyrm/incoming.py b/bookwyrm/incoming.py index 562225e79..cb5baafc0 100644 --- a/bookwyrm/incoming.py +++ b/bookwyrm/incoming.py @@ -195,7 +195,7 @@ def handle_block(activity): ''' blocking a user ''' # create "block" databse entry activitypub.Block(**activity).to_model(models.UserBlocks) - # the removing relationships is handled in post-save hook in model + # the removing relationships is handled in model save @app.task diff --git a/bookwyrm/models/relationship.py b/bookwyrm/models/relationship.py index 9f3bf07d1..6aaffae99 100644 --- a/bookwyrm/models/relationship.py +++ b/bookwyrm/models/relationship.py @@ -1,7 +1,6 @@ ''' defines relationships between users ''' from django.db import models, transaction from django.db.models import Q -from django.dispatch import receiver from bookwyrm import activitypub from .activitypub_mixin import ActivitypubMixin, ActivityMixin @@ -126,20 +125,15 @@ class UserBlocks(ActivityMixin, UserRelationship): status = 'blocks' activity_serializer = activitypub.Block + def save(self, *args, **kwargs): + ''' remove follow or follow request rels after a block is created ''' + super().save(*args, **kwargs) -@receiver(models.signals.post_save, sender=UserBlocks) -#pylint: disable=unused-argument -def execute_after_save(sender, instance, created, *args, **kwargs): - ''' remove follow or follow request rels after a block is created ''' - UserFollows.objects.filter( - Q(user_subject=instance.user_subject, - user_object=instance.user_object) | \ - Q(user_subject=instance.user_object, - user_object=instance.user_subject) - ).delete() - UserFollowRequest.objects.filter( - Q(user_subject=instance.user_subject, - user_object=instance.user_object) | \ - Q(user_subject=instance.user_object, - user_object=instance.user_subject) - ).delete() + UserFollows.objects.filter( + Q(user_subject=self.user_subject, user_object=self.user_object) | \ + Q(user_subject=self.user_object, user_object=self.user_subject) + ).delete() + UserFollowRequest.objects.filter( + Q(user_subject=self.user_subject, user_object=self.user_object) | \ + Q(user_subject=self.user_object, user_object=self.user_subject) + ).delete() From f974b9b89538f226a30c4854fa3947f51ff05e28 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 15 Feb 2021 12:51:34 -0800 Subject: [PATCH 3/3] Better blocking checks --- bookwyrm/models/relationship.py | 48 ++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/bookwyrm/models/relationship.py b/bookwyrm/models/relationship.py index 044b08acc..f4df3b83f 100644 --- a/bookwyrm/models/relationship.py +++ b/bookwyrm/models/relationship.py @@ -1,6 +1,6 @@ ''' defines relationships between users ''' from django.apps import apps -from django.db import models, transaction +from django.db import models, transaction, IntegrityError from django.db.models import Q from bookwyrm import activitypub @@ -60,6 +60,20 @@ class UserFollows(ActivitypubMixin, UserRelationship): status = 'follows' activity_serializer = activitypub.Follow + def save(self, *args, **kwargs): + ''' really really don't let a user follow someone who blocked them ''' + # blocking in either direction is a no-go + if UserBlocks.objects.filter( + Q( + user_subject=self.user_subject, + user_object=self.user_object, + ) | Q( + user_subject=self.user_object, + user_object=self.user_subject, + ) + ).exists(): + raise IntegrityError() + super().save(*args, **kwargs) @classmethod def from_request(cls, follow_request): @@ -78,23 +92,25 @@ class UserFollowRequest(ActivitypubMixin, UserRelationship): def save(self, *args, broadcast=True, **kwargs): ''' make sure the follow or block relationship doesn't already exist ''' - try: - UserFollows.objects.get( + # don't create a request if a follow already exists + if UserFollows.objects.filter( user_subject=self.user_subject, user_object=self.user_object, - ) - # blocking in either direction is a no-go - UserBlocks.objects.get( - user_subject=self.user_subject, - user_object=self.user_object, - ) - UserBlocks.objects.get( - user_subject=self.user_object, - user_object=self.user_subject, - ) - return None - except (UserFollows.DoesNotExist, UserBlocks.DoesNotExist): - super().save(*args, **kwargs) + ).exists(): + raise IntegrityError() + # blocking in either direction is a no-go + if UserBlocks.objects.filter( + Q( + user_subject=self.user_subject, + user_object=self.user_object, + ) | Q( + user_subject=self.user_object, + user_object=self.user_subject, + ) + ).exists(): + raise IntegrityError() + + super().save(*args, **kwargs) if broadcast and self.user_subject.local and not self.user_object.local: self.broadcast(self.to_activity(), self.user_subject)