launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07029
[Merge] lp:~rvb/maas/always-full_clean into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/always-full_clean into lp:maas with lp:~rvb/maas/ssh-key-bug-971681 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/always-full_clean/+merge/101079
This branch changes CommonInfo.save to always call full_clean (is skip_check=False which is true by default) before saving a model. This is not done by default by Django for backward compatibility reasons but we don't have that restriction (we already do that for nodes). The only trouble is that now SSHKey.save properly validates the content of the key so I had to rework the tests around SSHKey to have them pass again.
--
https://code.launchpad.net/~rvb/maas/always-full_clean/+merge/101079
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/always-full_clean into lp:maas.
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-04-06 09:58:19 +0000
+++ src/maasserver/models.py 2012-04-06 09:58:19 +0000
@@ -91,8 +91,9 @@
class CommonInfo(models.Model):
- """A base model which records the creation date and the last modification
- date.
+ """A base model which:
+ - calls full_clean before saving the model by default.
+ - records the creation date and the last modification date.
:ivar created: The creation date.
:ivar updated: The last modification date.
@@ -104,11 +105,13 @@
class Meta:
abstract = True
- def save(self, *args, **kwargs):
+ def save(self, skip_check=False, *args, **kwargs):
if not self.id:
self.created = datetime.date.today()
self.updated = datetime.datetime.today()
- super(CommonInfo, self).save(*args, **kwargs)
+ if not skip_check:
+ self.full_clean()
+ return super(CommonInfo, self).save(*args, **kwargs)
def generate_node_system_id():
@@ -546,11 +549,6 @@
super(Node, self).clean(*args, **kwargs)
self.clean_status()
- def save(self, skip_check=False, *args, **kwargs):
- if not skip_check:
- self.full_clean()
- return super(Node, self).save(*args, **kwargs)
-
def display_status(self):
"""Return status text as displayed to the user.
@@ -580,7 +578,6 @@
"""
mac = MACAddress(mac_address=mac_address, node=self)
- mac.full_clean()
mac.save()
return mac
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py 2012-04-06 09:58:19 +0000
+++ src/maasserver/testing/factory.py 2012-04-06 09:58:19 +0000
@@ -30,6 +30,10 @@
from maasserver.testing.enum import map_enum
import maastesting.factory
+# We have a limited number of public keys:
+# src/maasserver/tests/data/test_rsa{0, 1, 2, 3, 4}.pub
+MAX_PUBLIC_KEYS = 5
+
class Factory(maastesting.factory.Factory):
@@ -91,25 +95,29 @@
def make_sshkey(self, user, key_string=None):
if key_string is None:
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
key = SSHKey(key=key_string, user=user)
key.save()
return key
- def make_user_with_keys(self, n_keys=2, **kwargs):
- """Create a user with n `SSHKey`.
+ def make_user_with_keys(self, n_keys=2, user=None, **kwargs):
+ """Create a user with n `SSHKey`. If user is not None, use this user
+ instead of creating one.
Additional keyword arguments are passed to `make_user()`.
-
- Keys will have a comment of the form: <username>-key-<i> where i
- is the key index.
"""
- user = self.make_user(**kwargs)
+ if n_keys > MAX_PUBLIC_KEYS:
+ raise RuntimeError(
+ "Cannot create more than %d public keys." % MAX_PUBLIC_KEYS)
+ if user is None:
+ user = self.make_user(**kwargs)
+ keys = []
for i in range(n_keys):
- SSHKey(
- user=user,
- key='ssh-rsa KEY %s-key-%d' % (user.username, i)).save()
- return user
+ key_string = get_data('data/test_rsa%d.pub' % i)
+ key = SSHKey(user=user, key=key_string)
+ key.save()
+ keys.append(key)
+ return user, keys
def make_admin(self, username=None, password=None, email=None):
admin = self.make_user(
=== removed file 'src/maasserver/tests/data/test_rsa.pub'
--- src/maasserver/tests/data/test_rsa.pub 2012-03-28 10:45:23 +0000
+++ src/maasserver/tests/data/test_rsa.pub 1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
-ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDdrzzDZNwyMVBvBTT6kBnrfPZv/AUbkxj7G5CaMTdw6xkKthV22EntD3lxaQxRKzQTfCc2d/CC1K4ushCcRs1S6SQ2zJ2jDq1UmOUkDMgvNh4JVhJYSKc6mu8i3s7oGSmBado5wvtlpSzMrscOpf8Qe/wmT5fH12KB9ipJqoFNQMVbVcVarE/v6wpn3GZC62YRb5iaz9/M+t92Qhu50W2u+KfouqtKB2lwIDDKZMww38ExtdMouh2FZpxaoh4Uey5bRp3tM3JgnWcX6fyUOp2gxJRPIlD9rrZhX5IkEkZM8MQbdPTQLgIf98oFph5RG6w1t02BvI9nJKM7KkKEfBHt ubuntu@server-7476
=== added file 'src/maasserver/tests/data/test_rsa0.pub'
--- src/maasserver/tests/data/test_rsa0.pub 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_rsa0.pub 2012-04-06 09:58:19 +0000
@@ -0,0 +1,1 @@
+ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDdrzzDZNwyMVBvBTT6kBnrfPZv/AUbkxj7G5CaMTdw6xkKthV22EntD3lxaQxRKzQTfCc2d/CC1K4ushCcRs1S6SQ2zJ2jDq1UmOUkDMgvNh4JVhJYSKc6mu8i3s7oGSmBado5wvtlpSzMrscOpf8Qe/wmT5fH12KB9ipJqoFNQMVbVcVarE/v6wpn3GZC62YRb5iaz9/M+t92Qhu50W2u+KfouqtKB2lwIDDKZMww38ExtdMouh2FZpxaoh4Uey5bRp3tM3JgnWcX6fyUOp2gxJRPIlD9rrZhX5IkEkZM8MQbdPTQLgIf98oFph5RG6w1t02BvI9nJKM7KkKEfBHt ubuntu@server-7476
=== added file 'src/maasserver/tests/data/test_rsa1.pub'
--- src/maasserver/tests/data/test_rsa1.pub 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_rsa1.pub 2012-04-06 09:58:19 +0000
@@ -0,0 +1,1 @@
+ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC6Gkj1y8/0T7q/FqBSr9xRBO9GzT+JeoWNXaqhUBg179Zd53XM4qblVwz/rsMa70te8CYNIFU+GbcNY1tNCo78NlHjQA8H98COnbVWKxvABECHrJ8nbYB4lWH9wI8/uvR0um6yUb/tZYbiSqnQxhoGAF/uQQfhqzc+tc7uTjnsa6krrNqQCdpFbAVVy+vZzvcJl6CX8nu5uJ8jedWfXOZJFcQPH+VwkUT0oV+1zVeLpE4LFkRO52JrC9Dy1xgrYM0EhcrShBdD1GQx9IXdW4Z9PIaVcq/y4Qv574yHMvi+6hwG6xpCtRXmy0lG0LiG60c1yOredkO6U0MJIVbeZ/+r ubuntu@server-7493
=== added file 'src/maasserver/tests/data/test_rsa2.pub'
--- src/maasserver/tests/data/test_rsa2.pub 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_rsa2.pub 2012-04-06 09:58:19 +0000
@@ -0,0 +1,1 @@
+ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDKVdMk4Q+13uUvXjb6iU+oB2Auk0HpaILZ8Pw/V63PTJ+QXtEp0vTe6DEvr9uF2vl6tF+AosiG4krEwqBNGx/h8MmFO7BgNTxn9eU2VwfHzmQ2nqkXHsXgp66cNT0Yd0nfvVV/fsMpKN9fUaYrXjAlFxvC9iQ33Rp6vj/X+oqDvYf3xZjbuZy+BxdJnmiTAJcFouTyrdy1Em1EZITq5M4EXw93/O2vAPYSFPAeELBE+mIMJxOCY1Fm101oAqO0qof3Rb2hZxc2WINjmqZIxoi+sviU0ny/dIFknhYEg1Xh2hObPn0nN5+4VHjBTdRmpRXqggotc53sYC5udVmFsW8B ubuntu@server-7493
=== added file 'src/maasserver/tests/data/test_rsa3.pub'
--- src/maasserver/tests/data/test_rsa3.pub 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_rsa3.pub 2012-04-06 09:58:19 +0000
@@ -0,0 +1,1 @@
+ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDai2ir5yxckoYTHUbFL6pe01Kx+Dy6nw9p7LhFaBixUOh8G7eIgFBguYcir2ZKBfM/lbTnW+MSiGF2VMlXX0+X9Ux2iwPSJa2wIA7Cc5prCz/RnMRKQ+2S1JJuORoi8tDI0p1R0sGWMXCwaj30oRN0THWz884+d3YlDD/O39h74gnLNEx/TQig/r/Aev3VfeKO6dlbbX81vSad2JVncislyMq1TgJdhn2/JI8t+LW0xVc6ZgQr94YB2M2DNjFSisP2vDrV5LWM+IqiF8T/YHkcSsANr8WWvZWa79uHyRBU3xr2qZZqMjMVL0B/NOJYXyGBIJ7HQnlVLmqFenKl8ZtL ubuntu@server-7493
=== added file 'src/maasserver/tests/data/test_rsa4.pub'
--- src/maasserver/tests/data/test_rsa4.pub 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/data/test_rsa4.pub 2012-04-06 09:58:19 +0000
@@ -0,0 +1,1 @@
+ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDNDA4vXVTxHuKikIXeA6/K/X7hKpJcOJV0HcXUHlSNa9phNW0f8vbci+BxcLAqIz/U+BPiQ9lCxz7so+qCTFrM4poOdkTyup8VUxUqntiaxgiCJZ1of+eMe39+S9XQk6RogiCpExanhD9xPLkK/mLr5phnQwDjEDJwD4OOF0rYsbYoqje/0Pd+Tm0PIepq/qwsu5PAKPJU8dfnp8BWLCuIJ+DA2lfRUjmxWwLczfM/4hu1bZlYp1mzJJgMIOY92/pUToYxvBiIiKs3qWh6HC5Vxo5Vz4w5WLnTnIPDvpYBvWj8LGXJwHuhqlzed2icwPk8krip2BzwsHotru3UXtKf ubuntu@server-7493
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-04-06 09:58:19 +0000
+++ src/maasserver/tests/test_models.py 2012-04-06 09:58:19 +0000
@@ -767,7 +767,7 @@
class SSHKeyValidatorTest(TestCase):
def test_validates_rsa_public_key(self):
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
validate_ssh_public_key(key_string)
# No ValidationError.
@@ -941,7 +941,7 @@
"""Testing for the :class:`SSHKey`."""
def test_sshkey_validation_with_valid_key(self):
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
user = factory.make_user()
key = SSHKey(key=key_string, user=user)
key.full_clean()
@@ -956,7 +956,7 @@
def test_sshkey_display_with_real_life_key(self):
# With a real-life ssh-rsa key, the key_string part is cropped.
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
user = factory.make_user()
key = SSHKey(key=key_string, user=user)
display = key.display_html()
@@ -964,14 +964,14 @@
'ssh-rsa AAAAB3NzaC1yc2E… ubuntu@server-7476', display)
def test_sshkey_display_is_marked_as_HTML_safe(self):
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
user = factory.make_user()
key = SSHKey(key=key_string, user=user)
display = key.display_html()
self.assertIsInstance(display, SafeUnicode)
def test_sshkey_user_and_key_unique_together(self):
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
user = factory.make_user()
key = SSHKey(key=key_string, user=user)
key.save()
@@ -980,7 +980,7 @@
ValidationError, key2.full_clean)
def test_sshkey_same_key_can_be_used_by_different_users(self):
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
user = factory.make_user()
key = SSHKey(key=key_string, user=user)
key.save()
@@ -999,15 +999,12 @@
self.assertItemsEqual([], keys)
def test_get_keys_for_user_with_keys(self):
- user1 = factory.make_user_with_keys(n_keys=3, username='user1')
+ user1, created_keys = factory.make_user_with_keys(
+ n_keys=3, username='user1')
# user2
factory.make_user_with_keys(n_keys=2)
keys = SSHKey.objects.get_keys_for_user(user1)
- self.assertItemsEqual([
- 'ssh-rsa KEY user1-key-0',
- 'ssh-rsa KEY user1-key-1',
- 'ssh-rsa KEY user1-key-2',
- ], keys)
+ self.assertItemsEqual([key.key for key in created_keys], keys)
class FileStorageTest(TestCase):
=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py 2012-04-06 09:58:19 +0000
+++ src/maasserver/tests/test_views.py 2012-04-06 09:58:19 +0000
@@ -386,22 +386,14 @@
add_key_link = reverse('prefs-add-sshkey')
self.assertIn(add_key_link, get_content_links(response))
- def create_keys_for_user(self, user):
- key_strings = [
- get_data('data/test_rsa.pub'), get_data('data/test_dsa.pub')]
- return [
- factory.make_sshkey(
- user=self.logged_in_user, key_string=key_string)
- for key_string in key_strings]
-
def test_prefs_displays_compact_representation_of_users_keys(self):
- keys = self.create_keys_for_user(self.logged_in_user)
+ _, keys = factory.make_user_with_keys(user=self.logged_in_user)
response = self.client.get('/account/prefs/')
for key in keys:
self.assertIn(key.display_html(), response.content)
def test_prefs_displays_link_to_delete_ssh_keys(self):
- keys = self.create_keys_for_user(self.logged_in_user)
+ _, keys = factory.make_user_with_keys(user=self.logged_in_user)
response = self.client.get('/account/prefs/')
links = get_content_links(response)
for key in keys:
@@ -424,7 +416,7 @@
'#content form')])
def test_add_key_POST_adds_key(self):
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
response = self.client.post(
reverse('prefs-add-sshkey'), {'key': key_string})
@@ -432,7 +424,7 @@
self.assertTrue(SSHKey.objects.filter(key=key_string).exists())
def test_add_key_POST_fails_if_key_already_exists_for_the_user(self):
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
key = SSHKey(user=self.logged_in_user, key=key_string)
key.save()
response = self.client.post(
@@ -445,7 +437,7 @@
self.assertItemsEqual([key], SSHKey.objects.filter(key=key_string))
def test_key_can_be_added_if_same_key_already_setup_for_other_user(self):
- key_string = get_data('data/test_rsa.pub')
+ key_string = get_data('data/test_rsa0.pub')
key = SSHKey(user=factory.make_user(), key=key_string)
key.save()
response = self.client.post(
=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py 2012-04-04 17:38:19 +0000
+++ src/metadataserver/tests/test_api.py 2012-04-06 09:58:19 +0000
@@ -13,10 +13,12 @@
from collections import namedtuple
import httplib
-from textwrap import dedent
from maasserver.exceptions import Unauthorized
-from maasserver.models import NODE_STATUS
+from maasserver.models import (
+ NODE_STATUS,
+ SSHKey,
+ )
from maasserver.provisioning import get_provisioning_api_proxy
from maasserver.testing import reload_object
from maasserver.testing.factory import factory
@@ -151,7 +153,7 @@
def test_meta_data_view_lists_fields(self):
# Some fields only are returned if there is data related to them.
- user = factory.make_user_with_keys(n_keys=2, username='my-user')
+ user, _ = factory.make_user_with_keys(n_keys=2, username='my-user')
node = factory.make_node(owner=user)
client = self.make_node_client(node=node)
response = self.get('/latest/meta-data/', client)
@@ -214,7 +216,7 @@
'public-keys', response.content.decode('ascii').split('\n'))
def test_public_keys_listed_for_node_with_public_keys(self):
- user = factory.make_user_with_keys(n_keys=2, username='my-user')
+ user, _ = factory.make_user_with_keys(n_keys=2, username='my-user')
node = factory.make_node(owner=user)
response = self.get(
'/latest/meta-data/', self.make_node_client(node=node))
@@ -227,14 +229,15 @@
self.assertEqual(httplib.NOT_FOUND, response.status_code)
def test_public_keys_for_node_returns_list_of_keys(self):
- user = factory.make_user_with_keys(n_keys=2, username='my-user')
+ user, _ = factory.make_user_with_keys(n_keys=2, username='my-user')
node = factory.make_node(owner=user)
response = self.get(
'/latest/meta-data/public-keys', self.make_node_client(node=node))
self.assertEqual(httplib.OK, response.status_code)
- self.assertEquals(dedent("""\
- ssh-rsa KEY my-user-key-0
- ssh-rsa KEY my-user-key-1"""),
+ keys = SSHKey.objects.filter(user=user).values_list('key', flat=True)
+ expected_response = '\n'.join(keys)
+ self.assertItemsEqual(
+ expected_response,
response.content.decode('ascii'))
self.assertIn('text/plain', response['Content-Type'])
