users: Handle errors better during POSIX user operations

- If during an action, user does not exist ignore.

- If during an action, return a non-zero exist status.

- Catch an errors during actions as exceptions.

- Display a message that corresponding POSIX operation failed.

actions/change-user-password

@@ -30,5 +30,5 @@ fi
 echo "$username:$password" | chpasswd
 if [ $? -ne 0 ]; then
     echo "Failed: could not set user password"
-    exit
+    exit 2
 fi

actions/create-user

@@ -22,17 +22,17 @@ password="$2"
 adduser --disabled-password --gecos "" "$username"
 if [ $? -ne 0 ]; then
     echo "Failed to create user"
-    exit
+    exit 1
 fi
 
 adduser "$username" sudo
 if [ $? -ne 0 ]; then
     echo "Failed to add user to sudo group"
-    exit
+    exit 2
 fi
 
 echo "$username:$password" | chpasswd
 if [ $? -ne 0 ]; then
     echo "Failed to set user password"
-    exit
+    exit 3
 fi

actions/delete-user

@@ -31,4 +31,5 @@ if [ $? -eq 0 ]; then
     echo "Success: user deleted"
 else
     echo "Failed: userdel error"
+    exit 2
 fi

actions/disable-user

@@ -20,7 +20,14 @@
 
 username="$1"
 
+getent passwd "$username"
+if [ $? -ne 0 ]; then
+    echo "Failed: user not found"
+    exit
+fi
+
 usermod --expiredate 1 "$username"
 if [ $? -ne 0 ]; then
     echo "Failed"
+    exit 2
 fi

actions/enable-user

@@ -20,7 +20,14 @@
 
 username="$1"
 
+getent passwd "$username"
+if [ $? -ne 0 ]; then
+    echo "Failed: user not found"
+    exit
+fi
+
 usermod --expiredate "" "$username"
 if [ $? -ne 0 ]; then
     echo "Failed"
+    exit 2
 fi

plinth/modules/first_boot/forms.py

@@ -21,6 +21,7 @@ from django.core import validators
 from gettext import gettext as _
 
 from plinth import actions
+from plinth.errors import ActionError
 from plinth.modules.config import config
 
 
@@ -62,9 +63,13 @@ than 63 characters in length.'),
         user.set_password(self.cleaned_data['password'])
         if commit:
             user.save()
-            actions.superuser_run(
+            try:
-                'create-user',
+                actions.superuser_run(
-                [user.username, self.cleaned_data['password']])
+                    'create-user',
+                    [user.get_username(), self.cleaned_data['password']])
+            except ActionError:
+                messages.error(self.request,
+                               _('Creating POSIX system user failed.'))
             self.login_user()
 
         return user

plinth/modules/users/__init__.py

@@ -19,12 +19,9 @@
 Plinth module to manage users
 """
 
-from django.db.models.signals import post_delete
-from django.contrib.auth.models import User
 from gettext import gettext as _
 
 from plinth import cfg
-from plinth import actions
 
 __all__ = ['init']
 
@@ -36,14 +33,3 @@ def init():
     menu = cfg.main_menu.get('system:index')
     menu.add_urlname(_('Users and Groups'), 'glyphicon-user', 'users:index',
                      15)
-
-    post_delete.connect(delete_posix_user)
-
-
-def delete_posix_user(sender, instance=None, **kwargs):
-    try:
-        instance
-    except User.DoesNotExist:
-        pass
-    else:
-        actions.superuser_run('delete-user', [instance.username])

plinth/modules/users/forms.py

@@ -16,29 +16,42 @@
 #
 
 from django import forms
+from django.contrib import messages
 from django.contrib.auth.models import User
 from django.contrib.auth.forms import UserCreationForm, SetPasswordForm
 from gettext import gettext as _
 
 from plinth import actions
+from plinth.errors import ActionError
 
 
 class CreateUserForm(UserCreationForm):
     """Custom user create form with option to also create POSIX user."""
 
     add_posix_user = forms.BooleanField(
-        label=_('Also create POSIX user'),
+        label=_('Also create a POSIX system user'),
         required=False,
         help_text=_('This will allow the new user to log in to the system '
                     'through SSH. The new user will also have administrative '
                     'privileges (sudo).'))
 
+    def __init__(self, request, *args, **kwargs):
+        """Initialize the form with extra request argument."""
+        self.request = request
+        super(CreateUserForm, self).__init__(*args, **kwargs)
+
     def save(self, commit=True):
+        """Save the user model and create POSIX user if required."""
         user = super(CreateUserForm, self).save(commit)
         if commit and self.cleaned_data['add_posix_user']:
-            actions.superuser_run(
+            try:
-                'create-user',
+                actions.superuser_run(
-                [user.username, self.cleaned_data['password1']])
+                    'create-user',
+                    [user.get_username(), self.cleaned_data['password1']])
+            except ActionError:
+                messages.error(self.request,
+                               _('Creating POSIX system user failed.'))
+
         return user
 
 
@@ -46,29 +59,57 @@ class UserUpdateForm(forms.ModelForm):
     """When user is enabled/disabled, also enables/disables the POSIX user."""
 
     class Meta:
+        """Metadata to control automatic form building."""
         fields = ('username', 'groups', 'is_active')
         model = User
         widgets = {
             'groups': forms.widgets.CheckboxSelectMultiple(),
         }
 
+    def __init__(self, request, username, *args, **kwargs):
+        """Initialize the form with extra request argument."""
+        self.request = request
+        self.username = username
+        super(UserUpdateForm, self).__init__(*args, **kwargs)
+
     def save(self, commit=True):
+        """Enable/disable POSIX user after saving user model."""
         user = super(UserUpdateForm, self).save(commit)
+
         if commit:
-            if user.is_active:
+            try:
-                actions.superuser_run('enable-user', [user.username])
+                if user.is_active:
-            else:
+                    actions.superuser_run('enable-user', [user.get_username()])
-                actions.superuser_run('disable-user', [user.username])
+                else:
+                    actions.superuser_run('disable-user',
+                                          [user.get_username()])
+            except ActionError:
+                messages.error(
+                    self.request,
+                    _('Setting active status for POSIX system user failed.'))
+
         return user
 
 
 class UserChangePasswordForm(SetPasswordForm):
     """Custom form that also updates password for POSIX users."""
 
+    def __init__(self, request, *args, **kwargs):
+        """Initialize the form with extra request argument."""
+        self.request = request
+        super(UserChangePasswordForm, self).__init__(*args, **kwargs)
+
     def save(self, commit=True):
+        """Save the user model and change POSIX password as well."""
         user = super(UserChangePasswordForm, self).save(commit)
         if commit:
-            actions.superuser_run(
+            try:
-                'change-user-password',
+                actions.superuser_run(
-                [user.username, self.cleaned_data['new_password1']])
+                    'change-user-password',
+                    [user.get_username(), self.cleaned_data['new_password1']])
+            except ActionError:
+                messages.error(
+                    self.request,
+                    _('Changing POSIX system user password failed.'))
+
         return user

plinth/modules/users/views.py

@@ -25,8 +25,9 @@ from django.views.generic.edit import (CreateView, DeleteView, UpdateView,
 from django.views.generic import ListView
 from gettext import gettext as _
 
-from .forms import CreateUserForm, UserUpdateForm, UserChangePasswordForm
+from .forms import CreateUserForm, UserChangePasswordForm, UserUpdateForm
 from plinth import actions
+from plinth.errors import ActionError
 
 
 subsubmenu = [{'url': reverse_lazy('users:index'),
@@ -54,6 +55,12 @@ class UserCreate(ContextMixin, SuccessMessageMixin, CreateView):
     success_url = reverse_lazy('users:create')
     title = _('Create User')
 
+    def get_form_kwargs(self):
+        """Make the request object available to the form."""
+        kwargs = super(UserCreate, self).get_form_kwargs()
+        kwargs['request'] = self.request
+        return kwargs
+
 
 class UserList(ContextMixin, ListView):
     """View to list users."""
@@ -71,7 +78,15 @@ class UserUpdate(ContextMixin, SuccessMessageMixin, UpdateView):
     success_message = _('User %(username)s updated.')
     title = _('Edit User')
 
+    def get_form_kwargs(self):
+        """Make the requst object available to the form."""
+        kwargs = super(UserUpdate, self).get_form_kwargs()
+        kwargs['request'] = self.request
+        kwargs['username'] = self.object.username
+        return kwargs
+
     def get_context_data(self, **kwargs):
+        """Return the data to be used for rendering templates."""
         context = super(UserUpdate, self).get_context_data(**kwargs)
         output = actions.run('check-user-exists', [self.object.username])
         context['is_posix_user'] = 'User exists' in output
@@ -107,9 +122,17 @@ class UserDelete(ContextMixin, DeleteView):
         The SuccessMessageMixin doesn't work with the DeleteView on Django1.7,
         so set the success message manually here.
         """
-        message = _('User %s deleted.') % self.kwargs['slug']
         output = super(UserDelete, self).delete(*args, **kwargs)
+
+        message = _('User %s deleted.') % self.kwargs['slug']
         messages.success(self.request, message)
+
+        try:
+            actions.superuser_run('delete-user', [self.kwargs['slug']])
+        except ActionError:
+            messages.error(self.request,
+                           _('Deleting POSIX system user failed.'))
+
         return output
 
 
@@ -121,8 +144,9 @@ class UserChangePassword(ContextMixin, SuccessMessageMixin, FormView):
     success_message = _('Password changed successfully.')
 
     def get_form_kwargs(self):
-        """Make the user object available to the form"""
+        """Make the user object available to the form."""
         kwargs = super(UserChangePassword, self).get_form_kwargs()
+        kwargs['request'] = self.request
         kwargs['user'] = User.objects.get(username=self.kwargs['slug'])
         return kwargs
 
@@ -134,9 +158,17 @@ class UserChangePassword(ContextMixin, SuccessMessageMixin, FormView):
         return context
 
     def get_success_url(self):
+        """Return the URL to go to on successful sumbission."""
         return reverse('users:edit', kwargs={'slug': self.kwargs['slug']})
 
     def form_valid(self, form):
+        """Save the form if the submission is valid.
+
+        Django user session authentication hashes are based on password to have
+        the ability to logout all sessions on password change.  Update the
+        session authentications to ensure that the current sessions is not
+        logged out.
+        """
         form.save()
         update_session_auth_hash(self.request, form.user)
         return super(UserChangePassword, self).form_valid(form)