From 471a8a024153304e84763cd754d769d9fac7ae26 Mon Sep 17 00:00:00 2001 From: Trevor Peacock Date: Wed, 30 Jan 2019 14:53:10 +1100 Subject: [PATCH 1/3] wrap into single commit --- src/wiki/apps.py | 1 + src/wiki/checks.py | 29 ++++++++++ src/wiki/forms.py | 76 +++++++------------------ src/wiki/forms_account_handling.py | 89 ++++++++++++++++++++++++++++++ tests/core/test_checks.py | 42 +++++++++++++- tests/testdata/models.py | 7 +++ 6 files changed, 187 insertions(+), 57 deletions(-) create mode 100644 src/wiki/forms_account_handling.py diff --git a/src/wiki/apps.py b/src/wiki/apps.py index a62605eb1..cd5ec15ac 100644 --- a/src/wiki/apps.py +++ b/src/wiki/apps.py @@ -15,4 +15,5 @@ def ready(self): register(checks.check_for_required_installed_apps, checks.Tags.required_installed_apps) register(checks.check_for_obsolete_installed_apps, checks.Tags.obsolete_installed_apps) register(checks.check_for_context_processors, checks.Tags.context_processors) + register(checks.check_for_fields_in_custom_user_model, checks.Tags.fields_in_custom_user_model) load_wiki_plugins() diff --git a/src/wiki/checks.py b/src/wiki/checks.py index 7c745ceaf..04d615b11 100644 --- a/src/wiki/checks.py +++ b/src/wiki/checks.py @@ -7,6 +7,7 @@ class Tags: required_installed_apps = "required_installed_apps" obsolete_installed_apps = "obsolete_installed_apps" context_processors = "context_processors" + fields_in_custom_user_model = "fields_in_custom_user_model" REQUIRED_INSTALLED_APPS = ( @@ -30,6 +31,12 @@ class Tags: ('sekizai.context_processors.sekizai', 'E009'), ) +FIELDS_IN_CUSTOM_USER_MODEL = ( + # check function, field fetcher, required field type, error code + ('check_user_field', 'USERNAME_FIELD', 'CharField', 'E010'), + ('check_email_field', 'get_email_field_name()', 'EmailField', 'E011'), +) + def check_for_required_installed_apps(app_configs, **kwargs): errors = [] @@ -69,3 +76,25 @@ def check_for_context_processors(app_configs, **kwargs): ) ) return errors + + +def check_for_fields_in_custom_user_model(app_configs, **kwargs): + errors = [] + from wiki.conf import settings + if not settings.ACCOUNT_HANDLING: + return errors + import wiki.forms_account_handling + from django.contrib.auth import get_user_model + User = get_user_model() + for check_function_name, field_fetcher, required_field_type, error_code in FIELDS_IN_CUSTOM_USER_MODEL: + function = getattr(wiki.forms_account_handling, check_function_name) + if not function(User): + errors.append( + Error( + '%s.%s.%s refers to a field that is not of type %s' % (User.__module__, User.__name__, field_fetcher, required_field_type), + hint='If you have your own login/logout views, turn off settings.WIKI_ACCOUNT_HANDLING', + obj=User, + id='wiki.%s' % error_code, + ) + ) + return errors diff --git a/src/wiki/forms.py b/src/wiki/forms.py index b818a9077..50cec3a1a 100644 --- a/src/wiki/forms.py +++ b/src/wiki/forms.py @@ -1,11 +1,26 @@ -import random -import string + +__all__ = [ + 'UserCreationForm', + 'UserUpdateForm', + 'WikiSlugField', + 'SpamProtectionMixin', + 'CreateRootForm', + 'MoveForm', + 'EditForm', + 'SelectWidgetBootstrap', + 'TextInputPrepend', + 'CreateForm', + 'DeleteForm', + 'PermissionsForm', + 'DirFilterForm', + 'SearchForm', +] + from datetime import timedelta from django import forms from django.apps import apps from django.contrib.auth import get_user_model -from django.contrib.auth.forms import UserCreationForm from django.core import validators from django.core.validators import RegexValidator from django.forms.widgets import HiddenInput @@ -21,6 +36,8 @@ from wiki.core.plugins.base import PluginSettingsFormMixin from wiki.editors import getEditor +from .forms_account_handling import UserCreationForm, UserUpdateForm + validate_slug_numbers = RegexValidator( r'^[0-9]+$', _("A 'slug' cannot consist solely of numbers."), @@ -565,56 +582,3 @@ class SearchForm(forms.Form): 'placeholder': _('Search...'), 'class': 'search-query'}), required=False) - - -class UserCreationForm(UserCreationForm): - email = forms.EmailField(required=True) - - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - - # Add honeypots - self.honeypot_fieldnames = "address", "phone" - self.honeypot_class = ''.join( - random.choice(string.ascii_uppercase + string.digits) - for __ in range(10)) - self.honeypot_jsfunction = 'f' + ''.join( - random.choice(string.ascii_uppercase + string.digits) - for __ in range(10)) - - for fieldname in self.honeypot_fieldnames: - self.fields[fieldname] = forms.CharField( - widget=forms.TextInput(attrs={'class': self.honeypot_class}), - required=False, - ) - - def clean(self): - cd = super().clean() - for fieldname in self.honeypot_fieldnames: - if cd[fieldname]: - raise forms.ValidationError( - "Thank you, non-human visitor. Please keep trying to fill in the form.") - return cd - - class Meta: - model = User - fields = ("username", "email") - - -class UserUpdateForm(forms.ModelForm): - password1 = forms.CharField(label="New password", widget=forms.PasswordInput(), required=False) - password2 = forms.CharField(label="Confirm password", widget=forms.PasswordInput(), required=False) - - def clean(self): - cd = super().clean() - password1 = cd.get('password1') - password2 = cd.get('password2') - - if password1 and password1 != password2: - raise forms.ValidationError(_("Passwords don't match")) - - return cd - - class Meta: - model = User - fields = ['email'] diff --git a/src/wiki/forms_account_handling.py b/src/wiki/forms_account_handling.py new file mode 100644 index 000000000..ff1757d2d --- /dev/null +++ b/src/wiki/forms_account_handling.py @@ -0,0 +1,89 @@ +import random +import string + +import django.contrib.auth.models +from django import forms +from django.contrib.auth import get_user_model +from django.contrib.auth.forms import UserCreationForm +from django.core.exceptions import FieldDoesNotExist +from django.db.models.fields import CharField, EmailField +from django.utils.translation import gettext_lazy as _ +from wiki.conf import settings + + +def _get_field(model, field): + try: + return model._meta.get_field(field) + except FieldDoesNotExist: + return + + +User = get_user_model() + + +def check_user_field(user_model): + return isinstance(_get_field(user_model, user_model.USERNAME_FIELD), CharField) + + +def check_email_field(user_model): + return isinstance(_get_field(user_model, user_model.get_email_field_name()), EmailField) + + +# django parses the ModelForm (and Meta classes) on class creation, which fails with custom models without expected fields. +# We need to check this here, because if this module can't load then system checks can't run. +CustomUser = User \ + if (settings.ACCOUNT_HANDLING and check_user_field(User) and check_email_field(User)) \ + else django.contrib.auth.models.User + + +class UserCreationForm(UserCreationForm): + email = forms.EmailField(required=True) + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + # Add honeypots + self.honeypot_fieldnames = "address", "phone" + self.honeypot_class = ''.join( + random.choice(string.ascii_uppercase + string.digits) + for __ in range(10)) + self.honeypot_jsfunction = 'f' + ''.join( + random.choice(string.ascii_uppercase + string.digits) + for __ in range(10)) + + for fieldname in self.honeypot_fieldnames: + self.fields[fieldname] = forms.CharField( + widget=forms.TextInput(attrs={'class': self.honeypot_class}), + required=False, + ) + + def clean(self): + cd = super().clean() + for fieldname in self.honeypot_fieldnames: + if cd[fieldname]: + raise forms.ValidationError( + "Thank you, non-human visitor. Please keep trying to fill in the form.") + return cd + + class Meta: + model = CustomUser + fields = (CustomUser.USERNAME_FIELD, CustomUser.get_email_field_name()) + + +class UserUpdateForm(forms.ModelForm): + password1 = forms.CharField(label="New password", widget=forms.PasswordInput(), required=False) + password2 = forms.CharField(label="Confirm password", widget=forms.PasswordInput(), required=False) + + def clean(self): + cd = super().clean() + password1 = cd.get('password1') + password2 = cd.get('password2') + + if password1 and password1 != password2: + raise forms.ValidationError(_("Passwords don't match")) + + return cd + + class Meta: + model = CustomUser + fields = [CustomUser.get_email_field_name()] diff --git a/tests/core/test_checks.py b/tests/core/test_checks.py index 7e87418ce..07acf8b7c 100644 --- a/tests/core/test_checks.py +++ b/tests/core/test_checks.py @@ -3,7 +3,9 @@ from django.conf import settings from django.core.checks import Error, registry from django.test import TestCase -from wiki.checks import REQUIRED_CONTEXT_PROCESSORS, REQUIRED_INSTALLED_APPS, Tags +from wiki.checks import FIELDS_IN_CUSTOM_USER_MODEL, REQUIRED_CONTEXT_PROCESSORS, REQUIRED_INSTALLED_APPS, Tags + +from ..base import wiki_override_settings def _remove(settings, arg): @@ -40,3 +42,41 @@ def test_required_context_processors(self): ) ] self.assertEqual(errors, expected_errors) + + def test_custom_user_model_mitigation_required(self): + """ + Django & six check django.forms.ModelForm.Meta on definition, and raises an error if Meta.fields don't exist in Meta.model. + This causes problems in wiki.forms.UserCreationForm and wiki.forms.UserUpdateForm when a custom user model doesn't have fields django-wiki assumes. + There is some code in wiki.forms that detects this situation. + This check asserts that Django/six are still raising an exception on definition, and asserts the mitigation code in wiki.forms, + and that test_check_for_fields_in_custom_user_model below are required. + """ + from django.core.exceptions import FieldError + from django import forms + from ..testdata.models import VeryCustomUser + with self.assertRaisesRegex(FieldError, 'Unknown field\\(s\\) \\((email|username|, )+\\) specified for VeryCustomUser'): + class UserUpdateForm(forms.ModelForm): + class Meta: + model = VeryCustomUser + fields = ['username', 'email'] + + def test_check_for_fields_in_custom_user_model(self): + from django.contrib.auth import get_user_model + with wiki_override_settings(WIKI_ACCOUNT_HANDLING=False, AUTH_USER_MODEL='testdata.VeryCustomUser'): + errors = registry.run_checks(tags=[Tags.fields_in_custom_user_model]) + self.assertEqual(errors, []) + with wiki_override_settings(WIKI_ACCOUNT_HANDLING=True, AUTH_USER_MODEL='testdata.VeryCustomUser'): + errors = registry.run_checks(tags=[Tags.fields_in_custom_user_model]) + expected_errors = [ + Error( + '%s.%s.%s refers to a field that is not of type %s' % ( + get_user_model().__module__, get_user_model().__name__, field_fetcher, required_field_type), + hint='If you have your own login/logout views, turn off settings.WIKI_ACCOUNT_HANDLING', + obj=get_user_model(), + id='wiki.%s' % error_code, + ) + for check_function_name, field_fetcher, required_field_type, error_code in FIELDS_IN_CUSTOM_USER_MODEL] + self.assertEqual(errors, expected_errors) + with wiki_override_settings(WIKI_ACCOUNT_HANDLING=True): + errors = registry.run_checks(tags=[Tags.fields_in_custom_user_model]) + self.assertEqual(errors, []) diff --git a/tests/testdata/models.py b/tests/testdata/models.py index d390c3ae9..5d7a33076 100644 --- a/tests/testdata/models.py +++ b/tests/testdata/models.py @@ -1,3 +1,4 @@ +from django.contrib.auth.base_user import AbstractBaseUser from django.contrib.auth.models import AbstractUser from django.db import models @@ -8,3 +9,9 @@ class CustomUser(AbstractUser): class CustomGroup(models.Model): pass + + +# user with invalid renamed identifier, and no email field +class VeryCustomUser(AbstractBaseUser): + identifier = models.IntegerField() + USERNAME_FIELD = 'identifier' From e427e3f388d6af79a0b13dedc5810048cb378061 Mon Sep 17 00:00:00 2001 From: oleast Date: Thu, 15 Nov 2018 15:37:03 +0100 Subject: [PATCH 2/3] Use User.get_username() for article cache instead of User.__str__ --- src/wiki/models/article.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wiki/models/article.py b/src/wiki/models/article.py index a9fa8ae0a..393210f10 100644 --- a/src/wiki/models/article.py +++ b/src/wiki/models/article.py @@ -207,9 +207,9 @@ def get_cache_key(self): def get_cache_content_key(self, user=None): """Returns per-article-user cache key.""" - return "{key}:{user!s}".format( + return "{key}:{user}".format( key=self.get_cache_key(), - user=user if user else "") + user=user.get_username() if user else "") def get_cached_content(self, user=None): """Returns cached version of rendered article. From a871299b97493835c5a2bcc2bd962f88cb0c1945 Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Fri, 1 Feb 2019 14:05:24 +0100 Subject: [PATCH 3/3] Bump to 0.4.4 --- docs/release_notes.rst | 11 ++++++++++- src/wiki/__init__.py | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/docs/release_notes.rst b/docs/release_notes.rst index 64bb76d50..912234e16 100644 --- a/docs/release_notes.rst +++ b/docs/release_notes.rst @@ -11,13 +11,22 @@ Release plan * **0.5** should remove Django 1.11 support and target Bootstrap v4, if you are interested in this work, please get in touch on Github! -0.4.3 +0.4.4 ----- Fixed ~~~~~ * Projects fail to load with custom ``User`` models without a ``username`` field :url-issue:`865` (trevorpeacock) +* Use ``User.get_username()`` for article cache instead of ``User.__str__`` :url-issue:`931` (Ole Anders Stokker) + + +0.4.3 +----- + +Discarded release do to git errors (the actual fixes were not merged in). + +* Automated language updates from Transifex 0.4.2 diff --git a/src/wiki/__init__.py b/src/wiki/__init__.py index c68a0b338..f3d52978e 100644 --- a/src/wiki/__init__.py +++ b/src/wiki/__init__.py @@ -19,5 +19,5 @@ default_app_config = 'wiki.apps.WikiConfig' -VERSION = (0, 4, 3, 'final', 0) +VERSION = (0, 4, 4, 'final', 0) __version__ = get_version(VERSION)