From f085315d70d7cbb7f772b51ef6cb8cce8d6f42a2 Mon Sep 17 00:00:00 2001 From: Giebisch Date: Mon, 30 Jan 2023 16:12:14 +0100 Subject: [PATCH 01/12] Added Backend Part --- bookwyrm/forms/status.py | 1 + .../migrations/0174_auto_20230130_1240.py | 26 +++++++++++++++++++ bookwyrm/models/status.py | 3 +++ .../snippets/create_status/quotation.html | 13 ++++++++++ .../snippets/status/content_status.html | 4 +-- 5 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 bookwyrm/migrations/0174_auto_20230130_1240.py diff --git a/bookwyrm/forms/status.py b/bookwyrm/forms/status.py index 0800166bf..b562595ee 100644 --- a/bookwyrm/forms/status.py +++ b/bookwyrm/forms/status.py @@ -53,6 +53,7 @@ class QuotationForm(CustomForm): "sensitive", "privacy", "position", + "endposition", "position_mode", ] diff --git a/bookwyrm/migrations/0174_auto_20230130_1240.py b/bookwyrm/migrations/0174_auto_20230130_1240.py new file mode 100644 index 000000000..1b81c2087 --- /dev/null +++ b/bookwyrm/migrations/0174_auto_20230130_1240.py @@ -0,0 +1,26 @@ +# Generated by Django 3.2.16 on 2023-01-30 12:40 + +import django.core.validators +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('auth', '0012_alter_user_first_name_max_length'), + ('bookwyrm', '0173_default_user_auth_group_setting'), + ] + + operations = [ + migrations.AddField( + model_name='quotation', + name='endposition', + field=models.IntegerField(blank=True, null=True, validators=[django.core.validators.MinValueValidator(0)]), + ), + migrations.AlterField( + model_name='sitesettings', + name='default_user_auth_group', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.PROTECT, to='auth.group'), + ), + ] diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 19eab584d..d5dc8e9c1 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -329,6 +329,9 @@ class Quotation(BookStatus): position = models.IntegerField( validators=[MinValueValidator(0)], null=True, blank=True ) + endposition = models.IntegerField( + validators=[MinValueValidator(0)], null=True, blank=True + ) position_mode = models.CharField( max_length=3, choices=ProgressMode.choices, diff --git a/bookwyrm/templates/snippets/create_status/quotation.html b/bookwyrm/templates/snippets/create_status/quotation.html index a9ddb17f4..9cc76d5a9 100644 --- a/bookwyrm/templates/snippets/create_status/quotation.html +++ b/bookwyrm/templates/snippets/create_status/quotation.html @@ -65,6 +65,19 @@ uuid: a unique identifier used to make html "id" attributes unique and clarify j {% if not draft %}data-cache-draft="id_position_{{ book.id }}_{{ type }}"{% endif %} > +
+ +
{% endblock %} diff --git a/bookwyrm/templates/snippets/status/content_status.html b/bookwyrm/templates/snippets/status/content_status.html index e39284fcf..4c4d89341 100644 --- a/bookwyrm/templates/snippets/status/content_status.html +++ b/bookwyrm/templates/snippets/status/content_status.html @@ -99,9 +99,9 @@ — {% include 'snippets/book_titleby.html' with book=status.book %} {% if status.position %} {% if status.position_mode == 'PG' %} - {% blocktrans with page=status.position|intcomma %}(Page {{ page }}){% endblocktrans %} + {% blocktrans with page=status.position|intcomma %}(Page {{ page }}{% endblocktrans%}{% if status.endposition%} - {% blocktrans with endpage=status.endposition|intcomma %}{{ endpage }}{% endblocktrans %}{% endif%}) {% else %} - {% blocktrans with percent=status.position %}({{ percent }}%){% endblocktrans %} + {% blocktrans with percent=status.position %}({{ percent }}%{% endblocktrans %}{% if status.endposition%}{% blocktrans with endpercent=status.endposition|intcomma %} - {{ endpercent }}%{% endblocktrans %}{% endif %}) {% endif %} {% endif %}

From f65e0b7632b4c5f17cdd6335e4e0a1aa466532bd Mon Sep 17 00:00:00 2001 From: Giebisch Date: Mon, 6 Feb 2023 14:00:04 +0100 Subject: [PATCH 02/12] Add Quotation endposition test --- .../migrations/0174_auto_20230130_1240.py | 25 +++++++++++------ bookwyrm/tests/views/books/test_book.py | 27 +++++++++++++++++++ 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/bookwyrm/migrations/0174_auto_20230130_1240.py b/bookwyrm/migrations/0174_auto_20230130_1240.py index 1b81c2087..7337b6b46 100644 --- a/bookwyrm/migrations/0174_auto_20230130_1240.py +++ b/bookwyrm/migrations/0174_auto_20230130_1240.py @@ -8,19 +8,28 @@ import django.db.models.deletion class Migration(migrations.Migration): dependencies = [ - ('auth', '0012_alter_user_first_name_max_length'), - ('bookwyrm', '0173_default_user_auth_group_setting'), + ("auth", "0012_alter_user_first_name_max_length"), + ("bookwyrm", "0173_default_user_auth_group_setting"), ] operations = [ migrations.AddField( - model_name='quotation', - name='endposition', - field=models.IntegerField(blank=True, null=True, validators=[django.core.validators.MinValueValidator(0)]), + model_name="quotation", + name="endposition", + field=models.IntegerField( + blank=True, + null=True, + validators=[django.core.validators.MinValueValidator(0)], + ), ), migrations.AlterField( - model_name='sitesettings', - name='default_user_auth_group', - field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.PROTECT, to='auth.group'), + model_name="sitesettings", + name="default_user_auth_group", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + to="auth.group", + ), ), ] diff --git a/bookwyrm/tests/views/books/test_book.py b/bookwyrm/tests/views/books/test_book.py index e536ef736..dfa68a7e4 100644 --- a/bookwyrm/tests/views/books/test_book.py +++ b/bookwyrm/tests/views/books/test_book.py @@ -257,6 +257,33 @@ class BookViews(TestCase): self.assertEqual(mock.call_args[0][0], "https://openlibrary.org/book/123") self.assertEqual(result.status_code, 302) + @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async") + @patch("bookwyrm.activitystreams.add_status_task.delay") + def test_quotation_endposition(self, *_): + """make sure the endposition is served as well""" + view = views.Book.as_view() + + quote = models.Quotation.objects.create( + user=self.local_user, + book=self.book, + content="hi", + quote="wow", + position=12, + endposition=13, + ) + + request = self.factory.get("") + request.user = self.local_user + + with patch("bookwyrm.views.books.books.is_api_request") as is_api: + is_api.return_value = False + result = view(request, self.book.id, user_statuses="quotation") + self.assertIsInstance(result, TemplateResponse) + validate_html(result.render()) + print(result.render()) + self.assertEqual(result.status_code, 200) + self.assertEqual(result.context_data["statuses"].object_list[0].endposition, 13) + def _setup_cover_url(): """creates cover url mock""" From 21575fbf3f8e2c08c0ef9c7551978764606890a3 Mon Sep 17 00:00:00 2001 From: Giebisch Date: Mon, 6 Feb 2023 14:09:53 +0100 Subject: [PATCH 03/12] Unused variable fix --- bookwyrm/tests/views/books/test_book.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookwyrm/tests/views/books/test_book.py b/bookwyrm/tests/views/books/test_book.py index dfa68a7e4..a829c4a4b 100644 --- a/bookwyrm/tests/views/books/test_book.py +++ b/bookwyrm/tests/views/books/test_book.py @@ -263,7 +263,7 @@ class BookViews(TestCase): """make sure the endposition is served as well""" view = views.Book.as_view() - quote = models.Quotation.objects.create( + _ = models.Quotation.objects.create( user=self.local_user, book=self.book, content="hi", From 5bae00b3fe1ce4c31ac9d6881844133e3fc6f725 Mon Sep 17 00:00:00 2001 From: Chris Young Date: Thu, 9 Feb 2023 12:49:05 +0000 Subject: [PATCH 04/12] Expand TOTP validity window --- bookwyrm/forms/landing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookwyrm/forms/landing.py b/bookwyrm/forms/landing.py index bd9884bc3..132f3ba71 100644 --- a/bookwyrm/forms/landing.py +++ b/bookwyrm/forms/landing.py @@ -108,7 +108,7 @@ class Confirm2FAForm(CustomForm): otp = self.data.get("otp") totp = pyotp.TOTP(self.instance.otp_secret) - if not totp.verify(otp): + if not totp.verify(otp, valid_window=2): if self.instance.hotp_secret: # maybe it's a backup code? From 867b2ff542ddbd5c171570cc35795b6a4fd7c674 Mon Sep 17 00:00:00 2001 From: Chris Young Date: Mon, 13 Feb 2023 15:17:54 +0000 Subject: [PATCH 05/12] Specify TOTP validity window in settings.py --- bookwyrm/forms/landing.py | 3 ++- bookwyrm/settings.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/bookwyrm/forms/landing.py b/bookwyrm/forms/landing.py index 132f3ba71..1da4fc4f1 100644 --- a/bookwyrm/forms/landing.py +++ b/bookwyrm/forms/landing.py @@ -8,6 +8,7 @@ import pyotp from bookwyrm import models from bookwyrm.settings import DOMAIN +from bookwyrm.settings import TWO_FACTOR_LOGIN_VALIDITY_WINDOW from .custom_form import CustomForm @@ -108,7 +109,7 @@ class Confirm2FAForm(CustomForm): otp = self.data.get("otp") totp = pyotp.TOTP(self.instance.otp_secret) - if not totp.verify(otp, valid_window=2): + if not totp.verify(otp, valid_window=TWO_FACTOR_LOGIN_VALIDITY_WINDOW): if self.instance.hotp_secret: # maybe it's a backup code? diff --git a/bookwyrm/settings.py b/bookwyrm/settings.py index 61240dbfa..d8c554742 100644 --- a/bookwyrm/settings.py +++ b/bookwyrm/settings.py @@ -369,6 +369,7 @@ OTEL_EXPORTER_OTLP_HEADERS = env("OTEL_EXPORTER_OTLP_HEADERS", None) OTEL_SERVICE_NAME = env("OTEL_SERVICE_NAME", None) TWO_FACTOR_LOGIN_MAX_SECONDS = 60 +TWO_FACTOR_LOGIN_VALIDITY_WINDOW = 2 HTTP_X_FORWARDED_PROTO = env.bool("SECURE_PROXY_SSL_HEADER", False) if HTTP_X_FORWARDED_PROTO: From 94605530867bbe522fa0300cc47661612676b632 Mon Sep 17 00:00:00 2001 From: Chris Young Date: Fri, 17 Feb 2023 09:40:31 +0000 Subject: [PATCH 06/12] Read TOTP variables from .env --- .env.example | 6 ++++++ bookwyrm/settings.py | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.env.example b/.env.example index 4c1c2eefe..b47c683c5 100644 --- a/.env.example +++ b/.env.example @@ -120,3 +120,9 @@ OTEL_SERVICE_NAME= # for your instance: # https://docs.djangoproject.com/en/3.2/ref/settings/#secure-proxy-ssl-header HTTP_X_FORWARDED_PROTO=false + +# TOTP settings +# TWO_FACTOR_LOGIN_VALIDITY_WINDOW sets the number of codes either side +# which will be accepted. +TWO_FACTOR_LOGIN_VALIDITY_WINDOW=2 +TWO_FACTOR_LOGIN_MAX_SECONDS=60 diff --git a/bookwyrm/settings.py b/bookwyrm/settings.py index d8c554742..fc83fba9b 100644 --- a/bookwyrm/settings.py +++ b/bookwyrm/settings.py @@ -368,8 +368,8 @@ OTEL_EXPORTER_OTLP_ENDPOINT = env("OTEL_EXPORTER_OTLP_ENDPOINT", None) OTEL_EXPORTER_OTLP_HEADERS = env("OTEL_EXPORTER_OTLP_HEADERS", None) OTEL_SERVICE_NAME = env("OTEL_SERVICE_NAME", None) -TWO_FACTOR_LOGIN_MAX_SECONDS = 60 -TWO_FACTOR_LOGIN_VALIDITY_WINDOW = 2 +TWO_FACTOR_LOGIN_MAX_SECONDS = env.int("TWO_FACTOR_LOGIN_MAX_SECONDS") +TWO_FACTOR_LOGIN_VALIDITY_WINDOW = env.int("TWO_FACTOR_LOGIN_VALIDITY_WINDOW") HTTP_X_FORWARDED_PROTO = env.bool("SECURE_PROXY_SSL_HEADER", False) if HTTP_X_FORWARDED_PROTO: From d123cc6b0c52db4b20ecec45bec64b51b94ca96c Mon Sep 17 00:00:00 2001 From: Chris Young Date: Fri, 17 Feb 2023 11:36:21 +0000 Subject: [PATCH 07/12] Add default values if not in .env --- bookwyrm/settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bookwyrm/settings.py b/bookwyrm/settings.py index fc83fba9b..51f3fe8a7 100644 --- a/bookwyrm/settings.py +++ b/bookwyrm/settings.py @@ -368,8 +368,8 @@ OTEL_EXPORTER_OTLP_ENDPOINT = env("OTEL_EXPORTER_OTLP_ENDPOINT", None) OTEL_EXPORTER_OTLP_HEADERS = env("OTEL_EXPORTER_OTLP_HEADERS", None) OTEL_SERVICE_NAME = env("OTEL_SERVICE_NAME", None) -TWO_FACTOR_LOGIN_MAX_SECONDS = env.int("TWO_FACTOR_LOGIN_MAX_SECONDS") -TWO_FACTOR_LOGIN_VALIDITY_WINDOW = env.int("TWO_FACTOR_LOGIN_VALIDITY_WINDOW") +TWO_FACTOR_LOGIN_MAX_SECONDS = env.int("TWO_FACTOR_LOGIN_MAX_SECONDS", 60) +TWO_FACTOR_LOGIN_VALIDITY_WINDOW = env.int("TWO_FACTOR_LOGIN_VALIDITY_WINDOW", 2) HTTP_X_FORWARDED_PROTO = env.bool("SECURE_PROXY_SSL_HEADER", False) if HTTP_X_FORWARDED_PROTO: From 779d2b06941931ccfbc749c3d3c4d46451f18f3c Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 20 Feb 2023 10:32:03 -0800 Subject: [PATCH 08/12] Attempt to complete inbox requests synchronously When an inbox activity comes in from another fediverse instance, the behavior prior to this commit was always to immediately give a 200 response to the external server and then create a celery activity (usually in the MEDIUM_PRIORITY queue) to complete it. Instead, this would receive a request and try to complete it without making any http requests (which would make the request take too long to process). If an external request is required to complete the activity, a task is created and added to the queue. Ideally, this will cause some tasks to happen very promptly, and reduce the load on celery, which would help queued tasks happen more quickly as well. One downside is that this will make completing http requests from external servers slowing (since it's doing a bunch of thinking before responding). --- bookwyrm/activitypub/base_activity.py | 62 ++++++++++++++++++++++--- bookwyrm/activitypub/verbs.py | 65 +++++++++++++++++--------- bookwyrm/models/fields.py | 66 +++++++++++++++++++-------- bookwyrm/views/inbox.py | 15 +++++- 4 files changed, 160 insertions(+), 48 deletions(-) diff --git a/bookwyrm/activitypub/base_activity.py b/bookwyrm/activitypub/base_activity.py index 02e5395fc..6751f9c8e 100644 --- a/bookwyrm/activitypub/base_activity.py +++ b/bookwyrm/activitypub/base_activity.py @@ -100,9 +100,27 @@ class ActivityObject: # pylint: disable=too-many-locals,too-many-branches,too-many-arguments def to_model( - self, model=None, instance=None, allow_create=True, save=True, overwrite=True + self, + model=None, + instance=None, + allow_create=True, + save=True, + overwrite=True, + allow_external_connections=True, ): - """convert from an activity to a model instance""" + """convert from an activity to a model instance. Args: + model: the django model that this object is being converted to + (will guess if not known) + instance: an existing database entry that is going to be updated by + this activity + allow_create: whether a new object should be created if there is no + existing object is provided or found matching the remote_id + save: store in the database if true, return an unsaved model obj if false + overwrite: replace fields in the database with this activity if true, + only update blank fields if false + allow_external_connections: look up missing data if true, + throw an exception if false and an external connection is needed + """ model = model or get_model_from_type(self.type) # only reject statuses if we're potentially creating them @@ -127,7 +145,10 @@ class ActivityObject: for field in instance.simple_fields: try: changed = field.set_field_from_activity( - instance, self, overwrite=overwrite + instance, + self, + overwrite=overwrite, + allow_external_connections=allow_external_connections, ) if changed: update_fields.append(field.name) @@ -138,7 +159,11 @@ class ActivityObject: # too early and jank up users for field in instance.image_fields: changed = field.set_field_from_activity( - instance, self, save=save, overwrite=overwrite + instance, + self, + save=save, + overwrite=overwrite, + allow_external_connections=allow_external_connections, ) if changed: update_fields.append(field.name) @@ -162,7 +187,11 @@ class ActivityObject: # add many to many fields, which have to be set post-save for field in instance.many_to_many_fields: # mention books/users, for example - field.set_field_from_activity(instance, self) + field.set_field_from_activity( + instance, + self, + allow_external_connections=allow_external_connections, + ) # reversed relationships in the models for ( @@ -266,10 +295,22 @@ def get_model_from_type(activity_type): return model[0] +# pylint: disable=too-many-arguments def resolve_remote_id( - remote_id, model=None, refresh=False, save=True, get_activity=False + remote_id, + model=None, + refresh=False, + save=True, + get_activity=False, + allow_external_connections=True, ): - """take a remote_id and return an instance, creating if necessary""" + """take a remote_id and return an instance, creating if necessary. Args: + remote_id: the unique url for looking up the object in the db or by http + model: a string or object representing the model that corresponds to the object + save: whether to return an unsaved database entry or a saved one + get_activity: whether to return the activitypub object or the model object + allow_external_connections: whether to make http connections + """ if model: # a bonus check we can do if we already know the model if isinstance(model, str): model = apps.get_model(f"bookwyrm.{model}", require_ready=True) @@ -277,6 +318,13 @@ def resolve_remote_id( if result and not refresh: return result if not get_activity else result.to_activity_dataclass() + # The above block will return the object if it already exists in the database. + # If it doesn't, an external connection would be needed, so check if that's cool + if not allow_external_connections: + raise ActivitySerializerError( + "Unable to serialize object without making external HTTP requests" + ) + # load the data and create the object try: data = get_data(remote_id) diff --git a/bookwyrm/activitypub/verbs.py b/bookwyrm/activitypub/verbs.py index b8c0ae779..4b7514b5a 100644 --- a/bookwyrm/activitypub/verbs.py +++ b/bookwyrm/activitypub/verbs.py @@ -14,12 +14,12 @@ class Verb(ActivityObject): actor: str object: ActivityObject - def action(self): + def action(self, allow_external_connections=True): """usually we just want to update and save""" # self.object may return None if the object is invalid in an expected way # ie, Question type if self.object: - self.object.to_model() + self.object.to_model(allow_external_connections=allow_external_connections) # pylint: disable=invalid-name @@ -42,7 +42,7 @@ class Delete(Verb): cc: List[str] = field(default_factory=lambda: []) type: str = "Delete" - def action(self): + def action(self, allow_external_connections=True): """find and delete the activity object""" if not self.object: return @@ -52,7 +52,11 @@ class Delete(Verb): model = apps.get_model("bookwyrm.User") obj = model.find_existing_by_remote_id(self.object) else: - obj = self.object.to_model(save=False, allow_create=False) + obj = self.object.to_model( + save=False, + allow_create=False, + allow_external_connections=allow_external_connections, + ) if obj: obj.delete() @@ -67,11 +71,13 @@ class Update(Verb): to: List[str] type: str = "Update" - def action(self): + def action(self, allow_external_connections=True): """update a model instance from the dataclass""" if not self.object: return - self.object.to_model(allow_create=False) + self.object.to_model( + allow_create=False, allow_external_connections=allow_external_connections + ) @dataclass(init=False) @@ -80,7 +86,7 @@ class Undo(Verb): type: str = "Undo" - def action(self): + def action(self, allow_external_connections=True): """find and remove the activity object""" if isinstance(self.object, str): # it may be that something should be done with these, but idk what @@ -92,13 +98,28 @@ class Undo(Verb): model = None if self.object.type == "Follow": model = apps.get_model("bookwyrm.UserFollows") - obj = self.object.to_model(model=model, save=False, allow_create=False) + obj = self.object.to_model( + model=model, + save=False, + allow_create=False, + allow_external_connections=allow_external_connections, + ) if not obj: # this could be a follow request not a follow proper model = apps.get_model("bookwyrm.UserFollowRequest") - obj = self.object.to_model(model=model, save=False, allow_create=False) + obj = self.object.to_model( + model=model, + save=False, + allow_create=False, + allow_external_connections=allow_external_connections, + ) else: - obj = self.object.to_model(model=model, save=False, allow_create=False) + obj = self.object.to_model( + model=model, + save=False, + allow_create=False, + allow_external_connections=allow_external_connections, + ) if not obj: # if we don't have the object, we can't undo it. happens a lot with boosts return @@ -112,9 +133,9 @@ class Follow(Verb): object: str type: str = "Follow" - def action(self): + def action(self, allow_external_connections=True): """relationship save""" - self.to_model() + self.to_model(allow_external_connections=allow_external_connections) @dataclass(init=False) @@ -124,9 +145,9 @@ class Block(Verb): object: str type: str = "Block" - def action(self): + def action(self, allow_external_connections=True): """relationship save""" - self.to_model() + self.to_model(allow_external_connections=allow_external_connections) @dataclass(init=False) @@ -136,7 +157,7 @@ class Accept(Verb): object: Follow type: str = "Accept" - def action(self): + def action(self, allow_external_connections=True): """accept a request""" obj = self.object.to_model(save=False, allow_create=True) obj.accept() @@ -149,7 +170,7 @@ class Reject(Verb): object: Follow type: str = "Reject" - def action(self): + def action(self, allow_external_connections=True): """reject a follow request""" obj = self.object.to_model(save=False, allow_create=False) obj.reject() @@ -163,7 +184,7 @@ class Add(Verb): object: CollectionItem type: str = "Add" - def action(self): + def action(self, allow_external_connections=True): """figure out the target to assign the item to a collection""" target = resolve_remote_id(self.target) item = self.object.to_model(save=False) @@ -177,7 +198,7 @@ class Remove(Add): type: str = "Remove" - def action(self): + def action(self, allow_external_connections=True): """find and remove the activity object""" obj = self.object.to_model(save=False, allow_create=False) if obj: @@ -191,9 +212,9 @@ class Like(Verb): object: str type: str = "Like" - def action(self): + def action(self, allow_external_connections=True): """like""" - self.to_model() + self.to_model(allow_external_connections=allow_external_connections) # pylint: disable=invalid-name @@ -207,6 +228,6 @@ class Announce(Verb): object: str type: str = "Announce" - def action(self): + def action(self, allow_external_connections=True): """boost""" - self.to_model() + self.to_model(allow_external_connections=allow_external_connections) diff --git a/bookwyrm/models/fields.py b/bookwyrm/models/fields.py index d11f5fb1d..8a12736c9 100644 --- a/bookwyrm/models/fields.py +++ b/bookwyrm/models/fields.py @@ -67,7 +67,9 @@ class ActivitypubFieldMixin: self.activitypub_field = activitypub_field super().__init__(*args, **kwargs) - def set_field_from_activity(self, instance, data, overwrite=True): + def set_field_from_activity( + self, instance, data, overwrite=True, allow_external_connections=True + ): """helper function for assinging a value to the field. Returns if changed""" try: value = getattr(data, self.get_activitypub_field()) @@ -76,7 +78,9 @@ class ActivitypubFieldMixin: if self.get_activitypub_field() != "attributedTo": raise value = getattr(data, "actor") - formatted = self.field_from_activity(value) + formatted = self.field_from_activity( + value, allow_external_connections=allow_external_connections + ) if formatted is None or formatted is MISSING or formatted == {}: return False @@ -116,7 +120,7 @@ class ActivitypubFieldMixin: return {self.activitypub_wrapper: value} return value - def field_from_activity(self, value): + def field_from_activity(self, value, allow_external_connections=True): """formatter to convert activitypub into a model value""" if value and hasattr(self, "activitypub_wrapper"): value = value.get(self.activitypub_wrapper) @@ -138,7 +142,7 @@ class ActivitypubRelatedFieldMixin(ActivitypubFieldMixin): self.load_remote = load_remote super().__init__(*args, **kwargs) - def field_from_activity(self, value): + def field_from_activity(self, value, allow_external_connections=True): if not value: return None @@ -159,7 +163,11 @@ class ActivitypubRelatedFieldMixin(ActivitypubFieldMixin): if not self.load_remote: # only look in the local database return related_model.find_existing_by_remote_id(value) - return activitypub.resolve_remote_id(value, model=related_model) + return activitypub.resolve_remote_id( + value, + model=related_model, + allow_external_connections=allow_external_connections, + ) class RemoteIdField(ActivitypubFieldMixin, models.CharField): @@ -219,7 +227,9 @@ class PrivacyField(ActivitypubFieldMixin, models.CharField): super().__init__(*args, max_length=255, choices=PrivacyLevels, default="public") # pylint: disable=invalid-name - def set_field_from_activity(self, instance, data, overwrite=True): + def set_field_from_activity( + self, instance, data, overwrite=True, allow_external_connections=True + ): if not overwrite: return False @@ -234,7 +244,11 @@ class PrivacyField(ActivitypubFieldMixin, models.CharField): break if not user_field: raise ValidationError("No user field found for privacy", data) - user = activitypub.resolve_remote_id(getattr(data, user_field), model="User") + user = activitypub.resolve_remote_id( + getattr(data, user_field), + model="User", + allow_external_connections=allow_external_connections, + ) if to == [self.public]: setattr(instance, self.name, "public") @@ -295,13 +309,17 @@ class ManyToManyField(ActivitypubFieldMixin, models.ManyToManyField): self.link_only = link_only super().__init__(*args, **kwargs) - def set_field_from_activity(self, instance, data, overwrite=True): + def set_field_from_activity( + self, instance, data, overwrite=True, allow_external_connections=True + ): """helper function for assigning a value to the field""" if not overwrite and getattr(instance, self.name).exists(): return False value = getattr(data, self.get_activitypub_field()) - formatted = self.field_from_activity(value) + formatted = self.field_from_activity( + value, allow_external_connections=allow_external_connections + ) if formatted is None or formatted is MISSING: return False getattr(instance, self.name).set(formatted) @@ -313,7 +331,7 @@ class ManyToManyField(ActivitypubFieldMixin, models.ManyToManyField): return f"{value.instance.remote_id}/{self.name}" return [i.remote_id for i in value.all()] - def field_from_activity(self, value): + def field_from_activity(self, value, allow_external_connections=True): if value is None or value is MISSING: return None if not isinstance(value, list): @@ -326,7 +344,11 @@ class ManyToManyField(ActivitypubFieldMixin, models.ManyToManyField): except ValidationError: continue items.append( - activitypub.resolve_remote_id(remote_id, model=self.related_model) + activitypub.resolve_remote_id( + remote_id, + model=self.related_model, + allow_external_connections=allow_external_connections, + ) ) return items @@ -353,7 +375,7 @@ class TagField(ManyToManyField): ) return tags - def field_from_activity(self, value): + def field_from_activity(self, value, allow_external_connections=True): if not isinstance(value, list): return None items = [] @@ -366,7 +388,11 @@ class TagField(ManyToManyField): # tags can contain multiple types continue items.append( - activitypub.resolve_remote_id(link.href, model=self.related_model) + activitypub.resolve_remote_id( + link.href, + model=self.related_model, + allow_external_connections=allow_external_connections, + ) ) return items @@ -391,10 +417,14 @@ class ImageField(ActivitypubFieldMixin, models.ImageField): super().__init__(*args, **kwargs) # pylint: disable=arguments-differ,arguments-renamed - def set_field_from_activity(self, instance, data, save=True, overwrite=True): + def set_field_from_activity( + self, instance, data, save=True, overwrite=True, allow_external_connections=True + ): """helper function for assinging a value to the field""" value = getattr(data, self.get_activitypub_field()) - formatted = self.field_from_activity(value) + formatted = self.field_from_activity( + value, allow_external_connections=allow_external_connections + ) if formatted is None or formatted is MISSING: return False @@ -426,7 +456,7 @@ class ImageField(ActivitypubFieldMixin, models.ImageField): return activitypub.Document(url=url, name=alt) - def field_from_activity(self, value): + def field_from_activity(self, value, allow_external_connections=True): image_slug = value # when it's an inline image (User avatar/icon, Book cover), it's a json # blob, but when it's an attached image, it's just a url @@ -481,7 +511,7 @@ class DateTimeField(ActivitypubFieldMixin, models.DateTimeField): return None return value.isoformat() - def field_from_activity(self, value): + def field_from_activity(self, value, allow_external_connections=True): try: date_value = dateutil.parser.parse(value) try: @@ -495,7 +525,7 @@ class DateTimeField(ActivitypubFieldMixin, models.DateTimeField): class HtmlField(ActivitypubFieldMixin, models.TextField): """a text field for storing html""" - def field_from_activity(self, value): + def field_from_activity(self, value, allow_external_connections=True): if not value or value == MISSING: return None return clean(value) diff --git a/bookwyrm/views/inbox.py b/bookwyrm/views/inbox.py index 747b5eccd..9b2b238f8 100644 --- a/bookwyrm/views/inbox.py +++ b/bookwyrm/views/inbox.py @@ -64,7 +64,7 @@ class Inbox(View): high = ["Follow", "Accept", "Reject", "Block", "Unblock", "Undo"] priority = HIGH if activity_json["type"] in high else MEDIUM - activity_task.apply_async(args=(activity_json,), queue=priority) + sometimes_async_activity_task(activity_json, queue=priority) return HttpResponse() @@ -102,6 +102,19 @@ def raise_is_blocked_activity(activity_json): raise PermissionDenied() +def sometimes_async_activity_task(activity_json, queue=MEDIUM): + """Sometimes we can effectively respond to a request without queuing a new task, + and whever that is possible, we should do it.""" + activity = activitypub.parse(activity_json) + + # try resolving this activity without making any http requests + try: + activity.action(allow_external_connections=False) + except activitypub.ActivitySerializerError: + # if that doesn't work, run it asynchronously + activity_task.apply_async(args=(activity_json,), queue=queue) + + @app.task(queue=MEDIUM) def activity_task(activity_json): """do something with this json we think is legit""" From 0211dee0ff5e6a9e49a366835fdb835a15e1c55b Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 20 Feb 2023 11:09:42 -0800 Subject: [PATCH 09/12] Avoid unnecessary errors when a remote re-sends an Accept --- bookwyrm/models/relationship.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bookwyrm/models/relationship.py b/bookwyrm/models/relationship.py index c8a508117..422967855 100644 --- a/bookwyrm/models/relationship.py +++ b/bookwyrm/models/relationship.py @@ -171,7 +171,11 @@ class UserFollowRequest(ActivitypubMixin, UserRelationship): return with transaction.atomic(): - UserFollows.from_request(self) + try: + UserFollows.from_request(self) + except IntegrityError: + # this just means we already saved this relationship + pass if self.id: self.delete() From 12ed0f46f37ea2714c7512dffa7c2f03b26d2e5e Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 20 Feb 2023 12:23:18 -0800 Subject: [PATCH 10/12] Fixes mocks for tests --- bookwyrm/tests/test_signing.py | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/bookwyrm/tests/test_signing.py b/bookwyrm/tests/test_signing.py index 8a7f65249..cde193f08 100644 --- a/bookwyrm/tests/test_signing.py +++ b/bookwyrm/tests/test_signing.py @@ -95,7 +95,8 @@ class Signature(TestCase): def test_correct_signature(self): """this one should just work""" - response = self.send_test_request(sender=self.mouse) + with patch("bookwyrm.models.relationship.UserFollowRequest.accept"): + response = self.send_test_request(sender=self.mouse) self.assertEqual(response.status_code, 200) def test_wrong_signature(self): @@ -124,8 +125,12 @@ class Signature(TestCase): ) with patch("bookwyrm.models.user.get_remote_reviews.delay"): - response = self.send_test_request(sender=self.fake_remote) + with patch( + "bookwyrm.models.relationship.UserFollowRequest.accept" + ) as accept_mock: + response = self.send_test_request(sender=self.fake_remote) self.assertEqual(response.status_code, 200) + self.assertTrue(accept_mock.called) @responses.activate def test_key_needs_refresh(self): @@ -148,16 +153,28 @@ class Signature(TestCase): with patch("bookwyrm.models.user.get_remote_reviews.delay"): # Key correct: - response = self.send_test_request(sender=self.fake_remote) + with patch( + "bookwyrm.models.relationship.UserFollowRequest.accept" + ) as accept_mock: + response = self.send_test_request(sender=self.fake_remote) self.assertEqual(response.status_code, 200) + self.assertTrue(accept_mock.called) # Old key is cached, so still works: - response = self.send_test_request(sender=self.fake_remote) + with patch( + "bookwyrm.models.relationship.UserFollowRequest.accept" + ) as accept_mock: + response = self.send_test_request(sender=self.fake_remote) self.assertEqual(response.status_code, 200) + self.assertTrue(accept_mock.called) # Try with new key: - response = self.send_test_request(sender=new_sender) + with patch( + "bookwyrm.models.relationship.UserFollowRequest.accept" + ) as accept_mock: + response = self.send_test_request(sender=new_sender) self.assertEqual(response.status_code, 200) + self.assertTrue(accept_mock.called) # Now the old key will fail: response = self.send_test_request(sender=self.fake_remote) From 216be2aeead0282a1159d56cbfb61336e46642ac Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 20 Feb 2023 12:24:53 -0800 Subject: [PATCH 11/12] Fixes pylint complaints "fixes" as in silences, sorry --- bookwyrm/models/fields.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bookwyrm/models/fields.py b/bookwyrm/models/fields.py index 8a12736c9..a970e4124 100644 --- a/bookwyrm/models/fields.py +++ b/bookwyrm/models/fields.py @@ -120,6 +120,7 @@ class ActivitypubFieldMixin: return {self.activitypub_wrapper: value} return value + # pylint: disable=unused-argument def field_from_activity(self, value, allow_external_connections=True): """formatter to convert activitypub into a model value""" if value and hasattr(self, "activitypub_wrapper"): @@ -416,7 +417,7 @@ class ImageField(ActivitypubFieldMixin, models.ImageField): self.alt_field = alt_field super().__init__(*args, **kwargs) - # pylint: disable=arguments-differ,arguments-renamed + # pylint: disable=arguments-differ,arguments-renamed,too-many-arguments def set_field_from_activity( self, instance, data, save=True, overwrite=True, allow_external_connections=True ): From 43fe4331338eee4468566cec16d76af086d84f4c Mon Sep 17 00:00:00 2001 From: Giebisch Date: Thu, 23 Feb 2023 18:40:20 +0100 Subject: [PATCH 12/12] Quotation same start and endposition --- bookwyrm/templates/snippets/create_status/quotation.html | 3 +++ bookwyrm/templates/snippets/status/content_status.html | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/bookwyrm/templates/snippets/create_status/quotation.html b/bookwyrm/templates/snippets/create_status/quotation.html index 9cc76d5a9..bd1d817ad 100644 --- a/bookwyrm/templates/snippets/create_status/quotation.html +++ b/bookwyrm/templates/snippets/create_status/quotation.html @@ -65,6 +65,9 @@ uuid: a unique identifier used to make html "id" attributes unique and clarify j {% if not draft %}data-cache-draft="id_position_{{ book.id }}_{{ type }}"{% endif %} > +
+ {% trans "to" %} +