Merge lp:~rbanffy/maas/1510210-cant-delete-user into lp:~maas-committers/maas/trunk

[Ricardo Bánffy]

Allows administrators to delete a user that has no attached nodes, IP address reservation or SSH/SSL keys. Prevent self-deletion.

[Blake Rouse (blake-rouse)]

Looks generally good, but I think your to strict on the checking. We should not prevent the user from being deleted if they have ssh keys or ssl keys. Those are directly related to the user, we can just delete those when the user is deleted. Actually you can set those to CASCADE and they will be deleted for you, I believe.

review: Needs Fixing

[Ricardo Bánffy (rbanffy)]

> Looks generally good, but I think your to strict on the checking. We should
> not prevent the user from being deleted if they have ssh keys or ssl keys.
> Those are directly related to the user, we can just delete those when the user
> is deleted. Actually you can set those to CASCADE and they will be deleted for
> you, I believe.

Yes. That's an option. Just need to remove the extra checks and change the tests to reflect the new behavior.

[Ricardo Bánffy (rbanffy)]

Done

[Blake Rouse (blake-rouse)]

Looks good. But you need to update two test names.

review: Approve

[Ricardo Bánffy (rbanffy)]

> Looks good. But you need to update two test names.

/me slaps forehead

[MAAS Lander (maas-lander)]

The attempt to merge lp:~rbanffy/maas/1510210-cant-delete-user into lp:maas failed. Below is the output from the failed tests.

Get:1 http://security.ubuntu.com trusty-security InRelease [64.4 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates InRelease [64.4 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:3 http://security.ubuntu.com trusty-security/main Sources [97.7 kB]
Get:4 http://security.ubuntu.com trusty-security/universe Sources [31.0 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [241 kB]
Get:6 http://security.ubuntu.com trusty-security/main amd64 Packages [356 kB]
Get:7 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [140 kB]
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [117 kB]
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [636 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [324 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 2,071 kB in 4s (516 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-pyparsing python-seamicroclient python-simplejson python-simplestreams python-sphinx python-subunit python-tempita python-testresources python-testscenarios ...

=== modified file 'src/maasserver/api/tests/test_user.py'
--- src/maasserver/api/tests/test_user.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/api/tests/test_user.py	2015-10-28 10:52:32 +0000
@@ -19,6 +19,14 @@
 
 from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
+from maasserver.enum import (
+    IPADDRESS_TYPE,
+    NODE_STATUS,
+)
+from maasserver.models import (
+    SSHKey,
+    SSLKey,
+)
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
 from testtools.matchers import ContainsAll
@@ -164,3 +172,87 @@
         self.assertEqual(
             httplib.NOT_FOUND, response.status_code, response.status_code)
         self.assertItemsEqual([], User.objects.filter(username=nonuser))
+
+    def test_DELETE_requires_admin_privileges(self):
+        user = factory.make_User()
+        response = self.client.delete(
+            reverse('user_handler', args=[user.username]))
+        self.assertEqual(
+            httplib.FORBIDDEN, response.status_code, response.status_code)
+        self.assertTrue(User.objects.filter(username=user.username).exists())
+
+    def test_DELETE_requires_admin_privileges_with_invalid_user(self):
+        """If the user has no admin privileges, it doesn't matter if the user
+        being deleted exists or not, we will return a 403."""
+        nonuser = factory.make_name('nonuser')
+        response = self.client.delete(reverse('user_handler', args=[nonuser]))
+        self.assertEqual(
+            httplib.FORBIDDEN, response.status_code, response.status_code)
+
+    def test_DELETE_keeps_quiet_if_user_not_found(self):
+        self.become_admin()
+        nonuser = factory.make_name('nonuser')
+        response = self.client.delete(reverse('user_handler', args=[nonuser]))
+        self.assertEqual(
+            httplib.NO_CONTENT, response.status_code, response.status_code)
+
+    def test_DELETE_admin_cannot_delete_self(self):
+        self.become_admin()
+        user = self.logged_in_user
+        response = self.client.delete(
+            reverse('user_handler', args=[user.username]))
+        self.assertEqual(
+            httplib.BAD_REQUEST, response.status_code, response.status_code)
+        self.assertTrue(User.objects.filter(username=user.username).exists())
+        self.assertIn('cannot self-delete', response.content)
+
+    def test_DELETE_deletes_user(self):
+        self.become_admin()
+        user = factory.make_User()
+        response = self.client.delete(
+            reverse('user_handler', args=[user.username]))
+        self.assertEqual(
+            httplib.NO_CONTENT, response.status_code, response.status_code)
+        self.assertItemsEqual([], User.objects.filter(username=user.username))
+
+    def test_DELETE_user_with_node_fails(self):
+        self.become_admin()
+        user = factory.make_User()
+        factory.make_Node(owner=user, status=NODE_STATUS.DEPLOYED)
+        response = self.client.delete(
+            reverse('user_handler', args=[user.username]))
+        self.assertEqual(
+            httplib.BAD_REQUEST, response.status_code, response.status_code)
+        self.assertIn('assigned nodes cannot be deleted', response.content)
+
+    def test_DELETE_user_with_staticaddress_fails(self):
+        self.become_admin()
+        user = factory.make_User()
+        factory.make_StaticIPAddress(
+            user=user, alloc_type=IPADDRESS_TYPE.USER_RESERVED)
+        response = self.client.delete(
+            reverse('user_handler', args=[user.username]))
+        self.assertEqual(
+            httplib.BAD_REQUEST, response.status_code, response.status_code)
+        self.assertIn(
+            'with reserved IP addresses cannot be deleted', response.content)
+
+    def test_DELETE_user_with_sslkey_deletes_key(self):
+        self.become_admin()
+        user = factory.make_User()
+        key_id = factory.make_SSLKey(user=user).id
+        response = self.client.delete(
+            reverse('user_handler', args=[user.username]))
+        self.assertEqual(
+            httplib.NO_CONTENT, response.status_code, response.status_code)
+        self.assertFalse(SSLKey.objects.filter(id=key_id).exists())
+
+    def test_DELETE_user_with_sshkey_deletes_key(self):
+        self.become_admin()
+        user = factory.make_User()
+        key_id = factory.make_SSHKey(user=user).id
+        response = self.client.delete(
+            reverse('user_handler', args=[user.username]))
+        self.assertEqual(
+            httplib.NO_CONTENT, response.status_code, response.status_code)
+        self.assertFalse(SSHKey.objects.filter(id=key_id).exists())
 
=== modified file 'src/maasserver/api/users.py'
--- src/maasserver/api/users.py	2015-05-07 18:14:38 +0000
+++ src/maasserver/api/users.py	2015-10-28 10:52:32 +0000
@@ -18,6 +18,7 @@
     ]
 
 
+from django.core.exceptions import ValidationError
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import (
     admin_method,
@@ -28,6 +29,9 @@
     get_mandatory_param,
 )
 from maasserver.models import User
+from maasserver.utils.orm import get_one
+from piston.models import Consumer
+from piston.utils import rc
 
 
 class UsersHandler(OperationsHandler):
@@ -79,7 +83,7 @@
 class UserHandler(OperationsHandler):
     """Manage a user account."""
     api_doc_section_name = "User"
-    create = update = delete = None
+    create = update = None
 
     model = User
     fields = (
@@ -90,3 +94,24 @@
 
     def read(self, request, username):
         return get_object_or_404(User, username=username)
+
+    @admin_method
+    def delete(self, request, username):
+        """Deletes a user"""
+        if request.user.username == username:
+            raise ValidationError("An administrator cannot self-delete.")
+
+        user = get_one(User.objects.filter(username=username))
+
+        if user is not None:
+            if user.node_set.exists():
+                raise ValidationError(
+                    "A user with assigned nodes cannot be deleted.")
+            elif user.staticipaddress_set.exists():
+                raise ValidationError(
+                    "A user with reserved IP addresses cannot be deleted.")
+            else:
+                Consumer.objects.filter(user=user).delete()
+                user.delete()
+
+        return rc.DELETED
 
=== modified file 'src/maasserver/models/filestorage.py'
--- src/maasserver/models/filestorage.py	2015-05-07 18:14:38 +0000
+++ src/maasserver/models/filestorage.py	2015-10-28 10:52:32 +0000
@@ -26,6 +26,7 @@
     ForeignKey,
     Manager,
     Model,
+    PROTECT,
 )
 from django.utils.http import urlencode
 from maasserver import DefaultMeta
@@ -89,7 +90,8 @@
     # owner can be None: this is to support upgrading existing
     # installations where the files were not linked to users yet.
     owner = ForeignKey(
-        User, default=None, blank=True, null=True, editable=False)
+        User, default=None, blank=True, null=True, editable=False,
+        on_delete=PROTECT)
     key = CharField(
         max_length=36, unique=True, default=generate_filestorage_key,
         editable=False)
 
=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2015-10-28 02:02:45 +0000
+++ src/maasserver/models/node.py	2015-10-28 10:52:32 +0000
@@ -44,6 +44,7 @@
     IntegerField,
     Manager,
     ManyToManyField,
+    PROTECT,
     Q,
     SET_DEFAULT,
     SET_NULL,
@@ -447,7 +448,8 @@
         default=NODE_STATUS.DEFAULT)
 
     owner = ForeignKey(
-        User, default=None, blank=True, null=True, editable=False)
+        User, default=None, blank=True, null=True, editable=False,
+        on_delete=PROTECT)
 
     bios_boot_method = CharField(max_length=31, blank=True, null=True)
 
 
=== modified file 'src/maasserver/models/sshkey.py'
--- src/maasserver/models/sshkey.py	2015-05-07 18:14:38 +0000
+++ src/maasserver/models/sshkey.py	2015-10-28 10:52:32 +0000
@@ -22,6 +22,7 @@
 from django.contrib.auth.models import User
 from django.core.exceptions import ValidationError
 from django.db.models import (
+    CASCADE,
     ForeignKey,
     Manager,
     TextField,
@@ -124,7 +125,7 @@
 
     objects = SSHKeyManager()
 
-    user = ForeignKey(User, null=False, editable=False)
+    user = ForeignKey(User, null=False, editable=False, on_delete=CASCADE)
 
     key = TextField(
         null=False, editable=True, validators=[validate_ssh_public_key])
 
=== modified file 'src/maasserver/models/sslkey.py'
--- src/maasserver/models/sslkey.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/models/sslkey.py	2015-10-28 10:52:32 +0000
@@ -23,6 +23,7 @@
 from django.contrib.auth.models import User
 from django.core.exceptions import ValidationError
 from django.db.models import (
+    CASCADE,
     ForeignKey,
     Manager,
     TextField,
@@ -93,7 +94,7 @@
     """
     objects = SSLKeyManager()
 
-    user = ForeignKey(User, null=False, editable=False)
+    user = ForeignKey(User, null=False, editable=False, on_delete=CASCADE)
     key = TextField(
         null=False, blank=False, editable=True, validators=[validate_ssl_key])
 
 
=== modified file 'src/maasserver/models/staticipaddress.py'
--- src/maasserver/models/staticipaddress.py	2015-09-30 18:42:27 +0000
+++ src/maasserver/models/staticipaddress.py	2015-10-28 10:52:32 +0000
@@ -36,6 +36,7 @@
     ForeignKey,
     IntegerField,
     Manager,
+    PROTECT,
 )
 from maasserver import (
     DefaultMeta,
@@ -594,7 +595,8 @@
         validators=[validate_hostname])
 
     user = ForeignKey(
-        User, default=None, blank=True, null=True, editable=False)
+        User, default=None, blank=True, null=True, editable=False,
+        on_delete=PROTECT)
 
     objects = StaticIPAddressManager()