From f12e634bc9054d844f294b5adbb08c10a7619d8d Mon Sep 17 00:00:00 2001 From: Veiko Aasa Date: Thu, 17 Oct 2024 17:36:14 +0300 Subject: [PATCH] users: Delete or move home folder when user is deleted or renamed On user deletion, user's home folder is also deleted. Admins have an option to avoid deleting user's home by inactivating the user instead. This commit also removes user deletion buttons from the user's list page and adds this option to the user edit page. The user's edit form asks for a confirmation if the user deletion is requested. This change also means that the confirmation password is now required to delete a user. Also: - Add a simple username validation to the privileged actions. - Functional tests: Create a fixture to login as an admin before every test. - Functional tests: Add a test to check that SSH passwordless login works after user is renamed to validate correct SSH related path permissions. - Privileged tests: Add `test_` prefix to the generated random string which makes easier to check and cleanup created home folders. - Minor quote fixes. Tests performed in stable and testing containers: - Run all the users module tests twice, no failures in tests. - When user is the last admin, both "Active" and "Delete user" checkboxes are disabled. Closes #2451. [sunil] - Refactor the JS code: - Ensure that DOM elements are lookup after DOM content is loaded. - Styling changes. Reduce the number of globals, name the global names somewhat more unique. - Click the button instead of submitting the form to disable the button. - Template changes: - Add a body for the confirmation dialog to talk about disabling the user and deleting the home directory. - Change the label of the confirm button to make it more explicit (recommendation from many UX guides). - Styling. - Functional tests: - Fix visibility checking of an element to use the correct splinter API. - Simplify clicking the edit user link. - Minor update to form checkbox help text. Signed-off-by: Veiko Aasa Signed-off-by: Sunil Mohan Adapa Reviewed-by: Sunil Mohan Adapa --- plinth/modules/users/forms.py | 24 +++++- plinth/modules/users/privileged.py | 85 ++++++++++++++++--- plinth/modules/users/static/users.js | 60 +++++++++++++ .../modules/users/templates/users_delete.html | 26 ------ .../modules/users/templates/users_list.html | 13 +-- .../modules/users/templates/users_update.html | 38 +++++++++ plinth/modules/users/tests/test_functional.py | 52 ++++++++---- plinth/modules/users/tests/test_privileged.py | 47 +++++++--- plinth/modules/users/tests/test_views.py | 16 ++-- plinth/modules/users/urls.py | 2 - plinth/modules/users/views.py | 71 ++-------------- plinth/tests/functional/__init__.py | 18 ++-- 12 files changed, 299 insertions(+), 153 deletions(-) create mode 100644 plinth/modules/users/static/users.js delete mode 100644 plinth/modules/users/templates/users_delete.html diff --git a/plinth/modules/users/forms.py b/plinth/modules/users/forms.py index e4297a662..40419dda4 100644 --- a/plinth/modules/users/forms.py +++ b/plinth/modules/users/forms.py @@ -19,6 +19,7 @@ import plinth.forms import plinth.modules.ssh.privileged as ssh_privileged from plinth.modules import first_boot from plinth.utils import is_user_admin +from plinth.views import messages_error from . import get_last_admin_user, privileged from .components import UsersAndGroups @@ -247,11 +248,18 @@ class UserUpdateForm(ValidNewUsernameCheckMixin, PasswordConfirmForm, language = plinth.forms.LanguageSelectionFormMixin.language + delete = forms.BooleanField( + label=gettext_lazy('Delete user'), required=False, + help_text=gettext_lazy( + 'Deleting the user account will also remove all the files ' + 'related to the user. Deleting files can be avoided by ' + 'setting the user account as inactive.')) + class Meta: """Metadata to control automatic form building.""" fields = ('username', 'email', 'groups', 'ssh_keys', 'language', - 'is_active', 'confirm_password') + 'is_active', 'delete', 'confirm_password') model = User widgets = { 'groups': plinth.forms.CheckboxSelectMultipleWithReadOnly(), @@ -274,6 +282,7 @@ class UserUpdateForm(ValidNewUsernameCheckMixin, PasswordConfirmForm, if self.is_last_admin_user: self.fields['is_active'].disabled = True + self.fields['delete'].disabled = True def save(self, commit=True): """Update LDAP user name and groups after saving user model.""" @@ -287,6 +296,19 @@ class UserUpdateForm(ValidNewUsernameCheckMixin, PasswordConfirmForm, user.save() self.save_m2m() + if self.cleaned_data.get('delete'): + try: + # Remove system user + privileged.remove_user(user.get_username(), auth_username, + confirm_password) + except Exception as error: + messages_error(self.request, _('Failed to delete user.'), + error) + else: + # Remove Django user + user.delete() + return user + old_groups = privileged.get_user_groups(self.username) old_groups = [group for group in old_groups if group] diff --git a/plinth/modules/users/privileged.py b/plinth/modules/users/privileged.py index 23598b6b2..f672b8282 100644 --- a/plinth/modules/users/privileged.py +++ b/plinth/modules/users/privileged.py @@ -3,6 +3,7 @@ import logging import os +import pathlib import re import shutil import subprocess @@ -45,6 +46,12 @@ def _validate_password(username, password): raise PermissionError('Invalid credentials') +def _validate_username(username): + """Validate username.""" + if pathlib.Path(username).parts[-1] != username: + raise ValueError('Invalid username') + + @privileged def first_setup(): """Perform initial setup of LDAP.""" @@ -277,8 +284,7 @@ def _configure_ldapscripts(): def _lock_ldap_user(username: str): """Lock user.""" - if not _get_user_ids(username): - # User not found + if not _user_exists(username): return None # Replace command adds the attribute if it doesn't exist. @@ -286,13 +292,12 @@ def _lock_ldap_user(username: str): replace: pwdAccountLockedTime pwdAccountLockedTime: 000001010000Z ''' - _run(["ldapmodifyuser", username], input=input.encode()) + _run(['ldapmodifyuser', username], input=input.encode()) def _unlock_ldap_user(username: str): """Unlock user.""" - if not _get_user_ids(username): - # User not found + if not _user_exists(username): return None # Replace command without providing a value will remove the attribute @@ -344,41 +349,83 @@ def _disconnect_samba_user(username): raise +def _get_user_home(username): + """Return the user home directory.""" + output = subprocess.check_output(['getent', 'passwd', username], text=True) + return pathlib.Path(output.split(':')[5]) + + @privileged def create_user(username: str, password: secret_str, auth_user: str | None = None, auth_password: secret_str | None = None): """Create an LDAP user, set password and flush cache.""" + _validate_username(username) _validate_user(auth_user, auth_password) _run(['ldapadduser', username, 'users']) + _set_user_password(username, password) _flush_cache() _set_samba_user(username, password) @privileged -def remove_user(username: str, password: secret_str | None = None): +def remove_user(username: str, auth_user: str, auth_password: secret_str): """Remove an LDAP user.""" + _validate_username(username) + _validate_user(auth_user, auth_password) groups = _get_user_groups(username) - # require authentication if the user is last admin user - if _get_group_users('admin') == [username]: - _validate_password(username, password) - _delete_samba_user(username) for group in groups: _remove_user_from_group(username, group) - _run(['ldapdeleteuser', username]) + if _user_exists(username): + # remove the home folder if it's owned by the user + home_folder = _get_user_home(username) + if home_folder.is_dir(): + try: + owner = home_folder.owner() + except KeyError: # owner not found + pass + else: + if owner == username: + shutil.rmtree(home_folder, ignore_errors=True) + + _run(['ldapdeleteuser', username]) _flush_cache() +def _rename_ldap_user(old_username: str, new_username: str, + new_home: pathlib.Path | None): + """Rename LDAP user and user parameters.""" + _run(['ldaprenameuser', old_username, new_username]) + + input = f'''changetype: modify +replace: cn +cn: {new_username} +- +replace: gecos +gecos: {new_username} +''' + + if new_home: + input += f'''- +replace: homeDirectory +homeDirectory: {str(new_home)} +''' + + _run(['ldapmodifyuser', new_username], input=input.encode()) + + @privileged def rename_user(old_username: str, new_username: str): """Rename an LDAP user.""" + _validate_username(old_username) + _validate_username(new_username) groups = _get_user_groups(old_username) _delete_samba_user(old_username) @@ -386,7 +433,16 @@ def rename_user(old_username: str, new_username: str): for group in groups: _remove_user_from_group(old_username, group) - _run(['ldaprenameuser', old_username, new_username]) + old_home = _get_user_home(old_username) + new_home = old_home.with_name(new_username) + + if new_home.exists(): + new_home = None # Do not rename home + else: + if old_home.is_dir(): + old_home.rename(new_home) + + _rename_ldap_user(old_username, new_username, new_home) for group in groups: _add_user_to_group(new_username, group) @@ -465,6 +521,11 @@ def _get_user_ids(username: str) -> str | None: return process.stdout.decode().strip() +def _user_exists(username): + """Return whether the user exists.""" + return _get_user_ids(username) is not None + + def _get_group_users(groupname): """Return list of members in the group.""" try: diff --git a/plinth/modules/users/static/users.js b/plinth/modules/users/static/users.js new file mode 100644 index 000000000..1b62a65bb --- /dev/null +++ b/plinth/modules/users/static/users.js @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +/** + * @licstart The following is the entire license notice for the JavaScript + * code in this page. + * + * This file is part of FreedomBox. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + * @licend The above is the entire license notice for the JavaScript code + * in this page. + */ + +var deleteConfirmed = false; + +document.addEventListener('DOMContentLoaded', (event) => { + const form = document.querySelector('form.form-update'); + form.addEventListener('submit', onUserUpdateSubmit); + + const confirmDeleteButton = document.querySelector( + '#user-delete-confirm-dialog button.confirm'); + confirmDeleteButton.addEventListener('click', () => { + onUserDeleteConfirmed(form); + }); +}); + +// Show the confirmation dialog if the delete checkbox is selected +function onUserUpdateSubmit(event) { + const deleteUserCheckbox = document.getElementById('id_delete'); + if (!deleteUserCheckbox.checked) { + return; + } + + if (deleteConfirmed) { // Deletion is already confirmed + deleteConfirmed = false; + return; + } + + event.preventDefault(); + $("#user-delete-confirm-dialog").modal('show'); +}; + +// Submit the user edit form +function onUserDeleteConfirmed(form) { + deleteConfirmed = true; + $('#user-delete-confirm-dialog').modal('hide'); + // Click instead of submit to disable the submission button + form.querySelector('input[type=submit]').click(); +}; diff --git a/plinth/modules/users/templates/users_delete.html b/plinth/modules/users/templates/users_delete.html deleted file mode 100644 index 14b40f648..000000000 --- a/plinth/modules/users/templates/users_delete.html +++ /dev/null @@ -1,26 +0,0 @@ -{% extends "base.html" %} -{% comment %} -# SPDX-License-Identifier: AGPL-3.0-or-later -{% endcomment %} - -{% load bootstrap %} -{% load i18n %} - -{% block content %} - -

{% trans "Delete User" %}

- -

- {% blocktrans trimmed with username=object.username %} - Delete user {{ username }} permanently? - {% endblocktrans %} -

- -
- {% csrf_token %} - - -
- -{% endblock %} diff --git a/plinth/modules/users/templates/users_list.html b/plinth/modules/users/templates/users_list.html index edf6ee28f..a06ea9021 100644 --- a/plinth/modules/users/templates/users_list.html +++ b/plinth/modules/users/templates/users_list.html @@ -19,8 +19,8 @@
-
-
+
+
{% for user in object_list %}
- - - {% endif %}
{% endfor %}
diff --git a/plinth/modules/users/templates/users_update.html b/plinth/modules/users/templates/users_update.html index 35e678bbd..a0de5fd60 100644 --- a/plinth/modules/users/templates/users_update.html +++ b/plinth/modules/users/templates/users_update.html @@ -5,6 +5,7 @@ {% load bootstrap %} {% load i18n %} +{% load static %} {% block content %}

@@ -30,5 +31,42 @@ + + {% endblock %} +{% block page_js %} + +{% endblock %} diff --git a/plinth/modules/users/tests/test_functional.py b/plinth/modules/users/tests/test_functional.py index 4400833d3..1d16e18ce 100644 --- a/plinth/modules/users/tests/test_functional.py +++ b/plinth/modules/users/tests/test_functional.py @@ -54,16 +54,21 @@ _config_page_title_language_map = { @pytest.fixture(scope='module', autouse=True) def fixture_background(session_browser): + """Unset language.""" + yield + functional.login(session_browser) + functional.user_set_language(session_browser, _language_codes['None']) + + +@pytest.fixture(scope='function', autouse=True) +def fixture_login(session_browser): """Login.""" functional.login(session_browser) - yield - functional.user_set_language(session_browser, _language_codes['None']) def test_create_user(session_browser): """Test creating a user.""" - if functional.user_exists(session_browser, 'alice'): - functional.delete_user(session_browser, 'alice') + _delete_user(session_browser, 'alice') functional.create_user(session_browser, 'alice', email='alice@example.com') assert functional.user_exists(session_browser, 'alice') @@ -73,8 +78,7 @@ def test_create_user(session_browser): def test_rename_user(session_browser): """Test renaming a user.""" _non_admin_user_exists(session_browser, 'alice') - if functional.user_exists(session_browser, 'bob'): - functional.delete_user(session_browser, 'bob') + _delete_user(session_browser, 'bob') _rename_user(session_browser, 'alice', 'bob') assert not functional.user_exists(session_browser, 'alice') @@ -94,7 +98,6 @@ def test_non_admin_users_can_change_own_ssh_keys(session_browser): 'alice') _set_ssh_keys(session_browser, 'somekey456') assert _get_ssh_keys(session_browser) == 'somekey456' - functional.login(session_browser) def test_admin_users_can_change_other_users_ssh_keys(session_browser): @@ -121,6 +124,26 @@ def test_users_can_connect_passwordless_over_ssh(session_browser, _should_connect_passwordless_over_ssh(session_browser, tmp_path_factory) +def test_ssh_passwordless_after_user_rename(session_browser, tmp_path_factory): + """Test that users can connect passwordless after user is renamed.""" + username_old = 'bob' + username_new = 'bob2' + functional.app_enable(session_browser, 'ssh') + _non_admin_user_exists(session_browser, username_old, + groups=['freedombox-ssh']) + _delete_user(session_browser, username_new) + _configure_ssh_keys(session_browser, tmp_path_factory, + username=username_old) + _should_connect_passwordless_over_ssh(session_browser, tmp_path_factory, + username=username_old) + + _rename_user(session_browser, username_old, username_new) + + assert functional.user_exists(session_browser, username_new) + _should_connect_passwordless_over_ssh(session_browser, tmp_path_factory, + username=username_new) + + def test_users_cannot_connect_passwordless_over_ssh(session_browser, tmp_path_factory): """Test that users cannot connect passwordless over ssh if the keys aren't @@ -173,8 +196,6 @@ def test_user_states(session_browser, tmp_path_factory): _should_connect_passwordless_over_ssh(session_browser, tmp_path_factory, username=username) - functional.login(session_browser) - def test_admin_users_can_change_own_password(session_browser): """Test that admin users can change their own password.""" @@ -183,7 +204,6 @@ def test_admin_users_can_change_own_password(session_browser): 'testadmin') _change_password(session_browser, 'newpassword456') _can_log_in_with_password(session_browser, 'testadmin', 'newpassword456') - functional.login(session_browser) def test_admin_users_can_change_others_password(session_browser): @@ -191,7 +211,6 @@ def test_admin_users_can_change_others_password(session_browser): _non_admin_user_exists(session_browser, 'alice') _change_password(session_browser, 'secretsecret567', username='alice') _can_log_in_with_password(session_browser, 'alice', 'secretsecret567') - functional.login(session_browser) def test_non_admin_users_can_change_own_password(session_browser): @@ -201,7 +220,6 @@ def test_non_admin_users_can_change_own_password(session_browser): 'alice') _change_password(session_browser, 'newpassword123') _can_log_in_with_password(session_browser, 'alice', 'newpassword123') - functional.login(session_browser) def test_delete_user(session_browser): @@ -211,15 +229,19 @@ def test_delete_user(session_browser): assert not functional.user_exists(session_browser, 'alice') -def _admin_user_exists(session_browser, name): +def _delete_user(session_browser, name): + """Delete a user.""" if functional.user_exists(session_browser, name): functional.delete_user(session_browser, name) + + +def _admin_user_exists(session_browser, name): + _delete_user(session_browser, name) functional.create_user(session_browser, name, groups=['admin']) def _non_admin_user_exists(session_browser, name, groups=[]): - if functional.user_exists(session_browser, name): - functional.delete_user(session_browser, name) + _delete_user(session_browser, name) functional.create_user(session_browser, name, groups=groups) diff --git a/plinth/modules/users/tests/test_privileged.py b/plinth/modules/users/tests/test_privileged.py index 356c6c10c..907b1f96f 100644 --- a/plinth/modules/users/tests/test_privileged.py +++ b/plinth/modules/users/tests/test_privileged.py @@ -36,17 +36,17 @@ def _is_ldap_set_up(): pytestmark: list[pytest.MarkDecorator] = [ pytest.mark.usefixtures('needs_root', 'load_cfg', 'mock_privileged'), - pytest.mark.skipif(not _is_ldap_set_up(), reason="LDAP is not configured") + pytest.mark.skipif(not _is_ldap_set_up(), reason='LDAP is not configured') ] privileged_modules_to_mock = [ 'plinth.modules.users.privileged', 'plinth.modules.security.privileged' ] -def _random_string(length=8): +def _random_string(): """Return a random string created from lower case ascii.""" - return ''.join( - [random.choice(string.ascii_lowercase) for _ in range(length)]) + random_chars = [random.choice(string.ascii_lowercase) for _ in range(8)] + return 'test_' + ''.join(random_chars) def _get_password_hash(username): @@ -135,11 +135,8 @@ def _create_user(username=None, groups=None): def _delete_user(username): """Utility to delete an LDAP and Samba user""" - admin_password = None - if privileged.get_group_users('admin') == [username]: - _, admin_password = _get_admin_user_password() - - privileged.remove_user(username, admin_password) + admin_user, admin_password = _get_admin_user_password() + privileged.remove_user(username, admin_user, admin_password) def _create_admin_if_does_not_exist(): @@ -195,6 +192,13 @@ def test_create_user(): _create_user(username) +def test_create_invalid_user(): + """Test invalid username validation.""" + username = 'invalid/user' + with pytest.raises(ValueError): + _create_user(username) + + def test_change_user_password(): """Test changing user password.""" _create_admin_if_does_not_exist() @@ -293,6 +297,18 @@ def test_rename_user(): _rename_user(existing_user, new_username=new_username) +def test_rename_invalid_user(): + """Test rename invalid username""" + invalid_username = 'invalid/user' + valid_username = _random_string() + + with pytest.raises(ValueError): + _rename_user(invalid_username, new_username=valid_username) + + with pytest.raises(ValueError): + _rename_user(valid_username, new_username=invalid_username) + + def test_delete_user(): """Test to check whether LDAP users can be deleted""" _create_admin_if_does_not_exist() @@ -313,10 +329,17 @@ def test_delete_user(): def test_delete_non_existent_user(): - """Deleting a non-existent user should fail.""" + """Deleting a non-existent user doesn't fail.""" non_existent_user = _random_string() - with pytest.raises(subprocess.CalledProcessError): - privileged.remove_user(non_existent_user) + _delete_user(non_existent_user) + + +def test_delete_invalid_user(): + """Deleting invalid username should fail.""" + invalid_username = 'invalid/user' + + with pytest.raises(ValueError): + _delete_user(invalid_username) def test_groups(): diff --git a/plinth/modules/users/tests/test_views.py b/plinth/modules/users/tests/test_views.py index c0672b053..fdbd6de30 100644 --- a/plinth/modules/users/tests/test_views.py +++ b/plinth/modules/users/tests/test_views.py @@ -84,15 +84,14 @@ def make_request(request, view, as_admin=True, **kwargs): def test_users_list_view(rf): - """Test that users list view has correct view data.""" + """Test users list view has correct view data.""" with (patch('plinth.views.AppView.get_context_data', return_value={'is_enabled': True}), patch('plinth.views.AppView.app', return_value=None)): view = views.UserList.as_view() response, messages = make_request(rf.get('/'), view) - assert response.context_data['last_admin_user'] == 'admin' - assert response.status_code == 200 + assert response.status_code == 200 @pytest.mark.parametrize('username', ['test-new', 'test-neW@2_']) @@ -268,10 +267,15 @@ def test_update_user_without_permissions_view(rf): def test_delete_user_view(rf): """Test that user deletion succeeds.""" user = 'tester' + form_data = { + 'username': user, + 'delete': True, + 'confirm_password': 'adminpassword', + } - url = urls.reverse('users:delete', kwargs={'slug': user}) - request = rf.post(url) - view = views.UserDelete.as_view() + url = urls.reverse('users:edit', kwargs={'slug': user}) + request = rf.post(url, data=form_data) + view = views.UserUpdate.as_view() response, messages = make_request(request, view, as_admin=True, slug=user) assert list(messages)[0].message == 'User {} deleted.'.format(user) diff --git a/plinth/modules/users/urls.py b/plinth/modules/users/urls.py index 2e721b5d5..8123cd21d 100644 --- a/plinth/modules/users/urls.py +++ b/plinth/modules/users/urls.py @@ -16,8 +16,6 @@ urlpatterns = [ re_path(r'^sys/users/create/$', views.UserCreate.as_view(), name='create'), re_path(r'^sys/users/(?P[\w.@+-]+)/edit/$', non_admin_view(views.UserUpdate.as_view()), name='edit'), - re_path(r'^sys/users/(?P[\w.@+-]+)/delete/$', - views.UserDelete.as_view(), name='delete'), re_path(r'^sys/users/(?P[\w.@+-]+)/change_password/$', non_admin_view(views.UserChangePassword.as_view()), name='change_password'), diff --git a/plinth/modules/users/views.py b/plinth/modules/users/views.py index 93a849ab3..357bf1820 100644 --- a/plinth/modules/users/views.py +++ b/plinth/modules/users/views.py @@ -2,17 +2,14 @@ """Django views for user app.""" import django.views.generic -from django.contrib import messages from django.contrib.auth import update_session_auth_hash from django.contrib.auth.models import User from django.contrib.messages.views import SuccessMessageMixin from django.core.exceptions import PermissionDenied from django.http import HttpResponseRedirect from django.urls import reverse, reverse_lazy -from django.utils.translation import gettext as _ from django.utils.translation import gettext_lazy -from django.views.generic.edit import (CreateView, DeleteView, FormView, - UpdateView) +from django.views.generic.edit import (CreateView, FormView, UpdateView) import plinth.modules.ssh.privileged as ssh_privileged from plinth import translation @@ -20,7 +17,7 @@ from plinth.modules import first_boot from plinth.utils import is_user_admin from plinth.views import AppView -from . import get_last_admin_user, privileged +from . import privileged from .forms import (CreateUserForm, FirstBootForm, UserChangePasswordForm, UserUpdateForm) @@ -64,11 +61,6 @@ class UserList(AppView, ContextMixin, django.views.generic.ListView): title = gettext_lazy('Users') app_id = 'users' - def get_context_data(self, *args, **kwargs): - context = super().get_context_data(*args, **kwargs) - context['last_admin_user'] = get_last_admin_user() - return context - class UserUpdate(ContextMixin, SuccessMessageMixin, UpdateView): """View to update a user's details.""" @@ -113,7 +105,13 @@ class UserUpdate(ContextMixin, SuccessMessageMixin, UpdateView): def form_valid(self, form): """Set the user language if necessary.""" + + is_user_deleted = form.cleaned_data.get('delete') + if is_user_deleted: + self.success_message = gettext_lazy('User %(username)s deleted.') response = super().form_valid(form) + if is_user_deleted: + return HttpResponseRedirect(reverse_lazy('users:index')) # If user is updating their own profile then set the language for # current session too. @@ -124,59 +122,6 @@ class UserUpdate(ContextMixin, SuccessMessageMixin, UpdateView): return response -class UserDelete(ContextMixin, DeleteView): - """Handle deleting users, showing a confirmation dialog first. - - On GET, display a confirmation page. - On POST, delete the user. - """ - - template_name = 'users_delete.html' - model = User - slug_field = 'username' - success_url = reverse_lazy('users:index') - title = gettext_lazy('Delete User') - - def __init__(self, *args, **kwargs): - """Initialize view, override delete method.""" - super().__init__(*args, **kwargs) - - # Avoid a warning with Django 4.0 that delete member should not be - # overridden. Remove this line and _delete() after Django 4.0 reaches - # Debian Stable. - self.delete = self._delete - - def _delete_from_ldap(self): - """Remove user from LDAP and show a success/error message.""" - message = _('User {user} deleted.').format(user=self.kwargs['slug']) - messages.success(self.request, message) - - try: - privileged.remove_user(self.kwargs['slug']) - except Exception: - messages.error(self.request, _('Deleting LDAP user failed.')) - - def _delete(self, *args, **kwargs): - """Set the success message of deleting the user. - - The SuccessMessageMixin doesn't work with the DeleteView on Django1.7, - so set the success message manually here. - """ - output = super().delete(*args, **kwargs) - self._delete_from_ldap() - return output - - def form_valid(self, form): - """Perform additional operations after delete. - - Since Django 4.0, DeleteView inherits form_view and a call to delete() - is not made. - """ - response = super().form_valid(form) # NOQA, pylint: disable=no-member - self._delete_from_ldap() - return response - - class UserChangePassword(ContextMixin, SuccessMessageMixin, FormView): """View to change user password.""" diff --git a/plinth/tests/functional/__init__.py b/plinth/tests/functional/__init__.py index 3f771404f..b1060575f 100644 --- a/plinth/tests/functional/__init__.py +++ b/plinth/tests/functional/__init__.py @@ -664,12 +664,20 @@ def create_user(browser, name, password=None, groups=[], email=None): def delete_user(browser, name): """Delete a user.""" nav_to_module(browser, 'users') - delete_link = browser.links.find_by_href( - f'/plinth/sys/users/{name}/delete/') + browser.links.find_by_href(f'/plinth/sys/users/{name}/edit/').first.click() - with wait_for_page_update(browser): - delete_link.first.click() - submit(browser, form_class='form-delete') + browser.find_by_id('id_delete').check() + browser.find_by_id('id_confirm_password').fill( + config['DEFAULT']['password']) + + browser.find_by_css('.form-update input[type=submit]').first.click() + + confirm_button = browser.find_by_css( + '#user-delete-confirm-dialog button.confirm').first + eventually(lambda: confirm_button.visible) + assert confirm_button.visible + with wait_for_page_update(browser, expected_url='/plinth/sys/users/'): + confirm_button.click() def user_exists(browser, name):