From 526a3018e4ceb5ab9354900c2ebb1c40cad2d0c0 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Thu, 29 Feb 2024 18:28:26 -0800 Subject: [PATCH] users: Fix creating users with initial set of groups Fixes: #2409. When creating a user if one or more groups is selected, creation fails. This is because the fields contains group choices as (name, label) tuples instead of (group_id, label) tuples as expected by the many-to-many field mapping mechanism in ModelField class. Fix this by using the same mechanism used in UserUpdateForm, which is to reuse the base class form field (but adjust some properties). Tests: - During first boot - Django groups are fully created when form is accessed with blank database - In user creation/modify form: - Label appears are 'Permissions' - Choices appear fully and as 'Description (Group name)' - Help text is correct. - Choices are sorted on group name. - Django groups are fully created when form is accessed when a new group is added to code. - User can have no groups - Widget is multiple checkbox widget. Multiple groups can be selected. - User is added to proper ldap groups after submission - In user modify form: - If the user is last admin user, admin checkbox is checked and disabled. - Current list of groups is accurate shown when form is displayed. - Add remove of groups works as expected - Functional tests for gitweb and users apps pass Signed-off-by: Sunil Mohan Adapa Reviewed-by: Veiko Aasa --- plinth/modules/users/forms.py | 94 +++++++++++++++++------------ plinth/tests/functional/__init__.py | 3 +- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/plinth/modules/users/forms.py b/plinth/modules/users/forms.py index 41df46aa1..dea81f95e 100644 --- a/plinth/modules/users/forms.py +++ b/plinth/modules/users/forms.py @@ -50,6 +50,49 @@ class ValidNewUsernameCheckMixin: return True +class GroupsFieldMixin: + """Mixin to set common properties for the group field.""" + + def __init__(self, *args, **kwargs): + """Set basic properties for the groups field. + + Also ensure that all the groups are created in django. + """ + group_choices = dict(UsersAndGroups.get_group_choices()) + for group in group_choices: + Group.objects.get_or_create(name=group) + + super().__init__(*args, **kwargs) + + choices = [] + django_groups = sorted(self.fields['groups'].choices, + key=lambda choice: choice[1]) + for group_id, group_name in django_groups: + try: + group_id = group_id.value + except AttributeError: + pass + + # Show choices only from groups declared by apps. + if group_name in group_choices: + label = group_choices[group_name] + if group_name == 'admin' and self.is_last_admin_user: + label = {'label': label, 'disabled': True} + + choices.append((group_id, label)) + + self.fields['groups'].label = _('Permissions') + self.fields['groups'].choices = choices + self.fields['groups'].help_text = _( + 'Select which services should be available to the new ' + 'user. The user will be able to log in to services that ' + 'support single sign-on through LDAP, if they are in the ' + 'appropriate group.

Users in the admin group ' + 'will be able to log in to all services. They can also ' + 'log in to the system through SSH and have ' + 'administrative privileges (sudo).') + + @deconstructible class UsernameValidator(validators.RegexValidator): """Username validator. @@ -96,7 +139,7 @@ class PasswordConfirmForm(forms.Form): return confirm_password -class CreateUserForm(ValidNewUsernameCheckMixin, +class CreateUserForm(ValidNewUsernameCheckMixin, GroupsFieldMixin, plinth.forms.LanguageSelectionFormMixin, PasswordConfirmForm, UserCreationForm): """Custom user create form. @@ -105,17 +148,6 @@ class CreateUserForm(ValidNewUsernameCheckMixin, """ username = USERNAME_FIELD - groups = forms.MultipleChoiceField( - choices=UsersAndGroups.get_group_choices, - label=gettext_lazy('Permissions'), required=False, - widget=plinth.forms.CheckboxSelectMultiple, help_text=gettext_lazy( - 'Select which services should be available to the new ' - 'user. The user will be able to log in to services that ' - 'support single sign-on through LDAP, if they are in the ' - 'appropriate group.

Users in the admin group ' - 'will be able to log in to all services. They can also ' - 'log in to the system through SSH and have ' - 'administrative privileges (sudo).')) language = plinth.forms.LanguageSelectionFormMixin.language @@ -124,10 +156,14 @@ class CreateUserForm(ValidNewUsernameCheckMixin, fields = ('username', 'password1', 'password2', 'groups', 'language', 'confirm_password') + widgets = { + 'groups': plinth.forms.CheckboxSelectMultiple(), + } def __init__(self, request, *args, **kwargs): """Initialize the form with extra request argument.""" self.request = request + self.is_last_admin_user = False super().__init__(*args, **kwargs) self.fields['username'].widget.attrs.update({ 'autofocus': 'autofocus', @@ -140,6 +176,8 @@ class CreateUserForm(ValidNewUsernameCheckMixin, user = super().save(commit) if commit: + self.save_m2m() # Django 3.x does not call save_m2m() + user.userprofile.language = self.cleaned_data['language'] user.userprofile.save() auth_username = self.request.user.username @@ -155,7 +193,8 @@ class CreateUserForm(ValidNewUsernameCheckMixin, _('Creating LDAP user failed: {error}'.format( error=error))) - for group in self.cleaned_data['groups']: + groups = user.groups.values_list('name', flat=True) + for group in groups: try: privileged.add_user_to_group(user.get_username(), group, auth_username, @@ -173,7 +212,8 @@ class CreateUserForm(ValidNewUsernameCheckMixin, class UserUpdateForm(ValidNewUsernameCheckMixin, PasswordConfirmForm, - plinth.forms.LanguageSelectionFormMixin, forms.ModelForm): + GroupsFieldMixin, plinth.forms.LanguageSelectionFormMixin, + forms.ModelForm): """When user info is changed, also updates LDAP user.""" username = USERNAME_FIELD @@ -200,39 +240,15 @@ class UserUpdateForm(ValidNewUsernameCheckMixin, PasswordConfirmForm, def __init__(self, request, username, *args, **kwargs): """Initialize the form with extra request argument.""" - group_choices = dict(UsersAndGroups.get_group_choices()) - for group in group_choices: - Group.objects.get_or_create(name=group) - self.request = request self.username = username - super().__init__(*args, **kwargs) self.is_last_admin_user = get_last_admin_user() == self.username + super().__init__(*args, **kwargs) self.fields['username'].widget.attrs.update({ 'autocapitalize': 'none', 'autocomplete': 'username' }) - choices = [] - django_groups = sorted(self.fields['groups'].choices, - key=lambda choice: choice[1]) - for group_id, group_name in django_groups: - try: - group_id = group_id.value - except AttributeError: - pass - - # Show choices only from groups declared by apps. - if group_name in group_choices: - label = group_choices[group_name] - if group_name == 'admin' and self.is_last_admin_user: - label = {'label': label, 'disabled': True} - - choices.append((group_id, label)) - - self.fields['groups'].label = _('Permissions') - self.fields['groups'].choices = choices - if not is_user_admin(request): self.fields['is_active'].widget = forms.HiddenInput() self.fields['groups'].disabled = True diff --git a/plinth/tests/functional/__init__.py b/plinth/tests/functional/__init__.py index d2a6dcc51..2f86dae3a 100644 --- a/plinth/tests/functional/__init__.py +++ b/plinth/tests/functional/__init__.py @@ -614,7 +614,8 @@ def create_user(browser, name, password=None, groups=[]): browser.find_by_id('id_password2').fill(password) for group in groups: - browser.find_by_id(f'id_groups_{group}').check() + browser.find_by_xpath( + f'//label[contains(text(), "({group})")]/input').check() browser.find_by_id('id_confirm_password').fill( config['DEFAULT']['password'])