From e89bfdc57252c4b479afe5849f0ab99be9598dfd Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Thu, 24 Dec 2020 11:39:27 -0800 Subject: [PATCH 01/11] Makes registration user/localname fields more sensible --- bookwyrm/forms.py | 2 +- .../migrations/0030_auto_20201224_1939.py | 19 +++++++++++++++++++ bookwyrm/models/fields.py | 6 ++---- bookwyrm/models/user.py | 8 +++----- .../templates/snippets/register_form.html | 6 +++--- bookwyrm/view_actions.py | 11 ++++++----- 6 files changed, 34 insertions(+), 18 deletions(-) create mode 100644 bookwyrm/migrations/0030_auto_20201224_1939.py diff --git a/bookwyrm/forms.py b/bookwyrm/forms.py index 686ac8b1d..557cb00b0 100644 --- a/bookwyrm/forms.py +++ b/bookwyrm/forms.py @@ -45,7 +45,7 @@ class LoginForm(CustomForm): class RegisterForm(CustomForm): class Meta: model = models.User - fields = ['username', 'email', 'password'] + fields = ['localname', 'email', 'password'] help_texts = {f: None for f in fields} widgets = { 'password': PasswordInput() diff --git a/bookwyrm/migrations/0030_auto_20201224_1939.py b/bookwyrm/migrations/0030_auto_20201224_1939.py new file mode 100644 index 000000000..6de5d37fb --- /dev/null +++ b/bookwyrm/migrations/0030_auto_20201224_1939.py @@ -0,0 +1,19 @@ +# Generated by Django 3.0.7 on 2020-12-24 19:39 + +import bookwyrm.models.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('bookwyrm', '0029_auto_20201221_2014'), + ] + + operations = [ + migrations.AlterField( + model_name='user', + name='localname', + field=models.CharField(max_length=255, null=True, unique=True, validators=[bookwyrm.models.fields.validate_localname]), + ), + ] diff --git a/bookwyrm/models/fields.py b/bookwyrm/models/fields.py index 6dd3b496b..2649bb591 100644 --- a/bookwyrm/models/fields.py +++ b/bookwyrm/models/fields.py @@ -26,11 +26,11 @@ def validate_remote_id(value): ) -def validate_username(value): +def validate_localname(value): ''' make sure usernames look okay ''' if not re.match(r'^[A-Za-z\-_\.]+$', value): raise ValidationError( - _('%(value)s is not a valid remote_id'), + _('%(value)s is not a valid username'), params={'value': value}, ) @@ -156,7 +156,6 @@ class UsernameField(ActivitypubFieldMixin, models.CharField): _('username'), max_length=150, unique=True, - validators=[validate_username], error_messages={ 'unique': _('A user with that username already exists.'), }, @@ -168,7 +167,6 @@ class UsernameField(ActivitypubFieldMixin, models.CharField): del kwargs['verbose_name'] del kwargs['max_length'] del kwargs['unique'] - del kwargs['validators'] del kwargs['error_messages'] return name, path, args, kwargs diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 30eeffbc8..816625874 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -48,7 +48,8 @@ class User(OrderedCollectionPageMixin, AbstractUser): localname = models.CharField( max_length=255, null=True, - unique=True + unique=True, + validators=[fields.validate_localname], ) # name is your display name, which you can change at will name = fields.CharField(max_length=100, null=True, blank=True) @@ -157,10 +158,7 @@ class User(OrderedCollectionPageMixin, AbstractUser): return super().save(*args, **kwargs) # populate fields for local users - self.remote_id = 'https://%s/user/%s' % (DOMAIN, self.username) - self.localname = self.username - self.username = '%s@%s' % (self.username, DOMAIN) - self.actor = self.remote_id + self.remote_id = 'https://%s/user/%s' % (DOMAIN, self.localname) self.inbox = '%s/inbox' % self.remote_id self.shared_inbox = 'https://%s/inbox' % DOMAIN self.outbox = '%s/outbox' % self.remote_id diff --git a/bookwyrm/templates/snippets/register_form.html b/bookwyrm/templates/snippets/register_form.html index d6f5b98f8..213ad1e7a 100644 --- a/bookwyrm/templates/snippets/register_form.html +++ b/bookwyrm/templates/snippets/register_form.html @@ -1,10 +1,10 @@ {% csrf_token %}
- +
- +
- {% for error in register_form.username.errors %} + {% for error in register_form.localname.errors %}

{{ error | escape }}

{% endfor %}
diff --git a/bookwyrm/view_actions.py b/bookwyrm/view_actions.py index 78f762185..e126115b6 100644 --- a/bookwyrm/view_actions.py +++ b/bookwyrm/view_actions.py @@ -66,13 +66,13 @@ def register(request): if not form.is_valid(): errors = True - username = form.data['username'].strip() + localname = form.data['localname'].strip() email = form.data['email'] password = form.data['password'] - # check username and email uniqueness - if models.User.objects.filter(localname=username).first(): - form.add_error('username', 'User with this username already exists') + # check localname and email uniqueness + if models.User.objects.filter(localname=localname).first(): + form.add_error('localname', 'User with this username already exists') errors = True if errors: @@ -82,8 +82,9 @@ def register(request): } return TemplateResponse(request, 'login.html', data) + username = '%s@%s' % (localname, DOMAIN) user = models.User.objects.create_user( - username, email, password, local=True) + username, email, password, localname=localname, local=True) if invite: invite.times_used += 1 invite.save() From 7e987fc446516652f9b610e267b36cea2442fcf9 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 27 Dec 2020 15:18:48 -0800 Subject: [PATCH 02/11] Updates unit tests for new username handling still some failing tho --- .../tests/activitypub/test_base_activity.py | 3 ++- bookwyrm/tests/models/test_base_model.py | 18 +++++++++----- bookwyrm/tests/models/test_fields.py | 24 ++++++++++++------- bookwyrm/tests/models/test_import_model.py | 3 ++- .../tests/models/test_relationship_models.py | 3 ++- bookwyrm/tests/models/test_shelf_model.py | 3 ++- bookwyrm/tests/models/test_status_model.py | 3 ++- bookwyrm/tests/models/test_user_model.py | 3 ++- bookwyrm/tests/test_broadcast.py | 6 +++-- bookwyrm/tests/test_incoming.py | 3 ++- bookwyrm/tests/test_outgoing.py | 3 ++- bookwyrm/tests/test_signing.py | 9 ++++--- bookwyrm/tests/test_templatetags.py | 3 ++- bookwyrm/tests/test_view_actions.py | 23 +++++++++--------- 14 files changed, 68 insertions(+), 39 deletions(-) diff --git a/bookwyrm/tests/activitypub/test_base_activity.py b/bookwyrm/tests/activitypub/test_base_activity.py index 4cdc2c70f..8668c7e20 100644 --- a/bookwyrm/tests/activitypub/test_base_activity.py +++ b/bookwyrm/tests/activitypub/test_base_activity.py @@ -20,7 +20,8 @@ class BaseActivity(TestCase): def setUp(self): ''' we're probably going to re-use this so why copy/paste ''' self.user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') self.user.remote_id = 'http://example.com/a/b' self.user.save() diff --git a/bookwyrm/tests/models/test_base_model.py b/bookwyrm/tests/models/test_base_model.py index 65cf892e7..fea26b78e 100644 --- a/bookwyrm/tests/models/test_base_model.py +++ b/bookwyrm/tests/models/test_base_model.py @@ -22,7 +22,8 @@ class BaseModel(TestCase): def test_remote_id_with_user(self): ''' format of remote id when there's a user object ''' user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', local=True) + 'mouse', 'mouse@mouse.com', 'mouseword', + local=True, localname='mouse') instance = base_model.BookWyrmModel() instance.user = user instance.id = 1 @@ -51,7 +52,8 @@ class BaseModel(TestCase): def test_to_create_activity(self): ''' wrapper for ActivityPub "create" action ''' user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', local=True) + 'mouse', 'mouse@mouse.com', 'mouseword', + local=True, localname='mouse') object_activity = { 'to': 'to field', 'cc': 'cc field', @@ -81,7 +83,8 @@ class BaseModel(TestCase): def test_to_delete_activity(self): ''' wrapper for Delete activity ''' user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', local=True) + 'mouse', 'mouse@mouse.com', 'mouseword', + local=True, localname='mouse') MockSelf = namedtuple('Self', ('remote_id', 'to_activity')) mock_self = MockSelf( @@ -105,7 +108,8 @@ class BaseModel(TestCase): def test_to_update_activity(self): ''' ditto above but for Update ''' user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', local=True) + 'mouse', 'mouse@mouse.com', 'mouseword', + local=True, localname='mouse') MockSelf = namedtuple('Self', ('remote_id', 'to_activity')) mock_self = MockSelf( @@ -129,7 +133,8 @@ class BaseModel(TestCase): def test_to_undo_activity(self): ''' and again, for Undo ''' user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', local=True) + 'mouse', 'mouse@mouse.com', 'mouseword', + local=True, localname='mouse') MockSelf = namedtuple('Self', ('remote_id', 'to_activity')) mock_self = MockSelf( @@ -173,7 +178,8 @@ class BaseModel(TestCase): book = models.Edition.objects.create( title='Test Edition', remote_id='http://book.com/book') user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') user.remote_id = 'http://example.com/a/b' user.save() diff --git a/bookwyrm/tests/models/test_fields.py b/bookwyrm/tests/models/test_fields.py index 10c674d99..6e91cbc8d 100644 --- a/bookwyrm/tests/models/test_fields.py +++ b/bookwyrm/tests/models/test_fields.py @@ -173,7 +173,8 @@ class ActivitypubFields(TestCase): def test_privacy_field_set_activity_from_field(self): ''' translate between to/cc fields and privacy ''' user = User.objects.create_user( - 'rat', 'rat@rat.rat', 'ratword', local=True) + 'rat', 'rat@rat.rat', 'ratword', + local=True, localname='rat') public = 'https://www.w3.org/ns/activitystreams#Public' followers = '%s/followers' % user.remote_id @@ -230,7 +231,8 @@ class ActivitypubFields(TestCase): # it shouldn't match with this unrelated user: unrelated_user = User.objects.create_user( - 'rat', 'rat@rat.rat', 'ratword', local=True) + 'rat', 'rat@rat.rat', 'ratword', + local=True, localname='rat') # test receiving an unknown remote id and loading data responses.add( @@ -258,7 +260,8 @@ class ActivitypubFields(TestCase): # it shouldn't match with this unrelated user: unrelated_user = User.objects.create_user( - 'rat', 'rat@rat.rat', 'ratword', local=True) + 'rat', 'rat@rat.rat', 'ratword', + local=True, localname='rat') with patch('bookwyrm.models.user.set_remote_server.delay'): value = instance.field_from_activity(userdata) self.assertIsInstance(value, User) @@ -276,11 +279,13 @@ class ActivitypubFields(TestCase): ) userdata = json.loads(datafile.read_bytes()) user = User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') user.remote_id = 'https://example.com/user/mouse' user.save() User.objects.create_user( - 'rat', 'rat@rat.rat', 'ratword', local=True) + 'rat', 'rat@rat.rat', 'ratword', + local=True, localname='rat') value = instance.field_from_activity(userdata) self.assertEqual(value, user) @@ -290,9 +295,11 @@ class ActivitypubFields(TestCase): ''' test receiving a remote id of an existing object in the db ''' instance = fields.ForeignKey(User, on_delete=models.CASCADE) user = User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') User.objects.create_user( - 'rat', 'rat@rat.rat', 'ratword', local=True) + 'rat', 'rat@rat.rat', 'ratword', + local=True, localname='rat') value = instance.field_from_activity(user.remote_id) self.assertEqual(value, user) @@ -382,7 +389,8 @@ class ActivitypubFields(TestCase): def test_image_field(self): ''' storing images ''' user = User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') image_file = pathlib.Path(__file__).parent.joinpath( '../../static/images/default_avi.jpg') image = Image.open(image_file) diff --git a/bookwyrm/tests/models/test_import_model.py b/bookwyrm/tests/models/test_import_model.py index c703d08a4..c8e537129 100644 --- a/bookwyrm/tests/models/test_import_model.py +++ b/bookwyrm/tests/models/test_import_model.py @@ -52,7 +52,8 @@ class ImportJob(TestCase): unknown_read_data['Date Read'] = '' user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') job = models.ImportJob.objects.create(user=user) models.ImportItem.objects.create( job=job, index=1, data=currently_reading_data) diff --git a/bookwyrm/tests/models/test_relationship_models.py b/bookwyrm/tests/models/test_relationship_models.py index c5c619a02..a1232c1b3 100644 --- a/bookwyrm/tests/models/test_relationship_models.py +++ b/bookwyrm/tests/models/test_relationship_models.py @@ -16,7 +16,8 @@ class Relationship(TestCase): outbox='https://example.com/users/rat/outbox', ) self.local_user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', local=True) + 'mouse', 'mouse@mouse.com', 'mouseword', + local=True, localname='mouse') self.local_user.remote_id = 'http://local.com/user/mouse' self.local_user.save() diff --git a/bookwyrm/tests/models/test_shelf_model.py b/bookwyrm/tests/models/test_shelf_model.py index 9cdc7311b..2bce0a9c3 100644 --- a/bookwyrm/tests/models/test_shelf_model.py +++ b/bookwyrm/tests/models/test_shelf_model.py @@ -9,7 +9,8 @@ class Shelf(TestCase): def setUp(self): ''' look, a shelf ''' self.user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') self.shelf = models.Shelf.objects.create( name='Test Shelf', identifier='test-shelf', user=self.user) diff --git a/bookwyrm/tests/models/test_status_model.py b/bookwyrm/tests/models/test_status_model.py index d98b8b18d..a479a88b5 100644 --- a/bookwyrm/tests/models/test_status_model.py +++ b/bookwyrm/tests/models/test_status_model.py @@ -16,7 +16,8 @@ class Status(TestCase): def setUp(self): ''' useful things for creating a status ''' self.user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') self.book = models.Edition.objects.create(title='Test Edition') image_file = pathlib.Path(__file__).parent.joinpath( diff --git a/bookwyrm/tests/models/test_user_model.py b/bookwyrm/tests/models/test_user_model.py index 0454fb400..b7a41eabe 100644 --- a/bookwyrm/tests/models/test_user_model.py +++ b/bookwyrm/tests/models/test_user_model.py @@ -9,7 +9,8 @@ from bookwyrm.settings import DOMAIN class User(TestCase): def setUp(self): self.user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse@%s' % DOMAIN, 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') def test_computed_fields(self): ''' username instead of id here ''' diff --git a/bookwyrm/tests/test_broadcast.py b/bookwyrm/tests/test_broadcast.py index b80512193..0150fca82 100644 --- a/bookwyrm/tests/test_broadcast.py +++ b/bookwyrm/tests/test_broadcast.py @@ -7,10 +7,12 @@ from bookwyrm import models, broadcast class Book(TestCase): def setUp(self): self.user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') local_follower = models.User.objects.create_user( - 'joe', 'joe@mouse.mouse', 'jeoword', local=True) + 'joe', 'joe@mouse.mouse', 'jeoword', + local=True, localname='joe') self.user.followers.add(local_follower) with patch('bookwyrm.models.user.set_remote_server.delay'): diff --git a/bookwyrm/tests/test_incoming.py b/bookwyrm/tests/test_incoming.py index 0269c64ca..b2ba610e6 100644 --- a/bookwyrm/tests/test_incoming.py +++ b/bookwyrm/tests/test_incoming.py @@ -19,7 +19,8 @@ class Incoming(TestCase): def setUp(self): ''' we need basic things, like users ''' self.local_user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', local=True) + 'mouse', 'mouse@mouse.com', 'mouseword', + local=True, localname='mouse') self.local_user.remote_id = 'https://example.com/user/mouse' self.local_user.save() with patch('bookwyrm.models.user.set_remote_server.delay'): diff --git a/bookwyrm/tests/test_outgoing.py b/bookwyrm/tests/test_outgoing.py index 2c1d119cc..6b5d09e7c 100644 --- a/bookwyrm/tests/test_outgoing.py +++ b/bookwyrm/tests/test_outgoing.py @@ -23,7 +23,8 @@ class Outgoing(TestCase): outbox='https://example.com/users/rat/outbox', ) self.local_user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', local=True, + 'mouse', 'mouse@mouse.com', 'mouseword', + local=True, localname='mouse', remote_id='https://example.com/users/mouse', ) diff --git a/bookwyrm/tests/test_signing.py b/bookwyrm/tests/test_signing.py index bf2527647..0d55893d0 100644 --- a/bookwyrm/tests/test_signing.py +++ b/bookwyrm/tests/test_signing.py @@ -31,11 +31,14 @@ Sender = namedtuple('Sender', ('remote_id', 'key_pair')) class Signature(TestCase): def setUp(self): self.mouse = User.objects.create_user( - 'mouse', 'mouse@example.com', '', local=True) + 'mouse@%s' % DOMAIN, 'mouse@example.com', '', + local=True, localname='mouse') self.rat = User.objects.create_user( - 'rat', 'rat@example.com', '', local=True) + 'rat@%s' % DOMAIN, 'rat@example.com', '', + local=True, localname='rat') self.cat = User.objects.create_user( - 'cat', 'cat@example.com', '', local=True) + 'cat@%s' % DOMAIN, 'cat@example.com', '', + local=True, localname='cat') private_key, public_key = create_key_pair() diff --git a/bookwyrm/tests/test_templatetags.py b/bookwyrm/tests/test_templatetags.py index 6956553e5..004dd8930 100644 --- a/bookwyrm/tests/test_templatetags.py +++ b/bookwyrm/tests/test_templatetags.py @@ -16,7 +16,8 @@ class TemplateTags(TestCase): def setUp(self): ''' create some filler objects ''' self.user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'mouseword', local=True) + 'mouse@example.com', 'mouse@mouse.mouse', 'mouseword', + local=True, localname='mouse') with patch('bookwyrm.models.user.set_remote_server.delay'): self.remote_user = models.User.objects.create_user( 'rat', 'rat@rat.rat', 'ratword', diff --git a/bookwyrm/tests/test_view_actions.py b/bookwyrm/tests/test_view_actions.py index c846c2481..db227d137 100644 --- a/bookwyrm/tests/test_view_actions.py +++ b/bookwyrm/tests/test_view_actions.py @@ -18,7 +18,8 @@ class ViewActions(TestCase): def setUp(self): ''' we need basic things, like users ''' self.local_user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', local=True) + 'mouse', 'mouse@mouse.com', 'mouseword', + local=True, localname='mouse') self.local_user.remote_id = 'https://example.com/user/mouse' self.local_user.save() self.group = Group.objects.create(name='editor') @@ -51,7 +52,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nutria-user.user_nutria', + 'localname': 'nutria-user.user_nutria', 'password': 'mouseword', 'email': 'aa@bb.cccc' }) @@ -69,7 +70,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nutria ', + 'localname': 'nutria ', 'password': 'mouseword', 'email': 'aa@bb.ccc' }) @@ -88,7 +89,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nutria', + 'localname': 'nutria', 'password': 'mouseword', 'email': 'aa' }) @@ -102,7 +103,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nut@ria', + 'localname': 'nut@ria', 'password': 'mouseword', 'email': 'aa@bb.ccc' }) @@ -113,7 +114,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nutr ia', + 'localname': 'nutr ia', 'password': 'mouseword', 'email': 'aa@bb.ccc' }) @@ -124,7 +125,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nut@ria', + 'localname': 'nut@ria', 'password': 'mouseword', 'email': 'aa@bb.ccc' }) @@ -140,7 +141,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nutria ', + 'localname': 'nutria ', 'password': 'mouseword', 'email': 'aa@bb.ccc' }) @@ -158,7 +159,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nutria', + 'localname': 'nutria', 'password': 'mouseword', 'email': 'aa@bb.ccc', 'invite_code': 'testcode' @@ -173,7 +174,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nutria2', + 'localname': 'nutria2', 'password': 'mouseword', 'email': 'aa@bb.ccc', 'invite_code': 'testcode' @@ -185,7 +186,7 @@ class ViewActions(TestCase): request = self.factory.post( 'register/', { - 'username': 'nutria3', + 'localname': 'nutria3', 'password': 'mouseword', 'email': 'aa@bb.ccc', 'invite_code': 'dkfkdjgdfkjgkdfj' From 74a25f205bf398c5913728f61e7e6caaf03849b7 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 28 Dec 2020 14:14:22 -0800 Subject: [PATCH 03/11] Validator for username field --- bookwyrm/models/fields.py | 14 ++++++++++++-- bookwyrm/tests/models/test_fields.py | 14 ++++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/bookwyrm/models/fields.py b/bookwyrm/models/fields.py index 2649bb591..beddb2915 100644 --- a/bookwyrm/models/fields.py +++ b/bookwyrm/models/fields.py @@ -27,8 +27,17 @@ def validate_remote_id(value): def validate_localname(value): + ''' make sure localnames look okay ''' + if not re.match(r'^[A-Za-z\-_\.0-9]+$', value): + raise ValidationError( + _('%(value)s is not a valid username'), + params={'value': value}, + ) + + +def validate_username(value): ''' make sure usernames look okay ''' - if not re.match(r'^[A-Za-z\-_\.]+$', value): + if not re.match(r'^[A-Za-z\-_\.0-9]+@[A-Za-z\-_\.0-9]+\.[a-z]{2,}$', value): raise ValidationError( _('%(value)s is not a valid username'), params={'value': value}, @@ -147,7 +156,7 @@ class RemoteIdField(ActivitypubFieldMixin, models.CharField): class UsernameField(ActivitypubFieldMixin, models.CharField): ''' activitypub-aware username field ''' - def __init__(self, activitypub_field='preferredUsername'): + def __init__(self, activitypub_field='preferredUsername', **kwargs): self.activitypub_field = activitypub_field # I don't totally know why pylint is mad at this, but it makes it work super( #pylint: disable=bad-super-call @@ -156,6 +165,7 @@ class UsernameField(ActivitypubFieldMixin, models.CharField): _('username'), max_length=150, unique=True, + validators=[validate_username], error_messages={ 'unique': _('A user with that username already exists.'), }, diff --git a/bookwyrm/tests/models/test_fields.py b/bookwyrm/tests/models/test_fields.py index 6e91cbc8d..7935301e9 100644 --- a/bookwyrm/tests/models/test_fields.py +++ b/bookwyrm/tests/models/test_fields.py @@ -111,10 +111,16 @@ class ActivitypubFields(TestCase): self.assertEqual(instance.max_length, 150) self.assertEqual(instance.unique, True) with self.assertRaises(ValidationError): - instance.run_validators('one two') - instance.run_validators('a*&') - instance.run_validators('trailingwhite ') - self.assertIsNone(instance.run_validators('aksdhf')) + instance.run_validators('mouseexample.com') + instance.run_validators('mouse@example.c') + instance.run_validators('@example.com') + instance.run_validators('mouse@examplecom') + instance.run_validators('one two@fish.aaaa') + instance.run_validators('a*&@exampke.com') + instance.run_validators('trailingwhite@example.com ') + self.assertIsNone(instance.run_validators('mouse@example.com')) + self.assertIsNone(instance.run_validators('mo-2use@ex3ample.com')) + self.assertIsNone(instance.run_validators('aksdhf@sdkjf-df.cm')) self.assertEqual(instance.field_to_activity('test@example.com'), 'test') From 34e9847da39f8f3507464a7d1815190e5d8d34d6 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 4 Jan 2021 09:40:23 -0800 Subject: [PATCH 04/11] Still need to remove validator arg in username field --- bookwyrm/models/fields.py | 1 + 1 file changed, 1 insertion(+) diff --git a/bookwyrm/models/fields.py b/bookwyrm/models/fields.py index beddb2915..c6571ff4f 100644 --- a/bookwyrm/models/fields.py +++ b/bookwyrm/models/fields.py @@ -177,6 +177,7 @@ class UsernameField(ActivitypubFieldMixin, models.CharField): del kwargs['verbose_name'] del kwargs['max_length'] del kwargs['unique'] + del kwargs['validators'] del kwargs['error_messages'] return name, path, args, kwargs From e6f63951437ec111ba6b48a82e27200a09b75d91 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 4 Jan 2021 10:08:23 -0800 Subject: [PATCH 05/11] don't show coverage report when tests fail --- bw-dev | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bw-dev b/bw-dev index bf5e8f758..ec9627709 100755 --- a/bw-dev +++ b/bw-dev @@ -83,7 +83,7 @@ case "$1" in ;; pytest) shift 1 - execweb pytest "$@" + execweb pytest --no-cov-on-fail "$@" ;; test_report) execweb coverage report From 15b9f621361f64a1ec7e82179c6f0bf78108feb4 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 4 Jan 2021 10:08:45 -0800 Subject: [PATCH 06/11] Fixes users in views tests --- bookwyrm/tests/test_views.py | 38 +++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/bookwyrm/tests/test_views.py b/bookwyrm/tests/test_views.py index cc621eba7..e94affb22 100644 --- a/bookwyrm/tests/test_views.py +++ b/bookwyrm/tests/test_views.py @@ -9,6 +9,7 @@ from django.test import TestCase from django.test.client import RequestFactory from bookwyrm import models, views +from bookwyrm.activitypub import ActivitypubResponse from bookwyrm.connectors import abstract_connector from bookwyrm.settings import DOMAIN, USER_AGENT @@ -28,7 +29,8 @@ class Views(TestCase): local=True ) self.local_user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.mouse', 'password', local=True) + 'mouse@local.com', 'mouse@mouse.mouse', 'password', + local=True, localname='mouse') with patch('bookwyrm.models.user.set_remote_server.delay'): self.remote_user = models.User.objects.create_user( 'rat', 'rat@rat.com', 'ratword', @@ -52,7 +54,7 @@ class Views(TestCase): self.assertEqual( views.get_user_from_username('mouse'), self.local_user) self.assertEqual( - views.get_user_from_username('mouse@%s' % DOMAIN), self.local_user) + views.get_user_from_username('mouse@local.com'), self.local_user) with self.assertRaises(models.User.DoesNotExist): views.get_user_from_username('mojfse@example.com') @@ -343,7 +345,7 @@ class Views(TestCase): with patch('bookwyrm.views.is_api_request') as is_api: is_api.return_value = True result = views.user_page(request, 'mouse') - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) @@ -361,7 +363,7 @@ class Views(TestCase): with patch('bookwyrm.views.is_api_request') as is_api: is_api.return_value = True result = views.followers_page(request, 'mouse') - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) @@ -379,7 +381,7 @@ class Views(TestCase): with patch('bookwyrm.views.is_api_request') as is_api: is_api.return_value = True result = views.following_page(request, 'mouse') - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) @@ -399,7 +401,7 @@ class Views(TestCase): with patch('bookwyrm.views.is_api_request') as is_api: is_api.return_value = True result = views.status_page(request, 'mouse', status.id) - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) @@ -419,7 +421,7 @@ class Views(TestCase): with patch('bookwyrm.views.is_api_request') as is_api: is_api.return_value = True result = views.replies_page(request, 'mouse', status.id) - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) @@ -448,7 +450,7 @@ class Views(TestCase): with patch('bookwyrm.views.is_api_request') as is_api: is_api.return_value = True result = views.book_page(request, self.book.id) - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) @@ -489,7 +491,7 @@ class Views(TestCase): with patch('bookwyrm.views.is_api_request') as is_api: is_api.return_value = True result = views.editions_page(request, self.work.id) - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) @@ -508,7 +510,7 @@ class Views(TestCase): with patch('bookwyrm.views.is_api_request') as is_api: is_api.return_value = True result = views.author_page(request, author.id) - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) @@ -529,7 +531,7 @@ class Views(TestCase): with patch('bookwyrm.views.is_api_request') as is_api: is_api.return_value = True result = views.tag_page(request, tag.identifier) - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) @@ -550,18 +552,22 @@ class Views(TestCase): is_api.return_value = True result = views.shelf_page( request, self.local_user.username, shelf.identifier) - self.assertIsInstance(result, JsonResponse) + self.assertIsInstance(result, ActivitypubResponse) self.assertEqual(result.status_code, 200) def test_is_bookwyrm_request(self): - ''' tests the function that checks if a request came from a bookwyrm instance ''' + ''' checks if a request came from a bookwyrm instance ''' request = self.factory.get('', {'q': 'Test Book'}) self.assertFalse(views.is_bookworm_request(request)) - request = self.factory.get('', {'q': 'Test Book'}, - HTTP_USER_AGENT="http.rb/4.4.1 (Mastodon/3.3.0; +https://mastodon.social/)") + request = self.factory.get( + '', {'q': 'Test Book'}, + HTTP_USER_AGENT=\ + "http.rb/4.4.1 (Mastodon/3.3.0; +https://mastodon.social/)" + ) self.assertFalse(views.is_bookworm_request(request)) - request = self.factory.get('', {'q': 'Test Book'}, HTTP_USER_AGENT=USER_AGENT) + request = self.factory.get( + '', {'q': 'Test Book'}, HTTP_USER_AGENT=USER_AGENT) self.assertTrue(views.is_bookworm_request(request)) From 4090b336db90778438a4a01c3c3e12b11caeb871 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 4 Jan 2021 10:20:49 -0800 Subject: [PATCH 07/11] Fixes outgoing test users --- bookwyrm/tests/test_incoming.py | 4 +++- bookwyrm/tests/test_outgoing.py | 10 ++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/bookwyrm/tests/test_incoming.py b/bookwyrm/tests/test_incoming.py index 5cccbeb5d..980048bd4 100644 --- a/bookwyrm/tests/test_incoming.py +++ b/bookwyrm/tests/test_incoming.py @@ -19,7 +19,7 @@ class Incoming(TestCase): def setUp(self): ''' we need basic things, like users ''' self.local_user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', + 'mouse@example.com', 'mouse@mouse.com', 'mouseword', local=True, localname='mouse') self.local_user.remote_id = 'https://example.com/user/mouse' self.local_user.save() @@ -495,6 +495,8 @@ class Incoming(TestCase): incoming.handle_update_user({'object': userdata}) user = models.User.objects.get(id=self.local_user.id) self.assertEqual(user.name, 'MOUSE?? MOUSE!!') + self.assertEqual(user.username, 'mouse@example.com') + self.assertEqual(user.localname, 'mouse') def test_handle_update_edition(self): diff --git a/bookwyrm/tests/test_outgoing.py b/bookwyrm/tests/test_outgoing.py index caaba4e1e..344a374d4 100644 --- a/bookwyrm/tests/test_outgoing.py +++ b/bookwyrm/tests/test_outgoing.py @@ -21,14 +21,14 @@ class Outgoing(TestCase): self.factory = RequestFactory() with patch('bookwyrm.models.user.set_remote_server'): self.remote_user = models.User.objects.create_user( - 'rat', 'rat@rat.com', 'ratword', + 'rat', 'rat@email.com', 'ratword', local=False, remote_id='https://example.com/users/rat', inbox='https://example.com/users/rat/inbox', outbox='https://example.com/users/rat/outbox', ) self.local_user = models.User.objects.create_user( - 'mouse', 'mouse@mouse.com', 'mouseword', + 'mouse@local.com', 'mouse@mouse.com', 'mouseword', local=True, localname='mouse', remote_id='https://example.com/users/mouse', ) @@ -399,7 +399,8 @@ class Outgoing(TestCase): def test_handle_status_mentions(self): ''' @mention a user in a post ''' user = models.User.objects.create_user( - 'rat', 'rat@rat.com', 'password', local=True) + 'rat@local.com', 'rat@rat.com', 'password', + local=True, localname='rat') form = forms.CommentForm({ 'content': 'hi @rat', 'user': self.local_user.id, @@ -419,7 +420,8 @@ class Outgoing(TestCase): def test_handle_status_reply_with_mentions(self): ''' reply to a post with an @mention'ed user ''' user = models.User.objects.create_user( - 'rat', 'rat@rat.com', 'password', local=True) + 'rat@local.com', 'rat@rat.com', 'password', + local=True, localname='rat') form = forms.CommentForm({ 'content': 'hi @rat@example.com', 'user': self.local_user.id, From 3fe7b95786fc43f5369135c2ff4147d1effccc31 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 4 Jan 2021 10:42:31 -0800 Subject: [PATCH 08/11] Fixes setting remote user username on update --- bookwyrm/models/user.py | 10 ++++++---- bookwyrm/tests/models/test_fields.py | 1 + bookwyrm/tests/test_incoming.py | 4 ++++ 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index c9290e460..4cbe387f4 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -1,4 +1,5 @@ ''' database schema for user data ''' +import re from urllib.parse import urlparse from django.apps import apps @@ -13,6 +14,7 @@ from bookwyrm.models.status import Status, Review from bookwyrm.settings import DOMAIN from bookwyrm.signatures import create_key_pair from bookwyrm.tasks import app +from bookwyrm.utils import regex from .base_model import OrderedCollectionPageMixin from .base_model import ActivitypubMixin, BookWyrmModel from .federated_server import FederatedServer @@ -168,15 +170,15 @@ 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 self.id: - return super().save(*args, **kwargs) - - if not self.local: + 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) + if self.id or not self.local: + return super().save(*args, **kwargs) + # populate fields for local users self.remote_id = 'https://%s/user/%s' % (DOMAIN, self.localname) self.inbox = '%s/inbox' % self.remote_id diff --git a/bookwyrm/tests/models/test_fields.py b/bookwyrm/tests/models/test_fields.py index 7935301e9..e87855f67 100644 --- a/bookwyrm/tests/models/test_fields.py +++ b/bookwyrm/tests/models/test_fields.py @@ -111,6 +111,7 @@ class ActivitypubFields(TestCase): self.assertEqual(instance.max_length, 150) self.assertEqual(instance.unique, True) with self.assertRaises(ValidationError): + instance.run_validators('mouse') instance.run_validators('mouseexample.com') instance.run_validators('mouse@example.c') instance.run_validators('@example.com') diff --git a/bookwyrm/tests/test_incoming.py b/bookwyrm/tests/test_incoming.py index 980048bd4..5d8bd1107 100644 --- a/bookwyrm/tests/test_incoming.py +++ b/bookwyrm/tests/test_incoming.py @@ -487,6 +487,10 @@ class Incoming(TestCase): def test_handle_update_user(self): ''' update an existing user ''' + # we only do this with remote users + self.local_user.local = False + self.local_user.save() + datafile = pathlib.Path(__file__).parent.joinpath( 'data/ap_user.json') userdata = json.loads(datafile.read_bytes()) From 81e60cea16415f626a6a37ff22d5e8c03ecd39df Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 4 Jan 2021 10:48:48 -0800 Subject: [PATCH 09/11] Fixes login form --- bookwyrm/forms.py | 2 +- bookwyrm/templates/layout.html | 6 +++--- bookwyrm/templates/login.html | 4 ++-- bookwyrm/view_actions.py | 5 +++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/bookwyrm/forms.py b/bookwyrm/forms.py index 557cb00b0..152c2d768 100644 --- a/bookwyrm/forms.py +++ b/bookwyrm/forms.py @@ -35,7 +35,7 @@ class CustomForm(ModelForm): class LoginForm(CustomForm): class Meta: model = models.User - fields = ['username', 'password'] + fields = ['localname', 'password'] help_texts = {f: None for f in fields} widgets = { 'password': PasswordInput(), diff --git a/bookwyrm/templates/layout.html b/bookwyrm/templates/layout.html index 2e8fdcac9..a25658400 100644 --- a/bookwyrm/templates/layout.html +++ b/bookwyrm/templates/layout.html @@ -119,15 +119,15 @@ {% else %}
- {% if request.path != '/login' and request.path != '/login/' %} + {% if request.path != '/login' and request.path != '/login/' and request.path != '/user-login' %}
{% csrf_token %}
- - + +
diff --git a/bookwyrm/templates/login.html b/bookwyrm/templates/login.html index ce5d9e41c..40d57fa42 100644 --- a/bookwyrm/templates/login.html +++ b/bookwyrm/templates/login.html @@ -11,9 +11,9 @@ {% csrf_token %}
- +
- {{ login_form.username }} + {{ login_form.localname }}
diff --git a/bookwyrm/view_actions.py b/bookwyrm/view_actions.py index 965478f41..c600248a7 100644 --- a/bookwyrm/view_actions.py +++ b/bookwyrm/view_actions.py @@ -30,8 +30,9 @@ def user_login(request): ''' authenticate user login ''' login_form = forms.LoginForm(request.POST) - username = login_form.data['username'] - username = '%s@%s' % (username, DOMAIN) + print(login_form.data) + localname = login_form.data['localname'] + username = '%s@%s' % (localname, DOMAIN) password = login_form.data['password'] user = authenticate(request, username=username, password=password) if user is not None: From 9102d817678817e7e1c78ae702b4054325ee32ee Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 4 Jan 2021 11:14:10 -0800 Subject: [PATCH 10/11] Fixes checking invite validity --- bookwyrm/tests/test_view_actions.py | 7 ++++--- bookwyrm/view_actions.py | 5 +++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/bookwyrm/tests/test_view_actions.py b/bookwyrm/tests/test_view_actions.py index e1950e3f0..2e5258f66 100644 --- a/bookwyrm/tests/test_view_actions.py +++ b/bookwyrm/tests/test_view_actions.py @@ -173,7 +173,7 @@ class ViewActions(TestCase): self.assertEqual(response.status_code, 302) self.assertEqual(models.SiteInvite.objects.get().times_used, 1) - # invalid invite + # invite already used to max capacity request = self.factory.post( 'register/', { @@ -182,7 +182,8 @@ class ViewActions(TestCase): 'email': 'aa@bb.ccc', 'invite_code': 'testcode' }) - response = actions.register(request) + with self.assertRaises(PermissionDenied): + response = actions.register(request) self.assertEqual(models.User.objects.count(), 3) # bad invite code @@ -380,7 +381,7 @@ class ViewActions(TestCase): def test_untag(self): ''' remove a tag from a book ''' tag = models.Tag.objects.create(name='A Tag!?') - user_tag = models.UserTag.objects.create( + models.UserTag.objects.create( user=self.local_user, book=self.book, tag=tag) request = self.factory.post( '', { diff --git a/bookwyrm/view_actions.py b/bookwyrm/view_actions.py index c600248a7..d27701b2c 100644 --- a/bookwyrm/view_actions.py +++ b/bookwyrm/view_actions.py @@ -30,7 +30,6 @@ def user_login(request): ''' authenticate user login ''' login_form = forms.LoginForm(request.POST) - print(login_form.data) localname = login_form.data['localname'] username = '%s@%s' % (localname, DOMAIN) password = login_form.data['password'] @@ -60,6 +59,8 @@ def register(request): raise PermissionDenied invite = get_object_or_404(models.SiteInvite, code=invite_code) + if not invite.valid(): + raise PermissionDenied else: invite = None @@ -74,7 +75,7 @@ def register(request): # check localname and email uniqueness if models.User.objects.filter(localname=localname).first(): - form.add_error('localname', 'User with this username already exists') + form.errors['localname'] = ['User with this username already exists'] errors = True if errors: From 1d2b56da85a22bee4d09b903670f552296365ff8 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 4 Jan 2021 12:00:53 -0800 Subject: [PATCH 11/11] Fixes outgoing tests --- bookwyrm/tests/test_outgoing.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bookwyrm/tests/test_outgoing.py b/bookwyrm/tests/test_outgoing.py index 344a374d4..e004d1090 100644 --- a/bookwyrm/tests/test_outgoing.py +++ b/bookwyrm/tests/test_outgoing.py @@ -176,10 +176,10 @@ class Outgoing(TestCase): def test_existing_user(self): ''' simple database lookup by username ''' - result = outgoing.handle_remote_webfinger('@mouse@%s' % DOMAIN) + result = outgoing.handle_remote_webfinger('@mouse@local.com') self.assertEqual(result, self.local_user) - result = outgoing.handle_remote_webfinger('mouse@%s' % DOMAIN) + result = outgoing.handle_remote_webfinger('mouse@local.com') self.assertEqual(result, self.local_user) @@ -399,7 +399,7 @@ class Outgoing(TestCase): def test_handle_status_mentions(self): ''' @mention a user in a post ''' user = models.User.objects.create_user( - 'rat@local.com', 'rat@rat.com', 'password', + 'rat@%s' % DOMAIN, 'rat@rat.com', 'password', local=True, localname='rat') form = forms.CommentForm({ 'content': 'hi @rat', @@ -411,16 +411,16 @@ class Outgoing(TestCase): with patch('bookwyrm.broadcast.broadcast_task.delay'): outgoing.handle_status(self.local_user, form) status = models.Status.objects.get() + self.assertEqual(list(status.mention_users.all()), [user]) + self.assertEqual(models.Notification.objects.get().user, user) self.assertEqual( status.content, '

hi @rat

' % user.remote_id) - self.assertEqual(list(status.mention_users.all()), [user]) - self.assertEqual(models.Notification.objects.get().user, user) def test_handle_status_reply_with_mentions(self): ''' reply to a post with an @mention'ed user ''' user = models.User.objects.create_user( - 'rat@local.com', 'rat@rat.com', 'password', + 'rat', 'rat@rat.com', 'password', local=True, localname='rat') form = forms.CommentForm({ 'content': 'hi @rat@example.com',