launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07028
[Merge] lp:~rvb/maas/ssh-key-bug-971681 into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/ssh-key-bug-971681 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #971681 in MAAS: "Add the same ssh key multiple times"
https://bugs.launchpad.net/maas/+bug/971681
For more details, see:
https://code.launchpad.net/~rvb/maas/ssh-key-bug-971681/+merge/101067
This branch changes SSHKey to force 'user' and 'key' to be unique together. This is enforced at the db level so a South db migration was created.
I've been forced to use a small trick in SSHKeyForm to circumvent a known bug in Django: SSHKeyForm does not include the 'user' field (it's set from the context) and thus the uniqueness validation ('user' and 'key') was only happening when form.save was called. We want the uniqueness problem to be detected when the form gets validated to be able to display a nice error message to the user.
--
https://code.launchpad.net/~rvb/maas/ssh-key-bug-971681/+merge/101067
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/ssh-key-bug-971681 into lp:maas.
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-04-05 07:34:05 +0000
+++ src/maasserver/forms.py 2012-04-06 08:51:18 +0000
@@ -135,10 +135,30 @@
super(SSHKeyForm, self).__init__(*args, **kwargs)
self.user = user
- def save(self):
- key = super(SSHKeyForm, self).save(commit=False)
- key.user = self.user
- return key.save()
+ def validate_unique(self):
+ # This is a trick to work around a problem in Django.
+ # See https://code.djangoproject.com/ticket/13091#comment:19 for
+ # details.
+ # Without this overridden validate_unique the validation error that
+ # can occur if this user already has the same key registered would
+ # occur when save() would be called. The error would be an
+ # IntegrityError raised when inserting the new key in the database
+ # rather than a proper ValidationError raised by 'clean'.
+
+ # Set the instance user.
+ self.instance.user = self.user
+
+ # Allow checking against the missing attribute.
+ exclude = self._get_validation_exclusions()
+ exclude.remove('user')
+ try:
+ self.instance.validate_unique(exclude=exclude)
+ except ValidationError, e:
+ # Publish this error as a 'key' error rather than an 'general'
+ # error.
+ error = e.message_dict.pop('__all__')
+ e.message_dict['key'] = error
+ self._update_errors(e.message_dict)
class MultipleMACAddressField(forms.MultiValueField):
=== added file 'src/maasserver/migrations/0005_sshkey_user_and_key_unique_together.py'
--- src/maasserver/migrations/0005_sshkey_user_and_key_unique_together.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/migrations/0005_sshkey_user_and_key_unique_together.py 2012-04-06 08:51:18 +0000
@@ -0,0 +1,138 @@
+# flake8: noqa
+# SKIP this file when reformatting.
+# The rest of this file was generated by South.
+
+# encoding: utf-8
+import datetime
+
+from django.db import models
+from south.db import db
+from south.v2 import SchemaMigration
+
+
+class Migration(SchemaMigration):
+
+ def forwards(self, orm):
+
+ # Adding unique constraint on 'SSHKey', fields ['user', 'key']
+ db.create_unique('maasserver_sshkey', ['user_id', 'key'])
+
+
+ def backwards(self, orm):
+
+ # Removing unique constraint on 'SSHKey', fields ['user', 'key']
+ db.delete_unique('maasserver_sshkey', ['user_id', 'key'])
+
+
+ models = {
+ 'auth.group': {
+ 'Meta': {'object_name': 'Group'},
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}),
+ 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'})
+ },
+ 'auth.permission': {
+ 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'},
+ 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+ 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'})
+ },
+ 'auth.user': {
+ 'Meta': {'object_name': 'User'},
+ 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+ 'email': ('django.db.models.fields.EmailField', [], {'unique': 'True', 'max_length': '75', 'blank': 'True'}),
+ 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+ 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+ 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+ 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+ 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}),
+ 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
+ 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
+ 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}),
+ 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
+ },
+ 'contenttypes.contenttype': {
+ 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"},
+ 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+ 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
+ },
+ 'maasserver.config': {
+ 'Meta': {'object_name': 'Config'},
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
+ 'value': ('maasserver.fields.JSONObjectField', [], {'null': 'True'})
+ },
+ 'maasserver.filestorage': {
+ 'Meta': {'object_name': 'FileStorage'},
+ 'data': ('django.db.models.fields.files.FileField', [], {'max_length': '255'}),
+ 'filename': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '100'}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'})
+ },
+ 'maasserver.macaddress': {
+ 'Meta': {'object_name': 'MACAddress'},
+ 'created': ('django.db.models.fields.DateField', [], {}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'mac_address': ('maasserver.fields.MACAddressField', [], {'unique': 'True'}),
+ 'node': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['maasserver.Node']"}),
+ 'updated': ('django.db.models.fields.DateTimeField', [], {})
+ },
+ 'maasserver.node': {
+ 'Meta': {'object_name': 'Node'},
+ 'after_commissioning_action': ('django.db.models.fields.IntegerField', [], {'default': '0'}),
+ 'architecture': ('django.db.models.fields.CharField', [], {'default': "u'i386'", 'max_length': '10'}),
+ 'created': ('django.db.models.fields.DateField', [], {}),
+ 'error': ('django.db.models.fields.CharField', [], {'default': "u''", 'max_length': '255', 'blank': 'True'}),
+ 'hostname': ('django.db.models.fields.CharField', [], {'default': "u''", 'max_length': '255', 'blank': 'True'}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'owner': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}),
+ 'power_type': ('django.db.models.fields.CharField', [], {'default': "u''", 'max_length': '10', 'blank': 'True'}),
+ 'status': ('django.db.models.fields.IntegerField', [], {'default': '0', 'max_length': '10'}),
+ 'system_id': ('django.db.models.fields.CharField', [], {'default': "u'node-692e05e6-7fc4-11e1-a47c-00219bd0a2de'", 'unique': 'True', 'max_length': '41'}),
+ 'token': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['piston.Token']", 'null': 'True'}),
+ 'updated': ('django.db.models.fields.DateTimeField', [], {})
+ },
+ 'maasserver.sshkey': {
+ 'Meta': {'unique_together': "((u'user', u'key'),)", 'object_name': 'SSHKey'},
+ 'created': ('django.db.models.fields.DateField', [], {}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'key': ('django.db.models.fields.TextField', [], {}),
+ 'updated': ('django.db.models.fields.DateTimeField', [], {}),
+ 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"})
+ },
+ 'maasserver.userprofile': {
+ 'Meta': {'object_name': 'UserProfile'},
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'user': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['auth.User']", 'unique': 'True'})
+ },
+ 'piston.consumer': {
+ 'Meta': {'object_name': 'Consumer'},
+ 'description': ('django.db.models.fields.TextField', [], {}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'key': ('django.db.models.fields.CharField', [], {'max_length': '18'}),
+ 'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
+ 'secret': ('django.db.models.fields.CharField', [], {'max_length': '32'}),
+ 'status': ('django.db.models.fields.CharField', [], {'default': "'pending'", 'max_length': '16'}),
+ 'user': ('django.db.models.fields.related.ForeignKey', [], {'blank': 'True', 'related_name': "'consumers'", 'null': 'True', 'to': "orm['auth.User']"})
+ },
+ 'piston.token': {
+ 'Meta': {'object_name': 'Token'},
+ 'callback': ('django.db.models.fields.CharField', [], {'max_length': '255', 'null': 'True', 'blank': 'True'}),
+ 'callback_confirmed': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+ 'consumer': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['piston.Consumer']"}),
+ 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+ 'is_approved': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+ 'key': ('django.db.models.fields.CharField', [], {'max_length': '18'}),
+ 'secret': ('django.db.models.fields.CharField', [], {'max_length': '32'}),
+ 'timestamp': ('django.db.models.fields.IntegerField', [], {'default': '1333701735L'}),
+ 'token_type': ('django.db.models.fields.IntegerField', [], {}),
+ 'user': ('django.db.models.fields.related.ForeignKey', [], {'blank': 'True', 'related_name': "'tokens'", 'null': 'True', 'to': "orm['auth.User']"}),
+ 'verifier': ('django.db.models.fields.CharField', [], {'max_length': '10'})
+ }
+ }
+
+ complete_apps = ['maasserver']
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-04-05 13:58:19 +0000
+++ src/maasserver/models.py 2012-04-06 08:51:18 +0000
@@ -928,9 +928,6 @@
:ivar user: The user which owns the key.
:ivar key: The ssh public key.
"""
- class Meta:
- verbose_name_plural = "SSH keys"
-
objects = SSHKeyManager()
user = models.ForeignKey(User, null=False, editable=False)
@@ -938,6 +935,16 @@
key = models.TextField(
null=False, editable=True, validators=[validate_ssh_public_key])
+ class Meta:
+ verbose_name_plural = "SSH keys"
+ unique_together = ('user', 'key')
+
+ def unique_error_message(self, model_class, unique_check):
+ if unique_check == ('user', 'key'):
+ return "This key has already been added for this user."
+ return super(
+ SSHKey, self).unique_error_message(model_class, unique_check)
+
def __unicode__(self):
return self.key
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py 2012-03-30 07:20:53 +0000
+++ src/maasserver/testing/factory.py 2012-04-06 08:51:18 +0000
@@ -89,8 +89,9 @@
return User.objects.create_user(
username=username, password=password, email=email)
- def make_sshkey(self, user):
- key_string = get_data('data/test_rsa.pub')
+ def make_sshkey(self, user, key_string=None):
+ if key_string is None:
+ key_string = get_data('data/test_rsa.pub')
key = SSHKey(key=key_string, user=user)
key.save()
return key
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-04-05 12:18:20 +0000
+++ src/maasserver/tests/test_models.py 2012-04-06 08:51:18 +0000
@@ -970,6 +970,25 @@
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')
+ user = factory.make_user()
+ key = SSHKey(key=key_string, user=user)
+ key.save()
+ key2 = SSHKey(key=key_string, user=user)
+ self.assertRaises(
+ ValidationError, key2.full_clean)
+
+ def test_sshkey_same_key_can_be_used_by_different_users(self):
+ key_string = get_data('data/test_rsa.pub')
+ user = factory.make_user()
+ key = SSHKey(key=key_string, user=user)
+ key.save()
+ user2 = factory.make_user()
+ key2 = SSHKey(key=key_string, user=user2)
+ key2.full_clean()
+ # No ValidationError.
+
class SSHKeyManagerTest(TestCase):
"""Testing for the :class:`SSHKeyManager` model manager."""
=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py 2012-04-05 07:50:52 +0000
+++ src/maasserver/tests/test_views.py 2012-04-06 08:51:18 +0000
@@ -387,7 +387,12 @@
self.assertIn(add_key_link, get_content_links(response))
def create_keys_for_user(self, user):
- return [factory.make_sshkey(self.logged_in_user) for i in range(3)]
+ 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)
@@ -426,6 +431,31 @@
self.assertEqual(httplib.FOUND, response.status_code)
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 = SSHKey(user=self.logged_in_user, key=key_string)
+ key.save()
+ response = self.client.post(
+ reverse('prefs-add-sshkey'), {'key': key_string})
+
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertIn(
+ "This key has already been added for this user.",
+ response.content)
+ 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 = SSHKey(user=factory.make_user(), key=key_string)
+ key.save()
+ response = self.client.post(
+ reverse('prefs-add-sshkey'), {'key': key_string})
+ new_key = SSHKey.objects.get(key=key_string, user=self.logged_in_user)
+
+ self.assertEqual(httplib.FOUND, response.status_code)
+ self.assertItemsEqual(
+ [key, new_key], SSHKey.objects.filter(key=key_string))
+
def test_delete_key_GET(self):
# The 'Delete key' page displays a confirmation page with a form.
key = factory.make_sshkey(self.logged_in_user)
