From 0f5db963646a3b55044d2f4e2dc844c659295d08 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Wed, 9 Aug 2017 17:00:47 +0100 Subject: [PATCH] Add WAGTAILUSERS_PASSWORD_ENABLED and WAGTAILUSERS_PASSWORD_REQUIRED settings Fixes #3706. These options restore the ability to create users with no password set on the Django side, for setups where authentication is managed externally (e.g. LDAP) - this was inadvertently dropped in Wagtail 1.10 when the form validation was tightened up (#3007). Additionally, the password fields can now be removed entirely, to enforce the use of an external auth setup. --- CHANGELOG.txt | 1 + docs/advanced_topics/settings.rst | 12 + docs/releases/1.12.rst | 1 + wagtail/wagtailusers/forms.py | 31 ++- .../templates/wagtailusers/users/create.html | 8 +- .../templates/wagtailusers/users/edit.html | 8 +- wagtail/wagtailusers/tests.py | 221 +++++++++++++++++- 7 files changed, 267 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index e8c3c370eb..16ff18694e 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -12,6 +12,7 @@ Changelog * StreamBlock now accepts `min_num`, `max_num` and `block_counts` settings to control the minimum and maximum numbers of blocks (Edwar Baron, Matt Westcott) * Users can no longer remove their own active / superuser flags through Settings -> Users (Stein Strindhaug, Huub Bouma) * The `process_form_submission` method of form pages now return the created form submission object (cho-leukeleu) + * Added `WAGTAILUSERS_PASSWORD_ENABLED` and `WAGTAILUSERS_PASSWORD_REQUIRED` settings to permit creating users with no Django-side passwords, to support external authentication setups (Matt Westcott) * Fix: FieldBlocks in StreamField now call the field's `prepare_value` method (Tim Heap) * Fix: Initial disabled state of InlinePanel add button is now set correctly on non-default tabs (Matthew Downey) * Fix: Redirects with unicode characters now work (Rich Brennan) diff --git a/docs/advanced_topics/settings.rst b/docs/advanced_topics/settings.rst index 45eb260f7c..e741f52be5 100644 --- a/docs/advanced_topics/settings.rst +++ b/docs/advanced_topics/settings.rst @@ -262,6 +262,18 @@ This specifies whether users are allowed to change their passwords (enabled by d This specifies whether users are allowed to reset their passwords. Defaults to the same as ``WAGTAIL_PASSWORD_MANAGEMENT_ENABLED``. +.. code-block:: python + + WAGTAILUSERS_PASSWORD_ENABLED = True + +This specifies whether password fields are shown when creating or editing users through Settings -> Users (enabled by default). Set this to False (along with ``WAGTAIL_PASSWORD_MANAGEMENT_ENABLED`` and ``WAGTAIL_PASSWORD_RESET_ENABLED``) if your users are authenticated through an external system such as LDAP. + +.. code-block:: python + + WAGTAILUSERS_PASSWORD_REQUIRED = True + +This specifies whether password is a required field when creating a new user. True by default; ignored if ``WAGTAILUSERS_PASSWORD_ENABLED`` is false. If this is set to False, and the password field is left blank when creating a user, then that user will have no usable password, and will not be able to log in unless an alternative authentication system such as LDAP is set up. + Email Notifications ------------------- diff --git a/docs/releases/1.12.rst b/docs/releases/1.12.rst index 527e6473f4..ce140fe30d 100644 --- a/docs/releases/1.12.rst +++ b/docs/releases/1.12.rst @@ -28,6 +28,7 @@ Other features * StreamBlock now accepts ``min_num``, ``max_num`` and ``block_counts`` settings to control the minimum and maximum numbers of blocks (Edwar Baron, Matt Westcott) * Users can no longer remove their own active / superuser flags through Settings -> Users (Stein Strindhaug, Huub Bouma) * The ``process_form_submission`` method of form pages now return the created form submission object (cho-leukeleu) + * Added ``WAGTAILUSERS_PASSWORD_ENABLED`` and ``WAGTAILUSERS_PASSWORD_REQUIRED`` settings to permit creating users with no Django-side passwords, to support external authentication setups (Matt Westcott) Bug fixes ~~~~~~~~~ diff --git a/wagtail/wagtailusers/forms.py b/wagtail/wagtailusers/forms.py index b7baf73f5e..bdda86db66 100644 --- a/wagtail/wagtailusers/forms.py +++ b/wagtail/wagtailusers/forms.py @@ -67,7 +67,13 @@ class UsernameForm(forms.ModelForm): class UserForm(UsernameForm): required_css_class = "required" - password_required = True + @property + def password_required(self): + return getattr(settings, 'WAGTAILUSERS_PASSWORD_REQUIRED', True) + + @property + def password_enabled(self): + return getattr(settings, 'WAGTAILUSERS_PASSWORD_ENABLED', True) error_messages = { 'duplicate_username': _("A user with that username already exists."), @@ -95,12 +101,16 @@ class UserForm(UsernameForm): def __init__(self, *args, **kwargs): super(UserForm, self).__init__(*args, **kwargs) - if self.password_required: - self.fields['password1'].help_text = ( - mark_safe(password_validators_help_text_html()) - if django.VERSION >= (1, 9) else '') - self.fields['password1'].required = True - self.fields['password2'].required = True + if self.password_enabled: + if self.password_required: + self.fields['password1'].help_text = ( + mark_safe(password_validators_help_text_html()) + if django.VERSION >= (1, 9) else '') + self.fields['password1'].required = True + self.fields['password2'].required = True + else: + del self.fields['password1'] + del self.fields['password2'] # We cannot call this method clean_username since this the name of the # username field may be different, so clean_username would not be reliably @@ -144,9 +154,10 @@ class UserForm(UsernameForm): def save(self, commit=True): user = super(UserForm, self).save(commit=False) - password = self.cleaned_data['password1'] - if password: - user.set_password(password) + if self.password_enabled: + password = self.cleaned_data['password1'] + if password: + user.set_password(password) if commit: user.save() diff --git a/wagtail/wagtailusers/templates/wagtailusers/users/create.html b/wagtail/wagtailusers/templates/wagtailusers/users/create.html index 96cb47306e..ba64b3ace9 100644 --- a/wagtail/wagtailusers/templates/wagtailusers/users/create.html +++ b/wagtail/wagtailusers/templates/wagtailusers/users/create.html @@ -25,8 +25,12 @@ {% include "wagtailadmin/shared/field_as_li.html" with field=form.first_name %} {% include "wagtailadmin/shared/field_as_li.html" with field=form.last_name %} {% block extra_fields %}{% endblock extra_fields %} - {% include "wagtailadmin/shared/field_as_li.html" with field=form.password1 %} - {% include "wagtailadmin/shared/field_as_li.html" with field=form.password2 %} + {% if form.password1 %} + {% include "wagtailadmin/shared/field_as_li.html" with field=form.password1 %} + {% endif %} + {% if form.password2 %} + {% include "wagtailadmin/shared/field_as_li.html" with field=form.password2 %} + {% endif %} {% endblock fields %}
  • {% trans "Roles" %}
    • diff --git a/wagtail/wagtailusers/templates/wagtailusers/users/edit.html b/wagtail/wagtailusers/templates/wagtailusers/users/edit.html index de307d25fe..951bdca369 100644 --- a/wagtail/wagtailusers/templates/wagtailusers/users/edit.html +++ b/wagtail/wagtailusers/templates/wagtailusers/users/edit.html @@ -26,8 +26,12 @@ {% include "wagtailadmin/shared/field_as_li.html" with field=form.first_name %} {% include "wagtailadmin/shared/field_as_li.html" with field=form.last_name %} {% block extra_fields %}{% endblock extra_fields %} - {% include "wagtailadmin/shared/field_as_li.html" with field=form.password1 %} - {% include "wagtailadmin/shared/field_as_li.html" with field=form.password2 %} + {% if form.password1 %} + {% include "wagtailadmin/shared/field_as_li.html" with field=form.password1 %} + {% endif %} + {% if form.password2 %} + {% include "wagtailadmin/shared/field_as_li.html" with field=form.password2 %} + {% endif %} {% if form.is_active %} {% include "wagtailadmin/shared/field_as_li.html" with field=form.is_active %} {% endif %} diff --git a/wagtail/wagtailusers/tests.py b/wagtail/wagtailusers/tests.py index 5f056ba6f1..7bd6fbc2f8 100644 --- a/wagtail/wagtailusers/tests.py +++ b/wagtail/wagtailusers/tests.py @@ -123,6 +123,8 @@ class TestUserCreateView(TestCase, WagtailTestUtils): response = self.get() self.assertEqual(response.status_code, 200) self.assertTemplateUsed(response, 'wagtailusers/users/create.html') + self.assertContains(response, 'Password:') + self.assertContains(response, 'Password confirmation:') def test_create(self): response = self.post({ @@ -188,6 +190,131 @@ class TestUserCreateView(TestCase, WagtailTestUtils): users = get_user_model().objects.filter(username='testuser') self.assertEqual(users.count(), 0) + def test_create_with_missing_password(self): + """Password should be required by default""" + response = self.post({ + 'username': "testuser", + 'email': "test@user.com", + 'first_name': "Test", + 'last_name': "User", + 'password1': "", + 'password2': "", + }) + + # Should remain on page + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'wagtailusers/users/create.html') + + self.assertTrue(response.context['form'].errors['password1']) + + # Check that the user was not created + users = get_user_model().objects.filter(username='testuser') + self.assertEqual(users.count(), 0) + + @override_settings(WAGTAILUSERS_PASSWORD_REQUIRED=False) + def test_password_fields_exist_when_not_required(self): + """Password fields should still be shown if WAGTAILUSERS_PASSWORD_REQUIRED is False""" + response = self.get() + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'wagtailusers/users/create.html') + self.assertContains(response, 'Password:') + self.assertContains(response, 'Password confirmation:') + + @override_settings(WAGTAILUSERS_PASSWORD_REQUIRED=False) + def test_create_with_password_not_required(self): + """Password should not be required if WAGTAILUSERS_PASSWORD_REQUIRED is False""" + response = self.post({ + 'username': "testuser", + 'email': "test@user.com", + 'first_name': "Test", + 'last_name': "User", + 'password1': "", + 'password2': "", + }) + + # Should redirect back to index + self.assertRedirects(response, reverse('wagtailusers_users:index')) + + # Check that the user was created + users = get_user_model().objects.filter(username='testuser') + self.assertEqual(users.count(), 1) + self.assertEqual(users.first().email, 'test@user.com') + self.assertFalse(users.first().has_usable_password()) + + @override_settings(WAGTAILUSERS_PASSWORD_REQUIRED=False) + def test_optional_password_is_still_validated(self): + """When WAGTAILUSERS_PASSWORD_REQUIRED is False, password validation should still apply if a password _is_ supplied""" + response = self.post({ + 'username': "testuser", + 'email': "test@user.com", + 'first_name': "Test", + 'last_name': "User", + 'password1': "banana", + 'password2': "kumquat", + }) + + # Should remain on page + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'wagtailusers/users/create.html') + + self.assertTrue(response.context['form'].errors['password2']) + + # Check that the user was not created + users = get_user_model().objects.filter(username='testuser') + self.assertEqual(users.count(), 0) + + @override_settings(WAGTAILUSERS_PASSWORD_REQUIRED=False) + def test_password_still_accepted_when_optional(self): + """When WAGTAILUSERS_PASSWORD_REQUIRED is False, we should still allow a password to be set""" + response = self.post({ + 'username': "testuser", + 'email': "test@user.com", + 'first_name': "Test", + 'last_name': "User", + 'password1': "banana", + 'password2': "banana", + }) + + # Should redirect back to index + self.assertRedirects(response, reverse('wagtailusers_users:index')) + + # Check that the user was created + users = get_user_model().objects.filter(username='testuser') + self.assertEqual(users.count(), 1) + self.assertEqual(users.first().email, 'test@user.com') + self.assertTrue(users.first().has_usable_password()) + self.assertTrue(users.first().check_password('banana')) + + @override_settings(WAGTAILUSERS_PASSWORD_ENABLED=False) + def test_password_fields_not_shown_when_disabled(self): + """WAGTAILUSERS_PASSWORD_ENABLED=False should cause password fields to be removed""" + response = self.get() + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'wagtailusers/users/create.html') + self.assertNotContains(response, 'Password:') + self.assertNotContains(response, 'Password confirmation:') + + @override_settings(WAGTAILUSERS_PASSWORD_ENABLED=False) + def test_password_fields_ignored_when_disabled(self): + """When WAGTAILUSERS_PASSWORD_REQUIRED is False, users should always be created without a usable password""" + response = self.post({ + 'username': "testuser", + 'email': "test@user.com", + 'first_name': "Test", + 'last_name': "User", + 'password1': "banana", # not part of the form - should be ignored + 'password2': "kumquat", # not part of the form - should be ignored + }) + + # Should redirect back to index + self.assertRedirects(response, reverse('wagtailusers_users:index')) + + # Check that the user was created + users = get_user_model().objects.filter(username='testuser') + self.assertEqual(users.count(), 1) + self.assertEqual(users.first().email, 'test@user.com') + self.assertFalse(users.first().has_usable_password()) + class TestUserDeleteView(TestCase, WagtailTestUtils): def setUp(self): @@ -311,6 +438,8 @@ class TestUserEditView(TestCase, WagtailTestUtils): self.test_user = get_user_model().objects.create_user( username='testuser', email='testuser@email.com', + first_name='Original', + last_name='User', password='password' ) @@ -327,10 +456,71 @@ class TestUserEditView(TestCase, WagtailTestUtils): response = self.get() self.assertEqual(response.status_code, 200) self.assertTemplateUsed(response, 'wagtailusers/users/edit.html') + self.assertContains(response, 'Password:') + self.assertContains(response, 'Password confirmation:') def test_nonexistant_redirect(self): self.assertEqual(self.get(user_id=100000).status_code, 404) + def test_simple_post(self): + response = self.post({ + 'username': "testuser", + 'email': "test@user.com", + 'first_name': "Edited", + 'last_name': "User", + 'password1': "newpassword", + 'password2': "newpassword", + 'is_active': 'on' + }) + # Should redirect back to index + self.assertRedirects(response, reverse('wagtailusers_users:index')) + + # Check that the user was edited + user = get_user_model().objects.get(pk=self.test_user.pk) + self.assertEqual(user.first_name, 'Edited') + self.assertTrue(user.check_password('newpassword')) + + def test_password_optional(self): + """Leaving password fields blank should leave it unchanged""" + response = self.post({ + 'username': "testuser", + 'email': "test@user.com", + 'first_name': "Edited", + 'last_name': "User", + 'password1': "", + 'password2': "", + 'is_active': 'on' + }) + # Should redirect back to index + self.assertRedirects(response, reverse('wagtailusers_users:index')) + + # Check that the user was edited but password is unchanged + user = get_user_model().objects.get(pk=self.test_user.pk) + self.assertEqual(user.first_name, 'Edited') + self.assertTrue(user.check_password('password')) + + def test_validate_password(self): + """Password fields should be validated if supplied""" + response = self.post({ + 'username': "testuser", + 'email': "test@user.com", + 'first_name': "Edited", + 'last_name': "User", + 'password1': "banana", + 'password2': "kumquat", + 'is_active': 'on' + }) + # Should remain on page + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'wagtailusers/users/edit.html') + + self.assertTrue(response.context['form'].errors['password2']) + + # Check that the user was not edited + user = get_user_model().objects.get(pk=self.test_user.pk) + self.assertEqual(user.first_name, 'Original') + self.assertTrue(user.check_password('password')) + def test_edit_and_deactivate(self): response = self.post({ 'username': "testuser", @@ -355,7 +545,6 @@ class TestUserEditView(TestCase, WagtailTestUtils): # Check that the user is no longer active self.assertEqual(user.is_active, False) - def test_edit_and_make_superuser(self): response = self.post({ 'username': "testuser", @@ -471,6 +660,36 @@ class TestUserEditView(TestCase, WagtailTestUtils): # Should not redirect to index self.assertEqual(response.status_code, 200) + @override_settings(WAGTAILUSERS_PASSWORD_ENABLED=False) + def test_password_fields_not_shown_when_disabled(self): + """WAGTAILUSERS_PASSWORD_ENABLED=False should cause password fields to be removed""" + response = self.get() + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'wagtailusers/users/edit.html') + self.assertNotContains(response, 'Password:') + self.assertNotContains(response, 'Password confirmation:') + + @override_settings(WAGTAILUSERS_PASSWORD_ENABLED=False) + def test_password_fields_ignored_when_disabled(self): + """When WAGTAILUSERS_PASSWORD_REQUIRED is False, existing password should be left unchanged""" + response = self.post({ + 'username': "testuser", + 'email': "test@user.com", + 'first_name': "Edited", + 'last_name': "User", + 'is_active': 'on', + 'password1': "banana", # not part of the form - should be ignored + 'password2': "kumquat", # not part of the form - should be ignored + }) + + # Should redirect back to index + self.assertRedirects(response, reverse('wagtailusers_users:index')) + + # Check that the user was edited but password is unchanged + user = get_user_model().objects.get(pk=self.test_user.pk) + self.assertEqual(user.first_name, 'Edited') + self.assertTrue(user.check_password('password')) + class TestUserProfileCreation(TestCase, WagtailTestUtils): def setUp(self):