launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/delete-ssh-key-text-type into lp:launchpad.

Commit message:
Weaken type of key_text in person.deleteSSHKeysFromSSO so that more existing keys can be deleted.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/delete-ssh-key-text-type/+merge/352210
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/delete-ssh-key-text-type into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2018-02-24 09:11:39 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2018-08-02 16:35:09 +0000
@@ -3,6 +3,8 @@
 
 __metaclass__ = type
 
+import textwrap
+
 from storm.store import Store
 from zope.component import getUtility
 from zope.security.management import endInteraction
@@ -12,7 +14,10 @@
     IPersonSet,
     TeamMembershipStatus,
     )
-from lp.registry.interfaces.ssh import SSHKeyType
+from lp.registry.interfaces.ssh import (
+    ISSHKeySet,
+    SSHKeyType,
+    )
 from lp.registry.interfaces.teammembership import ITeamMembershipSet
 from lp.services.identity.interfaces.account import (
     AccountStatus,
@@ -668,6 +673,34 @@
         self.assertEqual(200, response.status)
         self.assertEqual(1, person.sshkeys.count())
 
+    def test_deleteSSHKeyFromSSO_allows_newlines(self):
+        # Adding these should normally be forbidden, but we want users to be
+        # able to delete existing rows.
+        with admin_logged_in():
+            person = removeSecurityProxy(self.factory.makePerson())
+            kind, data, comment = self.factory.makeSSHKeyText().split(" ", 2)
+            key_text = "%s %s %s\n" % (kind, textwrap.fill(data), comment)
+            key = getUtility(ISSHKeySet).new(person, key_text, check_key=False)
+            openid_id = person.account.openid_identifiers.any().identifier
+        response = self.deleteSSHKeyFromSSO(
+            openid_id, key.getFullKeyText())
+
+        self.assertEqual(200, response.status)
+        self.assertEqual(0, person.sshkeys.count())
+
+    def test_deleteSSHKeyFromSSO_allows_newlines_dry_run(self):
+        with admin_logged_in():
+            person = removeSecurityProxy(self.factory.makePerson())
+            kind, data, comment = self.factory.makeSSHKeyText().split(" ", 2)
+            key_text = "%s %s %s\n" % (kind, textwrap.fill(data), comment)
+            key = getUtility(ISSHKeySet).new(person, key_text, check_key=False)
+            openid_id = person.account.openid_identifiers.any().identifier
+        response = self.deleteSSHKeyFromSSO(
+            openid_id, key.getFullKeyText(), dry_run=True)
+
+        self.assertEqual(200, response.status)
+        self.assertEqual(1, person.sshkeys.count())
+
     def test_deleteSSHKeyFromSSO_is_restricted(self, dry_run=False):
         with admin_logged_in():
             target = self.factory.makePerson()

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2018-05-14 10:59:35 +0000
+++ lib/lp/registry/interfaces/person.py	2018-08-02 16:35:09 +0000
@@ -2352,8 +2352,9 @@
     @operation_parameters(
         openid_identifier=TextLine(
             title=_("OpenID identifier suffix"), required=True),
-        key_text=TextLine(
-            title=_("SSH key text"), required=True),
+        # This is more liberal than the type for adding keys, in order to
+        # avoid existing keys being undeleteable.
+        key_text=Text(title=_("SSH key text"), required=True),
         dry_run=Bool(_("Don't save changes")))
     @export_write_operation()
     @operation_for_version("devel")