Merge lp:~maxiberta/canonical-identity-provider/fix-authn-device-inline-form-type into lp:canonical-identity-provider/release

[Maximiliano Bertacchini]

Make admin AuthenticationDeviceInlineForm.device_type readonly instead of a choice field (LP: #1745156).

Make admin AuthenticationDeviceInlineForm.device_type a free text input instead of a choice field. The choice field was introduced a long time ago (LP: #1071778) but doesn't seem to make too much sense now, based on the fact that `AuthenticationDevice.device_type` is a `TextField(null=True)`, and the variety of values currently found:

sso_staging=> SELECT COUNT(*), device_type FROM identityprovider_authenticationdevice GROUP BY device_type;
 count | device_type
-------+--------------
    94 |
   114 | paper
     1 | yubi:hotp
   123 | google
    66 | google:totp
     2 | google:hotp
    25 | yubi
     2 | None
     2 | generic:hotp
   372 | generic
(10 rows)

[Daniel Manrique (roadmr)]

This should work, I made 2 comments related to usability but since this is pretty broken right now, we could even go with this code and refine things later.

review: Approve

[Daniel Manrique (roadmr)]

Replying to myself: Maxi suggested making the field read-only, which makes both my comments moot: if the field is non-editable, there's not much need for the help text I suggested, nor for the expansion of choices to allow using the selector.

Changing the type of device for an on-the-field device will completely change its behavior and essentially render it useless for the user, so it's not something we've ever done. Usually we just delete devices and let users recreate them. The most common admin operations are fiddling with the counter and *maybe* extracting the random key, which can still be done. The main point is to be able to save changed records, which can still be done if the field is read-only.

=== modified file 'src/identityprovider/admin.py'
--- src/identityprovider/admin.py	2016-12-06 16:53:22 +0000
+++ src/identityprovider/admin.py	2018-01-24 19:16:25 +0000
@@ -53,15 +53,6 @@
 )
 
 
-DEVICE_TYPE_CHOICES = [
-    (None, 'Unset'),
-    ('paper', 'Printable backup codes'),
-    ('yubi', 'YubiKey'),
-    ('google', 'Smartphone or tablet'),
-    ('generic', 'Generic HOTP/TOTP device')
-]
-
-
 def build_account_link(account):
     url = reverse(
         'admin:identityprovider_account_change', args=(account.id,))
@@ -219,8 +210,6 @@
                           label="OATH HOTP/TOTP key")
     name = forms.CharField(widget=forms.TextInput(attrs={'size': 15}),
                            label="Device name")
-    device_type = forms.ChoiceField(choices=DEVICE_TYPE_CHOICES,
-                                    label="Device type")
 
     class Meta:
         model = AuthenticationDevice
@@ -231,6 +220,7 @@
 
     model = AuthenticationDevice
     form = AuthenticationDeviceInlineForm
+    readonly_fields = ['device_type']
     extra = 0
 
 

=== modified file 'src/identityprovider/tests/test_admin.py'
--- src/identityprovider/tests/test_admin.py	2016-09-05 18:11:55 +0000
+++ src/identityprovider/tests/test_admin.py	2018-01-24 19:16:25 +0000
@@ -237,7 +237,7 @@
             key='some key',
             name='Some device',
             counter=124,
-            device_type=None,
+            device_type='paper',
         )
         parameters = {
             'devices-TOTAL_FORMS': '1',
@@ -246,7 +246,7 @@
             'devices-0-key': 'some key',
             'devices-0-name': 'Some device',
             'devices-0-counter': '123',
-            'devices-0-device_type': 'paper',
+            'devices-0-device_type': 'foobar',
         }
         self.post_account_change(**parameters)
 
@@ -254,7 +254,7 @@
         self.assertEqual(device.counter, 123)
         self.assertEqual(device.device_type, 'paper')
 
-    def get_device_inlineform(self, device_type):
+    def test_device_inlineform(self):
         device = AuthenticationDevice.objects.create(
             account=self.account,
             key='some key',
@@ -266,14 +266,10 @@
             'key': 'other key',
             'name': 'A device',
             'counter': '123',
-            'device_type': device_type,
+            'device_type': 'foobar',
         }
         form = AuthenticationDeviceInlineForm(post_data, instance=device)
-        return form
 
-    def test_device_inlineform_invalid_type(self):
-        form = self.get_device_inlineform(device_type='foobar')
-        device = AuthenticationDevice.objects.all()[0]
         expected_defaults = {
             'id': device.id,
             'key': 'some key',
@@ -282,16 +278,7 @@
             'device_type': None,
         }
         self.assertEqual(form.initial, expected_defaults)
-        self.assertEqual(form.is_valid(), False)
-
-        msg = ('Select a valid choice. foobar is not one of the '
-               'available choices.')
-        expected_errors = {'device_type': [msg]}
-        self.assertEqual(form.errors, expected_errors)
-
-    def test_device_inlineform_valid_type(self):
-        form = self.get_device_inlineform(device_type='None')
-        self.assertEqual(form.is_valid(), True)
+        self.assertTrue(form.is_valid())
 
     def test_multiple_preferred_emails(self):
         email = self.account.preferredemail