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

Proposed by Ricardo Bánffy on 2015-10-27
Status: Merged
Approved by: Ricardo Bánffy on 2015-10-28
Approved revision: 4427
Merged at revision: 4424
Proposed branch: lp:~rbanffy/maas/1510210-cant-delete-user
Merge into: lp:maas/trunk
Diff against target: 269 lines (+131/-6)
7 files modified
src/maasserver/api/tests/test_user.py (+92/-0)
src/maasserver/api/users.py (+26/-1)
src/maasserver/models/filestorage.py (+3/-1)
src/maasserver/models/node.py (+3/-1)
src/maasserver/models/sshkey.py (+2/-1)
src/maasserver/models/sslkey.py (+2/-1)
src/maasserver/models/staticipaddress.py (+3/-1)
To merge this branch: bzr merge lp:~rbanffy/maas/1510210-cant-delete-user
Reviewer Review Type Date Requested Status
Blake Rouse 2015-10-27 Approve on 2015-10-27
Review via email: mp+275789@code.launchpad.net

Commit message

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

To post a comment you must log in.
Blake Rouse (blake-rouse) wrote :

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

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

Done

Blake Rouse (blake-rouse) wrote :

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

review: Approve
Ricardo Bánffy (rbanffy) wrote :

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

/me slaps forehead

MAAS Lander (maas-lander) wrote :
Download full text (61.4 KiB)

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 ...

4427. By Ricardo Bánffy on 2015-10-28

Fix test

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api/tests/test_user.py'
2--- src/maasserver/api/tests/test_user.py 2014-09-10 16:20:31 +0000
3+++ src/maasserver/api/tests/test_user.py 2015-10-28 10:52:32 +0000
4@@ -19,6 +19,14 @@
5
6 from django.contrib.auth.models import User
7 from django.core.urlresolvers import reverse
8+from maasserver.enum import (
9+ IPADDRESS_TYPE,
10+ NODE_STATUS,
11+)
12+from maasserver.models import (
13+ SSHKey,
14+ SSLKey,
15+)
16 from maasserver.testing.api import APITestCase
17 from maasserver.testing.factory import factory
18 from testtools.matchers import ContainsAll
19@@ -164,3 +172,87 @@
20 self.assertEqual(
21 httplib.NOT_FOUND, response.status_code, response.status_code)
22 self.assertItemsEqual([], User.objects.filter(username=nonuser))
23+
24+ def test_DELETE_requires_admin_privileges(self):
25+ user = factory.make_User()
26+ response = self.client.delete(
27+ reverse('user_handler', args=[user.username]))
28+ self.assertEqual(
29+ httplib.FORBIDDEN, response.status_code, response.status_code)
30+ self.assertTrue(User.objects.filter(username=user.username).exists())
31+
32+ def test_DELETE_requires_admin_privileges_with_invalid_user(self):
33+ """If the user has no admin privileges, it doesn't matter if the user
34+ being deleted exists or not, we will return a 403."""
35+ nonuser = factory.make_name('nonuser')
36+ response = self.client.delete(reverse('user_handler', args=[nonuser]))
37+ self.assertEqual(
38+ httplib.FORBIDDEN, response.status_code, response.status_code)
39+
40+ def test_DELETE_keeps_quiet_if_user_not_found(self):
41+ self.become_admin()
42+ nonuser = factory.make_name('nonuser')
43+ response = self.client.delete(reverse('user_handler', args=[nonuser]))
44+ self.assertEqual(
45+ httplib.NO_CONTENT, response.status_code, response.status_code)
46+
47+ def test_DELETE_admin_cannot_delete_self(self):
48+ self.become_admin()
49+ user = self.logged_in_user
50+ response = self.client.delete(
51+ reverse('user_handler', args=[user.username]))
52+ self.assertEqual(
53+ httplib.BAD_REQUEST, response.status_code, response.status_code)
54+ self.assertTrue(User.objects.filter(username=user.username).exists())
55+ self.assertIn('cannot self-delete', response.content)
56+
57+ def test_DELETE_deletes_user(self):
58+ self.become_admin()
59+ user = factory.make_User()
60+ response = self.client.delete(
61+ reverse('user_handler', args=[user.username]))
62+ self.assertEqual(
63+ httplib.NO_CONTENT, response.status_code, response.status_code)
64+ self.assertItemsEqual([], User.objects.filter(username=user.username))
65+
66+ def test_DELETE_user_with_node_fails(self):
67+ self.become_admin()
68+ user = factory.make_User()
69+ factory.make_Node(owner=user, status=NODE_STATUS.DEPLOYED)
70+ response = self.client.delete(
71+ reverse('user_handler', args=[user.username]))
72+ self.assertEqual(
73+ httplib.BAD_REQUEST, response.status_code, response.status_code)
74+ self.assertIn('assigned nodes cannot be deleted', response.content)
75+
76+ def test_DELETE_user_with_staticaddress_fails(self):
77+ self.become_admin()
78+ user = factory.make_User()
79+ factory.make_StaticIPAddress(
80+ user=user, alloc_type=IPADDRESS_TYPE.USER_RESERVED)
81+ response = self.client.delete(
82+ reverse('user_handler', args=[user.username]))
83+ self.assertEqual(
84+ httplib.BAD_REQUEST, response.status_code, response.status_code)
85+ self.assertIn(
86+ 'with reserved IP addresses cannot be deleted', response.content)
87+
88+ def test_DELETE_user_with_sslkey_deletes_key(self):
89+ self.become_admin()
90+ user = factory.make_User()
91+ key_id = factory.make_SSLKey(user=user).id
92+ response = self.client.delete(
93+ reverse('user_handler', args=[user.username]))
94+ self.assertEqual(
95+ httplib.NO_CONTENT, response.status_code, response.status_code)
96+ self.assertFalse(SSLKey.objects.filter(id=key_id).exists())
97+
98+ def test_DELETE_user_with_sshkey_deletes_key(self):
99+ self.become_admin()
100+ user = factory.make_User()
101+ key_id = factory.make_SSHKey(user=user).id
102+ response = self.client.delete(
103+ reverse('user_handler', args=[user.username]))
104+ self.assertEqual(
105+ httplib.NO_CONTENT, response.status_code, response.status_code)
106+ self.assertFalse(SSHKey.objects.filter(id=key_id).exists())
107
108=== modified file 'src/maasserver/api/users.py'
109--- src/maasserver/api/users.py 2015-05-07 18:14:38 +0000
110+++ src/maasserver/api/users.py 2015-10-28 10:52:32 +0000
111@@ -18,6 +18,7 @@
112 ]
113
114
115+from django.core.exceptions import ValidationError
116 from django.shortcuts import get_object_or_404
117 from maasserver.api.support import (
118 admin_method,
119@@ -28,6 +29,9 @@
120 get_mandatory_param,
121 )
122 from maasserver.models import User
123+from maasserver.utils.orm import get_one
124+from piston.models import Consumer
125+from piston.utils import rc
126
127
128 class UsersHandler(OperationsHandler):
129@@ -79,7 +83,7 @@
130 class UserHandler(OperationsHandler):
131 """Manage a user account."""
132 api_doc_section_name = "User"
133- create = update = delete = None
134+ create = update = None
135
136 model = User
137 fields = (
138@@ -90,3 +94,24 @@
139
140 def read(self, request, username):
141 return get_object_or_404(User, username=username)
142+
143+ @admin_method
144+ def delete(self, request, username):
145+ """Deletes a user"""
146+ if request.user.username == username:
147+ raise ValidationError("An administrator cannot self-delete.")
148+
149+ user = get_one(User.objects.filter(username=username))
150+
151+ if user is not None:
152+ if user.node_set.exists():
153+ raise ValidationError(
154+ "A user with assigned nodes cannot be deleted.")
155+ elif user.staticipaddress_set.exists():
156+ raise ValidationError(
157+ "A user with reserved IP addresses cannot be deleted.")
158+ else:
159+ Consumer.objects.filter(user=user).delete()
160+ user.delete()
161+
162+ return rc.DELETED
163
164=== modified file 'src/maasserver/models/filestorage.py'
165--- src/maasserver/models/filestorage.py 2015-05-07 18:14:38 +0000
166+++ src/maasserver/models/filestorage.py 2015-10-28 10:52:32 +0000
167@@ -26,6 +26,7 @@
168 ForeignKey,
169 Manager,
170 Model,
171+ PROTECT,
172 )
173 from django.utils.http import urlencode
174 from maasserver import DefaultMeta
175@@ -89,7 +90,8 @@
176 # owner can be None: this is to support upgrading existing
177 # installations where the files were not linked to users yet.
178 owner = ForeignKey(
179- User, default=None, blank=True, null=True, editable=False)
180+ User, default=None, blank=True, null=True, editable=False,
181+ on_delete=PROTECT)
182 key = CharField(
183 max_length=36, unique=True, default=generate_filestorage_key,
184 editable=False)
185
186=== modified file 'src/maasserver/models/node.py'
187--- src/maasserver/models/node.py 2015-10-28 02:02:45 +0000
188+++ src/maasserver/models/node.py 2015-10-28 10:52:32 +0000
189@@ -44,6 +44,7 @@
190 IntegerField,
191 Manager,
192 ManyToManyField,
193+ PROTECT,
194 Q,
195 SET_DEFAULT,
196 SET_NULL,
197@@ -447,7 +448,8 @@
198 default=NODE_STATUS.DEFAULT)
199
200 owner = ForeignKey(
201- User, default=None, blank=True, null=True, editable=False)
202+ User, default=None, blank=True, null=True, editable=False,
203+ on_delete=PROTECT)
204
205 bios_boot_method = CharField(max_length=31, blank=True, null=True)
206
207
208=== modified file 'src/maasserver/models/sshkey.py'
209--- src/maasserver/models/sshkey.py 2015-05-07 18:14:38 +0000
210+++ src/maasserver/models/sshkey.py 2015-10-28 10:52:32 +0000
211@@ -22,6 +22,7 @@
212 from django.contrib.auth.models import User
213 from django.core.exceptions import ValidationError
214 from django.db.models import (
215+ CASCADE,
216 ForeignKey,
217 Manager,
218 TextField,
219@@ -124,7 +125,7 @@
220
221 objects = SSHKeyManager()
222
223- user = ForeignKey(User, null=False, editable=False)
224+ user = ForeignKey(User, null=False, editable=False, on_delete=CASCADE)
225
226 key = TextField(
227 null=False, editable=True, validators=[validate_ssh_public_key])
228
229=== modified file 'src/maasserver/models/sslkey.py'
230--- src/maasserver/models/sslkey.py 2015-09-24 16:22:12 +0000
231+++ src/maasserver/models/sslkey.py 2015-10-28 10:52:32 +0000
232@@ -23,6 +23,7 @@
233 from django.contrib.auth.models import User
234 from django.core.exceptions import ValidationError
235 from django.db.models import (
236+ CASCADE,
237 ForeignKey,
238 Manager,
239 TextField,
240@@ -93,7 +94,7 @@
241 """
242 objects = SSLKeyManager()
243
244- user = ForeignKey(User, null=False, editable=False)
245+ user = ForeignKey(User, null=False, editable=False, on_delete=CASCADE)
246 key = TextField(
247 null=False, blank=False, editable=True, validators=[validate_ssl_key])
248
249
250=== modified file 'src/maasserver/models/staticipaddress.py'
251--- src/maasserver/models/staticipaddress.py 2015-09-30 18:42:27 +0000
252+++ src/maasserver/models/staticipaddress.py 2015-10-28 10:52:32 +0000
253@@ -36,6 +36,7 @@
254 ForeignKey,
255 IntegerField,
256 Manager,
257+ PROTECT,
258 )
259 from maasserver import (
260 DefaultMeta,
261@@ -594,7 +595,8 @@
262 validators=[validate_hostname])
263
264 user = ForeignKey(
265- User, default=None, blank=True, null=True, editable=False)
266+ User, default=None, blank=True, null=True, editable=False,
267+ on_delete=PROTECT)
268
269 objects = StaticIPAddressManager()
270