Merge ~bjornt/maas:bug-1604702-too-big-to-index into maas:master

[Björn Tillenius]

LP: #1604702 - Unable to add a 16384-bit ssh key to my account from web UI

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug-1604702-too-big-to-index lp:~bjornt/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 354dbbd83faed0fb3e4af410eb75d55fe0c0f2e3

review: Approve

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

diff --git a/src/maasserver/forms/__init__.py b/src/maasserver/forms/__init__.py
index b3e640d..3a88e75 100644
--- a/src/maasserver/forms/__init__.py
+++ b/src/maasserver/forms/__init__.py
@@ -1100,7 +1100,7 @@ class KeyForm(MAASModelForm):
             self._errors.setdefault('key', self.error_class()).extend(error)
 
 
-class SSHKeyForm(KeyForm):
+class SSHKeyForm(MAASModelForm):
     key = UnstrippedCharField(
         label="Public key",
         widget=forms.Textarea(attrs={'rows': '5', 'cols': '30'}),
@@ -1110,6 +1110,10 @@ class SSHKeyForm(KeyForm):
         model = SSHKey
         fields = ["key"]
 
+    def __init__(self, user, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.instance.user = user
+
     def save(self, endpoint, request):
         sshkey = super(SSHKeyForm, self).save()
         create_audit_event(
diff --git a/src/maasserver/migrations/builtin/maasserver/0156_drop_ssh_unique_key_index.py b/src/maasserver/migrations/builtin/maasserver/0156_drop_ssh_unique_key_index.py
new file mode 100644
index 0000000..05e21d3
--- /dev/null
+++ b/src/maasserver/migrations/builtin/maasserver/0156_drop_ssh_unique_key_index.py
@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.11 on 2018-05-16 09:01
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('maasserver', '0155_add_globaldefaults_model'),
+    ]
+
+    operations = [
+        migrations.AlterUniqueTogether(
+            name='sshkey',
+            unique_together=set([]),
+        ),
+    ]
diff --git a/src/maasserver/models/sshkey.py b/src/maasserver/models/sshkey.py
index 06862e4..f0dfdf3 100644
--- a/src/maasserver/models/sshkey.py
+++ b/src/maasserver/models/sshkey.py
@@ -114,13 +114,6 @@ class SSHKey(CleanSave, TimestampedModel):
 
     class Meta(DefaultMeta):
         verbose_name = "SSH key"
-        unique_together = ('user', 'key', 'keysource')
-
-    def unique_error_message(self, model_class, unique_check):
-        if unique_check == ('user', 'key', 'keysource'):
-                return "This key has already been added for this user."
-        return super(
-            SSHKey, self).unique_error_message(model_class, unique_check)
 
     def __str__(self):
         return self.key
@@ -134,3 +127,20 @@ class SSHKey(CleanSave, TimestampedModel):
         if key_size is None:
             key_size = DEFAULT_KEY_DISPLAY
         return mark_safe(get_html_display_for_key(self.key, key_size))
+
+    def clean(self, *args, **kwargs):
+        """Make sure there are no duplicate keys.
+
+        Note that this could have been done with Meta.unique_together,
+        but it doesn't work for big keys, since the long text strings
+        can't be indexed.
+        """
+        super().clean(*args, **kwargs)
+        if not self._state.has_changed('key'):
+            return
+
+        existing_key = SSHKey.objects.filter(
+            keysource=self.keysource, user=self.user, key=self.key)
+        if existing_key.exists():
+            raise ValidationError(
+                'This key has already been added for this user.')
diff --git a/src/maasserver/models/tests/test_sshkey.py b/src/maasserver/models/tests/test_sshkey.py
index cf55bea..03bde0e 100644
--- a/src/maasserver/models/tests/test_sshkey.py
+++ b/src/maasserver/models/tests/test_sshkey.py
@@ -8,7 +8,6 @@ __all__ = []
 import random
 
 from django.core.exceptions import ValidationError
-from django.db import IntegrityError
 from django.utils.safestring import SafeString
 from maasserver.enum import KEYS_PROTOCOL_TYPE
 from maasserver.models import (
@@ -267,7 +266,7 @@ class SSHKeyTest(MAASServerTestCase):
         display = key.display_html()
         self.assertIsInstance(display, SafeString)
 
-    def test_sshkey_user_and_key_unique_together(self):
+    def test_sshkey_user_and_key_unique_together_create(self):
         protocol = random.choice(
             [KEYS_PROTOCOL_TYPE.LP, KEYS_PROTOCOL_TYPE.GH])
         auth_id = factory.make_name('auth_id')
@@ -280,28 +279,35 @@ class SSHKeyTest(MAASServerTestCase):
         self.assertRaises(
             ValidationError, key2.full_clean)
 
-    def test_sshkey_user_and_key_unique_together_db_level(self):
-        # Even if we hack our way around model-level checks, uniqueness
-        # of the user/key combination is enforced at the database level.
+    def test_sshkey_user_and_key_unique_together_change_key(self):
         protocol = random.choice(
             [KEYS_PROTOCOL_TYPE.LP, KEYS_PROTOCOL_TYPE.GH])
         auth_id = factory.make_name('auth_id')
         keysource = factory.make_KeySource(protocol=protocol, auth_id=auth_id)
-        key_string = get_data('data/test_rsa0.pub')
+        key_string1 = get_data('data/test_rsa1.pub')
+        key_string2 = get_data('data/test_rsa2.pub')
         user = factory.make_User()
-        existing_key = SSHKey(key=key_string, user=user, keysource=keysource)
-        existing_key.save()
-        # The trick to hack around the model-level checks: create a
-        # duplicate key for another user, then attach it to the same
-        # user as the existing key by updating it directly in the
-        # database.
-        redundant_key = SSHKey(
-            key=key_string, user=factory.make_User(), keysource=keysource)
-        redundant_key.save()
+        key1 = SSHKey(key=key_string1, user=user, keysource=keysource)
+        key1.save()
+        key2 = SSHKey(key=key_string2, user=user, keysource=keysource)
+        key2.save()
+        key2.key = key1.key
         self.assertRaises(
-            IntegrityError,
-            SSHKey.objects.filter(id=redundant_key.id).update,
-            user=user)
+            ValidationError, key2.full_clean)
+
+    def test_sshkey_same_key_can_be_used_by_different_sources(self):
+        auth_id = factory.make_name('auth_id')
+        keysource1 = factory.make_KeySource(
+            protocol=KEYS_PROTOCOL_TYPE.LP, auth_id=auth_id)
+        keysource2 = factory.make_KeySource(
+            protocol=KEYS_PROTOCOL_TYPE.GH, auth_id=auth_id)
+        key_string = get_data('data/test_rsa0.pub')
+        user = factory.make_User()
+        key1 = SSHKey(key=key_string, user=user, keysource=keysource1)
+        key1.save()
+        key2 = SSHKey(key=key_string, user=user, keysource=keysource2)
+        key2.save()
+        self.assertIsNone(key2.full_clean())
 
     def test_sshkey_same_key_can_be_used_by_different_users(self):
         key_string = get_data('data/test_rsa0.pub')