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):