Merge lp:~deadlight/canonical-identity-provider/ssh-form into lp:canonical-identity-provider/release

[Karl Williams]

Fixed the display bug on the SSH key form.

Modified the data passed to the form and removed the full HTML generation in favour of explicit validation of the data fields.

Fixed the display bug on the SSH key form.

Modified the data passed to the form and removed the full HTML generation in favour of explicit validation of the data fields.

QA:

To test, you can fake the SSH behaviour of the live app by applying this patch: https://code.launchpad.net/~maxiberta/canonical-identity-provider/vanilla-sshkeys-debug/+merge/377722

[Karl Williams (deadlight)]

Fixes 1859194

[Maximiliano Bertacchini (maxiberta)]

Sorry, didn't seem to work locally. Maybe am I missing something? Check inline for suggested fixes.
Please let me know if I can help.
Thanks!

review: Needs Fixing

[Karl Williams (deadlight)]

> Sorry, didn't seem to work locally. Maybe am I missing something? Check inline
> for suggested fixes.
> Please let me know if I can help.
> Thanks!

Yeah, thanks for the feedback. I made some changes to the template and neglected to revert them when I changed my plan with the python. I'll rework it.

[Karl Williams (deadlight)]

This is now ready for re-review.

[Maximiliano Bertacchini (maxiberta)]

Looks good to me, thanks +1.

review: Approve

[Anthony Dillon (ya-bo-ng)]

LGTM thanks +1

review: Approve

[Daniel Manrique (roadmr)]

The status had been set to merged in error apparently :/ I set to approved now, let the bot do its job.

[Otto Co-Pilot (otto-copilot)]

A commit message must be set
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/276/

[Daniel Manrique (roadmr)]

I hate you jenkins

review: Approve

=== modified file 'src/webui/forms.py'
--- src/webui/forms.py	2018-07-19 15:46:28 +0000
+++ src/webui/forms.py	2020-01-27 16:55:59 +0000
@@ -5,7 +5,7 @@
 import unicodedata
 
 from django import forms
-from django.utils.html import escape, format_html
+from django.utils.html import escape
 from django.utils.translation import ugettext_lazy as _
 
 from webservices.launchpad import (
@@ -83,7 +83,7 @@
 
 
 def format_ssh_key(ssh_key):
-    """Decide how to display 'ssh_key'.
+    """Parse and split 'ssh_key'.
 
     SSH keys are freeform text, so users absolutely *will* try and add keys
     that contain javascript, so we need to make sure they're escaped when
@@ -96,37 +96,20 @@
         # valid ssh key format. However, we need to exercise this code path
         # in tests to ensure that users malicously POST'ing bad key formats
         # will get a ValidationError.
-        # Just return the escaped data - this will never be run in production:
-        return format_html(
-            '<label>Bad Key Data: <span>{key}</span></label>',
-            key=escape(ssh_key))
+        # This should never happen in production but it returns a valid dict:
+        ssh_key_info = dict()
+        ssh_key_info['ssh_key'] = ssh_key
+        ssh_key_info['label'] = "Invalid key"
+        ssh_key_info['type'] = "Invalid key"
+        ssh_key_info['text'] = escape(ssh_key)
+        return ssh_key_info
     else:
-        return format_html(
-            u'<div class="key-display">'
-            u'  <h4>{comment}</h4> '
-            u'  <ul>'
-            u'    <li>'
-            u'      <dl>'
-            u'        <dt>{key_type_label}</dt>'
-            u'        <dd>{key_type}</dt>'
-            u'      </dl>'
-            u'    </li>'
-            u'    <li>'
-            u'      <dl>'
-            u'        <dt>{key_text_label}</dt>'
-            u'        <dd class="key-text">'
-            u'          <a href="#">{key_text}</a>'
-            u'        </dt>'
-            u'      </dl>'
-            u'    </li>'
-            u'  </ul>'
-            u'</div>',
-            comment=comment,
-            key_type_label=_("Type: "),
-            key_type=key_type,
-            key_text_label=_("Text: "),
-            key_text=key_text,
-        )
+        ssh_key_info = dict()
+        ssh_key_info['ssh_key'] = escape(ssh_key)
+        ssh_key_info['label'] = escape(comment)
+        ssh_key_info['type'] = escape(key_type)
+        ssh_key_info['text'] = escape(key_text)
+        return ssh_key_info
 
 
 class DeleteSSHKeyForm(forms.Form):

=== modified file 'src/webui/templates/account/ssh_keys.html'
--- src/webui/templates/account/ssh_keys.html	2020-01-22 12:52:26 +0000
+++ src/webui/templates/account/ssh_keys.html	2020-01-27 16:55:59 +0000
@@ -17,27 +17,27 @@
     {% if delete_key_form.fields.ssh_keys.choices %}
       <section class="delete-ssh-keys">
         <form action="" method="POST">
-            <ul class="p-list--divided">
+          <ul class="p-list--divided">
             {% csrf_token %}
             {% for value, key_info in delete_key_form.fields.ssh_keys.choices %}
-              <li class="p-list__item">
-                <input id="id_ssh_keys_{{ forloop.counter0 }}" name="ssh_keys" type="checkbox" value="{{ value }}"><label for="id_ssh_keys_{{ forloop.counter0 }}">{{ key_info.label }}</label>
-                <aside class="p-accordion" role="tablist" aria-multiselect="true">
-                  <ul class="p-accordion__list" style="margin-bottom: 0;">
-                    <li class="p-accordion__group">
-                      <button type="button" class="p-accordion__tab" id="tab{{ forloop.counter0 }}" role="tab" aria-controls="#tab{{ forloop.counter0 }}-section" aria-expanded="false">{% trans "Key details" %}</button>
-                      <section class="p-accordion__panel" id="tab{{ forloop.counter0 }}-section" role="tabpanel" aria-hidden="true" aria-labelledby="tab{{ forloop.counter0 }}-section">
-                        <dl>
-                          <dt>Type</dt>
-                          <dd>{{ key_info.type }}</dd>
-                          <dt>Text</dt>
-                          <dd><code>{{ key_info.text }}</code></dd>
-                        </dl>
-                      </section>
-                    </li>
-                  </ul>
-                </aside>
-              </li>
+            <li class="p-list__item">
+              <input id="id_ssh_keys_{{ forloop.counter0 }}" name="ssh_keys" type="checkbox" value="{{ value }}"><label for="id_ssh_keys_{{ forloop.counter0 }}">{{ key_info.label }}</label>
+              <aside class="p-accordion" role="tablist" aria-multiselect="true">
+                <ul class="p-accordion__list" style="margin-bottom: 0;">
+                  <li class="p-accordion__group">
+                    <button type="button" class="p-accordion__tab" id="tab{{ forloop.counter0 }}" role="tab" aria-controls="#tab{{ forloop.counter0 }}-section" aria-expanded="false">{% trans "Key details" %}</button>
+                    <section class="p-accordion__panel" id="tab{{ forloop.counter0 }}-section" role="tabpanel" aria-hidden="true" aria-labelledby="tab{{ forloop.counter0 }}-section">
+                      <dl>
+                        <dt>Type</dt>
+                        <dd>{{ key_info.type }}</dd>
+                        <dt>Text</dt>
+                        <dd><code>{{ key_info.text }}</code></dd>
+                      </dl>
+                    </section>
+                  </li>
+                </ul>
+              </aside>
+            </li>
             {% endfor %}
           </ul>
           <input class="p-button--negative" type="submit" name="delete_keys" value="{% trans "Delete selected keys" %}"/>

=== modified file 'src/webui/tests/test_forms.py'
--- src/webui/tests/test_forms.py	2018-07-19 16:05:43 +0000
+++ src/webui/tests/test_forms.py	2020-01-27 16:55:59 +0000
@@ -250,7 +250,19 @@
         self.assertNotIn(bad_content, formatted_key)
         self.assertIn(
             '&lt;script&gt;window.alert(&quot;boo!&quot;);&lt;/script&gt;',
-            formatted_key
+            formatted_key['text']
+        )
+        self.assertIn(
+            '&lt;script&gt;window.alert(&quot;boo!&quot;);&lt;/script&gt;',
+            formatted_key['label']
+        )
+        self.assertIn(
+            '&lt;script&gt;window.alert(&quot;boo!&quot;);&lt;/script&gt;',
+            formatted_key['type']
+        )
+        self.assertIn(
+            '&lt;script&gt;window.alert(&quot;boo!&quot;);&lt;/script&gt;',
+            formatted_key['ssh_key']
         )
 
     def test_unicode(self):
@@ -258,7 +270,13 @@
         # previously this would raise UnicodeDecodeError. Now it works as
         # expected.
         formatted_key = format_ssh_key(keytext)
-        self.assertIn('¿ʇuǝɯɯoɔ', formatted_key)
+        self.assertIn('¿ʇuǝɯɯoɔ', formatted_key['label'])
+
+    def test_invalid_keys(self):
+        invalid_key_text = "INVALID"
+        formatted_key = format_ssh_key(invalid_key_text)
+        self.assertIn(invalid_key_text, formatted_key['ssh_key'])
+        self.assertIn('Invalid key', formatted_key['label'])
 
 
 class DeleteSSHKeyFormTestCase(SSOBaseTestCase):